Skip to content

Conversation

@mscheltienne
Copy link
Member

@mscheltienne mscheltienne commented Nov 3, 2022

Closes #11297
cc @mmagnuski

Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a nitpick. thx @mscheltienne

Co-authored-by: Alexandre Gramfort <alexandre.gramfort@m4x.org>
auto-merge was automatically disabled November 3, 2022 14:48

Head branch was pushed to by a user without write access

@sam-s
Copy link

sam-s commented Nov 3, 2022

please add a test with n_jobs=-1
thank you.

@mscheltienne
Copy link
Member Author

@sam-s n_jobs=-1 will spawn as many workers as there are cores, which on CI is probably 2 anyway (depends on the CI provider).

And to explain the issue and the fix, the input to the // function is split on the channel axis by the number of workers. For instance, if you have 4 channels and 2 workers, both will receive 2 channels. If you have 4 channels and 4 workers, both will receive one channel. And now for the breaking edge-case, if you have more workers than channels, some workers were receiving arrays of shape (0, n_times), i.e. 0 channels, and were put to work on those empty arrays, thus returning nothing and raising during the concatenation of results.
The fix is thus very simple, you don't use more workers than channels even if the user provides an invalid number of jobs for the provided array.

@drammock drammock enabled auto-merge (squash) November 3, 2022 15:08
auto-merge was automatically disabled November 3, 2022 15:30

Head branch was pushed to by a user without write access

@larsoner
Copy link
Member

larsoner commented Nov 3, 2022

Marking for merge, thanks in advance @mscheltienne

@larsoner larsoner enabled auto-merge (squash) November 3, 2022 15:48
@larsoner larsoner merged commit 15d20b8 into mne-tools:main Nov 3, 2022
@mscheltienne mscheltienne deleted the fix_psd branch November 3, 2022 16:38
@sam-s
Copy link

sam-s commented Nov 3, 2022

@sam-s n_jobs=-1 will spawn as many workers as there are cores, which on CI is probably 2 anyway (depends on the CI provider).

I know, the issue is when -1 is replaced by the actual number of cores - I was afraid that this error might happen before the replacement (and thus you get something like 4 // -1 &c)
I suppose you thought about it already.
Thank you for fixing the issue so fast.

@mscheltienne
Copy link
Member Author

mscheltienne commented Nov 3, 2022

No risk on that side, arguments like n_jobs are checked and converted to a valid value at the beginning of the functions, and I did check locally with -1 anyway ;) Do note however that providing a number of jobs above the number of channels will not yield any performance benefit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

compute_psd: ValueError when n_jobs=-1 and only 1 channel

5 participants