FIX: proper channel-type-specific vmin/vmax in topo image plots by drammock · Pull Request #6230 · mne-tools/mne-python

drammock · 2019-04-25T17:06:44Z

color limits on epochs.plot_topo_image() are messed up. Example code:

import os
import mne
sample_data_folder = mne.datasets.sample.data_path()
sample_data_raw_file = os.path.join(sample_data_folder, 'MEG', 'sample',
                                    'sample_audvis_raw.fif')
raw = mne.io.read_raw_fif(sample_data_raw_file, verbose=False)
# get events
events = mne.find_events(raw, stim_channel='STI 014')
event_dict = {'auditory/left': 1, 'auditory/right': 2, 'visual/left': 3,
              'visual/right': 4, 'face': 5, 'button': 32}
# make epochs
raw.info['bads'].append('EEG 007')
reject_criteria = dict(mag=4e-12, grad=4e-10, eeg=150e-6, eog=250e-6)
epochs = mne.Epochs(raw, events, event_id=event_dict, reject=reject_criteria,
                    preload=True)
# plot
epochs.plot_topo_image(sigma=0.5)

Results on current master (note the same colorbar limits on the 2 popups)

overview plot

popup plot (magnetometer)

popup plot (gradiometer)

Results after this PR (note the different colorbars on the 2 popups)

overview plot

popup plot (magnetometer)

popup plot (gradiometer)

massich · 2019-04-25T17:08:48Z

