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

[ENH, WIP] Add multivariate connectivity methods #138

Merged
merged 67 commits into from
Jul 18, 2023

Conversation

tsbinns
Copy link
Collaborator

@tsbinns tsbinns commented Apr 23, 2023

@adam2392 Sorry for the delay, but I finally have something I think we can move forward with.

What I have implemented?

  • Added multivariate measures of the imaginary part of coherency (maximised imaginary coherence, MIC, and multivariate interaction measure, MIM; this includes spatial patterns of connectivity for MIC).
  • Added multivariate spectral Granger causality (GC). Also made a slight change to the implementation which keeps the final results the same, but significantly cleans up the code.
  • These methods have been implemented in the existing spectral_connectivity_epochs function, with the original structure of e.g. the indices parameter (i.e. no support for ragged indices added yet, as we discussed).
  • Updated the documentation of spectral_connectivity_epochs to reflect these new measures.
  • Added new attributes to the connectivity classes. These are used to store information associated with the multivariate measures (rank, n_lags, and patterns).
  • Support for parallelisation of these methods using the MNE wrapper for joblib is present.
  • Added a (basic) set of unit tests for the new measures.
  • Extensive examples for the newly-added measures. These include background information/explanations of the methods, demonstrations of how to use the various settings in the implementations, how to interpret the results, what the limitations of these methods are, etc...

What are the limitations?

  • Only bivariate or multivariate methods can be requested at any one time. This relates to the existing spectral_connectivity_epochs function making assumptions about the number of signals in the seeds/targets matching the number of connections, which is not the case for the multivariate methods at this time. It is quite easy (and clean) to adapt the functions to handle this differently when only multivariate methods are called, however things become much trickier when trying to work with bivariate and multivariate methods simultaneously. If you would like the ability to compute bivariate and multivariate methods simultaneously to be included, perhaps we can adapt some of the functions to make this easier, otherwise we could stick with the current limitation I have imposed.
  • Only one multivariate connection can be computed at any one time. This is not possible without changing the stucture of indices.
  • When computing multivariate connectivity, the number of seeds and targets for this single connection must be identical. Again, this is not possible without allowing indices be ragged.
  • Support for computing GC on frequency bands is not yet possible. This relates to the cross-frequency nature of GC measures. I had a solution which I had implemented in the code for my previous PR, however I think it would be good to discuss this particular issue before proceeding.

What have I not added?

  • No unit tests for checking whether the methods are "correct". I remember we discussed this in terms of adding some simulated data with a known connectivity distribution and testing the methods using this data. This is still on my to-do list; I just need to figure out how I could best do this with the smallest amount of data possible (so as not to bloat the package).
  • No version of the methods in spectral_connectivity_time. Again, this is still on my to-do list. I have structured the MIC/MIM and GC computation code in a way that we could very easily re-use the code I have added here and just switch out the time dimension (used in case of time-frequency modes) to store epoch information, so I do not consider this a major hurdle.

There is, however, a bigger issue: the assertion of the connectivity object in test_spectral_connectivity_parallel matching that which has been saved and then reloaded now fails, because there is ~1 kB size difference in the objects. As far as I can tell, all of the critical information in the objects are identical (e.g. the results are the same, all entries in attrs are the same). Interestingly, this only occurs for the multitaper and fourier modes. I checked in the original test, and the size of the saved and reloaded connectivity object is also not a perfect match, however it seems to be rounded to the same "~XX kB". My hunch would be that the attrs I added to the connectivity classes (rank, n_lags, and patterns), even when unfilled, are pushing the estimated size in the repr to be rounded up when combined with the pre-existing small difference in size that occurs when saving and reloading a connectivity object. Even though I think everything is in order, I am clearly not happy with this test suddenly failing, so if you have any ideas as to why this is happening, I would really appreciate it!

The test in question:
https://github.com/tsbinns/mne-connectivity/blob/100e235c8520664510401d73394df1c953a6721f/mne_connectivity/spectral/tests/test_spectral.py#L134-L145

An example of the reprs from the test with the multitaper mode:
image

What are the next steps?

