⚠ This page is served via a proxy. Original site: https://github.com
This service does not collect credentials or authentication data.
Skip to content

Conversation

@alejoe91
Copy link
Member

@alejoe91 alejoe91 commented Jan 7, 2026

Depends on #4316

Slices a sorting object based on an array ov valid periods. Periods are defined as a structured dtype as:

base_period_dtype = [
    ("start_sample_index", "int64"),
    ("end_sample_index", "int64"),
    ("segment_index", "int64"),
    ("unit_index", "int64"),
]

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

@alejoe91 alejoe91 added the core Changes to core module label Jan 7, 2026
@alejoe91 alejoe91 mentioned this pull request Jan 7, 2026
1 task
@alejoe91 alejoe91 marked this pull request as ready for review January 8, 2026 07:36
@alejoe91 alejoe91 requested a review from chrishalcrow January 8, 2026 07:36
@samuelgarcia
Copy link
Member

This is OK for me.
Make a clear documentation somwhere woule help ?

@alejoe91
Copy link
Member Author

@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!

@alejoe91 alejoe91 changed the title Implement select_sorting_periods Implement select_sorting_periods in metrics Jan 13, 2026
@alejoe91
Copy link
Member Author

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?

I think that extension by extension we could use valid_unit_periods when computed :)

@alejoe91
Copy link
Member Author

@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:
Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

oups!

Copy link
Member Author

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,)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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)
Copy link
Member

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 = [
Copy link
Member

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
Copy link
Member

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):
Copy link
Member

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]
Copy link
Member

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}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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)
Copy link
Member

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):
Copy link
Member

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):
Copy link
Member

Choose a reason for hiding this comment

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

ok
moved here

@samuelgarcia
Copy link
Member

Globally OK for me
I made a few comment and some of then should be discussed

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