Fix remove_artifacts sparsity handling for median/average modes (#3290)#4685
Open
Leonard013 wants to merge 1 commit into
Open
Fix remove_artifacts sparsity handling for median/average modes (#3290)#4685Leonard013 wants to merge 1 commit into
Leonard013 wants to merge 1 commit into
Conversation
When `sparsity` is provided with mode "median" or "average", `RemoveArtifactsRecordingSegment.get_traces` sparsified the traces to the masked channels but kept the artifact template full-channel. The np.dot and the subtraction then operated on mismatched channel dimensions and raised `ValueError: shapes ... not aligned`. Restrict the template to the sparse channels at use time so it aligns with the sparsified traces. The stored template (and `_kwargs`) stay full-channel, so serialization round-trips remain correct. The no-sparsity path is unchanged, and both artifact-provisioning paths (precomputed `artifacts` and templates computed via `estimate_templates`) are covered. Add a regression test exercising median and average modes with sparsity and multiple labels. Fixes SpikeInterface#3290 This change was authored with the assistance of Claude (Anthropic). Co-authored-by: Claude <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.
Fixes #3290
Why
Using
remove_artifacts(..., mode="median"|"average", artifacts=..., sparsity=...)and then calling
get_traces()raises:In
RemoveArtifactsRecordingSegment.get_traces(median/average branch), when asparsitymask is given the traces are sparsified to the masked channels:but the artifact template
self.artifacts[label]stays full-channel. Thesubsequent
np.dot(trace_slice_values.flatten(), artifact_slice_values.flatten())(and the later subtraction
traces[trace_slice][:, mask] -= ... artifact ...)therefore operate on mismatched channel counts. The
sparsityblock in__init__only asserts that every label has a mask; it never restricts the template.
What
Restrict the template to the sparse channels at use time, right where the
traces are sparsified:
and use this local
artifactfor the duration, the dot product, and thesubtraction.
Design notes:
self.artifacts[label]isfull-channel whether the template was passed in via
artifacts=or computed byestimate_templates, so slicing at use time handles both uniformly._kwargs["artifacts"]remain full-channel and
sparsityremains full masks, soto_dict()/load()round-trips reproduce identical traces (verified). Storing a pre-sliced template
in
_kwargsinstead would double-slice on reload —__init__would try to indexa full-length mask into an already-1-channel template (
IndexError) — which iswhy the slice is applied at use time, not stored.
mask is Nonethe template is used as-is.Tests
Added
test_remove_artifacts_sparsitycoveringmode="median"andmode="average"with asparsitymask and two labels (each masking a differentchannel subset). It asserts that:
get_traces()no longer raises,(
np.array_equal),not np.allclose).Before the fix this test fails at the
np.dotwithValueError: shapes (1200,) and (2400,) not aligned(600 samples × 2 sparsechannels vs 600 × 4 full channels); after the fix the whole file passes (
2 passed).I also verified (manually) the
to_dict()→load()round-trip reproduces identicaltraces, and the precomputed-
artifactspath with sparsity — happy to add either as acommitted test if you'd like it locked in.
Note (out of scope)
The exact minimal snippet in the issue supplies a 30-sample artifact while the
default
ms_before/ms_afterimply a 105-sample window, so it additionally hits aduration mismatch (
105 vs 30) that is independent of sparsity. This PR fixes thesparsity/channel-alignment bug (the subject of #3290); after the fix the channel
dimensions match (the error, if a mis-sized template is passed, reduces to the
pre-existing duration check).
Prepared with AI assistance (Claude Code): the reproduction, root-cause analysis,
fix, and test were AI-drafted; the reasoning and verification (including the rejected
store-pre-sliced approach that double-slices on reload) are documented above.