Adding further unit tests, as well as implementing these measures in spectral_connectivity_time are my two biggest targets. Once I have your input, I am happy to also start addressing the limitations I listed (e.g. by adding support for ragged indices). Making sure all tests are passing would also be nice!

One again, I am very sorry for the delay (a lot of other work got in the way), but I am still very excited to move forward with this! If there is anything you think would be best discussed over a call, I am very happy to do that again. I will also answer anything here as soon as I can.

Cheers,
Thomas

@tsbinns
Copy link
Collaborator Author

tsbinns commented May 21, 2023

DONE:

  • removed the explicit attributes for multivariate info, now just stored in attrs
  • general improvements to make the examples clearer
  • general documentation updates
  • fixed the bugs that were causing tests to fail

IN PROGRESS:

  • adding the methods to spectral_connectivity_time

@adam2392 adam2392 self-requested a review May 23, 2023 15:06
@adam2392
Copy link
Member

  • adding the methods to spectral_connectivity_time

Hi @tsbinns how is this part going? Would you like me to review anything? Thanks!

@tsbinns
Copy link
Collaborator Author

tsbinns commented Jun 22, 2023

  • adding the methods to spectral_connectivity_time

Hi @tsbinns how is this part going?

Hi @adam2392, this is still a WIP, however from Monday I have 2 full weeks to dedicate to this, so it will be finished then. I will tag you once I have it and all tests are passing.

Would you like me to review anything?

I would be interested to hear your thoughts on the examples I added (mic_mim.py and granger_causality.py) and documentation changes I made for spectral_connectivity_epochs. Do you think there are any improvements I could make? Cheers!

commit 44e21fe
Author: Thomas Samuel Binns <[email protected]>
Date:   Fri Jun 30 14:43:29 2023 +0200

    updated GC example

commit 648bb13
Author: Thomas Samuel Binns <[email protected]>
Date:   Fri Jun 30 13:59:53 2023 +0200

    updated docs

commit 21a61a1
Author: Thomas Samuel Binns <[email protected]>
Date:   Fri Jun 30 13:22:25 2023 +0200

    updated docs

commit 767719a
Author: Thomas Samuel Binns <[email protected]>
Date:   Fri Jun 30 13:03:41 2023 +0200

    bug fix patterns indexing

commit cd06739
Author: Thomas Samuel Binns <[email protected]>
Date:   Fri Jun 30 12:51:20 2023 +0200

    updated tests

commit 13335f8
Author: Thomas Samuel Binns <[email protected]>
Date:   Fri Jun 30 12:05:08 2023 +0200

    updated tests

commit 962a1ac
Author: Thomas Samuel Binns <[email protected]>
Date:   Thu Jun 29 16:49:53 2023 +0200

    bug fixes

commit 35f8b1f
Author: Thomas Samuel Binns <[email protected]>
Date:   Thu Jun 29 16:02:34 2023 +0200

    bug fix patterns dims

commit 754dfc7
Author: Thomas Samuel Binns <[email protected]>
Date:   Thu Jun 29 15:38:54 2023 +0200

    bug fixes

commit 9098b2e
Author: Thomas Samuel Binns <[email protected]>
Date:   Thu Jun 29 15:38:41 2023 +0200

    updated tests

commit 9dbfa3b
Author: Thomas Samuel Binns <[email protected]>
Date:   Tue Jun 27 17:55:04 2023 +0200

    removed redundant rank errors

commit 42d7097
Author: Thomas Samuel Binns <[email protected]>
Date:   Tue Jun 27 17:50:58 2023 +0200

    updated examples

commit 2a8b711
Author: Thomas Samuel Binns <[email protected]>
Date:   Tue Jun 27 17:44:05 2023 +0200

    updated time tests

commit 0c4d668
Author: Thomas Samuel Binns <[email protected]>
Date:   Tue Jun 27 17:43:43 2023 +0200

    update mvc implementation

commit 9f3d533
Author: Thomas Samuel Binns <[email protected]>
Date:   Tue Jun 27 17:42:46 2023 +0200

    bug fix rank estimation

commit 213e586
Author: Thomas Samuel Binns <[email protected]>
Date:   Tue Jun 27 17:42:32 2023 +0200

    bug fix results storage

commit aa4598a
Author: Thomas Samuel Binns <[email protected]>
Date:   Tue Jun 27 17:41:48 2023 +0200

    bug fix mic edge case

commit 1d8250e
Author: Thomas Samuel Binns <[email protected]>
Date:   Tue Jun 27 17:41:12 2023 +0200

    updated patterns format

commit e018586
Author: Thomas Samuel Binns <[email protected]>
Date:   Tue Jun 27 17:26:18 2023 +0200

    bug fix get_data

commit 3f9062e
Author: Thomas Samuel Binns <[email protected]>
Date:   Mon Jun 26 18:54:39 2023 +0200

    added mvc methods

commit f2e4dad
Author: Thomas Samuel Binns <[email protected]>
Date:   Mon Jun 26 18:54:22 2023 +0200

    bug fix mic

commit 2778693
Author: Thomas Samuel Binns <[email protected]>
Date:   Mon Jun 26 18:54:10 2023 +0200

    updated examples
@tsbinns
Copy link
Collaborator Author

tsbinns commented Jun 30, 2023

Hi @adam2392, I have now:

  • added support for the methods in spectral_connectivity_time
  • added tests for the methods in both spectral_connectivity_epochs and time (including a regression test for some example data for comparing results against the established MATLAB implementation of the methods)
  • fixed some bugs which I had previously missed

In terms of the goals of this PR I believe that is everything finished, assuming it is up to standard. I would be very grateful for any feedback!

Copy link
Member

@adam2392 adam2392 left a comment

Choose a reason for hiding this comment

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

Hi @tsbinns sorry for the delay!

I left some notes. This is looking good imo! The examples are a nice touch because for someone that doesn't understand what these methods were previously, I feel like I'm learning something.

The main nits I have are unit-test coverage. You can just add separate unit-tests to achieve these. Other things I reviewed are just nits on explaining a concept and documentation.

I think once these are addressed, maybe a few more small changes. Overall though the high-level is fine w/ me and once it's in the hands of users for v0.6, we'll see how it goes :)

mne_connectivity/base.py Show resolved Hide resolved
examples/granger_causality.py Outdated Show resolved Hide resolved
examples/granger_causality.py Show resolved Hide resolved
examples/granger_causality.py Outdated Show resolved Hide resolved
mne_connectivity/spectral/time.py Show resolved Hide resolved
mne_connectivity/spectral/time.py Show resolved Hide resolved
mne_connectivity/spectral/time.py Show resolved Hide resolved
mne_connectivity/spectral/time.py Show resolved Hide resolved
mne_connectivity/spectral/time.py Show resolved Hide resolved
@tsbinns
Copy link
Collaborator Author

tsbinns commented Jul 17, 2023

Thanks very much for the feedback @adam2392! I believe I have addressed everything, but please unresolve any comments you think I should work on further.

Also please let me know when you think it's appropriate for me to update the whats_new.rst.

Cheers!

@adam2392
Copy link
Member

@tsbinns feel free to add a changelog entry rn that summarizes this major feature effort! Follow the pattern there to link your name/github page.

I will take a look later in-depth on the code.

Copy link
Member

@adam2392 adam2392 left a comment

Choose a reason for hiding this comment

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

I think this LGTM once some final nits on grammar is fixed.

examples/mic_mim.py Outdated Show resolved Hide resolved
examples/mic_mim.py Outdated Show resolved Hide resolved
examples/mic_mim.py Outdated Show resolved Hide resolved
examples/mic_mim.py Outdated Show resolved Hide resolved
examples/granger_causality.py Outdated Show resolved Hide resolved
examples/granger_causality.py Outdated Show resolved Hide resolved
@adam2392
Copy link
Member

LGTM. Thanks @tsbinns and team!

@drammock
Copy link
Member

congrats on getting this one merged in! great effort.

@tsbinns tsbinns deleted the pr-2023_04_23 branch November 25, 2023 12:12
tsbinns added a commit to tsbinns/mne-connectivity that referenced this pull request Dec 15, 2023
* add multivariate con base class
* add multivariate imcoh classes
* add multivar methods

---------
Author: Thomas Samuel Binns <[email protected]>
Co-authored-by: Adam Li <[email protected]>
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.

3 participants