Skip to content

Fix off-by-one in remove_artifacts zeros mode leaving last artifact sample un-zeroed#4445

Draft
Copilot wants to merge 2 commits intomainfrom
copilot/remove-artifacts-recording-kilosort
Draft

Fix off-by-one in remove_artifacts zeros mode leaving last artifact sample un-zeroed#4445
Copilot wants to merge 2 commits intomainfrom
copilot/remove-artifacts-recording-kilosort

Conversation

Copy link

Copilot AI commented Mar 16, 2026

RemoveArtifactsRecordingSegment.get_traces (zeros mode) had an off-by-one in the boundary branch where the artifact start falls at or before the chunk start. The last sample of the artifact window was never zeroed, causing sorters to detect spurious spikes right at the end of blanked periods.

Root cause

In get_traces, trig is the trigger offset relative to start_frame. When trig - pad[0] <= 0 (artifact starts at/before chunk boundary) the code used:

traces[: trig + pad[1], :] = 0   # exclusive — last sample missed

while the normal (interior) case correctly used trig + pad[1] + 1. Additionally, the guard on the interior case used > 0 instead of >= 0, pushing the trig - pad[0] == 0 situation into the buggy branch unnecessarily.

Fix

# Before
if trig - pad[0] > 0 and trig + pad[1] < end_frame - start_frame:
    traces[trig - pad[0] : trig + pad[1] + 1, :] = 0
elif trig - pad[0] <= 0:
    traces[: trig + pad[1], :] = 0          # ← missed last sample

# After
if trig - pad[0] >= 0 and trig + pad[1] < end_frame - start_frame:
    traces[trig - pad[0] : trig + pad[1] + 1, :] = 0
elif trig - pad[0] <= 0:
    traces[: trig + pad[1] + 1, :] = 0      # ← inclusive

Test

Added test_remove_artifacts_zeros_boundary: requests a window slightly wider than the artifact so the boundary branch is exercised, then asserts every sample in [trigger - ms_before, trigger + ms_after] (inclusive) is zero. The existing tests never caught this because they requested windows exactly matching the artifact width, which fell into the full-window-zero branch instead.

Original prompt

This section details on the original issue you should resolve

<issue_title>Remove artifacts recording not used in Kilosort</issue_title>
<issue_description>Hello! I have been using spikeinterface's run_sorter function in two ways:

sorting = si.run_sorter('kilosort4', blanked_rec, folder=outputFolder, docker_image=False, verbose=True, remove_existing_folder=False, raise_error=True, **params_kilosort4)

where (1), blanked_rec is a variable (a RemoveArtifactsRecording) that comes from running this:

blanked_rec = si.remove_artifacts(recording=recording_to_blank, list_triggers=mStim_sample_idx, ms_before=ms_before, ms_after=ms_after) 

(we want to remove the artifact of microstimulation on the recording)

and where (2), blanked_rec is a loaded recording of (1) saved (a BinaryFolderRecording):

blanked_rec.save(folder=output_path)
...
blanked_rec=si.load(blanked_filename) 

However, in both cases we found that despite the blanked recording looking good when manually scrolling through it, kilosort seems to use an incorrect recording. When doing manual curation in phy, some spikes are detected during the period that is blanked, which is visible when plotting the spiketimes relative to the TTLs that were blanked (picture attached). Many units also have suspiciously low spikecounts.

Image

I think (1) didn't work because the RemoveArtifactsRecording is only a wrapper to the actual recording underneath. But I cannot explain why (2) didn't work either. Was there anything else wrong in my approach?

Any help is greatly appreciated!</issue_description>

<agent_instructions>Can you try to fix the bug and add a test for it in src/spikeinterface/preprocessing/test_remove_artifacts.py?</agent_instructions>

Comments on the Issue (you are @copilot in this section)

@samuelgarcia Hi. Sorry ffor this. What happen you you do plot_tarces in the blanked zone, is it flat in the the zone ? @samuelgarcia oups. This looks like a bug. @alejoe91 @aradiawilms could you share the recording and the code?

Is it possible that the recording has timestamps or a non-zero start time? in that case, the plot traces would use the recording timestamps, which might offset your expected stimuli.</comment_new>
<comment_new>@alejoe91
Note that both (1) and (2) should give the same traces. I also use remove_artifact to remove optogenetics stimulations and get the correct blanking in the output traces!</comment_new>
<comment_new>@alejoe91
@aradiawilms any update?</comment_new>
<comment_new>@samuelgarcia
@Dililianxice thank you very much for finding this bug.
We will try to fix it soon. Or maybe you ?</comment_new>


📱 Kick off Copilot coding agent tasks wherever you are with GitHub Mobile, available on iOS and Android.

…egression test

Co-authored-by: alejoe91 <17097257+alejoe91@users.noreply.github.com>
Copilot AI changed the title [WIP] Remove artifacts recording not used in Kilosort Fix off-by-one in remove_artifacts zeros mode leaving last artifact sample un-zeroed Mar 16, 2026
Copilot AI requested a review from alejoe91 March 16, 2026 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove artifacts recording not used in Kilosort

2 participants