search

mne-tools / mne-connectivity

Connectivity algorithms that leverage the MNE-Python API.
https://mne.tools/mne-connectivity/dev/index.html
BSD 3-Clause "New" or "Revised" License
68 stars 34 forks source link

[ENH] Add support for multivariate coherency method (canonical coherence) #163

Closed orabe closed 6 months ago

orabe commented 9 months ago

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):

https://github.com/mne-tools/mne-connectivity/blob/f9ff2ac7f1baea1c71d989e909fa5efae3df6b04/mne_connectivity/spectral/epochs_multivariate.py#L179-L185

Introduced a new class _MultivariateImCohEstBase for code specific to MIC & MIM

https://github.com/mne-tools/mne-connectivity/blob/f9ff2ac7f1baea1c71d989e909fa5efae3df6b04/mne_connectivity/spectral/epochs_multivariate.py#L341-L346

Introduced a new class _CaCohEst for code specific to CaCoh

https://github.com/mne-tools/mne-connectivity/blob/f9ff2ac7f1baea1c71d989e909fa5efae3df6b04/mne_connectivity/spectral/epochs_multivariate.py#L470-L475

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!

tsbinns commented 9 months ago

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 .git-blame-ignore-revs:

https://github.com/mne-tools/mne-connectivity/blob/f9ff2ac7f1baea1c71d989e909fa5efae3df6b04/.git-blame-ignore-revs#L1-L2

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.

adam2392 commented 9 months ago

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 .git-blame-ignore-revs:

https://github.com/mne-tools/mne-connectivity/blob/f9ff2ac7f1baea1c71d989e909fa5efae3df6b04/.git-blame-ignore-revs#L1-L2

Is it possible to rebase and/or just start a new branch based off main and copy over the changes, so this is unnecessary?

adam2392 commented 9 months ago

For mne main, https://github.com/mne-tools/mne-python/tree/main/mne/viz/backends it seems there was some restructuring.

I think in general, we can just maintain main consistency with mne main.

tsbinns commented 9 months ago

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 .git-blame-ignore-revs: https://github.com/mne-tools/mne-connectivity/blob/f9ff2ac7f1baea1c71d989e909fa5efae3df6b04/.git-blame-ignore-revs#L1-L2

Is it possible to rebase and/or just start a new branch based off main and copy over the changes, so this is unnecessary?

@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).

adam2392 commented 9 months ago

Okay https://github.com/mne-tools/mne-connectivity/pull/163/commits/8393f5a015ada3c228f01deb37d8f28adec60b55 should now fix the CIs here.

Can @orabe add a changelog entry?

In addition, will there be a relevant example added to illustrate the conceptual pros/cons of using the new method? If easier to do in a downstream PR, that's fine too.

tsbinns commented 9 months ago

In addition, will there be a relevant example added to illustrate the conceptual pros/cons of using the new method? If easier to do in a downstream PR, that's fine too.

Definitely.

Demonstrating the cons of this method versus MIC can easily be done with the same data we used for the MIC example.

Demonstrating the pros of this method really requires a dataset with recordings from two distinct locations, e.g. cortical & subcortical; cortical & muscular. Like we mentioned, if there is no such dataset in MNE we would just simulate something (not as fun!):

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.

adam2392 commented 9 months ago

Demonstrating the pros of this method really requires a dataset with recordings from two distinct locations, e.g. cortical & subcortical; cortical & muscular. Like we mentioned, if there is no such dataset in MNE we would just simulate something (not as fun!):

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.

@larsoner or @agramfort or @drammock or @britta-wstnr any ideas on open source datasets or even ones in mne-testing-data that fits this criterion?

drammock commented 9 months ago

any ideas on open source datasets or even ones in mne-testing-data that fits this criterion?

not off the top of my head. It's possible that one of the motor imagery datasets includes some actual movement trials, but even if so there's no guarantee that there were EMG channels recorded. Or maybe sleep datasets might have EMG or RESP channels?

adam2392 commented 8 months ago

Just checking up on this to see how things are progressing? I suppose if we can't leverage any of the datasets in MNE test-kit, then simulations are always obviously very informative as well if we can show a "pro simulation" and a "con simulation"?

tsbinns commented 8 months ago

I suppose if we can't leverage any of the datasets in MNE test-kit, then simulations are always obviously very informative as well if we can show a "pro simulation" and a "con simulation"?

@adam2392 definitely, and would be easy to simulate such scenarios so I don't see any problems there.

So far though neither of us have had time after the holidays to explore whether @drammock's suggestion for the real data would work. This will also likely be the case for the coming weeks due to some pressing deadlines, but it definitely will be done!

tsbinns commented 8 months ago

Can @orabe add a changelog entry?

