Remove duplicate code paths in transitions, GMM, simulators#33
Merged
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
This was referenced May 21, 2026
5 tasks
edeno
force-pushed
the
em-convergence-surfacing
branch
from
a696ce9 to
c275c35
Compare
The class was declared but never returned (the comment said "DEPRECATED: Use dictionary for now") and had no external callers. Remove the class, drop the now-unused `from dataclasses import dataclass` import, and fix the leftover `-> EncodingModel` return annotation on `fit_clusterless_gmm_encoding_model` to `-> dict` (matching the docstring and the actual returned shape). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The exact-discrete-transition M-step landed in c339fa9 (#24) and the aggregated path was removed in a19deb1. The original v1 entry-points (estimate_joint_distribution, multinomial_neg_log_likelihood + its jax.grad/hessian wrappers, and set_initial_discrete_transition) were left behind with no remaining production callers, only referenced by tests that exercised them or used them as a v1-vs-new oracle. Delete the three functions and the module-level grad/hessian bindings. Delete the test classes that exclusively exercised the v1 paths; rewrite the v1-as-oracle tests in test_discrete_transitions_estimation.py to call the canonical estimate_discrete_transition_counts_from_expanded_posteriors / ..._from_responses_... directly. 70 discrete-transition tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
simulate/simulate.py and simulate/sorted_spikes_simulation.py both defined functions with the same names but in two cases different semantics: - simulate_poisson_spikes was binary indicators in simulate.py vs raw Poisson counts in sorted_spikes_simulation.py. Keep the counts semantics (closer to spike-time events; indicator = count > 0). All in-repo consumers already compose correctly with counts. - get_trajectory_direction returned one value in simulate.py vs two values (direction_label, is_inbound) in sorted_spikes_simulation.py. Keep the two-value form (strictly more informative). Update the single one-value caller to unpack-and-discard. - simulate_position is a velocity-parameterized sinusoid in simulate.py vs a period-parameterized sinusoid in sorted_spikes_simulation.py. These have genuinely different trajectories and consolidating them would change goldens. Keep the velocity form as the canonical public name; rename the period form to the private _simulate_sinusoidal_position_by_period used only by the sorted-spikes simulator flows. The remaining utilities (simulate_time, simulate_place_field_firing_rate, simulate_neuron_with_place_field) are bit-identical; hoist into simulate/_common.py and re-export from simulate/__init__.py so callers can `from non_local_detector.simulate import …`. Update the weighted_place_fields notebook's import path. CHANGELOG documents the new public-surface conventions, the deletions in Groups 3c and 3d, and the deferral of the clusterless_kde consolidation pending the log-KDE Local NaN fix (#32). 21 simulator-contract tests pass; 8 snapshot + 4 golden regression tests pass with no diffs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
edeno
force-pushed
the
duplicate-path-cleanup
branch
from
3262556 to
23c1c5a
Compare
- clusterless_gmm.fit_clusterless_gmm_encoding_model: correct the Returns docstring to the nine keys the function actually returns. It previously listed occupancy_bins, log_occupancy_bins (real key is log_occupancy), position_time, and eight gmm_* config arguments that are never returned. Fix the CHANGELOG entry that incorrectly claimed the annotation matched the "documented and actual" return shape (the docstring was wrong). - test_discrete_transitions_estimation: add a magnitude guard (learned_transition[0, 2] < 0.5) to test_simulated_likelihoods_shift_learned_transition_to_matching_source. This restores the exact-vs-source-blind discrimination that the deleted aggregate-path negative control provided, without resurrecting v1 code. - test_simulator_contract: add test_simulate_poisson_spikes_returns_counts, a direct guard pinning the unified counts (not binary 0/1) semantics that the package now advertises; previously exercised only transitively. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Removes parallel implementations and dead code that CLAUDE.md prohibits ("don't leave parallel v1 / v2 implementations indefinitely"). This is the bare-deletes portion of the planned cleanup — the clusterless KDE consolidation is intentionally deferred (see below).
Removed
discrete_state_transitions.estimate_joint_distribution,multinomial_neg_log_likelihood,multinomial_gradient,multinomial_hessian,set_initial_discrete_transition: orphaned v1 M-step paths superseded by_aggregate_xi_by_state_jax/_transition_pair_stats_jaxin c339fa9 (Implement exact discrete-transition M-step #24) and a19deb1. No remaining production callers.likelihoods.clusterless_gmm.EncodingModeldataclass: declared but never returned; comment marked it deprecated. Return-type annotation onfit_clusterless_gmm_encoding_modelupdated todictto match the docstring.simulate_time,simulate_place_field_firing_rate,simulate_neuron_with_place_fielddefinitions insimulate/sorted_spikes_simulation.py: hoisted bit-identical copies intosimulate/_common.pyas the single source of truth.Changed
simulate.simulate_poisson_spikesnow returns Poisson counts consistently across the package. Previously two same-named functions disagreed (counts vs binary indicators). Callers needing a boolean indicator should apply> 0; all in-repo consumers already do this.simulate.get_trajectory_directionnow consistently returns(direction_label, is_inbound). The one-value variant has been removed.simulate.simulate_positionretains the velocity-based semantics fromsimulate/simulate.py. The period-based sinusoid that previously shared the name insimulate/sorted_spikes_simulation.pyis renamed to a private_simulate_sinusoidal_position_by_period(used only by the sorted-spikes simulator flows). Preserves golden-regression trajectories.Deferred
clusterless_kdeconsolidation (originally planned for this PR) is intentionally not in this PR. A benchmark uncovered anis_local=TrueNaN bug inclusterless_kde_logon the golden NonLocal fixture (clusterless_kde_log: is_local=True branch produces NaNs on golden NonLocal fixture #32). Switching the canonical path now would silently regresstest_nonlocal_detector_golden_regression(max posterior diff 0.97 vs 1e-6 tolerance). Will ship as a follow-up release once clusterless_kde_log: is_local=True branch produces NaNs on golden NonLocal fixture #32 is fixed. Both algorithm keys remain available;clusterless_kdecontinues to be the default.Test plan
uv run ruff check src/anduv run ruff format --check src/cleanuv run pytest src/non_local_detector/tests/test_golden_regression.py -v— 4 pass, no diffsuv run pytest -m snapshot -v— 8 pass, no diffsuv run pytest src/non_local_detector/tests/test_discrete_state_transitions.py src/non_local_detector/tests/test_discrete_transitions_estimation.py— 70 passuv run pytest src/non_local_detector/tests/test_simulator_contract.py src/non_local_detector/tests/test_simulator_properties.py— 21 passgrep "estimate_joint_distribution|multinomial_neg_log_likelihood|set_initial_discrete_transition|class EncodingModel" src/non_local_detector/ --include="*.py"returns zero matchesnotebooks/02_likelihood_models/weighted_place_fields.ipynbimport updated tofrom non_local_detector.simulate import …; new imports verified to resolve and runStacked on #30 (Phase 2 EM convergence surfacing).
🤖 Generated with Claude Code