SpikeInterface / SpikeInterface/spikeinterface

Proposal for handling the user interaction with time

Open
#3,157 12 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

discussion
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_times is in #3117.
  • @alejoe91 proposal, set_times_info as a setter that handles the above. I think this is a good idea because it reflects the other method we have get_times_kwargs and 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:

  1. set_times always sets the the time vector set_times and this is the final source of truth after setting.
  2. I propose adding a second function set_starting_time or shift_starting_time (do not know what's better) that is agnostic to the internal implementation. If there is a time vector it shifts so time_vector[0] = start_time and if there is a t_start then 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:

  1. This enables the workflow of shifting or doing whatever you want with the time_vector before setting it. So it does not get in the way of @JoeZiminski workflow (set_times is still set_times).
  2. @alejoe91 mention that you could use the workflow of get_times() , modify the vector and finally use set_times again. 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
  3. @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

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.