Done

https://github.com/mne-tools/mne-connectivity/blob/2a2a8ed498550af424636545d883d602a7452528/doc/whats_new.rst?plain=1#L27

tsbinns commented 8 months ago

Perhaps also add both of you to the CITATION file? https://github.com/mne-tools/mne-connectivity/blob/main/CITATION.cff

Done

https://github.com/mne-tools/mne-connectivity/blob/2a2a8ed498550af424636545d883d602a7452528/CITATION.cff#L23-L28

tsbinns commented 8 months ago

Perhaps also add both of you to the CITATION file? https://github.com/mne-tools/mne-connectivity/blob/main/CITATION.cff


@adam2392 Should this part of the file be updated as well?

https://github.com/mne-tools/mne-connectivity/blob/f8862976b2129e4ce72a5bf040a62d0008cdec5e/CITATION.cff#L31-L33

adam2392 commented 8 months ago

Yes that would be great! Thanks.

tsbinns commented 7 months ago

Sorry fo the delay in getting this sorted. I believe all existing comments have been addressed now.

In addition to these, the changes are:

Any feedback on the new examples is appreciated!

tsbinns commented 7 months ago

Maybe a question better directed to @larsoner (but ofc happy if others have suggestions):

In the new example there are some cells for simulating data & plotting results which are quite long and break up the flow of the example.

Since it's not necessary to read this code to understand the example, it would be great if there was some way to hide these cells in a drop-down which are closed by default, but people can open if they're interested.

I saw there were cell tags for .ipynb files to do this, but I couldn't find a way to translate this to cells in regular .py files.

Not an essential point, but would be nice. Any ideas?

adam2392 commented 7 months ago

In the new example there are some cells for simulating data & plotting results which are quite long and break up the flow of the example.

I noticed that in both examples you wrote, there is a simulating function for simulating data within a certain frequency band. I feel like this data-generating model will be useful in a variety of different examples. It is possible that there are other examples that leverage a similar code to generate data.

Perhaps in a short PR, we can abstract this out to an API actually that mne-connectivity supports. E.g. a function called make_signals_in_freq_bands inside mne_connectivity/datasets/frequency.py? This will also make this PR shorter hopefully and easier to review. WDYT?

tsbinns commented 7 months ago

I noticed that in both examples you wrote, there is a simulating function for simulating data within a certain frequency band. I feel like this data-generating model will be useful in a variety of different examples. It is possible that there are other examples that leverage a similar code to generate data.

Perhaps in a short PR, we can abstract this out to an API actually that mne-connectivity supports. E.g. a function called make_signals_in_freq_bands inside mne_connectivity/datasets/frequency.py? This will also make this PR shorter hopefully and easier to review. WDYT?

Have no problem creating a new PR to implement this.

Just looked through the existing examples and don't seem to be others using the same approach. Essentially though it's the same code as for many of the unit tests, just with some extra options for controlling time delays: https://github.com/mne-tools/mne-connectivity/blob/8652da468637d12b5ae7f9c3ee580cd76a55b3d4/mne_connectivity/spectral/tests/test_spectral.py#L24-L81

In theory this could be abstracted out not only for examples but could also be used for some of the existing simulations in the unit tests. Suppose this is a discussion that could be had in the new PR though.

tsbinns commented 7 months ago

These examples are quite long, so I wonder if it is worth piping the "major" conceptual content into a user guide file? It can be done in another PR I suppose, but conceptually I am thinking:

  • examples are short snippets of code demonstrating usage and basic concepts. Each relevant example can point to the relevant userguide/tutorial
  • user guide / tutorial has in-depth description of the concepts and usage

I think it's a sensible suggestion.

If others agree I could open a new PR already to trial this with the existing MIC example (which is more of a tutorial).

adam2392 commented 7 months ago

Just looked through the existing examples and don't seem to be others using the same approach. Essentially though it's the same code as for many of the unit tests, just with some extra options for controlling time delays: https://github.com/mne-tools/mne-connectivity/blob/8652da468637d12b5ae7f9c3ee580cd76a55b3d4/mne_connectivity/spectral/tests/test_spectral.py#L24-L81

In theory this could be abstracted out not only for examples but could also be used for some of the existing simulations in the unit tests. Suppose this is a discussion that could be had in the new PR though.

Agreed. Feel free to open up a separate GH Issue or PR then?

adam2392 commented 6 months ago

I've merged in #173, which I think will simplify the diff now

tsbinns commented 6 months ago

Thanks @adam2392. I merged main into this branch now so diff for examples is simplified.

tsbinns commented 6 months ago

I suppose one outstanding point would be whether people still feel the extended examples we have written be instead labelled as 'tutorials'.

drammock commented 6 months ago

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.