From c836f74c2e6d6acfa04a871cc8ad86908b930237 Mon Sep 17 00:00:00 2001 From: Graham Findlay Date: Thu, 9 Jul 2026 15:24:29 -0500 Subject: [PATCH 1/4] Fix `test_pipeline_equiv_to_step` `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? --- .../preprocessing/detect_artifacts.py | 29 ++++++++++++------- .../preprocessing/silence_periods.py | 19 +++++++----- .../preprocessing/tests/test_pipeline.py | 11 ++++++- 3 files changed, 41 insertions(+), 18 deletions(-) diff --git a/src/spikeinterface/preprocessing/detect_artifacts.py b/src/spikeinterface/preprocessing/detect_artifacts.py index 49af692996..95f286a85a 100644 --- a/src/spikeinterface/preprocessing/detect_artifacts.py +++ b/src/spikeinterface/preprocessing/detect_artifacts.py @@ -442,14 +442,14 @@ def compute( Cut-off frequency (Hz) for the Gaussian low-pass filter applied to the rectified signal when building the envelope. seed : int | None, default: None - Random seed forwarded to :func:`~spikeinterface.core.get_noise_levels`. - If ``None``, ``get_noise_levels`` uses ``seed=0``. + Random seed forwarded to :func:`~spikeinterface.core.get_random_data_chunks`. job_kwargs : dict | None, default: None Keyword arguments forwarded to :func:`run_node_pipeline` (e.g. ``n_jobs``, ``chunk_duration``). random_slices_kwargs : dict | None, default: None Additional keyword arguments forwarded to the ``random_slices_kwargs`` - argument of :func:`~spikeinterface.core.get_noise_levels`.""" + argument of :func:`~spikeinterface.core.get_random_data_chunks`. Any ``"seed"`` + key in this dictionary is overridden by the ``seed`` argument of this function.""" def detect_artifact_periods_by_envelope( @@ -584,13 +584,15 @@ def _transform_internal_dtype_to_artifact_dtype( sub_thr = artifacts[mask] if len(sub_thr) > 0: if not sub_thr["front"][0]: - local_thr = np.zeros(1, dtype=np.dtype(base_period_dtype + [("front", "bool")])) + local_thr = np.zeros(1, dtype=sub_thr.dtype) local_thr["sample_index"] = 0 + local_thr["segment_index"] = seg_index local_thr["front"] = True sub_thr = np.hstack((local_thr, sub_thr)) if sub_thr["front"][-1]: - local_thr = np.zeros(1, dtype=np.dtype(base_period_dtype + [("front", "bool")])) + local_thr = np.zeros(1, dtype=sub_thr.dtype) local_thr["sample_index"] = recording.get_num_samples(seg_index) + local_thr["segment_index"] = seg_index local_thr["front"] = False sub_thr = np.hstack((sub_thr, local_thr)) @@ -710,9 +712,8 @@ class DetectAndRemoveArtifactsRecording(SilencedPeriodsRecording): Keyword arguments forwarded to :func:`run_node_pipeline` (e.g. ``n_jobs``, ``chunk_duration``). noise_levels_kwargs : dict | None, default: None - Keyword arguments for `spikeinterface.core.get_noise_levels()` function. - - If none, `get_noise_levels` uses `seed=0` and `NoiseGeneratorRecording` generates a random seed using `numpy.random.default_rng`. + Keyword arguments for `spikeinterface.core.get_noise_levels()`, used only + when ``mode="noise"``. mode : "zeros" | "noise" | "apodization", default: "zeros" Determines what periods are replaced by. Can be one of the following: @@ -725,7 +726,9 @@ class DetectAndRemoveArtifactsRecording(SilencedPeriodsRecording): apodization_samples : int, default: 7 The factor used for the cosine taper when mode is "apodization". Higher values create a wider taper. seed : int | None, default: None - Random seed for `get_noise_levels` and `NoiseGeneratorRecording`. + Random seed, if applicable. When ``method="envelope"``, used for chunk + subsampling. When ``mode="noise"``, for the noise used to fill the + silenced periods. artifact_periods : np.ndarray | None, default: None Optionally, pre-computed artifact periods can be passed directly to the constructor to skip the detection step. If ``None``, artifact periods are detected on the fly using the specified method @@ -749,8 +752,14 @@ def __init__( else: if recording_to_detect is None: recording_to_detect = recording + detect_method_kwargs = dict(method_kwargs) if method_kwargs else {} + if method == "envelope": + detect_method_kwargs["seed"] = seed artifact_periods = detect_artifact_periods( - recording_to_detect, method=method, method_kwargs=method_kwargs, job_kwargs=job_kwargs + recording_to_detect, + method=method, + method_kwargs=detect_method_kwargs, + job_kwargs=job_kwargs, ) super().__init__( recording, diff --git a/src/spikeinterface/preprocessing/silence_periods.py b/src/spikeinterface/preprocessing/silence_periods.py index 1056488c76..d1c0bcded9 100644 --- a/src/spikeinterface/preprocessing/silence_periods.py +++ b/src/spikeinterface/preprocessing/silence_periods.py @@ -41,8 +41,10 @@ class SilencedPeriodsRecording(BasePreprocessor): noise_levels : array Noise levels if already computed seed : int | None, default: None - Random seed for `get_noise_levels` and `NoiseGeneratorRecording`. - If none, `get_noise_levels` uses `seed=0` and `NoiseGeneratorRecording` generates a random seed using `numpy.random.default_rng`. + Random seed for the noise fill (only used when ``mode="noise"``): both the + per-channel noise-level estimation (`spikeinterface.core.get_noise_levels`) and + the generated noise. Takes precedence over ``noise_levels_kwargs["random_slices_kwargs"]["seed"]`` + if present. **noise_levels_kwargs : Keyword arguments for `spikeinterface.core.get_noise_levels()` function Returns @@ -258,7 +260,7 @@ class DetectArtifactAndSilentPeriodsRecording(SilencedPeriodsRecording): See SilencedPeriodsRecording and detect_artifact_periods for details. """ - _precomputable_kwarg_names = ["artifacts"] + _precomputable_kwarg_names = ["periods"] def __init__( self, @@ -272,18 +274,21 @@ def __init__( **noise_levels_kwargs, ): - if artifacts is None: + if periods is None: from spikeinterface.preprocessing import detect_artifact_periods - artifacts = detect_artifact_periods( + detect_method_kwargs = dict(detect_artifact_kwargs) if detect_artifact_kwargs else {} + if detect_artifact_method == "envelope": + detect_method_kwargs["seed"] = seed + periods = detect_artifact_periods( recording, method=detect_artifact_method, - method_kwargs=detect_artifact_kwargs, + method_kwargs=detect_method_kwargs, job_kwargs=None, ) SilencedPeriodsRecording.__init__( - self, recording, periods=artifacts, mode=mode, noise_levels=noise_levels, seed=seed, **noise_levels_kwargs + self, recording, periods=periods, mode=mode, noise_levels=noise_levels, seed=seed, **noise_levels_kwargs ) # note self._kwargs["periods"] is done by SilencedPeriodsRecording and so the computaion is done once diff --git a/src/spikeinterface/preprocessing/tests/test_pipeline.py b/src/spikeinterface/preprocessing/tests/test_pipeline.py index 6a96d4a66c..a33ef013d7 100644 --- a/src/spikeinterface/preprocessing/tests/test_pipeline.py +++ b/src/spikeinterface/preprocessing/tests/test_pipeline.py @@ -61,7 +61,16 @@ def test_pipeline_equiv_to_step(): pp_dict = {pp_name: {}} - if pp_name in ["normalize_by_quantile", "center", "zscore", "whiten"]: + if pp_name in [ + "normalize_by_quantile", + "center", + "zscore", + "whiten", + "detect_and_remove_bad_channels", + "detect_and_interpolate_bad_channels", + "detect_and_remove_artifacts", + ]: # These steps aren't deterministic (because they sample random chunks), + # so make sure that both pipeline and class get the same seed. pp_dict[pp_name] = {"seed": 1205} pp_rec_from_class = pp_class(rec, seed=1205) elif pp_name == "blank_saturation": From 6298bb2a1f4dc12ab7ad62443d41cc554eb2ec17 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 9 Jul 2026 21:15:49 +0000 Subject: [PATCH 2/4] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- src/spikeinterface/preprocessing/tests/test_pipeline.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/spikeinterface/preprocessing/tests/test_pipeline.py b/src/spikeinterface/preprocessing/tests/test_pipeline.py index a33ef013d7..37376781b7 100644 --- a/src/spikeinterface/preprocessing/tests/test_pipeline.py +++ b/src/spikeinterface/preprocessing/tests/test_pipeline.py @@ -69,7 +69,7 @@ def test_pipeline_equiv_to_step(): "detect_and_remove_bad_channels", "detect_and_interpolate_bad_channels", "detect_and_remove_artifacts", - ]: # These steps aren't deterministic (because they sample random chunks), + ]: # These steps aren't deterministic (because they sample random chunks), # so make sure that both pipeline and class get the same seed. pp_dict[pp_name] = {"seed": 1205} pp_rec_from_class = pp_class(rec, seed=1205) From f6be33f6f62280c637cf10b72a9f654aa5ebf88a Mon Sep 17 00:00:00 2001 From: Graham Findlay Date: Mon, 13 Jul 2026 11:11:32 -0500 Subject: [PATCH 3/4] Expose and test `detect_and_silence_artifacts` Note that `DetectAndSilenceArtifactsRecording` is totally redundant with `DetectAndRemoveArtifactsRecording(..., mode="zeros")`. In fact, `DetectAndRemoveArtifactsRecording(..., mode="zeros")` is better because it allows you to specify `job_kwargs` that get forwarded to detection, and it allows you to detect on a different recording than the one that gets zero'd (e.g. detect on raw, silence on filtered). As far as I can see, it's a total superset of the functionality offered by `DetectAndSilenceArtifactsRecording`, and perhaps the latter should be removed, rather than exposed. --- .../preprocessing/preprocessing_classes.py | 11 +++++++---- .../preprocessing/silence_periods.py | 15 +++++++-------- .../preprocessing/tests/test_pipeline.py | 1 + 3 files changed, 15 insertions(+), 12 deletions(-) diff --git a/src/spikeinterface/preprocessing/preprocessing_classes.py b/src/spikeinterface/preprocessing/preprocessing_classes.py index 6ada71de44..0b78aa590a 100644 --- a/src/spikeinterface/preprocessing/preprocessing_classes.py +++ b/src/spikeinterface/preprocessing/preprocessing_classes.py @@ -31,7 +31,12 @@ from .clip import BlankSaturationRecording, blank_saturation, ClipRecording, clip from .common_reference import CommonReferenceRecording, common_reference from .remove_artifacts import RemoveArtifactsRecording, remove_artifacts -from .silence_periods import SilencedPeriodsRecording, silence_periods +from .silence_periods import ( + SilencedPeriodsRecording, + silence_periods, + DetectAndSilenceArtifactsRecording, + detect_and_silence_artifacts, +) from .phase_shift import PhaseShiftRecording, phase_shift from .zero_channel_pad import ZeroChannelPaddedRecording, zero_channel_pad from .deepinterpolation import DeepInterpolatedRecording, deepinterpolate, train_deepinterpolation @@ -50,8 +55,6 @@ from .unsigned_to_signed import UnsignedToSignedRecording, unsigned_to_signed from .detect_artifacts import DetectAndRemoveArtifactsRecording, detect_and_remove_artifacts -# from .silence_artifacts import SilencedArtifactsRecording, silence_artifacts - _all_preprocesser_dict = { # filter stuff FilterRecording: filter, @@ -75,6 +78,7 @@ DetectAndInterpolateBadChannelsRecording: detect_and_interpolate_bad_channels, # artifact/saturation handling DetectAndRemoveArtifactsRecording: detect_and_remove_artifacts, + DetectAndSilenceArtifactsRecording: detect_and_silence_artifacts, # misc RectifyRecording: rectify, ClipRecording: clip, @@ -92,7 +96,6 @@ DirectionalDerivativeRecording: directional_derivative, AstypeRecording: astype, UnsignedToSignedRecording: unsigned_to_signed, - # SilencedArtifactsRecording: silence_artifacts, } # we control import in the preprocessing init by setting an __all__ diff --git a/src/spikeinterface/preprocessing/silence_periods.py b/src/spikeinterface/preprocessing/silence_periods.py index d1c0bcded9..6a3384f205 100644 --- a/src/spikeinterface/preprocessing/silence_periods.py +++ b/src/spikeinterface/preprocessing/silence_periods.py @@ -5,7 +5,6 @@ from spikeinterface.core.core_tools import define_function_handling_dict_from_class from spikeinterface.core.recording_tools import get_noise_levels, get_chunk_with_margin from spikeinterface.core.generate import MockRecording -from spikeinterface.core.job_tools import split_job_kwargs from .basepreprocessor import BasePreprocessor, BasePreprocessorSegment from .normalize_scale import ScaleRecording @@ -253,11 +252,11 @@ def get_traces(self, start_frame, end_frame, channel_indices): ) -class DetectArtifactAndSilentPeriodsRecording(SilencedPeriodsRecording): +class DetectAndSilenceArtifactsRecording(SilencedPeriodsRecording): """ - Class doing artifact detection and lient at the same time. - - See SilencedPeriodsRecording and detect_artifact_periods for details. + Detect and silence artifacts in a single step. See + :func:`~spikeinterface.preprocessing.detect_artifact_periods` and + :class:`SilencedPeriodsRecording` for details. """ _precomputable_kwarg_names = ["periods"] @@ -290,10 +289,10 @@ def __init__( SilencedPeriodsRecording.__init__( self, recording, periods=periods, mode=mode, noise_levels=noise_levels, seed=seed, **noise_levels_kwargs ) - # note self._kwargs["periods"] is done by SilencedPeriodsRecording and so the computaion is done once + # self._kwargs["periods"] is done by SilencedPeriodsRecording, so the computation is done once # function for API -detect_artifacts_and_silent_periods = define_function_handling_dict_from_class( - source_class=DetectArtifactAndSilentPeriodsRecording, name="silence_artifacts" +detect_and_silence_artifacts = define_function_handling_dict_from_class( + source_class=DetectAndSilenceArtifactsRecording, name="detect_and_silence_artifacts" ) diff --git a/src/spikeinterface/preprocessing/tests/test_pipeline.py b/src/spikeinterface/preprocessing/tests/test_pipeline.py index 37376781b7..4cde6d3d7e 100644 --- a/src/spikeinterface/preprocessing/tests/test_pipeline.py +++ b/src/spikeinterface/preprocessing/tests/test_pipeline.py @@ -69,6 +69,7 @@ def test_pipeline_equiv_to_step(): "detect_and_remove_bad_channels", "detect_and_interpolate_bad_channels", "detect_and_remove_artifacts", + "detect_and_silence_artifacts", ]: # These steps aren't deterministic (because they sample random chunks), # so make sure that both pipeline and class get the same seed. pp_dict[pp_name] = {"seed": 1205} From b915d7fea38b3cc5f15b50d36dacf7fd5194734b Mon Sep 17 00:00:00 2001 From: Alessio Buccino Date: Tue, 14 Jul 2026 09:27:31 +0100 Subject: [PATCH 4/4] fix: remove superseeded detect_and_silence_artifacts --- doc/api.rst | 1 + .../preprocessing/preprocessing_classes.py | 3 -- .../preprocessing/silence_periods.py | 46 ------------------- .../preprocessing/tests/test_pipeline.py | 1 - 4 files changed, 1 insertion(+), 50 deletions(-) diff --git a/doc/api.rst b/doc/api.rst index 206b21b365..ce850d1291 100755 --- a/doc/api.rst +++ b/doc/api.rst @@ -223,6 +223,7 @@ spikeinterface.preprocessing .. autofunction:: detect_artifact_periods .. autofunction:: detect_artifact_periods_by_envelope .. autofunction:: detect_saturation_periods + .. autofunction:: detect_and_remove_artifacts .. autofunction:: directional_derivative .. autofunction:: filter .. autofunction:: gaussian_filter diff --git a/src/spikeinterface/preprocessing/preprocessing_classes.py b/src/spikeinterface/preprocessing/preprocessing_classes.py index 0b78aa590a..406e239380 100644 --- a/src/spikeinterface/preprocessing/preprocessing_classes.py +++ b/src/spikeinterface/preprocessing/preprocessing_classes.py @@ -34,8 +34,6 @@ from .silence_periods import ( SilencedPeriodsRecording, silence_periods, - DetectAndSilenceArtifactsRecording, - detect_and_silence_artifacts, ) from .phase_shift import PhaseShiftRecording, phase_shift from .zero_channel_pad import ZeroChannelPaddedRecording, zero_channel_pad @@ -78,7 +76,6 @@ DetectAndInterpolateBadChannelsRecording: detect_and_interpolate_bad_channels, # artifact/saturation handling DetectAndRemoveArtifactsRecording: detect_and_remove_artifacts, - DetectAndSilenceArtifactsRecording: detect_and_silence_artifacts, # misc RectifyRecording: rectify, ClipRecording: clip, diff --git a/src/spikeinterface/preprocessing/silence_periods.py b/src/spikeinterface/preprocessing/silence_periods.py index 6a3384f205..430348bbb3 100644 --- a/src/spikeinterface/preprocessing/silence_periods.py +++ b/src/spikeinterface/preprocessing/silence_periods.py @@ -250,49 +250,3 @@ def get_traces(self, start_frame, end_frame, channel_indices): silence_periods = define_function_handling_dict_from_class( source_class=SilencedPeriodsRecording, name="silence_periods" ) - - -class DetectAndSilenceArtifactsRecording(SilencedPeriodsRecording): - """ - Detect and silence artifacts in a single step. See - :func:`~spikeinterface.preprocessing.detect_artifact_periods` and - :class:`SilencedPeriodsRecording` for details. - """ - - _precomputable_kwarg_names = ["periods"] - - def __init__( - self, - recording, - detect_artifact_method="envelope", - detect_artifact_kwargs=dict(), - periods=None, - mode="zeros", - noise_levels=None, - seed=None, - **noise_levels_kwargs, - ): - - if periods is None: - from spikeinterface.preprocessing import detect_artifact_periods - - detect_method_kwargs = dict(detect_artifact_kwargs) if detect_artifact_kwargs else {} - if detect_artifact_method == "envelope": - detect_method_kwargs["seed"] = seed - periods = detect_artifact_periods( - recording, - method=detect_artifact_method, - method_kwargs=detect_method_kwargs, - job_kwargs=None, - ) - - SilencedPeriodsRecording.__init__( - self, recording, periods=periods, mode=mode, noise_levels=noise_levels, seed=seed, **noise_levels_kwargs - ) - # self._kwargs["periods"] is done by SilencedPeriodsRecording, so the computation is done once - - -# function for API -detect_and_silence_artifacts = define_function_handling_dict_from_class( - source_class=DetectAndSilenceArtifactsRecording, name="detect_and_silence_artifacts" -) diff --git a/src/spikeinterface/preprocessing/tests/test_pipeline.py b/src/spikeinterface/preprocessing/tests/test_pipeline.py index 4cde6d3d7e..37376781b7 100644 --- a/src/spikeinterface/preprocessing/tests/test_pipeline.py +++ b/src/spikeinterface/preprocessing/tests/test_pipeline.py @@ -69,7 +69,6 @@ def test_pipeline_equiv_to_step(): "detect_and_remove_bad_channels", "detect_and_interpolate_bad_channels", "detect_and_remove_artifacts", - "detect_and_silence_artifacts", ]: # These steps aren't deterministic (because they sample random chunks), # so make sure that both pipeline and class get the same seed. pp_dict[pp_name] = {"seed": 1205}