Skip to content

Fix test_pipeline_equiv_to_step#4675

Open
grahamfindlay wants to merge 2 commits into
SpikeInterface:mainfrom
grahamfindlay:fix/preprocessor-test-determinism
Open

Fix test_pipeline_equiv_to_step#4675
grahamfindlay wants to merge 2 commits into
SpikeInterface:mainfrom
grahamfindlay:fix/preprocessor-test-determinism

Conversation

@grahamfindlay

@grahamfindlay grahamfindlay commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

test_pipeline_equiv_to_step builds each preprocessor twice and tests the results for equivalence, but some nondeterministic preprocessors weren't being seeded, or the seeds weren't being forwarded to where they were needed. This was causing checks to fail on totally unrelated PRs/CI runs (e.g., #4620).

Fix 1A: DetectAndRemoveArtifactsRecording accepted a seed but didn't forward it to the "envelope" method's chunk subsampling. The docstring's description of seed handling was also wrong.

Fix 1B (surfaced in the process of testing & fixing 1A): the envelope detector emits threshold crossings as a structured array with named fields "sample_index", "segment_index", and "front". "front" is a bool indicating whether the crossing is a rising edge or a falling edge of the presumed artifact. _transform_internal_dtype_to_artifact_dtype converts the crossings into (start, end) pairs. But if a segment ended while an artifact was still ongoing (a trailing rising edge with no matching falling edge), it makes up a closing falling edge at the last sample of the segment so the pairs are complete. But to do this, it was allocating a single record with the wrong dtype (no "sample_index"). Then trying to write to it, raising a ValueError. Just needed to use the right dtype.

Fix 2: Seeded detect_and_remove_bad_channels,
detect_and_interpolate_bad_channels, detect_and_remove_artifacts in test_pipeline_equiv_to_step.

Fix 3A: silence_periods in theory is vulnerable to the same issues, if it were tested with mode="noise", so I fixed it too: DetectArtifactAndSilentPeriodsRecording also needed to forward seed to the envelope method. And SilencedPeriodsRecordings needed its docstring's description of seed behavior fixed.

Fix 3B: While here, I found that the DetectArtifactAndSilentPeriodsRecording wasn't even constructing because it referenced an undefined "artifacts" that seems like it was replaced by "periods" at some point. I guess this raises the question: if nobody noticed, is the class used/needed?

grahamfindlay and others added 2 commits July 9, 2026 15:24
`test_pipeline_equiv_to_step` builds each preprocessor twice and tests
the results for equivalence, but some nondeterministic preprocessors
weren't being seeded, or the seeds weren't being forwarded to where they
were needed. This was causing checks to fail on totally unrelated PRs/CI
runs.

Fix 1A: `DetectAndRemoveArtifactsRecording` accepted a seed but didn't
forward it to the "envelope" method's chunk subsampling. The docstring's
description of seed handling was also wrong.

Fix 1B (surfaced in the process of testing & fixing 1A): the envelope
detector emits threshold crossings as a structured array with named fields
"sample_index", "segment_index", and "front". "front" is a `bool`
indicating whether the crossing is a rising edge or a falling edge of the
presumed artifact. `_transform_internal_dtype_to_artifact_dtype` converts
the crossings into `(start, end)` pairs. But if a segment ended while
an artifact was still ongoing (a trailing rising edge with no
matching falling edge), it makes up a closing falling edge at the last
sample of the segment so the pairs are complete. But to do this, it
was allocating a single record with the wrong dtype (no "sample_index").
Then trying to write to it, raising a `ValueError`. Just needed to use
the right dtype.

Fix 2: Seeded `detect_and_remove_bad_channels`,
`detect_and_interpolate_bad_channels`, `detect_and_remove_artifacts` in
`test_pipeline_equiv_to_step`.

Fix 3A: `silence_periods` in theory is vulnerable to the same issues, if
it were tested with `mode="noise"`, so I fixed it too:
`DetectArtifactAndSilentPeriodsRecording` also needed to forward
`seed` to the envelope method. And `SilencedPeriodsRecordings` needed
its docstring's description of seed behavior fixed.

Fix 3B: While here, I found that the `DetectArtifactAndSilentPeriodsRecording`
wasn't even constructing because it referenced an undefined "artifacts"
that seems like it was replaced by "periods" at some point. I guess this
raises the question: if nobody noticed, is the class used/needed?
@grahamfindlay

Copy link
Copy Markdown
Contributor Author

Side note: I would remove the seed keyword argument from detect_artifact_periods_by_envelope, since it is redundant with random_slices_kwargs, and having to reconcile them adds complexity. But I didn't want to subtract from the existing API.

@alejoe91 alejoe91 added the preprocessing Related to preprocessing module label Jul 10, 2026
@samuelgarcia

Copy link
Copy Markdown
Member

Thank you Graham for checking in detail all this.
We merged it to quickly last time.

@chrishalcrow

chrishalcrow commented Jul 11, 2026

Copy link
Copy Markdown
Member

Thanks @grahamfindlay

DetectArtifactAndSilentPeriods was added to main 3 months ago, as part of a refactoring of the artifacts-related steps. So I'm not too surprised no one has noticed this. Also, we are not exposing it (d'oh). Any chance you could add this? You need to add a couple of lines here:

https://github.com/SpikeInterface/spikeinterface/blob/main/src/spikeinterface/preprocessing/preprocessing_classes.py

and then make sure the name of the function and the name argument match here:

detect_artifacts_and_silent_periods = define_function_handling_dict_from_class(

If we had exposed it, it would have been included in some generic preprocessing tests and we would have caught the no-construction bug. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

preprocessing Related to preprocessing module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants