mne-python icon indicating copy to clipboard operation
mne-python copied to clipboard

[ENH] Use spatial_colors by default in Evoked.plot()

Open HuseyinOrkun opened this issue 3 years ago • 9 comments

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.

HuseyinOrkun avatar Aug 09 '22 09:08 HuseyinOrkun

Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴🏽‍♂️

welcome[bot] avatar Aug 09 '22 09:08 welcome[bot]

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

drammock avatar Aug 09 '22 15:08 drammock

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 passes picks='mag' or picks='eeg' (i.e., all channels of one type). So I think setting spatial_colors automatically maybe should take into account the value of picks as well.

drammock avatar Aug 10 '22 15:08 drammock

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.

drammock avatar Aug 10 '22 15:08 drammock

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 passes picks='mag' or picks='eeg' (i.e., all channels of one type). So I think setting spatial_colors automatically maybe should take into account the value of picks as 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

hoechenberger avatar Aug 12 '22 07:08 hoechenberger

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?

drammock avatar Aug 12 '22 11:08 drammock

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?

hoechenberger avatar Aug 12 '22 11:08 hoechenberger

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:

  1. Ignore picks when auto-setting spatial colors
  2. If picks is just one single channel, spatial_colors="auto" becomes False.

Anything else seems either too arbitrary or too much "magic"

drammock avatar Aug 12 '22 11:08 drammock

If picks is just one single channel, spatial_colors="auto" becomes False.

I like this!

hoechenberger avatar Aug 12 '22 13:08 hoechenberger

Ready to be reviewed

HuseyinOrkun avatar Sep 06 '22 12:09 HuseyinOrkun

🎉 Congrats on merging your first pull request! 🥳 Looking forward to seeing more from you in the future! 💪

welcome[bot] avatar Sep 07 '22 10:09 welcome[bot]