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, DOC: add EEG referencing tutorial #6648

Merged
merged 15 commits into from
Aug 28, 2019
Merged

Conversation

drammock
Copy link
Member

@drammock drammock commented Aug 8, 2019

This is ready for review but should probably wait until a solution to #6647 is merged in.

@drammock drammock changed the title DOC: add EEG referencing tutorial WIP, DOC: add EEG referencing tutorial Aug 9, 2019
@codecov
Copy link

codecov bot commented Aug 9, 2019

Codecov Report

Merging #6648 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #6648      +/-   ##
==========================================
- Coverage   89.48%   89.48%   -0.01%     
==========================================
  Files         420      420              
  Lines       75580    75582       +2     
  Branches    12381    12382       +1     
==========================================
  Hits        67635    67635              
- Misses       5138     5139       +1     
- Partials     2807     2808       +1

@drammock
Copy link
Member Author

here's the revised rendering: https://14689-1301584-gh.circle-artifacts.com/0/dev/auto_tutorials/preprocessing/plot_55_setting_eeg_reference.html

@wmvanvliet thanks for the feedback, I've re-worked the introduction and I think it's much improved. @cbrnr please check whether I've explained add_reference_channels to your satisfaction.

@cbrnr
Copy link
Contributor

cbrnr commented Aug 14, 2019

Yes, very nice! There's a small formatting error in "signal without restoring the signal at EEG 999)" (the closing parenthesis shouldn't be there and EEG 999 should be in code font).

@drammock
Copy link
Member Author

@cbrnr yep, that formatting error is already fixed in 3aea686

@drammock
Copy link
Member Author

@agramfort
Copy link
Member

agramfort commented Aug 16, 2019

@cbrnr feel free to merge if you're happy.

@drammock
Copy link
Member Author

This should wait for a solution to #6647, which I hope to have today

@cbrnr
Copy link
Contributor

cbrnr commented Aug 16, 2019

OK, please ping me if it works.

@jona-sassenhagen
Copy link
Contributor

Looks good so far.

@drammock
Copy link
Member Author

NB: after #6670 is merged, this should get rebased and have the Circle rendering checked.

@drammock
Copy link
Member Author

rebased to incorporate #6670, let's see if the docs look OK.

@drammock
Copy link
Member Author

@drammock drammock changed the title WIP, DOC: add EEG referencing tutorial MRG, DOC: add EEG referencing tutorial Aug 27, 2019
@cbrnr
Copy link
Contributor

cbrnr commented Aug 28, 2019

I've pushed a few minor improvements. This tutorial is really nice, there's just one point I'd like to see addressed before merging. The last section talks about why average referenced data is important for source imaging, but it strongly recommends the "average-reference-as-projection approach". However, the reasons given are just reasons for the average reference in general. I really miss an explanation why users should prefer projection=True in this scenario.

Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

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

@cbrnr merge if you're happy

@drammock
Copy link
Member Author

@cbrnr are you happy with the new wording?

@cbrnr cbrnr merged commit dcf0131 into mne-tools:master Aug 28, 2019
@cbrnr
Copy link
Contributor

cbrnr commented Aug 28, 2019

Yes! Thanks @drammock!

@drammock drammock deleted the tut-eeg-ref branch August 29, 2019 21:13
alexrockhill pushed a commit to alexrockhill/mne-python that referenced this pull request Oct 1, 2019
* add EEG referencing tutorial

* fix sentence fragment

* clarify warning

* WIP add section on custom refs

* capitalization

* fix ch. name too long

* address Marijn's comments; fix discussion of mne.add_reference_channels

* fix spelling typo

* fix italics

* make plots easier to read

* STY: slightly better prose flow

* address Jona's comments

* address Clemens's comments; other minor tweaks

* Minor improvements

* clarify avg. ref. proj. reasoning
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.

6 participants