MRG: Faster topomaps #4224

larsoner · 2017-04-25T18:09:12Z

This is an effort to make our topomap plotting faster interactively and in testing. cc @jona-sassenhagen can you push your changes to the tests?

Still WIP because I need to modify the picking to use PatchCollection (which I think should be possible) instead of iteration over Circle objects. @jaeilepp did you ever try this approach?

jona-sassenhagen · 2017-04-25T18:10:03Z

@jona-sassenhagen can you push your changes to the tests?

I'm still profiling, sorry ...

larsoner · 2017-04-25T18:11:01Z

Okay, well if you get anything useful let me know. In the meantime feel free to try this branch, too.

codecov-io · 2017-04-25T19:04:23Z

Codecov Report

Merging #4224 into master will decrease coverage by 0.04%.
The diff coverage is 90%.

@@            Coverage Diff             @@
##           master    #4224      +/-   ##
==========================================
- Coverage   86.23%   86.19%   -0.05%     
==========================================
  Files         357      358       +1     
  Lines       64558    65004     +446     
  Branches     9831     9911      +80     
==========================================
+ Hits        55673    56030     +357     
- Misses       6182     6242      +60     
- Partials     2703     2732      +29

larsoner · 2017-04-25T19:12:03Z

Looks like it successfully cut down testing time by about 5 minutes. The longest topomap test is now only 15 sec:

https://travis-ci.org/mne-tools/mne-python/jobs/225709024#L3282

jona-sassenhagen · 2017-04-25T19:29:35Z

Awesome! With a bit of luck I'll be able to shave off another second or two :D

larsoner · 2017-04-25T19:35:32Z

@jaeilepp I'm not going to work on this until tomorrow sometime, so if you feel like messing with the PatchCollection selection stuff in the meantime, feel free.

larsoner · 2017-04-25T19:39:10Z

mne/time_frequency/tfr.py

        from ..viz import plot_tfr_topomap
        if abs(eclick.x - erelease.x) < .1 or abs(eclick.y - erelease.y) < .1:
            return
-        plt.ion()  # turn interactive mode on


I don't think we should mess with matplotlib interactive status. Messing with global states is not a nice thing for a package to do.

It would be better to document any functions that need interactivity, telling people to activate interactive mode (maybe with docs somewhere about how to do it).

larsoner · 2017-04-25T19:51:03Z

@kingjr feel free to try this to see if it works any better for you. If not, let me know which call(s) are slow for you.

jona-sassenhagen · 2017-04-25T20:14:43Z

mne/viz/topomap.py

        for x, y in zip(pos_x, pos_y):
-            ax.add_artist(Circle(xy=(x, y), radius=0.003, color='k'))
+            patches += [Circle(xy=(x, y), radius=0.003)]


Why not

patches = [Circle(xy=(x, y), radius=0.003) for x, y in zip(pos_x, pos_y)]

Yeah that's better, just bad translation by me. Actually I'm not sure if this is really any better than ax.plot(), I should probably try that, too. (Need to check and see if the subset of selected points is also accessible there, though, just like with PatchCollection, though.)

We should probably benchmark all 3(+?) methods we're using:
ax.plot
patches
ax.scatter

jona-sassenhagen · 2017-04-25T20:18:06Z

mne/viz/topomap.py

        for x, y in zip(pos_x, pos_y):
-            ax.add_artist(Circle(xy=(x, y), radius=0.003, color='k'))
+            patches += [Circle(xy=(x, y), radius=0.003)]
+        ax.add_collection(PatchCollection(patches, color='k'))


@jaeilepp it's probably suboptimal that the legend for spatial_colors and this one use different code ...

spatial_colors doing ax.scatter(pos_x, pos_y, color=colors, s=25, marker='.', zorder=1)

My guess is that it has to do with selection. These sensors need to be Lasso-able. The existing code makes it easy. But I think it should be possible at least with PatchCollection if not also whatever scatter returns with a bit of extra logic at the selection end.

... although we'd have to check to see if you can do point-by-point color with scatter like you can with PatchCollection

