[MRG, ENH] Use spatial_colors by default in Evoked.plot() #11017

HuseyinOrkun · 2022-08-09T09:52:17Z

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.

…ot(). Which makes spatial_colors True if channel locations exists and False otherwise. Changes docstring in plot_evoked() to include string too Adds option chek to plot_evoked for spatial_colors argument

welcome · 2022-08-09T09:52:19Z

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

mne/evoked.py

drammock

I made a couple suggestions below. I also want to ask: 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 picks as well. @agramfort (or other devs) what is your opinion here?

mne/viz/evoked.py

changes argument definition from str to 'auto'

drammock · 2022-08-09T15:54:58Z

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

agramfort

@HuseyinOrkun you'll also need to add a line in latest.inc file to document the improvement.

mne/viz/evoked.py

- Fixes comment in evoked.py

drammock · 2022-08-10T15:08:23Z

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 · 2022-08-10T15:13:11Z

also, there are a lot of CI errors, here are links to a couple:

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

the RuntimeWarnings seen in the CI errors indicate that one of our tests does not specify a value for spatial_colors (so they assume the old default of False). Fixed by changing the test to explicitly say spatial_colors=False probably?

The other errors you'll need to dig a bit deeper, not obvious to me why your changes here would cause the shape mismatches or the "setting array element with sequence" errors.

doc/changes/latest.inc

hoechenberger · 2022-08-12T07:59:34Z

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

drammock · 2022-08-12T11:00:46Z

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?

hoechenberger · 2022-08-12T11:09:45Z

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?

drammock · 2022-08-12T11:52:46Z

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"

hoechenberger · 2022-08-12T13:42:41Z

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

I like this!

…1, spatial_colors 'auto' becomes False - Adds spatial_colors = False to relevant tests to avoid errors arising from the change in the default argument - Fixes latest.inc issue

- Removes some wrong arguments

mne/viz/evoked.py

mne/viz/ica.py

mne/viz/tests/test_evoked.py

mne/viz/evoked.py

Adds a default 'auto' option for spatial_colors argument in Evoked.plot(). Which makes spatial_colors True if channel locations exists and False otherwise. Moves logic for checking spatial_colors argument to a private function named _check_spatial_colors. Changes docstring in plot_evoked() to include string. Adds option check to plot_evoked for spatial_colors argument. Adds changes to doc changes Adds logic for checking the number of picks, if number of picks is 1, spatial_colors 'auto' becomes False Adds spatial_colors = False to relevant tests to avoid errors arising from the change in the default argument Adds check if picks length is one or picks is None

agramfort

Lets see if CIs are green.

doc/changes/latest.inc

fixes duplicate line

HuseyinOrkun · 2022-09-06T12:13:57Z

Ready to be reviewed

hoechenberger

Thanks, great work! I added a few suggestions for phrasing. Otherwise I think this looks good!

doc/changes/latest.inc

mne/viz/evoked.py

Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>

doc/changes/latest.inc

welcome · 2022-09-07T10:45:11Z

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

…1017)" This reverts commit 879829e.

…1017)" (#11151) This reverts commit 879829e.

…11017) * 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 too Adds option chek to plot_evoked for spatial_colors argument * simplifies code in _plot_evoked() changes argument definition from str to 'auto' * Fixes style issue * - Adds changes to doc changes - Fixes comment in evoked.py * - Fix to the location check logic * - Adds logic for checking the number of picks, if number of picks is 1, spatial_colors 'auto' becomes False - Adds spatial_colors = False to relevant tests to avoid errors arising from the change in the default argument - Fixes latest.inc issue * - Adds check if picks is None - Removes some wrong arguments * - styling * [ENH] Use spatial_colors by default in Evoked.plot() mne-tools#11017 Adds a default 'auto' option for spatial_colors argument in Evoked.plot(). Which makes spatial_colors True if channel locations exists and False otherwise. Moves logic for checking spatial_colors argument to a private function named _check_spatial_colors. Changes docstring in plot_evoked() to include string. Adds option check to plot_evoked for spatial_colors argument. Adds changes to doc changes Adds logic for checking the number of picks, if number of picks is 1, spatial_colors 'auto' becomes False Adds spatial_colors = False to relevant tests to avoid errors arising from the change in the default argument Adds check if picks length is one or picks is None * [ENH] Use spatial_colors by default in Evoked.plot() (mne-tools#11017) Adds a default 'auto' option for spatial_colors argument in Evoked.plot(). Which makes spatial_colors True if channel locations exists and False otherwise. Moves logic for checking spatial_colors argument to a private function named _check_spatial_colors. Changes docstring in plot_evoked() to include string. Adds option check to plot_evoked for spatial_colors argument. Adds changes to doc changes Adds logic for checking the number of picks, if number of picks is 1, spatial_colors 'auto' becomes False Adds spatial_colors = False to relevant tests to avoid errors arising from the change in the default argument Adds check if picks length is one or picks is None * Update latest.inc fixes duplicate line * Update doc/changes/latest.inc Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com> * Update mne/viz/evoked.py Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com> * Update mne/viz/evoked.py Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com> * Update doc/changes/latest.inc Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>

Adds a default 'auto' option for spatial_colors argument in Evoked.pl…

3a6dc90

…ot(). Which makes spatial_colors True if channel locations exists and False otherwise. Changes docstring in plot_evoked() to include string too Adds option chek to plot_evoked for spatial_colors argument

HuseyinOrkun marked this pull request as ready for review August 9, 2022 12:14

HuseyinOrkun changed the title ~~[WIP, ENH] Use spatial_colors by default in Evoked.plot()~~ [ENH] Use spatial_colors by default in Evoked.plot() Aug 9, 2022

agramfort reviewed Aug 9, 2022

