SpikeInterface / spikeinterface

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

Add `shift start time` function. #3509

Closed JoeZiminski closed 2 days ago

JoeZiminski commented 3 weeks ago

This PR adds a 'shift start time' function that a user can use to set a new start time for their recording.

This follows from discussions on the time API #3117 and #3157. Briefly, there are two ways to handle time in SI, a t_start recording attribute that is used to set the start time. Under this regime, the (regularly) spaced timeseries is regenerated on the fly from t_start, num samples and sampling frequency. Alternatively, a (more memory heavy) time_vector can be set to explicitly set the time array for a recording. As I understand it this is useful in the case there are gaps in the recording or there is some clock drift. From the user perspective, they always get a time vector back from get_times().

This PR implements a shift_start_time as suggested by @h-mayorquin as a convenient way for users to shift the start time of their recording, abstracting away the technical details of the implementation under the hood (t_start vs time_vector).

I have updated the PR #3072 containing documentation on times, the relevant proposed section can be found here.

samuelgarcia commented 3 weeks ago

HI Joe. I am not sure this approach is persistent to json dump/load cycle. If this is a need then we should have a dedicated class for that handle explicitly the shbift in the kwargs

JoeZiminski commented 3 weeks ago

Hey @zm711 this is a good point, we (@h-mayorquin) decided that from a user perspective the implementation under the hood should not leak through the API. So as far as the user is concerned the user just as 'times' and there is no distinction between the t_start implementation and time_vector. But maybe this is useful to have somewhere in the docstring for the more interested user, maybe notes?

Thanks @samuelgarcia I'm not 100% sure I understand. This PR is editing the existing segment attributes, will these not be propagated to a newly saved version of the recording? This would be an important test.

e.g. (pseudocode, I can't remember the exact commands)

recording.save("some_path")  # save with original times

recording.shift_start_time(10)  # shift times

recording.save("some_path")  # the saved data will not be updated?
load_recording - si.load("some_path")  # this will not have the updated times?
h-mayorquin commented 3 weeks ago

Also, @JoeZiminski @zm711 I am against adding notes about internal implementation details on the docstring (for the reason that Joel gave) but I think a top level comment on the function can work.

My reasoning is that if you change the internal implementation then you need to update the docstring which should be user facing and unaffected by this.

zm711 commented 3 weeks ago

I'm fine seeing myself off of this thread. I honestly never use this machinery for my purposes so it is really hard for me to imagine the point of this and how I as a user would interact with these functions. So what is implementation vs what is user facing is hard for me to parse. Maybe a call at some point (dedicated just to time machinery) between those who care would be helpful (at least for me). I agree with Heberto not to leak internal stuff, but so I would like to understand internal better.

JoeZiminski commented 1 week ago

Thanks @h-mayorquin I agree a top level comment is nice for this. Cheers for the feedback @zm711! Yes I think a call would be nice, @samuelgarcia also mentioned at some point he will do some time-alignment stuff so that might be a good opportunity.

JoeZiminski commented 2 days ago

@alejoe91 this is good to go from my end