Skip to content

MRG: "GFP" trace in STC time viewer is actually RMS#8965

Merged
hoechenberger merged 3 commits intomne-tools:mainfrom
hoechenberger:stc-rms
Feb 27, 2021
Merged

MRG: "GFP" trace in STC time viewer is actually RMS#8965
hoechenberger merged 3 commits intomne-tools:mainfrom
hoechenberger:stc-rms

Conversation

@hoechenberger
Copy link
Copy Markdown
Member

@hoechenberger hoechenberger commented Feb 26, 2021 •
edited
Loading

I've also rearranged the respective formula such that it becomes immediately obvious that we're dealing with RMS values.

See #8775 for our lengthy discussion on GFP vs RMS naming, albeit for sensor-level data.

@hoechenberger hoechenberger added VIZ backport-candidate on-merge: backport to maint/1.12 labels Feb 26, 2021
@hoechenberger hoechenberger force-pushed the stc-rms branch 2 times, most recently from b8e3513 to ab2afcf Compare February 26, 2021 18:23
@hoechenberger hoechenberger marked this pull request as draft February 26, 2021 18:24
@hoechenberger
"GFP" trace in STC time viewer is actually RMS
300bc18 
I've also rearranged the formula such that it becomes
immediately obvious that we're dealing with RMS values.

See mne-tools#8775 for our lengthy discussion on this for sensor-level
data.
@hoechenberger hoechenberger marked this pull request as ready for review February 26, 2021 18:37
@hoechenberger hoechenberger changed the title FIX: "GFP" trace in STC time viewer is actually RMS MRG: "GFP" trace in STC time viewer is actually RMS Feb 27, 2021
@hoechenberger
Copy link
Copy Markdown
Member Author

hoechenberger commented Feb 27, 2021

Rendering looks good:
https://25463-1301584-gh.circle-artifacts.com/0/dev/auto_tutorials/source-modeling/plot_mne_dspm_source_localization.html#visualization

agramfort
agramfort reviewed Feb 27, 2021
View reviewed changes
Copy link
Copy Markdown
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.

Besides lgtm

@agramfort
Copy link
Copy Markdown
Member

agramfort commented Feb 27, 2021 via email

@hoechenberger
Copy link
Copy Markdown
Member Author

hoechenberger commented Feb 27, 2021

@agramfort Ok I've addressed this, also in viz/evoked.py

agramfort
agramfort approved these changes Feb 27, 2021
View reviewed changes
Copy link
Copy Markdown
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.

Thx @hoechenberger 👍

@hoechenberger hoechenberger merged commit fe6775b into mne-tools:main Feb 27, 2021
@hoechenberger hoechenberger deleted the stc-rms branch February 27, 2021 12:33
@larsoner
Copy link
Copy Markdown
Member

larsoner commented Mar 1, 2021

FYI this had CI failures beyond just pip and pip pre, fixed by #8971

@hoechenberger
Copy link
Copy Markdown
Member Author

hoechenberger commented Mar 1, 2021

Thanks for taking care of this, @larsoner

@hoechenberger hoechenberger removed the backport-candidate on-merge: backport to maint/1.12 label Mar 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@agramfort agramfort agramfort approved these changes

Assignees

No one assigned

Labels

VIZ

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@hoechenberger @agramfort @larsoner