-
Notifications
You must be signed in to change notification settings - Fork 240
Valid unit periods extension #4299
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
base: main
Are you sure you want to change the base?
Conversation
|
Depends on #4302 |
for more information, see https://pre-commit.ci
| "amplitude_scalings", | ||
| ), "Invalid amplitude_extension. It can be either 'spike_amplitudes' or 'amplitude_scalings'" | ||
| sorting = sorting_analyzer.sorting | ||
| total_duration = sorting_analyzer.get_total_duration() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello - do we need to be careful about stuff like this? Here the total_duration of the entire recording. It's used later to compute a firing rate, and this firing rate is the total num spikes divided by total duration, rather than num-spike-in-periods/duration-of-periods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case is the num spikes in period. So I think this is correct, but I'll double check carefully!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case is the num spikes in period. So I think this is correct, but I'll double check carefully!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand. In this case, it should be num spike in the period? Then I think the code is incorrect: Num spikes are computed using count_num_spikes_per_unit, which is computed using the full recording time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah it's the CV metrics! you are right, I'll take a closer look and push some fixes
Co-authored-by: Chris Halcrow <[email protected]>
Co-authored-by: Chris Halcrow <[email protected]>
…select_sorting_periods_core
Implements #4294