Fix annotate_muscle_zscore crash with n_jobs='cuda'#13994
Conversation
Add n_jobs_hilbert so filtering can use CUDA while the Hilbert transform keeps integer-only parallelization.
|
Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴 |
| raw_copy.apply_hilbert( | ||
| envelope=True, n_jobs=n_jobs_hilbert if n_jobs_hilbert is not None else 1 | ||
| ) |
There was a problem hiding this comment.
Hmmm... this isn't backward compatible. Previously, if a user provided n_jobs=4 for example, this would use 4 jobs. So a better pattern I think is:
if n_jobs != "cuda" and n_jobs_hilbert is None:
n_jobs_hilbert = n_jobs
And you say in the docstring that n_jobs_hilbert will default to the value of n_jobs when n_jobs != "cuda".
| n_jobs_hilbert : int | None | ||
| Number of jobs for the Hilbert transform. Cannot be ``'cuda'``. | ||
| Defaults to ``1`` when ``None``. Use when ``n_jobs='cuda'`` to keep | ||
| filtering on GPU while Hilbert runs on CPU. |
There was a problem hiding this comment.
I'm afraid this description is unhelpful for any user looking to annotate muscle artifacts. It requires deep knowledge of the inner workings of the function to understand. Could we design this in such a way that a user would know what to set this to even if they do not have any prior knowledge that a Hilbert transform is involved, let alone that this cannot be done on the GPU?
3 participants
Fixes #13993
Summary
n_jobs_hilbertparameter toannotate_muscle_zscoreso filtering and Hilbert transform can use separate parallelization settingsn_jobs='cuda'while Hilbert defaults ton_jobs=1whenn_jobs_hilbertis not setn_jobs='cuda', explicit integern_jobs/n_jobs_hilbert, and unchanged default behaviorTest plan
n_jobs='cuda',n_jobs=4/n_jobs_hilbert=2, default)mne/preprocessing/tests/test_artifact_detection.py::test_muscle_annotation_n_jobs_cudatest_muscle_annotationtests