Skip to content

Add get_start_time/get_end_time functions and use them in get_duration#3623

Merged
alejoe91 merged 4 commits intoSpikeInterface:mainfrom
alejoe91:use-times-in-get-durations
Jan 21, 2025
Merged

Add get_start_time/get_end_time functions and use them in get_duration#3623
alejoe91 merged 4 commits intoSpikeInterface:mainfrom
alejoe91:use-times-in-get-durations

Conversation

@alejoe91
Copy link
Copy Markdown
Member

Since the __repr__ uses the get_durations function, I added an option to avoid using timestamps to estimate duration.
In fact, this can be quite slow for long zarr datasets with timestamps, since the whole array is loaded in mem when calling get_times().

@alejoe91 alejoe91 added the core Changes to core module label Jan 15, 2025
@alejoe91 alejoe91 requested review from JoeZiminski and h-mayorquin and removed request for JoeZiminski January 15, 2025 10:10
@h-mayorquin
Copy link
Copy Markdown
Collaborator

h-mayorquin commented Jan 15, 2025

Where do you think the loading into memory is happening?
Maybe here in the asarray:

https://github.com/h-mayorquin/spikeinterface/blob/80820853354fbc2c6a56bb5dade4c94c8dacb321/src/spikeinterface/core/baserecording.py#L895-L898

Because the duration only requires the last and the first timestamps:

https://github.com/h-mayorquin/spikeinterface/blob/80820853354fbc2c6a56bb5dade4c94c8dacb321/src/spikeinterface/core/baserecording.py#L237-L240

Maybe we can just not transform to array if the timestamps are memmap and or zarr provided they are read only?

My concern is that changing the repr with these defaults will display wrong information for some cases but maybe I am wrong.

@alejoe91
Copy link
Copy Markdown
Member Author

I think that transforming to array is very convenient to cache the timestamps in memory and overall.it improves performance. Another option could be to have a get_start_time and get_end_time, which will only access the required samples.

@h-mayorquin
Copy link
Copy Markdown
Collaborator

Fair enough, changing the way that get_times work would probably require more discussion.

Adding private methods like this:

get_start_time and get_end_time, which will only access the required samples

Seems like a great idea to me. That way, we can use that in get duration without touching anything else.

@alejoe91 alejoe91 changed the title Add use_times option in get_duration/get_total_duration Add get_start_time/get_stop_time functions and use them in get_duration Jan 21, 2025
@alejoe91
Copy link
Copy Markdown
Member Author

@h-mayorquin done :) much faster now

@h-mayorquin
Copy link
Copy Markdown
Collaborator

This looks good but there are some tests that are failing due to tolerance I guess?

Two points for discussion:

  1. We have start_frame and end_frame in the signature of get_traces. Shouldn't we have get_start_time and get_end_time to be consistent in the nomenclature? what do you think? To be honest, a mistake that I commonly make when calling get_traces is using stop_frame instead of end_frame...
  2. Are you OK making these methods public? I think that's a good idea but the cautious thing would be to make them private and release them later in case we need to make some fixes that we find through usage.

@alejoe91
Copy link
Copy Markdown
Member Author

Changed to get_end_time and fixed its behavior (we have good tests for time handling! thanks @JoeZiminski!)

I'm ok in exposing these functions as public since their behavior is quite straightforward.

Copy link
Copy Markdown
Collaborator

@h-mayorquin h-mayorquin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@alejoe91 alejoe91 changed the title Add get_start_time/get_stop_time functions and use them in get_duration Add get_start_time/get_end_time functions and use them in get_duration Jan 21, 2025
@alejoe91 alejoe91 merged commit d0919ad into SpikeInterface:main Jan 21, 2025
@alejoe91 alejoe91 deleted the use-times-in-get-durations branch March 20, 2026 09:39
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.

2 participants