mne/viz/topo.py


 def plot_topo_image_epochs(epochs, layout=None, sigma=0., vmin=None,
-                           vmax=None, colorbar=True, order=None, cmap='RdBu_r',
+                           vmax=None, colorbar=None, order=None, cmap='RdBu_r',


You changed default value of colorbar in a public function but did not update its docstring. (see code-guidelines, to know more about mne-docstring conventions).

massich · 2019-04-25T17:09:38Z

mne/viz/topo.py

+    if colorbar is None:
+        colorbar = (len(set(ch_types)) == 1)
+    if colorbar and vmin is None and vmax is None:
+        vmin, vmax = vlim_array[0]


This chunk is really large. Consider breaking it down with existing functions or add some comments describing the workflow. Keep the amount comments to as few as possible, also consider if factoring the code with functions.

massich · 2019-04-25T17:10:55Z

mne/viz/topo.py

@@ -853,23 +857,44 @@ def plot_topo_image_epochs(epochs, layout=None, sigma=0., vmin=None,
    -------
    fig : instance of matplotlib.figure.Figure


I know our policy says "avoid unnecessary cosmetic changes in PRs", but since you're already changing this docstring, could you add a cross-reference to mne.Epochs and matplotlib.figure.Figure here?

OK, I also will add one for matplotlib.pyplot.imshow OK?

massich · 2019-04-25T17:18:44Z

mne/viz/topo.py

-        Display or not a colorbar.
+    colorbar : bool | None
+        Display or not a colorbar. If ``None`` (the default) a colorbar will be
+        shown only if all channels are of the same type.


When chenging the default you could do a pass so that all of them follow the recomended starnad. That shows the default at the end. here are some examples:

- colorbar : bool | None - Display or not a colorbar. If ``None`` (the default) a colorbar will be - shown only if all channels are of the same type. + colorbar : bool | None + Display or not a colorbar. If ``None`` a colorbar will be + shown only if all channels are of the same type. Defaults to ``None``

- show : bool - Show figure if True. + show : Bool + Weather to show the figure or not. Defaults to ``True``.

massich

just some nitpicking.

massich · 2019-04-25T17:26:44Z

mne/viz/topo.py

-    colorbar : bool
-        Display or not a colorbar.
+    colorbar : bool | None
+        Whether to display a colorbar or not. If ``None`` a colorbar will be


Instead of a colorbar please use the colorbar because there is always only one.

Also, since you introduced the quoting in the docstring (None), do it consistently everywhere.

massich · 2019-04-25T17:28:45Z

mne/viz/topo.py

        The color of tick labels in the colorbar. Defaults to white.
    show : bool
-        Show figure if True.
+        Whether to show the figure. Defaults to True.


idem True

drammock · 2019-04-25T17:31:29Z

OK @massich I think this is ready to go.

larsoner

LGTM +1 for merge from me once CIs come back happy/green

codecov · 2019-04-25T18:34:29Z

Codecov Report

Merging #6230 into master will increase coverage by 1.72%.
The diff coverage is 96.42%.

@@            Coverage Diff             @@
##           master    #6230      +/-   ##
==========================================
+ Coverage    87.3%   89.02%   +1.72%     
==========================================
  Files         413      414       +1     
  Lines       74559    74799     +240     
  Branches    12301    12351      +50     
==========================================
+ Hits        65094    66592    +1498     
+ Misses       6594     5302    -1292     
- Partials     2871     2905      +34

massich · 2019-04-26T09:19:43Z

@drammock thx for making the CIs green again.

But you introduced this new behaviour for colorbar=None and there is no test covering those new code paths. That's why the CodeCov report (above) is complaining. Can you modify or add a test (in mne/viz/tests/test_topo.py) that covers this new usage?

You can check coverage of mne/viz/topo.py locally using

pytest --cov=mne.viz.topo --cov-report=term-missing mne/viz/tests/test_topo.py

from the mne-python root directory. Also, a reminder that you can run all tests locally using

pytest mne/

or just a single test by doing:

pytest mne/path/to/test_foo.py -k name_of_test_i_have_modified

drammock · 2019-04-26T10:09:01Z

OK @massich I've added a test that hits the new lines.

massich · 2019-04-26T13:44:13Z

Thx, @drammock

…tools#6230)

FIX: proper channel-type-specific vmin/vmax in topo image plots

ed84626

massich suggested changes Apr 25, 2019

View reviewed changes

drammock added 3 commits April 25, 2019 09:12

DOC: update colorbar docstring

466aff8

DOC: add cross-references

147ed2e

ENH, DOC: add code comments

1a36c42

massich reviewed Apr 25, 2019

View reviewed changes

DOC: specify default correctly

a1e2ca4

massich approved these changes Apr 25, 2019

View reviewed changes

massich requested a review from larsoner April 25, 2019 17:29

STY: docstring formatting

d727cb2

larsoner approved these changes Apr 25, 2019

View reviewed changes

drammock added 2 commits April 25, 2019 12:20

FIX: typo in variable name (azure)

f919110

FIX: another broken test

4b21b5d

add auto-colorbar test

acb926f

massich merged commit 6a3a438 into mne-tools:master Apr 26, 2019

drammock deleted the vlims-topo branch April 26, 2019 14:06

jeythekey pushed a commit to jeythekey/mne-python that referenced this pull request Apr 27, 2019

FIX: proper channel-type-specific vmin/vmax in topo image plots (mne-…

66809db

…tools#6230)

		@@ -853,23 +857,44 @@ def plot_topo_image_epochs(epochs, layout=None, sigma=0., vmin=None,
		-------
		fig : instance of matplotlib.figure.Figure

Uh oh!

Conversation

drammock commented Apr 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Results on current master (note the same colorbar limits on the 2 popups)

overview plot

popup plot (magnetometer)

popup plot (gradiometer)

Results after this PR (note the different colorbars on the 2 popups)

overview plot

popup plot (magnetometer)

popup plot (gradiometer)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

massich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drammock commented Apr 25, 2019

Uh oh!

larsoner 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!

codecov bot commented Apr 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

massich commented Apr 26, 2019 • edited by drammock Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drammock commented Apr 26, 2019

Uh oh!

massich commented Apr 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

drammock commented Apr 25, 2019 •

edited

Loading

larsoner left a comment •

edited

Loading

codecov bot commented Apr 25, 2019 •

edited

Loading

massich commented Apr 26, 2019 •

edited by drammock

Loading