Docstring + scaffolding hygiene; delete abandoned regression_report#36
Open
edeno wants to merge 16 commits into
Open
Docstring + scaffolding hygiene; delete abandoned regression_report#36edeno wants to merge 16 commits into
edeno wants to merge 16 commits into
Conversation
…erior 2D
Three independently-revertible bug fixes plus their behavioral tests.
- analysis.distance1D.get_map_speed: trailing boundary speed was
inserted before the last array element instead of appended; for
every chunk with three or more time bins, the final two speed
samples were misordered. Switch np.insert(arr, -1, val) to
np.append(arr, val).
- models.base._DetectorBase.estimate_parameters: post-EM cleanup of
the encoding-model data did nothing because the hasattr check used
trailing-underscore form while write sites use leading-underscore.
Spike-time and waveform-feature arrays were retained for the
lifetime of every fitted model.
- models.cont_frag_model.ContFrag{SortedSpikes,Clusterless}Classifier
.get_posterior: raised for 2D environments because it summed over
the dim name "position" which only exists for 1D. Collapse every
dim whose name matches "position" or ends with "_position",
preserving the @staticmethod decorator and the singular "state"
output dim name.
Tests:
- Parameterized boundary-handling tests for get_map_speed across
n_time in {3, 4, 5, 100}, plus a regression test that distinguishes
the np.append output from the buggy np.insert(-1) output.
- Assertion that _encoding_model_data is no longer present after
estimate_parameters completes.
- 2D and 1D get_posterior tests for both ContFrag classifiers.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Seven small correctness fixes plus tests; one docstring clarification;
one CHANGELOG file.
- environment.Environment.fit_place_grid: tuple place_bin_size with a
track_graph previously dereferenced a dead local and crashed inside
np.linspace. Raises ValidationError with example.
- environment: remove order_boundary and get_track_boundary_points
(both called the removed nx.from_scipy_sparse_matrix, both unused
outside the module). Drop the now-unused NearestNeighbors import.
- continuous_state_transitions.RandomWalk._handle_no_track_graph:
direction-aware mode on environments without an N-D track graph
previously crashed inside networkx; raises ConfigurationError with
a hint pointing to the two valid configurations.
- visualization.figurl_2D.process_decoded_data: linearized bin indices
stored as uint16 silently wrapped for grids larger than 65535 cells
(e.g., 256x256). Raise ValueError at the boundary; keep the uint16
dtype because the downstream sortingview schema receives the field
as uint16.
- simulate.clusterless_simulation: replace both hardcoded
mark_spacing=10 occurrences with the module-level MARK_SPACING
constant. Encoding marks previously landed at {0,10,20,30} while
replay generators used {0,5,10,15}, silently invalidating any test
mixing the two.
- model_checking.highest_posterior_density.get_HPD_spatial_coverage:
docstring advertised 2D support but the implementation only used a
1D bin width. Branch on dim names and use the 2D cell area
(bin_width_x * bin_width_y) when x_position and y_position are
present. Signature preserved.
- model_checking.posterior_consistency.posterior_consistency_hpd_overlap:
docstring expanded to flag the asymmetric containment-coefficient
semantics (|A intersect B| / min(|A|,|B|)) and point readers at
Sorensen-Dice / Jaccard for symmetric overlap. No code change.
Tests cover each fix in isolation. The figurl overflow tests use
pytest.importorskip("sortingview"); the mark-spacing test asserts
exact replay-side spacing and bounds encoding-side spread below the
pre-fix value (encoding marks have additive Gaussian noise, so the
literal grid is not directly observable).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two paired optimizer-surfacing fixes that make previously-silent fitting failures visible to the user. - likelihoods.sorted_spikes_glm.fit_poisson_regression: BFGS result.success was discarded after every call. Place-field coefficients were silently set to wherever the optimizer happened to be when it gave up. Emit UserWarning with the BFGS message, iteration count, and final loss when not res.success. - likelihoods.gmm.GaussianMixtureModel: converged_ was set as the heuristic n_iter < max_iter, ignoring the real lower-bound delta convergence flag returned by _em_fit_while_loop. The plumbing discarded the flag at _fit_single. Thread the real converged through _fit_single's return tuple (now 6 elements) up to fit; emit UserWarning at end of fit when not converged. Initialize converged_=False and n_iter_=0 before the n_init loop so the warning machinery is robust to the all-inits-fail case. Tests for each warning path: monkeypatched BFGS-failure for the GLM (real non-convergence depends on SciPy version and design conditioning, so a deterministic monkeypatch is the right shape for this single-call site); max_iter=1 + tol=1e-20 for the GMM. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Make previously-silent _DetectorBase EM failures visible. - Capture both return values of check_converged. When is_increasing is False, log a warning describing iteration, before/after log-likelihoods, and change magnitude; append the iteration to a new em_monotonicity_violations_ list (initialized empty before the loop so re-fits reset cleanly). - After the EM while loop exits, set converged_ and n_iter_ unconditionally. When not converged_, emit a UserWarning with max_iter, the final log-likelihood change, and the tolerance. - The final E-step now checks that the post-M-step log-likelihood did not decrease by more than tolerance; a UserWarning flags inconsistencies between the E-step and the M-step output that the prior code would have silently propagated through the returned posterior. - Document the three new attributes on _DetectorBase (inherited by every detector subclass): converged_, n_iter_, em_monotonicity_violations_. Tests: monkeypatched check_converged to force a violation on iteration 2; max_iter=1, tolerance=1e-20 to force a max-iter exit; well-behaved-fit-emits-no-warning negative case using warnings.catch_warnings(record=True) filtered by message substring (tolerant of Groups 2a/2b warnings unrelated to EM monotonicity). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When every state has -inf log-likelihood at a timestep, _condition_on previously returned an invalid all-zero posterior (probs * exp(-inf) = 0, then divided by eps in _normalize) and propagated log_norm=-inf silently through the scan. Replace with a fall-back-to-predicted policy plus a host-side diagnostic count. - _condition_on now uses jnp.where(ll_is_finite, ...) to return new_probs_normal on the normal path and probs unchanged on the all-impossible path, with log_norm=-inf marking the degenerate step in the marginal log-likelihood. Numerically byte-identical to the prior implementation on well-behaved inputs (verified by hand-derivation test plus golden regression). - Add _count_degenerate_timesteps and _warn_if_degenerate_timesteps helpers in core.py. Each public filter (filter, filter_covariate_dependent, chunked_filter_smoother, chunked_filter_smoother_covariate_dependent) now emits a single logger.warning per invocation when any degenerate timesteps are detected. The chunked drivers aggregate the count across chunks and warn once after the forward pass. - logger.warning (not warnings.warn) because the filter runs many times per fit and warnings.warn dedup would suppress legitimate signal across iterations; logger.warning integrates with the existing per-module logger configuration. Tests cover: all-inf input falls back to predicted (probs and log_norm); partial-inf still normalizes correctly; well-behaved input matches legacy normalization byte-for-byte; filter on a 3-timestep input with middle timestep all-inf emits the warning and returns predicted-distribution posterior at that step. CHANGELOG.md merges Phase 1 Changed entries with Phase 2's new Added/Changed bullets and notes the GLM EM convergence behavior surfaced by these changes as a follow-up investigation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
check_converged's is_increasing return value used a hardcoded -1e-3 slack, while the EM final-E-step consistency warning compares the change against the caller-supplied tolerance. For tight tolerances (<<1e-3) the final-E-step warning would fire while no in-loop warning did; for loose tolerances the in-loop check would warn at decreases the final-E-step check considered noise. Thread tolerance into the is_increasing comparison so both checks use the same slack. The default tolerance=1e-4 makes is_increasing slightly stricter than the prior -1e-3 (decreases of 1e-4 to 1e-3 that were previously treated as noise now flag). This is the intent of the surrounding Phase 2 work — surface previously-silent inconsistencies, not normalize them away. CHANGELOG points at issue #31 for the open follow-up on the GLM EM M-step (which uses the Local-only marginal as weights for an emission shared across states, breaking EM monotonicity in a way that this tolerance change does not address). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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>
Both Environment and ObservationModel previously overrode __eq__ in ways that silently subverted correctness, and lost hashability as a side effect (a dataclass with a custom __eq__ but eq=True sets __hash__ = None). - Environment.__eq__(self, other: str) returned the result of self.environment_name == other. This made env_a == env_b return False whenever they had the same name (because the right operand was treated as a string), broke symmetry of ==, and made Environment unhashable. The override existed only to allow environments.index(name) to find an env by string name; replace every such call site with a new find_environment_by_name helper. Restore identity-based __hash__ (the post-fit numpy / networkx attributes carried by Environment have no meaningful value-hash). - ObservationModel.__eq__ compared only (environment_name, encoding_group), silently merging distinct decoding states (is_local / is_no_spike) in set / np.unique / equality checks. This was inconsistent with the @DataClass(order=True) auto-__lt__ which always used all four fields. Remove the override; add unsafe_hash=True so all four fields participate in the hash (all are hashable primitives). A user-visible consequence in models.base: np.unique over a list of observation_models now treats (env, group, is_local=True) and (env, group, is_local=False) as distinct, fitting one encoding model per entry instead of collapsing. Default model configurations don't trigger this; a hand-built list that mixed Local and Non-Local observation models sharing (env, group) would. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two paired _DetectorBase refactors that bring detector internals in line with sklearn's BaseEstimator conventions, fixing sklearn.clone() round-tripping without losing the mutable-default isolation guarantees. (a) algorithm_params: lazy resolution instead of __init__-time copy. clusterless_algorithm_params and sorted_spikes_algorithm_params now default to None on every detector constructor and are stored as-is. sklearn.clone() requires constructors to store params unchanged; the previous defensive dict-copy at __init__ time violated that contract. The dict copy + default resolution moves into two new helper methods on ClusterlessDetector and SortedSpikesDetector: _resolve_clusterless_algorithm_params() and _resolve_sorted_spikes_algorithm_params(). Each returns a fresh dict at every call, so the mutable-default-kwarg sharing bug is still fixed: two detectors using defaults never alias the same dict. The two fit sites (clusterless and sorted-spikes encoding fits) now call the helpers. User-visible side effect: a user-supplied params dict is no longer defensively copied at construction. d.clusterless_algorithm_params IS the user's dict (sklearn convention). Mutations the user makes between construction and fit will propagate. To get isolated state, pass a fresh dict (dict(my_params)). Documented in CHANGELOG and in each constructor's docstring. (b) discrete_initial_conditions: fitted attribute, not param mutation. The EM loop previously overwrote self.discrete_initial_conditions (the user's constructor argument) with acausal_state_probabilities[0] on every iteration. This violated sklearn's contract that get_params() reflects the user's spec, and broke sklearn.clone(fitted_model). The fitted distribution now lives on self.discrete_initial_conditions_ (trailing-underscore convention). Initialized at the start of estimate_parameters so readers see the fitted form post-fit. Documented in the _DetectorBase Attributes docstring. Tests cover both refactors: TestAlgorithmParamsResolution (10 tests) verifies the resolver contract (defaults are None, resolver returns fresh copies, sklearn.clone() round-trips for both default and user-supplied params); test_em_predict_consistency adds three tests (constructor-arg preservation, fitted-attribute existence, sklearn.clone(fitted_model) round-trip). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Make routinely-silent degeneracies in the likelihood, environment, and simulator paths visible without changing well-behaved numerics. - likelihoods.common.safe_log / safe_divide gain an opt-in return_clamp_count=True that also returns how many entries hit the eps floor. Default single-value return is unchanged (back-compat). - likelihoods.sorted_spikes_kde.fit_sorted_spikes_kde_encoding_model previously replaced NaN KDE marginal-density bins with zero silently. It now counts those bins and emits a UserWarning (NaN density signals a degenerate KDE: zero-variance/identical spike features or position_std=0). The zeroing is preserved; no numerical change. The count is NOT added to the returned encoding-model dict because that dict is splatted as **encoding_model into the predict functions, which would reject an unexpected 'diagnostics' kwarg. - Environment.get_bin_ind warns when off-grid positions snap to the nearest interior bin and the max snap distance exceeds 2 x place_bin_size (likely a tracking glitch). New helper _snap_warn_threshold. Snap behavior itself is unchanged. - simulate.simulate_poisson_spikes and simulate.simulate_multiunit_with_place_fields warn when called with neither seed nor rng (OS-entropy, non-reproducible). Pass seed= or rng= to silence. Tests cover each surfacing path. Broader per-fit clamp aggregation (planned for clusterless_kde / gmm / glm) is deferred: those fits call safe_log inside JAX scans where threading the count out cleanly needs deeper surgery. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
_setup_track_graph added temporary nodes/edges directly to the passed-in graph. In the same-edge branch it adds a node_ahead <-> node_behind shortcut whose endpoints are real track nodes that the caller never removes (only the actual_position/head/mental_position temp nodes were deleted). _get_ahead_behind_distance reused a single copy across its whole time loop, so that shortcut persisted between time steps and shortened later shortest-path distances. Make _setup_track_graph operate on a fresh per-call copy and return it; drop the caller's shared-copy + manual node-removal cleanup. Each time step is now isolated. Regression tests assert the input graph is not mutated and that a prior call on a different edge pair does not change the mental-position distance computed by a later call. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Acting on the multi-agent PR review of #35: - sorted_spikes_kde: the local-likelihood *decode* path had a second silent NaN-density zeroing site that the fit-path warning did not cover (a user decoding a pre-fit degenerate model got no signal). Count and warn there too. - simulate.make_fragmented_replay had the same unseeded-RNG pattern as the two functions instrumented earlier but was missed; add the reproducibility warning. - Drop test_setup_track_graph_no_cross_call_leak: the review showed it passed on the un-fixed code (the path-graph topology never leaks a load-bearing edge), giving false safety. Replace with test_repeated_calls_do_not_accumulate_temp_nodes, which genuinely fails pre-fix (the old in-place mutation accumulated temp nodes across calls). The existing does-not-mutate-input test already guards the core fix. - _snap_warn_threshold: accept list as well as tuple place_bin_size so the diagnostic path can't itself raise TypeError. - Soften the get_bin_ind snap warning message (a large inter-arm gap on a multi-arm track is routine, not necessarily a glitch) and document in the docstring that out-of-bounds positions in an all-interior environment are not covered by the warning (they are edge-clamped without entering the snap path); full detection is a follow-up. - Add the two missing coverage tests the review flagged: the 2D anisotropic place_bin_size threshold (2 x max, not min) and the make_multiunit unseeded warning. Golden (4) + snapshot (8) still pass; no numerical change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Documentation-quality pass (no runtime behavior change) plus one dead-file deletion. - Strip internal scaffolding from shipped code: the "Tier 1/2/3" comments and "issue #26" references in models/base.py, and the "issue #26" comments/docstrings in two test files. Comments now describe code intent, not ticket numbers (CLAUDE.md forbids scaffolding refs in shipped artifacts). - Fix core.filter_covariate_dependent log_likelihoods shape annotation: (n_time, n_states) -> (n_time, n_state_bins). Verified against the function body (initial_distribution, continuous_transition_matrix, and outputs are all n_state_bins); the plain filter/viterbi are generic-n_states and left unchanged. - Fix no_spike.predict_no_spike_log_likelihood docstring to match the shipped implementation: `time` is a per-bin axis (not bin edges) and the output is (len(time), 1). Updated the examples accordingly. - Add the previously-undocumented discrete_transition_prior_weight and frozen_discrete_transition_rows constructor params to the public docstrings of SortedSpikesDecoder, ClusterlessDecoder, the Cont/Frag classifiers, and the MultiEnvironment classifiers (only NonLocal*Detector documented them before). - Tighten annotations: RandomWalkDirection.encoding_group str -> str | int (matches the default 0 and ObservationModel); figurl_2D static_track_animation any -> typing.Any. - Delete tests/regression_report.py: abandoned scaffolding with a placeholder parse and no callers. Lint clean; 877 tests collect; snapshot (8) and golden (4) pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…CHANGELOG names Acting on the comment-analyzer + code-reviewer review of #36: - Correct the new discrete_transition_prior_weight docs on the three detector classes. They wrongly said positive weight "pulls the estimate toward the initial transition matrix"; the M-step prior is a data-adaptive Dirichlet (concentration/stickiness), not the initial matrix. Use the accurate _DetectorBase/non_local_model phrasing ("data-adaptive Dirichlet prior scaling"). - no_spike.predict_no_spike_log_likelihood: the function annotation still read list[list[float]] after the docstring was fixed to list[np.ndarray]; update the annotation to match (the impl boolean-mask-indexes each neuron's spikes, requiring ndarray). - Soften two base.py validation comments to match what the validators actually check ("arrays and finite", "array, finite, monotonically increasing") rather than overstating dtype/1D assertions. - Fix two CHANGELOG naming errors: the touched class is EmpiricalMovement (not RandomWalkDirection, which doesn't exist), and the deleted placeholder was parse_pytest_summary (not parse_pytest_output). Lint clean; docs-only, no runtime change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Addressed the review (latest commit):
Confirmed correct by both reviewers: the |
This was referenced May 28, 2026
Open
edeno
force-pushed
the
silent-floor-diagnostics
branch
from
3d46b57 to
b5b9854
Compare
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
Phase 6 — documentation-quality pass (no runtime behavior change) plus one dead-file deletion.
Changed (docs / annotations)
models/base.py, and "issue Need to run predict after last EM step #26" comments/docstrings in two test files.grep "Tier [123]|Claude Code|issue #" src/is now empty.core.filter_covariate_dependentlog_likelihoodsshape annotation(n_time, n_states)→(n_time, n_state_bins). Verified against the function body; plainfilter/viterbi(genericn_states, internally consistent) left unchanged.no_spike.predict_no_spike_log_likelihooddocstring to match the shipped implementation (timeis per-bin, output(len(time), 1)).discrete_transition_prior_weight/frozen_discrete_transition_rowsparam docs toSortedSpikesDecoder,ClusterlessDecoder, the Cont/Frag classifiers, and the MultiEnvironment classifiers.RandomWalkDirection.encoding_groupannotationstr→str | int;figurl_2Dstatic_track_animationany→typing.Any.Removed
tests/regression_report.py: abandoned scaffolding (placeholderparse_pytest_outputreturning hardcoded zeros, no callers, docstrings referencing an external "Claude Code workflow" that doesn't ship with the library).Stacked on
PR #35 (
silent-floor-diagnostics, Phase 5).Test plan
ruff check/ruff format --checkclean.pytest --cocollects 877 tests (no orphaned import of the deleted file).pytest -m snapshot— 8 pass, no diffs.pytest test_golden_regression.py— 4 pass, no diffs.no_spikeoutput shape verified(100, 1)matching the corrected docstring.Note: this PR only touches docstrings, comments, two type annotations, and deletes a dead test helper — no numerical or control-flow change.
🤖 Generated with Claude Code