WIP, BUG: Make plot_evoked() not plot GFP for single channel#8774
Merged
cbrnr merged 6 commits intomne-tools:masterfrom Jan 23, 2021
Merged
WIP, BUG: Make plot_evoked() not plot GFP for single channel#8774cbrnr merged 6 commits intomne-tools:masterfrom
cbrnr merged 6 commits intomne-tools:masterfrom
Conversation
drammock
approved these changes
Jan 22, 2021
Member
drammock
left a comment
There was a problem hiding this comment.
changes look reasonable. +1 for merge when CIs are happy
Member
Author
|
Please do not merge yet, I figured there's actually a bug in how we calculate GFP. |
hoechenberger
added a commit
to hoechenberger/mne-python
that referenced
this pull request
Jan 23, 2021
The reason for mne-tools#8772 is simply that we always calculated the GFP incorrectly, i.e. without re-referencing sensor signals to average first. When doing that, I now correctly get a flat GFP (0 at all time points) for single-channel recordings. Closes mne-tools#8772, mne-tools#8774
agramfort
approved these changes
Jan 23, 2021
Member
|
@cbrnr merge if happy |
Contributor
|
Thanks @hoechenberger! |
Member
Author
|
Oh noes this wasn't supposed to be merged… can we revert? |
Contributor
|
Can you open a new PR to revert? Sorry about that, I thought you'd addressed the issue (the PR was not a draft and @agramfort said OK to merge). |
Member
Author
|
Super weird, Was this simply reset once @agramfort submitted his review? |
hoechenberger
added a commit
to hoechenberger/mne-python
that referenced
this pull request
Jan 23, 2021
…ne-tools#8774)" This reverts commit ae8a466.
hoechenberger
added a commit
that referenced
this pull request
Jan 23, 2021
Contributor
|
Hmmm, must've been otherwise it wouldn't have let me merge... Very strange! |
Member
|
It's my fault.... Sorry and thanks for reverting. |
hoechenberger
added a commit
to hoechenberger/mne-python
that referenced
this pull request
Jan 23, 2021
The reason for mne-tools#8772 is simply that we always calculated the GFP incorrectly, i.e. without re-referencing sensor signals to average first. When doing that, I now correctly get a flat GFP (0 at all time points) for single-channel recordings. Closes mne-tools#8772, mne-tools#8774
larsoner
added a commit
that referenced
this pull request
Jan 27, 2021
* GFP was not derived from average-referenced signal as it must be The reason for #8772 is simply that we always calculated the GFP incorrectly, i.e. without re-referencing sensor signals to average first. When doing that, I now correctly get a flat GFP (0 at all time points) for single-channel recordings. Closes #8772, #8774 * Add GFP to EEG tutorial [skip azp][skip github] * Fix doc build [skip azp][skip github] * style [skip azp][skip github] * phrasing [skip github][skip azp] * Small fixes [skip azp][skip github] * Apply suggestions from code review [skip azp][skip github] Co-authored-by: Daniel McCloy <dan@mccloy.info> * Fix [skip azp][skip github] * Small fixes + adjust color [skip azp][skip github] * Add gfp='power', gfp='power-only' * Better docstring rendering * use np.linalg.norm Co-authored-by: Alexandre Gramfort <alexandre.gramfort@m4x.org> * power -> rms * GFP for EEG, RMS for MEG * Touch Evoked tutorial * Update changelog * Fix tests * FIX: Doc and test * Frobenius norm -> RMS Co-authored-by: Daniel McCloy <dan@mccloy.info> Co-authored-by: Alexandre Gramfort <alexandre.gramfort@m4x.org> Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
larsoner
added a commit
that referenced
this pull request
Jan 27, 2021
* GFP was not derived from average-referenced signal as it must be The reason for #8772 is simply that we always calculated the GFP incorrectly, i.e. without re-referencing sensor signals to average first. When doing that, I now correctly get a flat GFP (0 at all time points) for single-channel recordings. Closes #8772, #8774 * Add GFP to EEG tutorial [skip azp][skip github] * Fix doc build [skip azp][skip github] * style [skip azp][skip github] * phrasing [skip github][skip azp] * Small fixes [skip azp][skip github] * Apply suggestions from code review [skip azp][skip github] Co-authored-by: Daniel McCloy <dan@mccloy.info> * Fix [skip azp][skip github] * Small fixes + adjust color [skip azp][skip github] * Add gfp='power', gfp='power-only' * Better docstring rendering * use np.linalg.norm Co-authored-by: Alexandre Gramfort <alexandre.gramfort@m4x.org> * power -> rms * GFP for EEG, RMS for MEG * Touch Evoked tutorial * Update changelog * Fix tests * FIX: Doc and test * Frobenius norm -> RMS Co-authored-by: Daniel McCloy <dan@mccloy.info> Co-authored-by: Alexandre Gramfort <alexandre.gramfort@m4x.org> Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #8772