Skip to content

Add get_start_time, get_end_time, and get_last_spike_frame to BaseSorting#4525

Merged
h-mayorquin merged 8 commits intoSpikeInterface:mainfrom
h-mayorquin:fix_registering_time
Apr 21, 2026
Merged

Add get_start_time, get_end_time, and get_last_spike_frame to BaseSorting#4525
h-mayorquin merged 8 commits intoSpikeInterface:mainfrom
h-mayorquin:fix_registering_time

Conversation

@h-mayorquin
Copy link
Copy Markdown
Collaborator

BaseSorting has no public way to query the start or end time of a segment. BaseRecording has both (#3623). This PR adds get_start_time and get_end_time to BaseSorting, plus a get_last_spike_frame helper. With a registered recording, get_end_time returns the recording's end time. Without one, it returns the time of the last spike in the segment (the best the sorting can do on its own). get_start_time reads _t_start directly in both cases.

Adding these accessors however creates an ambiguity: some sorting extractors set _t_start on their segments at init. For example, neo sorting extractors set _t_start from the signal stream (e.g. Neuralynx infers it here). If that sorting registers a recording that also starts at 100.0s, get_start_time would need to decide which value to return.

The decision here is that registering a recording is a statement that "these are the right timestamps" (see #4519). In consequence, register_recording now copies the recording's start times into the sorting segments' _t_start. This replaces whatever the extractor had set at init, so get_start_time returns consistent values.

@alejoe91 alejoe91 added the core Changes to core module label Apr 17, 2026
Comment thread src/spikeinterface/core/basesorting.py Outdated
@alejoe91
Copy link
Copy Markdown
Member

LGTM! Just a minor comment. Did you remove the get_last_spike_frame? In that case, let's change the title of the PR

h-mayorquin and others added 3 commits April 21, 2026 06:42
@h-mayorquin
Copy link
Copy Markdown
Collaborator Author

LGTM! Just a minor comment. Did you remove the get_last_spike_frame? In that case, let's change the title of the PR

I think it was a merge conflict it is now on the PR again.

@alejoe91
Copy link
Copy Markdown
Member

Merging as soon as tests pass :)

@chrishalcrow
Copy link
Copy Markdown
Member

This is great!! A consequence I didn't realise: I usually don't load the recording when I load an analyzer. Previously this meant the analyzer had no time information in it. So if you did any time stuff, it would assume _t_start=0. But if I opened the analyzer with the recording, it would get the real _t_start. So my time information changed if I loaded the analyzer with the recording or not.

Now, when you run run_sorter, this registers the recording the sorting get _t_start. If I then save an analyzer then load it without the recording, the time information consistent with the recording. Nice!

@h-mayorquin
Copy link
Copy Markdown
Collaborator Author

Hi, @chrishalcrow, I did think about that scenario and I am reverting @alejoe91 suggestion of setting _t_start = None that we did above to handle this:

That case only works if we copy the recording's start time into _t_start at registration. register_recording now copies recording.get_start_time() into each segment's _t_start, so get_start_time just reads it and the value persists across save/load without the recording.

One case this does not fully address: if the registered recording has an explicit time_vector with irregular spacing, the copy approach only preserves the first timestamp (time_vector[0], note that this is bettter than losing everything) in the sorting's _t_start. I am unsure what to do this in this case but I think it warrants a separate issue and is related to #4519.

@h-mayorquin h-mayorquin merged commit 651c8ea into SpikeInterface:main Apr 21, 2026
15 checks passed
@h-mayorquin h-mayorquin deleted the fix_registering_time branch April 21, 2026 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Changes to core module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants