Valid unit periods extension#4299
Conversation
|
Depends on #4302 |
for more information, see https://pre-commit.ci
|
|
||
| def _merge_extension_data( | ||
| self, merge_unit_groups, new_unit_ids, new_sorting_analyzer, censor_ms=None, verbose=False, **job_kwargs | ||
| ): |
There was a problem hiding this comment.
You should add here a comment for the strategy of merging period so that the code will be easier to read.
There was a problem hiding this comment.
I added a comment in the user defined section. I think it's ok since no-one will ever check the docstring for this
| job_name = f"computing false positives and negatives" | ||
|
|
||
| # parallel | ||
| with ProcessPoolExecutor( |
There was a problem hiding this comment.
We need to be carrfull with th use of ProcessPoolExecutor.
on window this imply spawn and so datta duplication here this will be sorting + amplitudes duplicated
in case of a spwan
There was a problem hiding this comment.
then we should make a base executor. We have the same issue in other extensions...
|
Revised, @samuelgarcia @chrishalcrow can you take a final look? We can discuss on the merge strategy with user defined periods, but I think union is a good choice (see my comment) |
| ) | ||
| ) | ||
|
|
||
| user_defined_periods = cast_periods_to_unit_period_dtype(user_defined_periods) |
for more information, see https://pre-commit.ci
|
Impressive and complicated PR! I push some test for the remapping low level code. |
| spike waveform. In case of spatio-temporal collisions, a multi-linear fit is performed using the templates of all units | ||
| involved in the collision. | ||
|
|
||
| **NOTE:** computing amplitude scalings is highly recommended before calculating amplitude-based quality metrics, such as |
There was a problem hiding this comment.
Because they are much more robust. Spike amplitudes only sample the voltage value on the max channel, amplitude scalings use sparse templates and fit them spike-by-spike, also handling collisions. Downside is it's slower to compute
There was a problem hiding this comment.
@alejoe91 Thanks for the explanation.
Currently, the documentation is unclear. Both spike_amplitudes and amplitude_scalings have the same note but users won't know whether they need to compute both or only one of them, which one is the recommended one (if any) or what happens if you compute both.
Currently, spike_amplitudes is the default everywhere: amplitude_cutoff, noise cutoff will use spike_amplitudes by default even if amplitude_scalings is available; ampitude_cv receives the extension as argument (default is spike_amplitudes) and amplitude median does not even consider amplitude_scalings.
Maybe the note should just say, one of the two is required or just recommend one of them. I particularly prefer spike_amplitudes, even if noisier, it is more understandable and a lot less margin for error.
5 participants
Implements #4294