I'm not sure I understand you correctly, but the (colored) legend for plot_joint is currently done with scatter.

Checking the history, your guess seems correct: I submitted this with add_artist patches and @jaeilepp turned it into scatter when he added interactivity.

I think you must have this backward, because right now it's add_artist, so you must have done scatter and @jaeilepp did add_artist...

But in any case, we should figure out which is fastest among ax.plot, ax.scatter, and PatchCollection. Then we can try from fastest to slowest to see if sub-selection can be done. Do you have time to do some profiling?

I think you must have this backward, because right now it's add_artis

I meant this:
https://github.com/mne-tools/mne-python/blob/master/mne/viz/evoked.py#L167

Do you have time to do some profiling?

Not today, I got to sleep :) I can set up a preliminary benchmark, but nothing thorough.

Well if you have time tomorrow morning feel free, otherwise I'll give it a shot

@Eric89GXL
http://nbviewer.jupyter.org/gist/jona-sassenhagen/d46a30bc48bf5223bebeb827d2b20bb2

I think the scatter option might give another decent boost.
Though most of the test comes from creating (and perhaps closing) the new axes, not the actual plot (it obscures the differences between the actual sensor plotting: I think scatter is actually ~2x as fast as the patches, not just a bit.)
So then it's possible we could save a lot with the axes divider pattern ..?

See also matplotlib/matplotlib#6664. If it were faster to make axes I wouldn't have had to do crazy refactoring of the topo code to use a single Axes, either. But in the meantime, yeah using a single axes instance could help. But let's do it in another PR.

larsoner · 2017-04-26T01:13:32Z

So scatter, plot, PatchCollection, and master ordered by speed. I don't think plot allows per-vertex colors but scatter and PatchCollection do. So now we just need to check if those (with scatter preferred) when picked give the indices of the subselection.

jaeilepp · 2017-04-26T05:32:37Z

Still WIP because I need to modify the picking to use PatchCollection (which I think should be possible) instead of iteration over Circle objects. @jaeilepp did you ever try this approach?

No, I haven't really touched the topomap code too much.

kingjr · 2017-04-26T15:01:56Z

Seems like a x2 gain for me. Have you tried on the slider? There the gain should be noticeable.

larsoner · 2017-04-26T17:19:50Z

Is that 2x enough to be happy, be acceptable, or be better but still crappy? Is there an example or tutorial with the slider?

kingjr · 2017-04-27T14:42:36Z

Is that 2x enough to be happy, be acceptable, or be better but still crappy?

yes, it's a good start

http://mne-tools.github.io/dev/auto_examples/visualization/make_report.html?highlight=slider

larsoner · 2017-04-28T15:32:22Z

Okay now we use SciPy for the interpolation instead of using our own in-house radial basis function interpolation. I changed it not to use Rbf at all because it's slower than CloughTocher2DInterpolator but otherwise looks the same. In any case, things should be faster, especially in the case of animations and hopefully also in interactive mode (because we configure the interpolator once, more or less, instead of multiple times). Now the overhead is mostly in the plotting functions, and isn't too bad.

The example you listed @kingjr now rapidly pops up figures for me so I think the overhead is now mostly in figure/axis creation. Can you see if it got any faster for you?

Now I think I just need to make the selection bits work now that we use scatter, but it would be good if @jona-sassenhagen you could try it to make sure it's working well at your end, too.

jona-sassenhagen · 2017-04-28T16:39:52Z

@Eric89GXL I never use interactive functions, can't say anything to these.

In the notebook, I'm seeing a sizeable 30-50% speedup.

larsoner · 2017-04-28T16:45:50Z

Okay, selection works now. Ready for review/merge from my end.

jona-sassenhagen · 2017-04-28T16:46:17Z

mne/viz/topomap.py

