Waveform tools speedup #1799
Waveform tools speedup #1799
Conversation
for more information, see https://pre-commit.ci
…waveform_tools_speedup
…waveform_tools_speedup # Conflicts: # src/spikeinterface/core/tests/test_waveform_tools.py
|
@alejoe91 @h-mayorquin : if you would have time to review this it would be super usefull for me to continue work working on tridesclous2 and spykingcircus2 |
There was a problem hiding this comment.
Looks great @samuelgarcia
A couple of small comments:
ditribute->distribute- I would use
get_traces_with_margininstead ofget_traces. I think it makes the margin handling more readable
|
I started reviewing this, I will finish in the following days. |
…waveform_tools_speedup
for more information, see https://pre-commit.ci
|
@yger : when this will be merge could you rewrite this function here https://github.com/SpikeInterface/spikeinterface/blob/main/src/spikeinterface/sortingcomponents/tools.py#L21 |
|
@alejoe91 I put back the test of sharemem on window and now it is working!!! |
Co-authored-by: Alessio Buccino <[email protected]>
| dtype=None, | ||
| sparsity_mask=None, | ||
| copy=False, | ||
| job_name=None, |
There was a problem hiding this comment.
Isn't job_name a part of job_kwargs ?
There was a problem hiding this comment.
job_name is for ChunkRecordingExecutor and is not part of job_kwargs.
There was a problem hiding this comment.
Shouln't it be?
job_kwargs is basically al the keyword arguments of ChunkRecordingExecutor.
There was a problem hiding this comment.
yes. certainlly. but not in this PR
There was a problem hiding this comment.
OK, here you go:
#1986
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
This looks good to me. Thanks for bearing with me, @samuelgarcia. I learned while doing this PR and I know that that's taxing in your time but this is when I can pick your brain appart and learn about the design decisions that you are making.
Some pending issues for future PR or issues:
- I still think it will be a good idea to refer to spikes as spike_vector whenever they are, well, spike_vectors.
- Isn't the
job_namealready part ofjob_kwargs? - I wish we had a test for not extracting spikes whose support would go outside of the borders.
When we want to return_scale=True we need another dtype so it is good to control this externally. |
Waveforms extarctor was extracting waveforms into one file (or one sharemem) per unit.
This lead to "too many file open" in many situtation.
Here I propose another implementation into a unique file.
PROS:
CONS:
The idea of this PR is only to add the underlying machinery.
This PR do not change the WaveformExtactor internals.
It will be done in a separted PR using this new
extract_waveforms_to_single_buffer()to fix the "too many file open"