search

SpikeInterface / spikeinterface

A Python-based module for creating flexible and robust spike sorting pipelines.
https://spikeinterface.readthedocs.io
MIT License
518 stars 186 forks source link

Remove artifact function with explicit timestamps #619

Open khl02007 opened 2 years ago

khl02007 commented 2 years ago

RemoveArtifactsRecording takes as input list_triggers, which are in frames. On the other hand, ms_before and ms_after are in milliseconds. This is converted to frames using the sampling rate later. As a result, one cannot make use of ms_before and ms_after if one uses explicit timestamps (perhaps because there are missing samples in the recording). @alejoe91 @samuelgarcia what do you think about changing this such that get_times is used to infer the conversion from ms to frames? This means the padding must be computed separately for each trigger, so the performance may take a hit. Alternatively, could make the user specify frames_before and frames_after, which are computed manually before calling remove artifact.

alejoe91 commented 2 years ago

@khl02007 was this fixed?

khl02007 commented 2 years ago

will test this out and let you know

khl02007 commented 2 years ago

oops, sorry I thought this was the one about masking out single samples. I don't think there has been a PR to address this - I had opened it to ask you guys your thoughts. what do you think - specify frames before and after, or keep the ms before and after and just infer the number of frames it corresponds to with the explicit timestamps?

alejoe91 commented 1 year ago

@khl02007 I would keep ms_before and ms_after and add optionally frames_before and frames_after. If the latter are givem, the ms_* params are ignored. @khl02007 does that sound ok?

khl02007 commented 1 year ago

@alejoe91 that sounds like it would work. happy new year!

alejoe91 commented 1 year ago

Happy new year to you too Kyu!!!

khl02007 commented 1 year ago

By the way, happy to make these changes and submit a PR if you are OK with that

alejoe91 commented 1 year ago

I'm super OK with that! :)

h-mayorquin commented 1 year ago

@alejoe91 Did this idea never pan out? Does RemoveArtifactsRecording now have that functionality?