-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Improve ica.plot_overlay on Raw instances #11830
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you touch some tutorial (make some trivial change to line breaks, wording, etc.) that uses this code so we can see the effect in CircleCI? Hopefully the main ICA tutorial would do it...
Co-authored-by: Eric Larson <[email protected]>
…e-python into ica-plot-overlay
MWE if you want to check on combined M/EEG data:
|
comparing the MAG plot versus main it seems like we have less useful result now (where EEG is better). Maybe we should do:
? |
I think in principle MAG could have the same cancellation problems as EEG where a polarity reversal averaged across all channels (e.g., negative in left hemi and positive in right) could make an artifact look smaller than it is. So even though in this example the ECG for example looks less ECG-like, it seems safer in principle |
We could have a third pane for MAGs with the average. It's true that in this case the average is more informative than the RMS, but as for EEG it will be very case dependent. |
Would it be too complex to have the average shown as well as long as it's not EEG-with-an-average-ref? Like show it for mags, grads, eeg-without-average-ref, etc. |
Sure, I can add back the average except if |
I was thinking you would check if there was an average ref projector and not show in that case. The
I guess we could... but I think as long as we show the STD, the mean being near-zero should make sense if your ref was avg. Users in principle shouldn't be surprised by this |
Yes,
I slightly prefer option (3), restore the average plot for all channel types except EEG. But no strong opinion, any preference? |
I'm fine with (3) or (2). The advantage of (2) is it's a bit more backward-compatible -- people can see the same information they've had previously. So I think I'm +1 on (2) and + 0.5 on (3) |
(2) done, good to go on my side. From the tutorial: |
So in: You can see the average over EEG is non-zero. This suggests it should be shown on this PR. So maybe the check that makes it not be shown needs to be "has average ref and it has been applied to the data" rather than just "has average ref". |
Oops, I'll fix that 🙈 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks pretty good! thanks for tackling this. A few minor suggestions/comments below.
Co-authored-by: Daniel McCloy <[email protected]>
…e-python into ica-plot-overlay
Co-authored-by: Eric Larson <[email protected]> Co-authored-by: Daniel McCloy <[email protected]>
Fixes #11805
Using
ica.plot_overlay
on a raw will now open one figure per channel type, with 2 panes for EEG and MEG data. The second pane now shows the GFP or the RMS instead of the channel average.When used on evoked, the figure is already split by channel type and I don't think it's worth adding GFP/RMS for evoked responses. The difference in the overlay should be clear enough.