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
66 stars 34 forks source link

[MRG] Add dPLI connectivity measure #79

Closed kenjimarshall closed 2 years ago

kenjimarshall commented 2 years ago

PR Description

Closes: #77

Implementation of dPLI added to the spectral_connectivity_epochs module.

Merge checklist

Maintainer, please confirm the following before merging:

adam2392 commented 2 years ago

Hi @kenjimarshall thanks for the contribution! Do you mind adding a few extra things?

It would be nice to be able to communicate to users what are the (nuanced) differences pros/cons of dPLI vs other related measures. Perhaps the title of the example could be something along the lines of Directed vs Weighted Phase Lag Index

kenjimarshall commented 2 years ago

Hi @adam2392 yes I'll work on the example! And update the whats_new.rst.

As per the unit tests, all the errors are from test_spectral_connectivity_time_resolved and test_time_resolved_spectral_conn_regression whenever the multitaper method is used. In particular, a shape mismatch keeps getting raised at line 219 in time.py.

E.g.

___________ test_spectral_connectivity_time_resolved[multitaper-coh] ___________
mne_connectivity/spectral/tests/test_spectral.py:476: in test_spectral_connectivity_time_resolved
    data, freqs=freqs, method=method, mode=mode)
mne_connectivity/spectral/time.py:219: in spectral_connectivity_time
    conn[epoch_idx, ...] = np.stack(conn_tr, axis=1)
E   ValueError: shape mismatch: value array of shape (2,6,3,18,256) could not be broadcast to indexing result of shape (2,6,18,256)

I'm not sure if/how my changes are causing this, but I can take a closer look at it if you'd like.

adam2392 commented 2 years ago

As per the unit tests, all the errors are from test_spectral_connectivity_time_resolved and test_time_resolved_spectral_conn_regression whenever the multitaper method is used. In particular, a shape mismatch keeps getting raised at line 219 in time.py.

___________ test_spectral_connectivity_time_resolved[multitaper-coh] ___________
mne_connectivity/spectral/tests/test_spectral.py:476: in test_spectral_connectivity_time_resolved
    data, freqs=freqs, method=method, mode=mode)
mne_connectivity/spectral/time.py:219: in spectral_connectivity_time
    conn[epoch_idx, ...] = np.stack(conn_tr, axis=1)
E   ValueError: shape mismatch: value array of shape (2,6,3,18,256) could not be broadcast to indexing result of shape (2,6,18,256)

Is dPLI a symmetric measure, or is it asymmetric because it is directed? If so, then it's because the data structure constructed assumes a symmetric array. I can take a look later

kenjimarshall commented 2 years ago

Yes dPLI is asymmetric, so that could be it. Let me know how I should approach this.

adam2392 commented 2 years ago

Hmmm... weird. I cannot reproduce the error locally.

Can you give me permissions on your fork account? In the meantime, feel free to work on the example.

kenjimarshall commented 2 years ago

I just gave you write permissions on the fork. I will work on the example in the meantime.

adam2392 commented 2 years ago

@kenjimarshall I believe I fixed the issue. It was unrelated to your additions.

The spectral connectivity over time has some issues, and are being sorted out right now still in #73

kenjimarshall commented 2 years ago

Great thank you!

kenjimarshall commented 2 years ago

I'm getting some warnings when I build the docs locally for spectral_connectivity_time

WARNING: [numpydoc] Validation warnings while processing docstring for 'mne_connectivity.spectral_connectivity_time':
  PR01: Parameters {'sm_kernel'} not documented
  PR02: Unknown parameters {'kernel'}
  PR09: Parameter "sm_freqs" description should finish with "."

I just made those minor changes to fix circleCI.

adam2392 commented 2 years ago

@kenjimarshall I've merged in extra changes from main branch here. Ping me when you want me to review the example

adam2392 commented 2 years ago

Hi @kenjimarshall any challenges finishing this off?

kenjimarshall commented 2 years ago

Hi @adam2392 thanks for checking in. I'm actually just going through final revisions today and should have it submitted either today or tomorrow.

kenjimarshall commented 2 years ago

Hi @adam2392 -- the example is now included

adam2392 commented 2 years ago

Great start and contribution @kenjimarshall ! I'm pinging some other folks here to take a look, since examples should always be pretty generally understandable by a new user/viewer.

This might go through some reviews if you're okay with that. Just want to iterate to make sure the example is as good as possible before merging in.

kenjimarshall commented 2 years ago

Hi @adam2392, I just wanted to check in on this PR. Let me know if there are any other changes I can make while the example awaits further review.

adam2392 commented 2 years ago

https://output.circle-artifacts.com/output/job/c5dd2ea3-543e-4542-9cd9-aad54a3459c6/artifacts/0/dev/auto_examples/dpli_wpli_pli.html#sphx-glr-auto-examples-dpli-wpli-pli-py

@agramfort @britta-wstnr @larsoner any thoughts?

adam2392 commented 2 years ago

I'll give the other MNE folks some time in case they wanted to check it out before merging in by end of the day today, but it looks good to me.

adam2392 commented 2 years ago

Thanks @kenjimarshall !

kenjimarshall commented 2 years ago

My pleasure!