-
Notifications
You must be signed in to change notification settings - Fork 241
Implement select_sorting_periods in metrics
#4302
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
|
This is OK for me. |
Co-authored-by: Chris Halcrow <[email protected]>
Co-authored-by: Chris Halcrow <[email protected]>
|
@chrishalcrow I refactored a few metrics to make sure durations, spike counts, and bins are properly accounted for when slicing with periods. Happy to discuss about this! |
select_sorting_periodsselect_sorting_periods in metrics
I think that extension by extension we could use |
|
@samuelgarcia @chrishalcrow tests added! all good now :) |
| cumulative_segment_samples = np.cumsum([0] + segment_samples[:-1]) | ||
| for unit_id in unit_ids: | ||
| firing_histograms = [] | ||
| if num_spikes[unit_id] == 0: |
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 - this num_spikes seems to be the number of spikes of the original sorting before unit periods are selected, which causes problems down the line.
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.
oups!
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.
| if cls.needs_job_kwargs: | ||
| args += (job_kwargs,) | ||
| if cls.supports_periods: | ||
| args += (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.
| args += (periods,) | |
| args += (periods,) | |
| else: | |
| raise ValueError("This metric do not support periods") |
| if metric_names is None: | ||
| metric_names = self.params["metric_names"] | ||
|
|
||
| periods = self.params.get("periods", None) |
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 guess this is to be backward compatible.
I think it would be better to make the caompatibility in the _handle_backward_compatibility_on_load no ?
|
|
||
| # can't use _misc_metric_name_to_func as some functions compute several qms | ||
| # e.g. isi_violation and synchrony | ||
| quality_metrics = [ |
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.
this could be infered by the classe no ?
| spike_locations_in_bin = spike_locations_array[i0:i1][direction] | ||
|
|
||
| unit_index = sorting_analyzer.sorting.id_to_index(unit_id) | ||
| mask = spikes_in_bin["unit_index"] == unit_index |
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.
is this fast ?
| import numpy as np | ||
|
|
||
|
|
||
| def create_ground_truth_pc_distributions(center_locations, total_points): |
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.
not needed anymore ?
| sorting = sorting_analyzer.sorting | ||
| for unit_id in sorting.unit_ids: | ||
| unit_index = sorting.id_to_index(unit_id) | ||
| periods_unit = periods[periods["unit_index"] == unit_index] |
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 have the intuition that lloping only once over periods and suming directly in a prepared vector will be way faster than this repetition of masking in the loop.
No ?
| num_samples_in_period += period["end_sample_index"] - period["start_sample_index"] | ||
| total_samples[unit_id] = num_samples_in_period | ||
| else: | ||
| total_samples = {unit_id: sorting_analyzer.get_total_samples() for unit_id in sorting_analyzer.unit_ids} |
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.
| total_samples = {unit_id: sorting_analyzer.get_total_samples() for unit_id in sorting_analyzer.unit_ids} | |
| total = sorting_analyzer.get_total_samples() | |
| total_samples = {unit_id: total for unit_id in sorting_analyzer.unit_ids} |
| samples_per_period = sorting_analyzer.get_num_samples(segment_index) // num_periods | ||
| if bin_size_s is not None: | ||
| bin_size_samples = int(bin_size_s * sorting_analyzer.sampling_frequency) | ||
| print(samples_per_period / bin_size_samples) |
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.
oups
| return total_durations | ||
|
|
||
|
|
||
| def compute_periods(sorting_analyzer, num_periods, bin_size_s=None): |
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 think we should find a better for this.
Like create_regular_periods or something.
The compute is suggesting an algo method.
No ?
| return np.concatenate(all_periods) | ||
|
|
||
|
|
||
| def create_ground_truth_pc_distributions(center_locations, total_points): |
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.
ok
moved here
|
Globally OK for me |
Depends on #4316
Slices a sorting object based on an array ov valid periods. Periods are defined as a structured dtype as:
EDIT:
Refactored computation of spike train metrics, to make sure that periods are consistently taken into account. Added 2 utils functions to compute durations per unit and bin edges per unit, that optionally use the provided periods