+    """
+
+    def __init__(self, pos):
+        from scipy.spatial.qhull import Delaunay


Uhhhh Delaunay again, I recognize this now! This is like a second math ed to me!

jona-sassenhagen · 2017-04-28T16:55:33Z

mne/viz/topomap.py

-        for x, y in zip(pos_x, pos_y):
-            ax.add_artist(Circle(xy=(x, y), radius=0.003, color='k'))
+        ax.scatter(pos_x, pos_y, s=0.25, marker='o',
+                   edgecolor=['k'] * len(pos_x), facecolor='none')


edgecolor='k'?

I prefer this explicit list-mode because we set them independently elsewhere later. (Also, it's possible that this might help mpl optimize for such a state we expect could come later...)

agramfort · 2017-04-28T17:27:42Z

seems to work fine for me ! @Eric89GXL <https://github.com/Eric89GXL> I am just wondering about the change of behavior on the edges of the head when spatial sampling is poor (eg with small number of EEG sensors)

larsoner · 2017-04-28T19:58:05Z

I tried to make an example:

import mne
evoked = mne.read_evokeds(mne.datasets.testing.data_path() +
                          '/MEG/sample/sample_audvis-ave.fif')[0]
evoked.pick_types(meg=False, eeg=True)
evoked.pick_channels(evoked.ch_names[:10])
evoked.plot_topomap()

But even on master our (misleading) re-centering / re-scaling makes it weird (would have expected all the electrodes to be at the front of the head, not distributed all over):

It's not changed much by this PR:

Let me know if you have a better example...

agramfort · 2017-04-29T15:10:43Z

ok +1 for merge for me then.

larsoner · 2017-04-29T23:51:04Z

@jaeilepp all yours then

jaeilepp · 2017-05-01T09:20:07Z

Thanks @Eric89GXL

mmagnuski · 2017-05-01T09:22:56Z

But even on master our (misleading) re-centering / re-scaling makes it weird (would have expected all the electrodes to be at the front of the head, not distributed all over)

I also don't like how channels are re-scaled to match the whole head - are there plans to change it?

jona-sassenhagen · 2017-05-01T09:52:55Z

How do you like outlines="skirt"?

ENH: Faster topomaps

70838ae

jona-sassenhagen mentioned this pull request Apr 25, 2017

WIP speed up topomap tests #4225

Merged

larsoner commented Apr 25, 2017

View reviewed changes

jona-sassenhagen reviewed Apr 25, 2017

View reviewed changes

jona-sassenhagen mentioned this pull request Apr 26, 2017

[MRG] Adding AverageTFR.plot_joint() method #4134

Closed

ENH: Use SciPy for faster interpolation

a48c9ce

larsoner changed the title ~~WIP: Faster topomaps (hopefully)~~ MRG: Faster topomaps (hopefully) Apr 28, 2017

FIX: Fix TFR selection

7d084a9

jona-sassenhagen reviewed Apr 28, 2017

View reviewed changes

larsoner changed the title ~~MRG: Faster topomaps (hopefully)~~ MRG: Faster topomaps Apr 28, 2017

jona-sassenhagen reviewed Apr 28, 2017

View reviewed changes

jaeilepp merged commit a42677f into mne-tools:master May 1, 2017

MRG: Faster topomaps #4224

MRG: Faster topomaps #4224

Conversation

larsoner commented Apr 25, 2017

jona-sassenhagen commented Apr 25, 2017

larsoner commented Apr 25, 2017

codecov-io commented Apr 25, 2017 • edited

Codecov Report

larsoner commented Apr 25, 2017

jona-sassenhagen commented Apr 25, 2017

larsoner commented Apr 25, 2017

Choose a reason for hiding this comment

larsoner commented Apr 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larsoner commented Apr 26, 2017

jaeilepp commented Apr 26, 2017

kingjr commented Apr 26, 2017

larsoner commented Apr 26, 2017 via email

kingjr commented Apr 27, 2017

larsoner commented Apr 28, 2017

jona-sassenhagen commented Apr 28, 2017

larsoner commented Apr 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agramfort commented Apr 28, 2017 via email

larsoner commented Apr 28, 2017

agramfort commented Apr 29, 2017 via email

larsoner commented Apr 29, 2017 via email

jaeilepp commented May 1, 2017

mmagnuski commented May 1, 2017

jona-sassenhagen commented May 1, 2017 via email

codecov-io commented Apr 25, 2017 •

edited