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. |
4 participants
Implements #4294