[ENH] Use spatial_colors by default in Evoked.plot()
Reference issue
Fixes #10612 with respect to comment
What does this implement/fix?
Adds a default 'auto' option for spatial_colors argument in Evoked.plot(). Which makes spatial_colors True if channel locations exists and False otherwise. Changes docstring in plot_evoked() to include string Adds option chek to plot_evoked for spatial_colors argument
Additional information
Any additional information you think is important.
Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴🏽♂️
style error: https://dev.azure.com/mne-tools/mne-python/_build/results?buildId=21083&view=logs&j=c1ec3add-0fcd-52ea-530a-328457bbfb2e&t=056d958c-96b0-5ff4-dcb2-798466eed616&l=15
before this gets merge, I'd still like to get other devs' opinion on this question:
what should happen when user passes
picks? For example, we probably don't want spatial_colors when user passes just one or a few channels as a pick. But we probably do want spatial_colors when user passespicks='mag'orpicks='eeg'(i.e., all channels of one type). So I think settingspatial_colorsautomatically maybe should take into account the value ofpicksas well.
also, there are a lot of CI errors, here are links to a couple:
- https://dev.azure.com/mne-tools/mne-python/_build/results?buildId=21091&view=logs&j=305851a9-a7bb-55db-0042-7e2b6f48aa1c&t=4d37777d-f36a-53aa-9217-6386d15dddcd&l=3240
- https://dev.azure.com/mne-tools/mne-python/_build/results?buildId=21091&view=logs&j=c04f1cb7-a28e-5e8a-4074-704b3a6c72b1&t=c5a3e5f7-2b1e-57e1-93fb-c99e1ea8bb1a&l=1133
FYI our tests treat any warning as an error, unless the warning is explicitly caught/expected in the test. From a quick glance at the error summary linked above, it appears that you're raising a RuntimeWarning? That probably shouldn't happen... I'll look at your code now.
before this gets merge, I'd still like to get other devs' opinion on this question:
what should happen when user passes
picks? For example, we probably don't want spatial_colors when user passes just one or a few channels as a pick. But we probably do want spatial_colors when user passespicks='mag'orpicks='eeg'(i.e., all channels of one type). So I think settingspatial_colorsautomatically maybe should take into account the value ofpicksas well.
plot_joint() always uses spatial colors, hence unless we want to implement an inconsistency, we should either use spatial colors in evoked.plot() by default (without further preconditions), or we should reconsider plot_joint() as well
Ok, I'm fine changing both of we have to. So then what would you recommend @hoechenberger? Do you think we should take picks into account or not?
So then what would you recommend @hoechenberger? Do you think we should take picks into account or not?
I think taking picks into account would create some sort of convolutional interaction between the two parameters. I do see your point thought that if you only have very few or a single trace, defaulting to colors may not be great.
How about introducing an arbitrary threshold of, say, 3 or 5 (or even 1?); and spatial_colors defaulting to None, which will act as True for n_chs > threshold, and as False otherwise?
How about introducing an arbitrary threshold of, say, 3 or 5 (or even 1?); and spatial_colors defaulting to None, which will act as True for n_chs > threshold, and as False otherwise?
After thinking more about this and reading your ideas, I'm leaning towards one of 2 possibilities:
- Ignore picks when auto-setting spatial colors
- If picks is just one single channel,
spatial_colors="auto"becomes False.
Anything else seems either too arbitrary or too much "magic"
If picks is just one single channel, spatial_colors="auto" becomes False.
I like this!
Ready to be reviewed
🎉 Congrats on merging your first pull request! 🥳 Looking forward to seeing more from you in the future! 💪