-
Notifications
You must be signed in to change notification settings - Fork 34
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
[ENH] Add support for multivariate coherency method (canonical coherence) #163
Conversation
This reverts commit 3d281ff.
Just to add, we started work on this when v0.6 was still in development, so we had to format the already-written code with black, hence the new entry in mne-connectivity/.git-blame-ignore-revs Lines 1 to 2 in f9ff2ac
Regarding the unit tests:
All unit tests are passing on our local machines, and we haven't made any changes to the dependencies or the visualisation, so I'm not sure where this comes from. I anybody has come across this before and knows the solution that would be great, otherwise I will try to address this in the coming weeks. |
Is it possible to rebase and/or just start a new branch based off |
For I think in general, we can just maintain |
@adam2392 Trying to rebase this was a mess, was much easier for me to just revert the change (authorship for some of the CaCoh is a bit messed up, but the rest of the package is unaffected). |
Agreed. Feel free to open up a separate GH Issue or PR then? |
Co-authored-by: Adam Li <adam2392@gmail.com>
This reverts commit f87edae.
I've merged in #173, which I think will simplify the diff now |
Thanks @adam2392. I merged main into this branch now so diff for examples is simplified. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m okay merging this now and slating a release for the next version!
I suppose one outstanding point would be whether people still feel the extended examples we have written be instead labelled as 'tutorials'. |
MNE-Connectivity doesn't separate "examples" from "tutorials" the way that MNE-Python does. So I'd say that's a larger conversation about docs restructuring that should be done in a separate issue / PR. |
This PR adds support for another multivariate connectivity method (canonical coherence; CaCoh), taking advantage of the multivariate framework implemented in v0.6. It is authored by myself and @tsbinns.
Method paper: Vidaurre et al. (2019)
As mentioned in #155, canonical coherence is a multivariate method based on coherency. It shares a number of similarities with the recently added methods MIC & MIM, but it is a more appropriate method to use for datasets where you don't expect volume conduction artefacts to be a big issue (e.g. connectivity between: cortical and subcortical recordings; cortex & EMG recordings, etc...).
Members of my own and other groups would be very interested in having this method implemented in MNE, as currently there is no implementation in any signal processing package (Python or MATLAB; only some MATLAB scripts provided by the authors on request).
Changes
Because CaCoh and MIC/MIM are based on similar principles, several bits of code can be shared between them. Accordingly, we:
Refactored the
_MultivariateCohEstBase
class to accommodate all multivariate coherency methods (CaCoh, MIC, & MIM):mne-connectivity/mne_connectivity/spectral/epochs_multivariate.py
Lines 179 to 185 in f9ff2ac
Introduced a new class
_MultivariateImCohEstBase
for code specific to MIC & MIMmne-connectivity/mne_connectivity/spectral/epochs_multivariate.py
Lines 341 to 346 in f9ff2ac
Introduced a new class
_CaCohEst
for code specific to CaCohmne-connectivity/mne_connectivity/spectral/epochs_multivariate.py
Lines 470 to 475 in f9ff2ac
There are no API changes (beyond the ability to specify a new method).
Unit Tests
Unit tests for CaCoh have been implemented similarly to the other multivariate methods, which in most cases was just the addition of the
"cacoh"
method parameter to existing tests.Notably, a regression test based on results from MATLAB code of the implementation was added, as for the other multivariate methods.
Outstanding Tasks
So far, the only thing missing is an example notebook. The format would be similar to that for MIC & MIM, and would ideally involve a dataset with recordings from two distinct sites (e.g. EEG & EMG, or EEG & subcortical LFPs).
We had a brief look through the MNE datasets, but didn't notice anything that quite fit this. Do you know if there is one like this we missed? If not, we can always show an example with some simulated data.
Any suggestions for the example data or code changes are of course very welcome!