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

[DOC] Example comparing spectral connectivity computed over time or over trials #129

Merged
merged 17 commits into from
Mar 10, 2023

Conversation

Avoide
Copy link
Contributor

@Avoide Avoide commented Feb 28, 2023

PR Description

This PR adds an example comparing spectral connectivity over time or trials and closes #70

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>"

@Avoide Avoide marked this pull request as draft February 28, 2023 14:55
@Avoide Avoide marked this pull request as ready for review February 28, 2023 14:55
@Avoide
Copy link
Contributor Author

Avoide commented Feb 28, 2023

Hi @adam2392, here is a draft for an example comparing spectral connectivity computed over time or over trials on simulated data and sample EEG data.

@adam2392 adam2392 enabled auto-merge (squash) February 28, 2023 17:56
@adam2392 adam2392 disabled auto-merge February 28, 2023 17:56
@adam2392 adam2392 self-requested a review March 2, 2023 19:23
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.

Thanks for the PR @Avoide ! I left some preliminary review comments to re-organize the code and notes a bit better.

There are also some style failures on the CI. Do you mind fixing those? You can run make pep to check these locally.

@Avoide
Copy link
Contributor Author

Avoide commented Mar 3, 2023

@adam2392 I should have fixed the code style errors and have updated the places you mentioned in the review in the new commit

@Avoide Avoide requested a review from adam2392 March 3, 2023 10:15
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.

There are some errors on the CI still: https://github.com/mne-tools/mne-connectivity/actions/runs/4322408221/jobs/7573270726. You can check these locally running make pep.

Hmm... for some reason the CI build docs is not working. Can you investigate? Does it build properly on your local?

@Avoide
Copy link
Contributor Author

Avoide commented Mar 5, 2023

There are some errors on the CI still: https://github.com/mne-tools/mne-connectivity/actions/runs/4322408221/jobs/7573270726. You can check these locally running make pep.

Hmm... for some reason the CI build docs is not working. Can you investigate? Does it build properly on your local?

hmm I didn't see those spelling errors when I ran make pep locally. But I just fixed them according to the error on the CI job in the newest commit. Hopefully that should work.

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.

Looking good! Leaving some more in-depth review comments now.

For now, the cirlceCI is failing due to some upstream issues in mne-python. Can you add mne-qt-browser to the requirements_doc.txt file for the circleCI to correctly build the documentation?

So I'm leaving some additional comments based on rendering the docs locally. Is it possible to add colorbars to the plots?

examples/compare_connectivity_over_time_over_trial.py Outdated Show resolved Hide resolved
examples/compare_connectivity_over_time_over_trial.py Outdated Show resolved Hide resolved
requirements_doc.txt Outdated Show resolved Hide resolved
@Avoide
Copy link
Contributor Author

Avoide commented Mar 7, 2023

I updated the example, and also added circleci to the requirement_doc.txt.
There was also a small typo in contributing.md (requirement_doc.text, which I fixed to requirement_doc.txt).

@adam2392
Copy link
Member

adam2392 commented Mar 7, 2023

https://output.circle-artifacts.com/output/job/21c46aa5-0fb7-4326-8cdf-bb256e43ca8a/artifacts/0/dev/auto_examples/compare_connectivity_over_time_over_trial.html#sphx-glr-auto-examples-compare-connectivity-over-time-over-trial-py

  • can you add a label to the colorbars?
  • looks like high connectivity at 0.15-0.2 seconds after the visual task. Are there any references for this?

@adam2392
Copy link
Member

adam2392 commented Mar 8, 2023

Rendered docs: https://output.circle-artifacts.com/output/job/03472a75-4383-47d6-923f-9364a9a1b443/artifacts/0/dev/index.html

Very cool! It looks a lot better now. I'll take a closer look later.

@adam2392
Copy link
Member

adam2392 commented Mar 8, 2023

So it seems that circleCI always blocks your docs build because perhaps you haven't connected your GH account to there :(. Any chance you can just create an account and then the docs build should proceed without someone in the future explicitly running it? (Annoying feature, but I guess there for security) Xref: https://support.circleci.com/hc/en-us/articles/360037744473-What-is-an-Unregistered-User-

Can you also add the colorbar title?

@Avoide
Copy link
Contributor Author

Avoide commented Mar 9, 2023

Sure, I just registered my GH at circleCI. Let's see if it works.

I also added the label "Connectivity" for the colorbars.

Edit: It seems to work automatically now
https://output.circle-artifacts.com/output/job/1a786059-dccd-46c5-a7c2-01a0526d6f42/artifacts/0/dev/auto_examples/compare_connectivity_over_time_over_trial.html

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.

Some final nitpicks and then this LGTM for merge.

Some open-ended comments, so lmk if you disagree.

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

adam2392 commented Mar 9, 2023

If a comment is resolved/addressed, you can feel free to click the "Resolve conversation"

@Avoide
Copy link
Contributor Author

Avoide commented Mar 10, 2023

Thanks for the all comments. And the practical instructions are also useful to know, as this is my first PR.

I have incorporated all your newest comments in the latest commit.

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 adam2392 merged commit 406dd8b into mne-tools:main Mar 10, 2023
@adam2392
Copy link
Member

Thanks @Avoide!

tsbinns pushed a commit to tsbinns/mne-connectivity that referenced this pull request Dec 15, 2023
…ver trials (mne-tools#129)

* Added example doc for con over time vs over trial
* Added mne-qt-browser to req_doc
* Added reference bibliography for mirror-game paradigm and theta power among occipital lobe and frontal lobe

---------
Authored-by: glia <[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.

Example demonstrating difference between spectral connectivity over Epochs or over time
2 participants