-
Notifications
You must be signed in to change notification settings - Fork 240
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. |
| rng = np.random.default_rng() | ||
|
|
||
| # number of random periods | ||
| n_periods = 10_000 |
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.
duration, num_units and n_periods are all quite large for a test. Is it slow??
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 agree we should put a small number
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.
takes about 1 second
Co-authored-by: Chris Halcrow <[email protected]>
| ("unit_index", "int64"), | ||
| ] | ||
|
|
||
| base_period_dtype = [ |
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.
move all dtypes to base
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.
done in #4314
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
yger
left a comment
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 looks good to me, I think this is a great extension to be able to compute metrics on sub_periods only. A couple of minor comments, and I think everything is not too slow, but good work!
|
|
||
| # Check that all spikes in the sliced sorting are within the periods | ||
| for segment_index in range(sorting.get_num_segments()): | ||
| periods_in_segment = periods[periods["segment_index"] == segment_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.
do we assume not too many periods, just to be sure that all these masks won't take too long?
| spike_vector = sorting.to_spike_vector() | ||
| spike_vector_list = sorting.to_spike_vector(concatenated=False) | ||
| keep_mask = np.zeros(len(spike_vector), dtype=bool) | ||
| all_global_indices = spike_vector_to_indices(spike_vector_list, unit_ids=sorting.unit_ids, absolute_index=True) |
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.
why not using self.sorting_analyzer.sorting.get_spike_vector_to_indices(), and thus make use of a possible cache?
| for segment_index in range(sorting.get_num_segments()): | ||
| global_indices_segment = all_global_indices[segment_index] | ||
| # filter periods by segment | ||
| periods_in_segment = periods[periods["segment_index"] == segment_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.
assuming not too many periods, these masks would be fine. Otherwise, we'll need to optimize
|
We don't want to extend these quality metrics to the extension() ? What if someone wants to only get the ISI, CCG or anything else only on the periods? Would it be easy to slice the sorting, and then compute only on the sub sorting? Are the extension robust w.r.t. periods? |
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