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

Fix t_starts not propagated to save_to_memory. #3120

Merged
merged 12 commits into from
Jul 9, 2024

Conversation

JoeZiminski
Copy link
Collaborator

@JoeZiminski JoeZiminski commented Jul 1, 2024

This PR closes #2515 by propagated t_starts to memory recording objects during save_to_memory. This is tested in #3117.

This works to propagate the times to save to memory, and tests are passing, but I havn't considered other uses for these recording classes. So it would be great if others could let me know if this might break anything elsewhere! I can't see why it would as it uses default kwargs that maintain the old behaviour, but still I'm paranoid 😅

Tests are added here that cover propagation of time information when stored as t_start and time_vector to various methods of saving the data. Also ,tests for other time-related functions (e.g. sample_time_to_index() are added).

@JoeZiminski JoeZiminski changed the title Fix t_starts not propagated to save memory. Fix t_starts not propagated to save_to_memory. Jul 1, 2024
@JoeZiminski JoeZiminski force-pushed the fix_save_to_memory_t_start branch from 4005c75 to 16d4089 Compare July 1, 2024 22:37
@alejoe91 alejoe91 added this to the 0.101.0 milestone Jul 2, 2024
@alejoe91 alejoe91 added the core Changes to core module label Jul 2, 2024
Copy link
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.

Let's move the corresponding tests from #3117 to here.

I suspect that you tested this in #3117 because you wanted to have a canonical way for seting the t_start in your tests but you don't really need it, t_start is a public attribute of the segment class.

Plus, you are adding interface in #3117 and the tests should not depend on the newly added interface. Having t_start as an attribute of the segment class is a deper contract that the interface at the recording class and the tests of the interface should rely on the machinery that is already baked in on the data model.

@JoeZiminski
Copy link
Collaborator Author

Thanks both, I will move the tests over now

@JoeZiminski
Copy link
Collaborator Author

@h-mayorquin @samuelgarcia @alejoe91 I re-requested review as added tests from #3117 to here, these cover all saving behaviour for both time representations + some other time-related functions. Herberto I tried to reduce the indirection on the fixtures by a level, let me know if it is still not clear.

Copy link
Member

@alejoe91 alejoe91 left a comment

Choose a reason for hiding this comment

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

LGTM!!

Removed the deprecated tests ;)

@alejoe91 alejoe91 dismissed h-mayorquin’s stale review July 9, 2024 08:40

tests were added

@alejoe91 alejoe91 merged commit 00e1cf9 into SpikeInterface:main Jul 9, 2024
15 checks passed
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.

Times not propagated by .save_to_memory()
4 participants