Fix silence period and job_kwargs #4071
Conversation
for more information, see https://pre-commit.ci
| mode="zeros", | ||
| noise_levels=None, | ||
| seed=None, | ||
| job_kwargs=dict(), |
There was a problem hiding this comment.
| job_kwargs=dict(), | |
| **job_kwargs, |
Should it be like this?
And then load the _shared_job_kwargs doc?
I'll test this PR now!
|
So it appropriately used the global_job kwargs now during Exposing job_kwargs worked fine for Copy of script I ran and terminal output. > import spikeinterface.full as si
> from spikeinterface.sortingcomponents.peak_detection import detect_peaks
> rec, _ = si.generate_ground_truth_recording([600], 30000, 64, 30)
> rec = si.common_reference(si.bandpass_filter(rec))
> rec_silenced = si.silence_periods(rec, [[30000,30040],[50000, 50400]], mode='noise', job_kwargs=dict(n_jobs=7))
noise_level (workers: 7 processes): 100%|██████████| 20/20 [00:00<00:00, 50.22it/s]
> si.set_global_job_kwargs(**dict(n_jobs=7))
> peaks = detect_peaks(rec_silenced)
noise_level (workers: 7 processes): 100%|██████████| 20/20 [00:00<00:00, 46.42it/s]
detect peaks using locally_exclusive (workers: 7 processes): 100%|██████████| 600/600 [00:13<00:00, 43.03it/s]I will say this went much faster than when I was testing and each process would go back and recalculate noise_levels with n_jobs=1. |
…nto fix_silence_period
for more information, see https://pre-commit.ci
|
@samuelgarcia @zm711 This should also now fix the problem with detect_peaks. |
for more information, see https://pre-commit.ci
…nto fix_silence_period
|
At least on my computer it still said it was computing import spikeinterface.full as si
rec, _ = si.generate_ground_truth_recording([600], 30000, 64, 30)
rec = si.common_reference(si.bandpass_filter(rec))
rec1 = si.silence_periods(rec, [[30000,30040],[50000, 50400]], mode='noise', job_kwargs=dict(n_jobs=7))
from spikeinterface.sortingcomponents.peak_detection import detect_peaks
si.set_global_job_kwargs(**dict(n_jobs=7))
peaks= detect_peaks(rec1)
noise_level (workers: 7 processes): 100%|██████████| 20/20 [00:00<00:00, 46.48it/s]
noise_level (workers: 7 processes): 100%|██████████| 20/20 [00:00<00:00, 46.86it/s]
detect peaks using locally_exclusive (workers: 7 processes): 100%|██████████| 600/600 [00:36<00:00, 16.55it/s] |
|
Is this because it is assuming |
for more information, see https://pre-commit.ci
… into fix_silence_period
| self._kwargs = dict( | ||
| recording=recording, list_periods=list_periods, mode=mode, seed=seed, noise_levels=noise_levels | ||
| ) | ||
| self._kwargs.update(noise_levels_kwargs) |
There was a problem hiding this comment.
I don't play with the _kwargs. Will this update lead to incompatible keys between spikeinterface versions?
There was a problem hiding this comment.
Actually this is a good question, and we should discuss this PR with @samuelgarcia . Indeed, moving the computation of noise levels into the detect_peaks() and not in the node themselves is the only option to control finely the job_kwargs. Not sure we need to save the noise_level_kwargs because the noise_levels are cached per recording I think, and not recomputed during parallel processing. I'll double check with @samuelgarcia
…fix_silence_period
for more information, see https://pre-commit.ci
Putative attempt to fix #4038, by explicitly exposing job_kwargs to the silence_period function, in order to allow the user to set them explicitly. In addition, noise_levels are now saved explicitly in extra_kwargs. In theory, once a first call to noise_levels has been made, they should be cached and thus no recomputation should be made.