View reviewed changes

mne/evoked.py Show resolved Hide resolved

drammock reviewed Aug 9, 2022

View reviewed changes

mne/viz/evoked.py Outdated Show resolved Hide resolved

mne/viz/evoked.py Outdated Show resolved Hide resolved

simplifies code in _plot_evoked()

32905af

changes argument definition from str to 'auto'

Fixes style issue

b11bf69

HuseyinOrkun requested review from agramfort and drammock August 10, 2022 11:14

agramfort reviewed Aug 10, 2022

View reviewed changes

mne/viz/evoked.py Outdated Show resolved Hide resolved

- Adds changes to doc changes

31f2a38

- Fixes comment in evoked.py

drammock reviewed Aug 10, 2022

View reviewed changes

doc/changes/latest.inc Outdated Show resolved Hide resolved

- Fix to the location check logic

16330a4

HuseyinOrkun added 3 commits August 22, 2022 16:20

- Adds logic for checking the number of picks, if number of picks is …

69c9682

…1, spatial_colors 'auto' becomes False - Adds spatial_colors = False to relevant tests to avoid errors arising from the change in the default argument - Fixes latest.inc issue

- Adds check if picks is None

ad21a22

- Removes some wrong arguments

- styling

33656a1

hoechenberger reviewed Aug 23, 2022

View reviewed changes

mne/viz/evoked.py Outdated Show resolved Hide resolved

agramfort reviewed Aug 24, 2022

View reviewed changes

mne/viz/ica.py Show resolved Hide resolved

mne/viz/tests/test_evoked.py Show resolved Hide resolved

mne/viz/evoked.py Outdated Show resolved Hide resolved

HuseyinOrkun changed the title ~~[ENH] Use spatial_colors by default in Evoked.plot()~~ [WIP, ENH] Use spatial_colors by default in Evoked.plot() Aug 25, 2022

HuseyinOrkun marked this pull request as draft August 25, 2022 14:28

HuseyinOrkun changed the title ~~[WIP, ENH] Use spatial_colors by default in Evoked.plot()~~ [ENH] Use spatial_colors by default in Evoked.plot() Aug 25, 2022

HuseyinOrkun marked this pull request as ready for review August 26, 2022 06:10

Merge branch 'main' into spatial_colors_by_default_in_Evoked.plot()

41c494e

HuseyinOrkun requested review from agramfort, drammock and hoechenberger and removed request for agramfort, drammock and hoechenberger September 5, 2022 16:06

agramfort reviewed Sep 5, 2022

View reviewed changes

doc/changes/latest.inc Outdated Show resolved Hide resolved

Update latest.inc

f60cfc5

fixes duplicate line

HuseyinOrkun changed the title ~~[ENH] Use spatial_colors by default in Evoked.plot()~~ [MRG, ENH] Use spatial_colors by default in Evoked.plot() Sep 6, 2022

hoechenberger reviewed Sep 6, 2022

View reviewed changes

doc/changes/latest.inc Outdated Show resolved Hide resolved

mne/viz/evoked.py Outdated Show resolved Hide resolved

mne/viz/evoked.py Outdated Show resolved Hide resolved

HuseyinOrkun and others added 3 commits September 7, 2022 11:07

Update doc/changes/latest.inc

cf19d44

Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>

Update mne/viz/evoked.py

ef130c1

Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>

Update mne/viz/evoked.py

261361b

Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>

hoechenberger reviewed Sep 7, 2022

View reviewed changes

doc/changes/latest.inc Outdated Show resolved Hide resolved

Update doc/changes/latest.inc

8bbb36d

hoechenberger enabled auto-merge (squash) September 7, 2022 09:42

hoechenberger approved these changes Sep 7, 2022

View reviewed changes

hoechenberger merged commit 879829e into mne-tools:main Sep 7, 2022

larsoner mentioned this pull request Sep 9, 2022

BUG: spatial_colors PR broke an example #11149

Closed

drammock added a commit that referenced this pull request Sep 9, 2022

Revert "[MRG, ENH] Use spatial_colors by default in Evoked.plot() (#1…

6d747d0

…1017)" This reverts commit 879829e.

drammock mentioned this pull request Sep 9, 2022

Revert "[MRG, ENH] Use spatial_colors by default in Evoked.plot()" #11151

Merged

drammock added a commit that referenced this pull request Sep 9, 2022

Revert "[MRG, ENH] Use spatial_colors by default in Evoked.plot() (#1…

880e883

…1017)" (#11151) This reverts commit 879829e.

drammock mentioned this pull request Sep 23, 2022

Fix spatial colors #11201

Merged

Uh oh!

[MRG, ENH] Use spatial_colors by default in Evoked.plot() #11017

[MRG, ENH] Use spatial_colors by default in Evoked.plot() #11017

Uh oh!

Conversation

HuseyinOrkun commented Aug 9, 2022

Reference issue

What does this implement/fix?

Additional information

Uh oh!

welcome bot commented Aug 9, 2022

Uh oh!

Uh oh!

drammock left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

drammock commented Aug 9, 2022

Uh oh!

agramfort left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

drammock commented Aug 10, 2022

Uh oh!

drammock commented Aug 10, 2022

Uh oh!

drammock left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hoechenberger commented Aug 12, 2022

Uh oh!

drammock commented Aug 12, 2022

Uh oh!

hoechenberger commented Aug 12, 2022

Uh oh!

drammock commented Aug 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hoechenberger commented Aug 12, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

agramfort left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

HuseyinOrkun commented Sep 6, 2022

Uh oh!

hoechenberger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

welcome bot commented Sep 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

drammock left a comment •

edited

Loading

drammock commented Aug 12, 2022 •

edited

Loading