MRG +1 plot_compare_evokeds topo functionality #6211

jona-sassenhagen · 2019-04-24T15:22:19Z

Another attempt. It's not really hard, but it's my 3rd attempt so ...

mne.viz.plot_compare_evokeds gains the ability to plot to a topographical layout.
This will be slow for MEG data, but is often used for ERP papers.

Not ready for reviews yet.

Closes #4746
Closes #5746
Closes #6166
Closes #6039

Redo of #5568

jona-sassenhagen · 2019-04-24T15:35:41Z

See https://nbviewer.jupyter.org/gist/jona-sassenhagen/5128318bd01bb7305a42048f2de1fa2c/Untitled4.ipynb

codecov · 2019-04-25T09:14:02Z

Codecov Report

Merging #6211 into master will increase coverage by 2.14%.
The diff coverage is 90.86%.

@@            Coverage Diff             @@
##           master    #6211      +/-   ##
==========================================
+ Coverage   87.21%   89.36%   +2.14%     
==========================================
  Files         413      415       +2     
  Lines       74522    76125    +1603     
  Branches    12314    12957     +643     
==========================================
+ Hits        64998    68031    +3033     
+ Misses       6647     5200    -1447     
- Partials     2877     2894      +17

tutorials/plot_visualize_evoked.py

drammock · 2019-04-25T14:32:19Z

done pushing commits? title still says WIP

jona-sassenhagen · 2019-04-25T14:33:14Z

done pushing commits? title still says WIP

I guess I need to add more tests and maybe another example, and extend the docs. But the code is ready for review.

drammock

mne/viz/tests/test_evoked.py

mne/viz/evoked.py

jona-sassenhagen · 2019-04-26T09:01:29Z

In that case, I think the user can most easily use iter_topography themselves, or loop over axes and picks and call plot_compare_evokeds manually ... If we want to support a custom layout, we need a whole new kwarg and a bunch of extra infrastructure.

We might want to have an example like that though, using make_1020_rois.

jona-sassenhagen · 2019-04-26T10:15:32Z

mne/viz/topo.py

+            if not do_legend:
+                ax.set(xticklabels=[], yticklabels=[])
+                plt.setp(ax.get_xticklines(), visible=False)
+                plt.setp(ax.get_yticklines(), visible=False)


The legend only makes sense if you have ticks on the axes really.

same comment as above about state machine vs obj-oriented approach:

tklines = list(ax.get_xticklines()) + list(ax.get_yticklines()) list(tk.set_visible(False) for tk in tklines)

drammock

LGTM as long as CIs are green

mne/viz/tests/test_evoked.py

mne/viz/topo.py

drammock · 2019-04-26T11:05:50Z

mne/viz/topo.py

+            if not do_legend:
+                ax.set(xticklabels=[], yticklabels=[])
+                plt.setp(ax.get_xticklines(), visible=False)
+                plt.setp(ax.get_yticklines(), visible=False)


same comment as above about state machine vs obj-oriented approach:

tklines = list(ax.get_xticklines()) + list(ax.get_yticklines()) list(tk.set_visible(False) for tk in tklines)

drammock

can you find a way to test lines 159-161? Otherwise +1 for merge (assuming green CIs)

mne/viz/topo.py

drammock · 2019-04-26T14:52:16Z

good to go as soon as CIs come back green. Thanks @jona-sassenhagen for sticking with this one!

jona-sassenhagen · 2019-04-26T14:54:26Z

Thank you @drammock , @mmagnuski can you do another pass?

agramfort · 2019-04-27T08:20:51Z

mne/viz/evoked.py

+        this_evokeds = evokeds[cond]
+        # this will fail if evokeds do not have the same structure
+        # (e.g. channel count)
+        res = _get_data_and_ci(this_evokeds, scaling=scaling, picks=picks,


can you write

data, ci = _get_data_and_ci(...)

it makes it explicit what is returned.

then _get_data_and_ci would have to return data, None when no ci is requested, currently it returns (data, )

agramfort · 2019-04-27T08:21:48Z

mne/viz/evoked.py

+    # first, copy to avoid overwriting
+    styles = deepcopy(styles)
+    colors = deepcopy(colors)
+    linestyles = deepcopy(linestyles)


you don't see to overwrite in this function. Can you do the copies just before the overwrites so it's clearer why you need it?

agramfort · 2019-04-27T08:23:14Z

mne/viz/evoked.py

+        d = data_dict[condition].T
+        ax.plot(times, d, zorder=1000, label=condition, clip_on=False,
+                **styles[condition])
+        if np.any(d > 0) or all_positive:


you should write:

if all_positive or np.any(d > 0):

so the np.any(d > 0) is done again once all_positive is True. It saves some computation.

agramfort · 2019-04-27T08:23:52Z

mne/viz/evoked.py

+                **styles[condition])
+        if np.any(d > 0) or all_positive:
+            any_positive = True
+        if np.any(d < 0):


idem. You should not need to do this computation once any_negative has been set to True

agramfort · 2019-04-27T08:25:39Z

mne/viz/evoked.py

