Warn on degenerate-KDE NaN density and large off-grid bin snaps#35
Open
edeno wants to merge 5 commits into
Open
Warn on degenerate-KDE NaN density and large off-grid bin snaps#35edeno wants to merge 5 commits into
edeno wants to merge 5 commits into
Conversation
edeno
added a commit
that referenced
this pull request
May 28, 2026
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>
Contributor
Author
|
Addressed the multi-agent review (commit 3d46b57):
Golden (4) + snapshot (8) still pass; no numerical change. The lone broad-suite failure remains the pre-existing flaky |
6c7c59c to
30bae04
Compare
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>
3d46b57 to
b5b9854
Compare
Follow-up to the PR #35 review (post-rebase onto main). Doc accuracy (the review's top finding): - The distance1D "edge-leak fix" was mis-described. Every edge `_setup_track_graph` adds is incident to a temporary node, so the old per-step `remove_node` cleanup already fully restored the graph and shortest-path distances are byte-identical before and after. The "node_ahead<->node_behind shortcut" mechanism never existed. Reworded the code comment, the TestSetupTrackGraphIsolation docstring, and the CHANGELOG entry (moved Fixed -> Changed, dropped the `_get_ahead_behind_distance` typo) to describe it accurately as a defensive isolation refactor. No code change to the graph logic. Snap-warning recalibration (test-first): - `Environment.get_bin_ind`'s warning threshold was `2 x place_bin_size`, unrelated to the inter-arm gap width, so routine into-gap snaps on multi-arm linearized tracks (~edge_spacing wide) cried wolf. Added `_max_inter_arm_gap()` and made the threshold `2 x place_bin_size + max(edge_spacing)`; open-field grids (no track graph) are unchanged. Rewrote test_off_grid_far_snap_warns to a genuine far snap, added test_into_gap_snap_does_not_warn plus two threshold unit tests. Warning message updated to match. Golden/snapshot byte-identical (warning only). Test/CHANGELOG gaps: - Added a predict-time (is_local) KDE NaN-density warning test, a make_fragmented_replay unseeded-RNG warning test, a seeded multiunit no-warning assertion, and return_clamp_count custom-`condition` tests for safe_log/safe_divide. Tightened NaN/snap warning matches to assert the reported count. CHANGELOG now lists the predict-side KDE warning and the third simulator. Verification: ruff clean; 103 env/analysis/kde/simulator tests, 4 golden, 8 snapshot all pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Scope reduction: drop the speculative / low-value pieces and keep only the two changes that prevent or flag a wrong scientific result. Removed (reverted to main exactly): - `safe_log` / `safe_divide` `return_clamp_count`: an opt-in primitive no production caller uses (the consumer was explicitly deferred). YAGNI — it should land in the PR that consumes it. - Simulator unseeded-RNG `UserWarning`s (`simulate_poisson_spikes`, `simulate_multiunit_with_place_fields`, `make_fragmented_replay`): a reproducibility nudge that leans toward noise; some runs are intentionally unseeded. - `_setup_track_graph` per-call-copy "edge-leak fix": established to be a no-op refactor (every added edge was incident to a temporary node, so the old per-step cleanup already restored the graph; distances byte-identical). Pure churn, dropped. Kept: - `likelihoods.sorted_spikes_kde`: count NaN KDE marginal-density bins and warn instead of silently zeroing them (fit + local-predict). NaN density means a degenerate KDE that otherwise yields a plausible-but-wrong place field. - `Environment.get_bin_ind`: warn on large off-grid snaps, with the threshold recalibrated to `2*place_bin_size + max(edge_spacing)` so routine into-gap snaps on multi-arm tracks don't cry wolf. PR diff vs main is now 5 files (2 features + their tests + CHANGELOG). Verification: ruff clean; 91 env/kde/analysis/simulator tests, 4 golden, 8 snapshot all pass. 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
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
Surfaces two previously-silent degeneracies as warnings, without changing any well-behaved numerics. Scope was trimmed from the original Phase-5 work down to the two changes that actually flag (or prevent) a wrong scientific result.
Changed
likelihoods.sorted_spikes_kde—fit_sorted_spikes_kde_encoding_modelandpredict_sorted_spikes_kde_log_likelihood(local branch) previously replaced NaN KDE marginal-density bins with zero silently. They now count those bins and emit aUserWarning. NaN density usually means a degenerate KDE (zero-variance / identical spike features, orposition_std=0) that otherwise yields a plausible-but-wrong place field. The zeroing behavior itself is preserved — no numerical change for well-behaved inputs.Added
Environment.get_bin_indnow emits aUserWarningwhen off-grid positions are snapped to the nearest interior bin and the max snap distance exceeds2 × place_bin_size + max(edge_spacing)(a likely tracking glitch or out-of-bounds position). The inter-arm-gap term keeps routine into-gap snaps on multi-arm linearized tracks from crying wolf; open-field grids have no gap term. The snap behavior itself is unchanged.Trimmed from the original PR
These pieces were dropped as speculative / low-value (reverted to
mainexactly):safe_log/safe_dividereturn_clamp_count— an opt-in primitive with no production consumer (the consumer was explicitly deferred). YAGNI; it should land in the PR that actually uses it.simulate_poisson_spikes,simulate_multiunit_with_place_fields,make_fragmented_replay) — a reproducibility nudge that leans toward noise; some runs are intentionally unseeded._setup_track_graphper-call-copy "edge-leak fix" — on review this was found to be a no-op refactor: every temporary edge the function adds is incident to a temporary node, so the old per-stepremove_nodecleanup already fully restored the graph and shortest-path distances are byte-identical before and after. Dropped as churn.Test plan
get_bin_indsnap (far snap warns, into-gap snap stays quiet, threshold includesedge_spacing).uv run pytest -m snapshot— 8 pass, no diffs.uv run pytest test_golden_regression.py— 4 pass, no diffs.ruff check/ruff format --check— clean.Notes
main(PRs Surface silent convergence and optimizer failures during fitting #30 / Remove duplicate code paths in transitions, GMM, simulators #33 / Restore hashability and sklearn.clone() contract on detectors #34 already merged); conflicts resolved.edges_.🤖 Generated with Claude Code