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
WIP epochs.plot_image(gfp=True) #4227
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4227 +/- ##
=========================================
- Coverage 86.24% 86.2% -0.05%
=========================================
Files 357 358 +1
Lines 64566 65037 +471
Branches 9831 9922 +91
=========================================
+ Hits 55685 56064 +379
- Misses 6180 6237 +57
- Partials 2701 2736 +35 |
What do people think? Do we even want an I like it because I like "survey"-style plots that give you a super fast impression when called with defaults, and otherwise you always have to specify picks and you don't get the full answer either. Also we should have |
no objection with the gfp image
|
Alternative API suggestion, inspired by the
(vs. Thoughts? |
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.
could you add an image of the plot when you have a chance? would be cool to check it out
@@ -65,7 +65,7 @@ def plot_epochs_image(epochs, picks=None, sigma=0., vmin=None, | |||
The scalings of the channel types to be applied for plotting. | |||
If None, defaults to `scalings=dict(eeg=1e6, grad=1e13, mag=1e15, | |||
eog=1e6)`. | |||
cmap : matplotlib colormap | (colormap, bool) | 'interactive' | |||
cmap : None | matplotlib colormap | (colormap, bool) | 'interactive' |
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.
what does none yield?
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.
Ok, need to doc this ...
The point is, for the GFP we don't need a diverging cmap (None -> Reds), so I've switched from the RdBu_r default to None.
mne/viz/epochs.py
Outdated
gfp : bool | ||
If True, plot the Global Field Power (one figure per channel type). | ||
The GFP is calculated on a single-trial basis for the image, but the | ||
time series plot below shows the GFP of the average across epochs, |
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.
just curious, why on the average of epochs instead of the average of individual GFPs?
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.
No reason I think. I wrote the code this way without thinking, then realized that there are two possible options, and decided to just doc the one I chose. If you think it should go the other way around, I'll do it like that.
I should probably reword it though. It's not the most obvious way of saying "data.std(ch_ax).mean(trial_ax) vs. data.mean(trial_ax).std(ch_ax)
".
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.
"data.std(ch_ax).mean(trial_ax) vs. data.mean(trial_ax).std(ch_ax)".
that clarifies it much better
I like |
YAGNI? will you really use more than GFP? Now on the positive side and
think it's more explicit than GFP jargon :)
|
@agramfort I sometimes stuff model fits or similar stuff into mne insts for plotting convenience. If your values are e.g. r or R^2, the std doesn't make so much sense anymore, but maybe the median or some other metric would. Or what if the data isn't averaged to a common ref anymore, but still on a common scale (e.g. the bipolar stuff)? Essentially a bunch of data transformations might require a different measure. Not saying it's super important, but it's only like 3 lines of code, 2 lines of doc and 2 lines of test so ... |
... any chance of having this one to actually default to Reason: if called without specifying I doubt it would hurt any existing pipelines, because these probably don't rely on leaving |
(This is a very high SNR data set though.) Actually, "inferno" or 'viridis' look much better than "Reds" for a lot of my tests |
Votes on:
I'm for the 1st option in all cases. |
evoked = epochs.average() | ||
for ch_type in types_: | ||
picks_ = list(picks[types == ch_type]) | ||
data_ = data[:, picks_, :].std(1) |
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.
you should not use std as you you don't want to subtract the mean for GFP
giving a try to this PR gives me:
I just tried epochs.plot_image(gfp=True) after running |
how about a group_by param as we have in raw.plot now?
picks will be used to select what channels to consider and if group_by=None
then show individual channels and if group_by='type' show GFP by channel
type. But we could have group_by='selection'
wdyt?
|
|
no it would give you the GFP image per selection of channels like raw.plot
does it to make the butterfly rendering by selection
|
Sorry if misunderstanding conv up to now but was directed here to provide input if possible (and doesn't seem to be in same train of thought? could be wrong) is it possible to have a: epochs.plot_image(average_over_picks = bool) param where instead of plotting all chans in picks, you plot one image of average voltage/field at all chosen channels? sometimes valuable if looking at broader cortical (e.g. visual) response ? something like: e.g. use:
I could be on the wrong path here, but would create new array of single epoch data with only one 'channel' (average of picked channels) which could be stored back as an EpochArray to feed into the original epochs.plot_image code? |
You're essentially asking for the "reduce" or "aggregate" option proposed upthread.
Although usually the GFP is a better measure than the mean for eeg data ...
|
ah ok, makes sense. mean would keep consistency with other non-python packages like FieldTrip, no? useful if people are working in both alongside each other as a sanity check (this is happening in my lab as a couple of us are trying to move people away from matlab...) |
I'm sure if we can convince a few MATLAB users to switch with that, it'll be possible to implement this :) |
@agramfort I finally tried the group_by thing in plot_raw and 1. we should have this for 10/20 data too (it's a predictable mapping from name to ROI) and I'd like to do that eventually, 2. i think that should be parallel to the aggregation. partially because if you have a ROI-like parameter, grouping by ROI means aggregating via the mean actually makes sense and is very common. E.g.,
Also you have to admit switching between grouping channels to aggregating over them via the GFP is really overloading the group_by keyword :) |
Btw @schekroud don't use this style in Python:
Almost always, you can do something like this:
And usually
That's much more readable! In this case, I think you want something like:
or just straight-forward
This would give you the data averaged over channels. My idea for this function is to have:
where |
Also you have to admit switching between grouping channels to aggregating
over them via the GFP is really overloading the group_by keyword :)
what do you mean? I am not sure about the name aggregate but otherwise I
think the API is nice.
|
You mean you'd be okay with something like:
? |
yes
but maybe aggregate is better? no strong feeling. I feel that aggregate is
really geeky...
|
thanks for the tips - only been python-ing for a (relatively) short period so brevity of code is an issue... maybe |
@agramfort I think I'm the only one who prefers aggregate :) We have @schekroud |
+1 for combine
it also matches the split-apply-combine approach of pandas
|
My 2c rankings:
|
we need both group_by and combine
|
Oh god, we need some consensus here :) So what do we call this param? @jaeilepp maybe you have an opinion? |
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.
My vote goes for combine.
ax3 = axes[-1] | ||
evoked = epochs.average(picks) | ||
data = epochs.get_data()[:, picks, :] | ||
axes += axes[-1] |
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.
What is going on here?
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.
Don't review the code right now, I've been putting it on hold until we decide on the API.
Ok, then I'll do |
yes but we agree that we need both group_by and combine right?
|
Should it default to |
no strong feelings, |
I do like "reduce" because it's standard to talk about map-reduce operations, but I think "combine" is more common in MNE so +1 for that from me |
I'm working on this again ... next thing I'll do. |
Did something very weird. I'll open a new PR. |
I'm still annoyed by the fact that epochs.plot_image throws so many figures at you.
This allows one to plot the single-trial GFP.