+            (picks == 'meg' or picks in _DATA_CH_TYPES_SPLIT) or
+            (isinstance(picks, Iterable) and
+             all(pick == 'meg' or pick in _DATA_CH_TYPES_SPLIT
+                 for pick in picks)))):


can you add a comment to explain in which case gfp is necessary?

agramfort · 2019-04-27T08:26:59Z

mne/viz/evoked.py

+                    for _ in range(len(ch_types)))
    else:
-        axes = (plt.subplots(figsize=(8, 6))[1] for _ in range(len(ch_types)))
+        axes = ["topo" for _ in ch_types]


axes = ["topo"] * len(ch_types)

agramfort · 2019-04-27T08:38:53Z

ok. Then maybe don't write res[-1] but rather res[1]

…

agramfort · 2019-04-27T14:45:00Z

thx @jona-sassenhagen

jona-sassenhagen added 4 commits April 24, 2019 15:14

init

cf94ea2

basic topo functionality

1ff78b6

progress

7433ecc

better

0a9098c

jona-sassenhagen added HARD VIZ labels Apr 24, 2019

jona-sassenhagen added 2 commits April 24, 2019 17:24

...

e0fca7d

del legend title

b20801d

interactive

da4c711

jona-sassenhagen added 6 commits April 25, 2019 11:26

gfpmagic

0f24a95

fixes

ba20190

fix split_legend

4d1e652

:D

0fd7aa0

fix missing ticks

654ef8e

fix docstrings

97b2f58

jona-sassenhagen requested review from cbrnr, drammock and mmagnuski April 25, 2019 14:30

jona-sassenhagen commented Apr 25, 2019

View reviewed changes

tutorials/plot_visualize_evoked.py Show resolved Hide resolved

drammock requested changes Apr 25, 2019

View reviewed changes

fix pep

cf375b7

jona-sassenhagen requested a review from drammock April 25, 2019 16:05

jona-sassenhagen changed the title ~~WIP plot_compare_evokeds topo functionality~~ MRG plot_compare_evokeds topo functionality Apr 25, 2019

larsoner reviewed Apr 25, 2019

View reviewed changes

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

mmagnuski reviewed Apr 25, 2019

View reviewed changes

mne/viz/evoked.py Show resolved Hide resolved

pep again

17969b6

jona-sassenhagen added 3 commits April 26, 2019 12:08

move legend to iter_topo

7d262a0

spelling

cc7deaf

comment

eb3850b

jona-sassenhagen commented Apr 26, 2019

View reviewed changes

test for iter_topo change

03c3c49

drammock approved these changes Apr 26, 2019

View reviewed changes

dans comments

b614605

jona-sassenhagen changed the title ~~MRG plot_compare_evokeds topo functionality~~ WIP plot_compare_evokeds topo functionality Apr 26, 2019

fix picks

cd21935

jona-sassenhagen changed the title ~~WIP plot_compare_evokeds topo functionality~~ MRG plot_compare_evokeds topo functionality Apr 26, 2019

jona-sassenhagen added 3 commits April 26, 2019 15:57

...

bbeb173

another dumb commit to deal with np.bool_

8f45464

...

c776fa8

drammock requested changes Apr 26, 2019

View reviewed changes

mne/viz/topo.py Show resolved Hide resolved

add iter_topo test

fd30246

drammock approved these changes Apr 26, 2019

View reviewed changes

drammock changed the title ~~MRG plot_compare_evokeds topo functionality~~ MRG +1 plot_compare_evokeds topo functionality Apr 26, 2019

jona-sassenhagen mentioned this pull request Apr 26, 2019

y-limits of subplots in plot_evoked_topo #6039

Closed

larsoner requested a review from mmagnuski April 26, 2019 21:19

agramfort reviewed Apr 27, 2019

View reviewed changes

comments

dc10327

agramfort merged commit 3d33e8e into mne-tools:master Apr 27, 2019

jona-sassenhagen deleted the plot_comp_topo branch April 27, 2019 16:14

jdpigeon mentioned this pull request Apr 28, 2019

Add electrode names to topo map Overview figure in Analyze screen makebrainwaves/BrainWaves#44

Open

drammock mentioned this pull request May 9, 2019

MRG, FIX: avoid spurious "showing first five" message #6288

Merged

Uh oh!

MRG +1 plot_compare_evokeds topo functionality #6211

MRG +1 plot_compare_evokeds topo functionality #6211

Uh oh!

Conversation

jona-sassenhagen commented Apr 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jona-sassenhagen commented Apr 24, 2019

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!

Uh oh!

drammock commented Apr 25, 2019

Uh oh!

jona-sassenhagen commented Apr 25, 2019

Uh oh!

drammock left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jona-sassenhagen commented Apr 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drammock left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drammock left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

drammock commented Apr 26, 2019

Uh oh!

jona-sassenhagen commented Apr 26, 2019

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agramfort commented Apr 27, 2019 via email

Uh oh!

agramfort commented Apr 27, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jona-sassenhagen commented Apr 24, 2019 •

edited

Loading

codecov bot commented Apr 25, 2019 •

edited

Loading