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 time vector case to get_durations. #3118

Conversation

JoeZiminski
Copy link
Collaborator

@JoeZiminski JoeZiminski commented Jul 1, 2024

This PR extends BaseRecording.get_duration() (and the associated get_total_duration()) but allowing it to handle the time_vector case. It is tested in #3117 (see that PR for why).

In reality, this should only have an extremely minimal effect, only in cases with some irregularities in the time-interval spacing. Nonetheless I guess it makes sense to use the true time_vector for the duration if it exists as it will be different form estimating based on number of sample and sampling frequency.

The calculation is t_stop - t_start + sampling_step. The sampling step is introduced due to fencepost error. For example, if you have ts=0.1, num_samples=10, duration=1, you will have 10*0.1=1, a 1s recording as expected. These will be represented by times [0, 0.1, ..., 0.9]. Therefore we need to add 1x ts to t_stop - t_start to get the true duration. As the ts is usually a very small number the duration calculated in this way is not nice and round as it usually is with num_samples / sampling_frequency.

@JoeZiminski JoeZiminski changed the title Add time vector case to 'get_durations'. Add time vector case to get_durations. Jul 1, 2024
@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
@samuelgarcia
Copy link
Member

We agree with this but we should check carfully that the internal use of this get_total_duration/get_dutaion is correcyly use anywhere in the code.
And also the sortinganalyzer.get_ total_duration should be change also.

@JoeZiminski
Copy link
Collaborator Author

JoeZiminski commented Jul 8, 2024

Good point I will list their uses in the codebase below. I don't think any of these should be affected:

recording.get_duration()

  • In baserecording.py it is only used for __repr__ methods.
  • In widgets/traces.py, it is used to set the end time of the recording to display, but only in the has has_time_vector is False, so should not be affected by this change. It is also used for the ipywidgets slider, I tested this and it did not error.
  • In NoiseGeneratorRecording for silence_periods.py. I tested this and NoiseGeneratorRecording works okay with any float duration, it will just round to nearest num samples.
  • In generate_byhrid_recordings() where they are passed to generate_sorting_kwargs for generate_sorting. Same as above.
  • In test_benchmark_motion_interpolation() I think this is OK, as is currently working and would require the test to be changed to break in some way.

get_total_duration() (recording and sorting_analyzer)

The two places the function is used that is not tests / docs is:

  • widgets/peak_activivity.py Had a look through the code and see no reason why this would lead to any problems. Tested it and runs okay.
  • qualitymetrics/misc_metrics.py The sorting_analyzer.get_total_duration() function is used a lot in misc_metrics to calculate firing rate (num_spikes / total_duration). The new implementation should not be a problem.

@JoeZiminski
Copy link
Collaborator Author

Oh yes @samuelgarcia the sorting analyzer needs changing too! How to do you think it is best to structure this, to avoid duplciating the generation code across sorter and recording? Can the sorting_analyzer function call the get_total_duration() on the self.recording?

@alejoe91
Copy link
Member

alejoe91 commented Jul 9, 2024

@JoeZiminski since the analyzer can be recordingless, I would do the following:

def get_total_duration(self) -> float:
    if self.has_recording() or self.has_temporary_recording():
        duration = self.recording.get_total_duration()
    else:
        duration = self.get_total_samples() / self.sampling_frequency
    return duration

Get total samples is always avalailable since it's saved as a recording attribute.

@JoeZiminski
Copy link
Collaborator Author

Cool thanks @alejoe91 I will add this mode. For testing, I can't figure out how to make a recordingless analyser 😄 I guess it is not with create_sorting_analzyer?

@alejoe91
Copy link
Member

Cool thanks @alejoe91 I will add this mode. For testing, I can't figure out how to make a recordingless analyser 😄 I guess it is not with create_sorting_analzyer?

Not the most elegant of solutions, but this works:

sorting_analyzer._recording = None

:)

@alejoe91
Copy link
Member

@JoeZiminski I pushed a small fix in hybrid tools! Thanks for adding tests. Let us know when it's done!

@JoeZiminski
Copy link
Collaborator Author

Thanks @alejoe91 will push final commit shortly!

@JoeZiminski
Copy link
Collaborator Author

Hey @alejoe91 good to go from my end!

@samuelgarcia samuelgarcia merged commit a14ee81 into SpikeInterface:main Jul 15, 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.

4 participants