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

[MRG] Add dPLI connectivity measure #79

Merged
merged 22 commits into from
Mar 30, 2022
Merged

[MRG] Add dPLI connectivity measure #79

merged 22 commits into from
Mar 30, 2022

Conversation

kenjimarshall
Copy link
Contributor

PR Description

Closes: #77

Implementation of dPLI added to the spectral_connectivity_epochs module.

Merge checklist

Maintainer, please confirm the following before merging:

  • All comments resolved
  • This is not your own PR
  • All CIs are happy
  • PR title starts with [MRG]
  • whats_new.rst is updated
  • PR description includes phrase "closes <#issue-number>"

@adam2392
Copy link
Member

adam2392 commented Feb 10, 2022

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

  • can you add a change entry to the what's_new.rst file inside doc/?
  • take a look at the breaking unit tests?
  • would you be willing to contribute a short example inside examples/ showing the difference (perhaps a simulation similar to one of the existing examples, or in the attached paper) between dPLI and PLI?

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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

@adam2392
Copy link
Member

adam2392 commented Feb 10, 2022

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
Copy link
Contributor Author

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

@adam2392
Copy link
Member

@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
Copy link
Contributor Author

Great thank you!

@kenjimarshall
Copy link
Contributor Author

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
Copy link
Member

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

@adam2392
Copy link
Member

adam2392 commented Mar 2, 2022

Hi @kenjimarshall any challenges finishing this off?

@kenjimarshall
Copy link
Contributor Author

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
Copy link
Contributor Author

Hi @adam2392 -- the example is now included

@adam2392 adam2392 requested a review from larsoner March 4, 2022 17:26
@adam2392
Copy link
Member

adam2392 commented Mar 4, 2022

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
Copy link
Contributor Author

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.

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.

LGTM!

@adam2392
Copy link
Member

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 adam2392 merged commit 2d47ae7 into mne-tools:main Mar 30, 2022
@adam2392
Copy link
Member

Thanks @kenjimarshall !

@adam2392 adam2392 deleted the nf_dpli branch March 30, 2022 21:13
@kenjimarshall
Copy link
Contributor Author

My pleasure!

tsbinns pushed a commit to tsbinns/mne-connectivity that referenced this pull request Dec 15, 2023
* Add and document DPLIEst_ class in epochs.py

* Add DPLI parametrization to test_spectral_connectivity

* Fix citation

* Fix circleCI

* Adding fix

* Fixed pep

* Fix doc for sm_kernel and sm_freqs in spectral_connectivity_time

* Add wPLI/dPLI/PLI example

* Update whatsnew

* Update mne_connectivity/spectral/time.py

Co-authored-by: Adam Li <adam2392@gmail.com>

* Update mne_connectivity/spectral/time.py

Co-authored-by: Adam Li <adam2392@gmail.com>

* Add imaginary and heaviside definitions

* Define sgn

* Update mne_connectivity/spectral/time.py

* Fix lagging/leading plot

* Update doc/whats_new.rst

Co-authored-by: Adam Li <adam2392@gmail.com>
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.

Adding the dPLI connectivity measure
2 participants