Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] n_jobs=-1 not working in spectral_connectivity_epochs #177

Merged
merged 1 commit into from
Mar 12, 2024

Conversation

tsbinns
Copy link
Collaborator

@tsbinns tsbinns commented Mar 12, 2024

Suggested fix for #176 where n_jobs=-1 does not result in CSD computation being properly parallelised over epochs.

This fix makes sure n_jobs=-1 is converted to the maximum number of workers before CSD computation occurs.

I think also when refactoring some of the multivariate connectivity methods some months ago the n_jobs parameter was not being properly passed to the classes. This is also now fixed.

As far as I understand, these issues are specific to spectral_connectivity_epochs() and do not affect spectral_connectivity_time().

The current unit tests for parallelisation were only using n_jobs > 1 and never n_jobs=-1. However, even if n_jobs=-1 was being used this issue would not be caught, as the tests only check that multiple workers can be allocated (which they can) but not that e.g. the proper number of epoch blocks are worked on in parallel.
Not sure how this could be checked. Is there some way the logging output could be queried?

@tsbinns
Copy link
Collaborator Author

tsbinns commented Mar 12, 2024

OSF seems to be pretty unstable right now which is causing build_docs to fail.

@larsoner
Copy link
Member

Not sure how this could be checked. Is there some way the logging output could be queried?

Well you could use

with mne.utils.catch_logging(verbose="debug") as log:
    ...
log = log.getvalue()
assert "Got -1" not in log
assert "after requesting -1" in log

or some better check. See the message you should get in

https://github.com/mne-tools/mne-python/blob/90067893e330c1941c055be22bc5f442ad320ec3/mne/parallel.py#L122

But even this would pass on main since this is in parallel_func of MNE. To catch this bug at the MNE-Connectivity level you'd have to get some sort of logging message from mne-connectivity itself, so you'd have to add it.

@tsbinns
Copy link
Collaborator Author

tsbinns commented Mar 12, 2024

@larsoner Okay, so I could add e.g. a debug log message in _get_n_epochs() that n_epochs were allocated to the epoch block?

If you think this is a test worth adding I am happy to look into it.

@larsoner
Copy link
Member

I'm fine either way -- if it's only a few lines and is logger.debug then you can add it if you want. But this is also a pretty clear bugfix to me as is so I'm okay merging now if you want to move on to other stuff

@tsbinns
Copy link
Collaborator Author

tsbinns commented Mar 12, 2024

Sure, then I'm happy to merge as well if you don't think it's essential.

@larsoner larsoner merged commit 8833305 into mne-tools:main Mar 12, 2024
11 checks passed
@larsoner
Copy link
Member

Thanks @tsbinns !

@tsbinns tsbinns deleted the pr-bugfix_njobs branch March 12, 2024 15:56
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.

2 participants