SpikeInterface / SpikeInterface/spikeinterface
Proposal for handling the user interaction with time
Nobody has claimed this yet.
- Dominant language
- Python
- Stars
- 847
- Forks
- 280
- Avg merge
- 3d 9h
- Merged PRs (30d)
- 29
Description
From today's meeting.
Context:
Spikeinterface has two internal representation time:
cheap: sampling_frequency and t_start
expensive: time_vector
Fine control of the time
Right now t_start is a public attribute at the RecordingSegment level and it is harder to access than maybe it needs be. We need a more comfortable setter. Plus, the setter should handle the fact that the cheap and expensive representations are mutually exclusive.
- A proposal implementation that overloads the current
set_timesis in #3117. - @alejoe91 proposal,
set_times_infoas a setter that handles the above. I think this is a good idea because it reflects the other method we haveget_times_kwargsand it is a nice symmetry from getters to setters. I am +1 on that
End user API
My proposal for how end users should interact with time:
- set_times always sets the the time vector
set_timesand this is the final source of truth after setting. - I propose adding a second function
set_starting_timeorshift_starting_time(do not know what's better) that is agnostic to the internal implementation. If there is a time vector it shifts sotime_vector[0] = start_timeand if there is at_startthen it just shifts or changes that values.
Setting the correct times and shifting the current times are natural concepts that don't rely on internal implementation details and the functions above don't leak internal details to users.
Some extra notes:
- This enables the workflow of shifting or doing whatever you want with the
time_vectorbefore setting it. So it does not get in the way of @JoeZiminski workflow (set_timesis stillset_times). - @alejoe91 mention that you could use the workflow of
get_times(), modify the vector and finally useset_timesagain. I think this is good and very close to what I want but has the drawback that you always end with the expensive representation internally which I think we can avoid. Plus, right now this workflow throws a warning - @samuelgarcia mentioned that the same mechanism should be implemented for the sorting. Maybe using the registered recording method is an overkill.
In brief, I think we should create an API that does not leak the internal implementation details of how spikeinterface represents time. We might decide down the line to change the internal representation of time and this should not affect the API. Thus, I think those functions and their docstrings should not make references or rely on understanding of t_start and time_vector.
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with the RecordingSegment time API and review the proposed implementation in #3117 alongside get_times() and get_times_kwargs(). Resolve the setter naming and behavior for both time representations, while keeping the public API independent of internal details. Done means the API and its docstrings have an agreed design and the proposed time-setting and shifting workflows are covered.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- backend-api-design
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 25/100