Valid unit periods extension #4299
Conversation
|
Depends on #4302 |
for more information, see https://pre-commit.ci
| return sorting_analyzer | ||
|
|
||
| def _prepare_sorting_analyzer(self, format, sparse, extension_class): | ||
| def _compute_extensions_recursively(self, sorting_analyzer, extension_class, params): |
There was a problem hiding this comment.
Oh boy, this is a handy lil function!
| axs[0].set_ylabel("FP Rate") | ||
| axs[1].set_ylabel("FN Rate") |
There was a problem hiding this comment.
Is it possible to make these more descriptive? As a user I forget which one is related to amplitudes and which one is related to rp violations. But there isn't much room on the axis. Hmm.
| periods_for_unit = np.zeros(len(starts), dtype=unit_period_dtype) | ||
| periods_for_unit_w_margins = np.zeros(len(starts), dtype=unit_period_dtype) | ||
| for i, start in enumerate(starts): | ||
| end = start + period_size_samples |
There was a problem hiding this comment.
| end = start + period_size_samples | |
| end = min(start + period_size_samples, n_samples) |
At the moment, end can go past the recording length.
| ), "Either period_duration_s_absolute or period_target_num_spikes must be positive." | ||
| assert isinstance(period_target_num_spikes, (int)), "period_target_num_spikes must be an integer." | ||
|
|
||
| # user_defined_periods formatting |
There was a problem hiding this comment.
I think in this section we're being too lenient with the user and it's a bit of a mess. I think you should reuse the logic from core (
) and be a bit stricter on what we demand.|
|
||
|
|
||
| class ComputeValidUnitPeriods(AnalyzerExtension): | ||
| """Compute good time periods per unit based on quality metrics. |
There was a problem hiding this comment.
This docstring needs a tidy up
| sorting_analyzer: SortingAnalyzer | None = None, | ||
| segment_index: int | None = None, | ||
| unit_ids: list | None = None, | ||
| show_only_units_with_valid_periods: bool = True, |
There was a problem hiding this comment.
I would default this to False - I wanna see why my bad unit are failing!
| # need to recompute for merged units | ||
| recompute = True | ||
| else: | ||
| # in case of user-defined periods, just merge periods |
There was a problem hiding this comment.
I would have taken the intersection of the user periods, instead of merging them?
There was a problem hiding this comment.
why? If you merge, you probably want to keep the units you had before. Anyways, I think this user-defined period won't be really used much in "production", but the idea is to be able to test new ideas
| recompute_dict[extension_name] = extension.params | ||
|
|
||
| if len(recompute_dict) > 0: | ||
| recompute_dict = _sort_extensions_by_dependency(recompute_dict) |
There was a problem hiding this comment.
| recompute_dict = _sort_extensions_by_dependency(recompute_dict) |
compute_several_extensions handles this
| recompute_dict = {} | ||
| for extension_name, extension in self.extensions.items(): | ||
| extensions_to_compute = _sort_extensions_by_dependency( | ||
| {ext.name: ext.params for ext in self.extensions.values()} |
There was a problem hiding this comment.
| {ext.name: ext.params for ext in self.extensions.values()} | |
| {ext.extension_name: ext.params for ext in self.extensions.values()} |
4 participants
Implements #4294