Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add recording.time_slice like recording.frame_slice #3034

Merged
merged 2 commits into from
Jun 19, 2024

Conversation

h-mayorquin
Copy link
Collaborator

Many times when I use spikeinterface I wish had this facility. recording.time_slice() to avoid having to do the conversion myself. This PR adds the functionality and a test.

That said, I am not sure I really like name, any suggestions @zm711, @JoeZiminski?

I am thinking on discoverability.
I think that if the methods started by slice it would be nice to get the auto-completition to either complete it to time or frame but because they are not

Maybe there is a better name?

@h-mayorquin h-mayorquin added the core Changes to core module label Jun 17, 2024
@h-mayorquin h-mayorquin self-assigned this Jun 17, 2024
@zm711
Copy link
Collaborator

zm711 commented Jun 18, 2024

I don't really see another name. I think frame_slice and time_slice are parsimonious and if someone discovers one it would be easier to discover the other. I think some functions use time_range which is I think is a better name, but then would you want frame_range (which sounds awkward to me....)?

@alejoe91
Copy link
Member

I like time_slice too! Thanks for this @h-mayorquin

@h-mayorquin
Copy link
Collaborator Author

All right. Then the name as it is is good.

@alejoe91 alejoe91 merged commit 220a2a5 into SpikeInterface:main Jun 19, 2024
11 checks passed
@samuelgarcia
Copy link
Member

merci

@h-mayorquin h-mayorquin deleted the add_time_slice branch June 19, 2024 13:33
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.

4 participants