Skip to content
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

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

Merged
merged 9 commits into from Apr 26, 2019

Conversation

@drammock
Copy link
Member

@drammock drammock commented Apr 25, 2019

closes #6112

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

before_mixed

popup plot (magnetometer)

before_mag_popup

popup plot (gradiometer)

before_grad_popup

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

overview plot

after_mixed

popup plot (magnetometer)

after_mag_popup

popup plot (gradiometer)

after_grad_popup

@@ -798,7 +802,7 @@ def _plot_update_evoked_topo_proj(params, bools):


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',
Copy link
Contributor

@massich massich Apr 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

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]
Copy link
Contributor

@massich massich Apr 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

mne/viz/topo.py Outdated
@@ -853,23 +857,44 @@ def plot_topo_image_epochs(epochs, layout=None, sigma=0., vmin=None,
-------
fig : instance of matplotlib.figure.Figure
Copy link
Contributor

@massich massich Apr 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@drammock drammock Apr 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

mne/viz/topo.py Outdated
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.
Copy link
Contributor

@massich massich Apr 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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``.

Copy link
Contributor

@massich massich left a comment

just some nitpicking.

colorbar : bool
Display or not a colorbar.
colorbar : bool | None
Whether to display a colorbar or not. If ``None`` a colorbar will be
Copy link
Contributor

@massich massich Apr 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@massich massich Apr 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

mne/viz/topo.py Outdated
font_color : color
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.
Copy link
Contributor

@massich massich Apr 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idem True

@massich massich requested a review from larsoner Apr 25, 2019
@drammock
Copy link
Member Author

@drammock drammock commented Apr 25, 2019

OK @massich I think this is ready to go.

Copy link
Member

@larsoner larsoner left a comment

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

@codecov
Copy link

@codecov codecov bot commented Apr 25, 2019

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
Copy link
Contributor

@massich massich commented Apr 26, 2019

@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
Copy link
Member Author

@drammock drammock commented Apr 26, 2019

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

@massich
Copy link
Contributor

@massich massich commented Apr 26, 2019

Thx, @drammock

@massich massich merged commit 6a3a438 into mne-tools:master Apr 26, 2019
6 checks passed
@drammock drammock deleted the vlims-topo branch Apr 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants