[BUG, MRG] Replace image_interp with interpolation in topomap plot arguments #10617

alexrockhill · 2022-05-09T23:12:18Z

alexrockhill · 2022-05-09T23:18:31Z

I checked and the scipy nearest interpolator with the 64 grid resolution that we use and especially the matplotlib followup interpolation looks really bad compared to the Voronoi plots . I left the Voronoi plotting as is for now but maybe someday it would be nice to refactor the topomap to return something more general than the image as this would be compatible with the polygons. Or even make the cubic/linear interpolation polygons but that is probably too crazy...

larsoner · 2022-05-09T23:18:49Z

My suggestion in the issue that seemed more or less accepted was not to change the argument name, but just make image_interp do what people would expect...

alexrockhill · 2022-05-09T23:22:22Z

My suggestion in the issue that seemed more or less accepted was not to change the argument name, but just make image_interp do what people would expect...

Hmmm I should have read more closely, sorry I was concentrating more on what it said under option 4... This took a bit longer than I thought but a lot of it can be reverted quickly. Now that it's done and should pass all the tests, what do you think about doing it this slightly more painful but in the end less-of-a-misnomer way? I will do the followup to remove image_interp at version 1.2 if so.

agramfort · 2022-05-10T05:33:46Z

Yes let’s avoid unnecessary deprecation

mmagnuski · 2022-05-10T15:37:01Z

I prefer changes this way, but the consesus seems to be toward repurposing image_interp and treating it as a bug fix. To make sure that people who used image_interp the way it worked before I would raise an informative error when something else from 'cubic' or 'nearest' is passed to image_interp. The error could state that to get the old behavior one shoud use image_interp='cubic' and then ax.images[0].set_interpolation(...) as @larsoner suggested.

I have also one question regarding the voronoi plotting behavior that can be seen below:

especially around the edges you can see points that are colored not according to the nearest channel in the 2d projection (some polygons seem to have exapnded / invaded the territory of other channels). Why does this happen, is this a bug?

alexrockhill · 2022-05-10T15:41:17Z

I agree with you both @larsoner and @agramfort, I just should have written it that way in the first place!

Should be good to go now.

mmagnuski · 2022-05-10T15:43:27Z

mne/viz/topomap.py

        self.border = border
        self.tri = tri
+        self.interp = {'cubic': CloughTocher2DInterpolator,
+                       'nearest': CloughTocher2DInterpolator,  # hack not used


if it is not used then I'd remove it. :)

It has to do with using the fill polygon method for the Voronoi diagrams but you still need an underlying image to have a consistent return type...

I don't understand - the _GridData object shouldn't be used when image_interp='nearest'.

You need the extent which is in the _setup_interp method, which is not easy to untangle

mmagnuski · 2022-05-10T15:49:03Z

Btw, since we've changed the behavior of image_interp I'd make sure it is not used in the old way anywhere in the tutorials/examples. And since we now have both linear and nearest interpolation - it would be cool to for example extend this example with the interpolation options.

alexrockhill · 2022-05-10T15:49:12Z

To answer your question @mmagnuski, they shouldn't invade other channels' space but the extrapolation near the edges is a bit different, it is still the nearest channel though, that's just what you need to do to fill that space outside the edges.

It would be really nice to add an informative error, but unfortunately, image_interp is widely dispersed (in animate_topomap and other places) so it's not so simple as adding it once. If you think of a good way to do this, I'm happy to add it, but I would prefer not to have it all over.

doc/changes/latest.inc

mmagnuski · 2022-05-10T16:06:02Z

mne/viz/topomap.py

    # find mask limits
    extent, Xi, Yi, interp = _setup_interp(
-        pos, res, extrapolate, sphere, outlines, border)
+        pos, res, image_interp, extrapolate, sphere, outlines, border)


the lines below (set_values and set_locations) should be done only for linear and cubic interpolation. Voronoi does not use the interpolated grid Zi.

You have to get the extent though...

Zi is used for the contours too...

right, the contours... but in such case iI don't think it makes much sense to plot cubic-interpolated contours, and nearest interpolated topomap. One option is to turn off the contours for nearest interpolation - at least for now.

mmagnuski · 2022-05-10T16:09:15Z

mne/viz/topomap.py

    yi = np.linspace(ymin, ymax, res)
    Xi, Yi = np.meshgrid(xi, yi)
-    interp = _GridData(pos, extrapolate, clip_origin, clip_radius, border)
+    interp = _GridData(pos, image_interp, extrapolate,


only when image_interp is cubic or linear, otherwise could be None.

Other functions depend on this return

mmagnuski · 2022-05-10T16:18:01Z

@alexrockhill
The error could be used in _setup_interp, I think - this should be the common point for all the various functions that use topomap plotting. But this would mean, moving the current check for image_interp values from plot_topomap there.

alexrockhill · 2022-05-10T17:34:43Z

None of the examples use image_interp as far as I can tell btw, adding it to the one linked above

alexrockhill · 2022-05-10T18:40:46Z

Ok so my best solution this round is to plot the nearest interpolation as an image and then add Voronoi on top but only for non-animated topomaps, for animated topomaps, there is no Voronoi overlay. The issue is that the interp object is returned and the Zi that a call to interpolate from it is used for the contours. Basically everything is designed around an image and the Voronoi polygons, although they look really nice, don't fit at all. So the the best option IMO, is to just overlay them for now and maybe consider a more integrated fix down the road.

mmagnuski · 2022-05-10T20:51:27Z

@alexrockhill
Ok, I understand the issue with the contours. But plotting cubic-interpolated countours with nearest voronoi topo for me is a worse solution that turning the contours off when image_interp='nearest'. Could you try how the contours look if the Zi is taken from nearest interpolation?

alexrockhill · 2022-05-10T20:58:19Z

@alexrockhill Ok, I understand the issue with the contours. But plotting cubic-interpolated countours with nearest voronoi topo for me is a worse solution that turning the contours off when image_interp='nearest'. Could you try how the contours look if the Zi is taken from nearest interpolation?

The Zis are taken from the nearest scipy interpolation using the res so they're pretty accurate, they just look terrible compared to the Voronoi diagram because they are all blocky (see above).

i.e. it's already taken from the nearest not the cubic interpolation.

mmagnuski · 2022-05-10T22:05:16Z

Ok. How does these contours look like now then? The figure you pasted here before shows the nearest interpolated image, but without the contours, and the figure I pasted looks like having cubic interpolated contours - thats why I was confused. I'll try to pull and test this PR locally tomorrow and have a look myself! :) Thanks @alexrockhill for all the work and sorry for the complications that stem from my insistence to do things the other way. :)

alexrockhill · 2022-05-10T22:12:00Z

I think compromising made this a lot better, it's nice to have the contours look reasonable. Here they are:

They are slightly offset from the Voronoi diagram which looks nice actually, having them be right on the lines would be a bit confusing. The one downside is that they look a bit jagged but I think that's the best we can do.

larsoner · 2022-05-11T17:02:12Z

I think compromising made this a lot better, it's nice to have the contours look reasonable. Here they are:

Agreed it already looks okay/usable. We can always make it look better in future PRs if it's not perfect

alexrockhill · 2022-05-11T19:25:08Z

Sounds good, okay to merge?

agramfort

@mmagnuski merge if happy.

thx @alexrockhill

mmagnuski · 2022-05-11T20:01:31Z

LGTM too, merging then! Thanks @alexrockhill! 🚀

* upstream/main: (24 commits) Use less memory when loading EDF file (mne-tools#10638) BUG: fix ipython console accessibility after MNEQtBrowser in Spyder (mne-tools#10637) WIP: Fix mne.time_frequency.multitaper Nyquist adjustment slightly incorrect (mne-tools#10541) BUG: Fix bug with fNIRS reordering (mne-tools#10630) MNT: PyQt6 for pip pre 3.10 (mne-tools#10636) CI: Fix conda (mne-tools#10628) MRG: Update installer links to point to 1.0.3_0 (mne-tools#10622) [BUG, MRG] fix info write access (mne-tools#10626) [MRG] Fixed group_by titles in evoked_plot. (mne-tools#10618) [BUG, MRG] Replace image_interp with interpolation in topomap plot arguments (mne-tools#10617) MAINT: Fix timeout (mne-tools#10624) Simplify mne-tools#10619 (mne-tools#10620) Drop EEG rejection thresholds when replacing EEG with CSD channels (mne-tools#10619) Add get_montage support for fNIRS (mne-tools#10611) ENH: Improve make_head_surface options (mne-tools#10592) [ENH, MRG] Add citation for intracranial JOSS paper (mne-tools#10616) [ENH] Add sys info for mne-icalabel (mne-tools#10615) MRG: Add "highlight" parameter to Evoked.plot() to conveniently highlight time periods (mne-tools#10614) MRG: Allow to pass array of "average" values to plot_evoked_topomap() (mne-tools#10610) fix: snirf length units (mne-tools#10613) ...

alexrockhill added 3 commits May 9, 2022 16:10

replace image_interp with interpolation in topomap plots

664c354

[skip ci]

89af46d

add PR number to latest

d578a9a

alexrockhill requested review from larsoner and mmagnuski May 9, 2022 23:13

alexrockhill added 2 commits May 9, 2022 16:29

fix latest

ac2327d

fix test

3f1eacd

no deprecation

da88a49

mmagnuski reviewed May 10, 2022

View reviewed changes

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

mmagnuski reviewed May 10, 2022

View reviewed changes

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

mmagnuski reviewed May 10, 2022

View reviewed changes

review

0297a2f

alexrockhill changed the title ~~[BUG, WIP] Replace image_interp with interpolation in topomap plot arguments~~ [BUG, MRG] Replace image_interp with interpolation in topomap plot arguments May 10, 2022

alexrockhill added 2 commits May 10, 2022 11:49

add informative error

2d634ff

pandas error again

6680c55

rerun tests

cb5f35a

one more try

6940c25

agramfort approved these changes May 11, 2022

View reviewed changes

mmagnuski approved these changes May 11, 2022

View reviewed changes

mmagnuski merged commit be56457 into mne-tools:main May 11, 2022

alexrockhill deleted the interp branch May 11, 2022 20:03

mscheltienne mentioned this pull request Jul 21, 2022

[ENH] Add topographic map feature via MNE mne-tools/mne-icalabel#71

Merged

mmagnuski mentioned this pull request Feb 2, 2023

Documentation for interpolations could be wrong #11443

Closed

Uh oh!

[BUG, MRG] Replace image_interp with interpolation in topomap plot arguments #10617

[BUG, MRG] Replace image_interp with interpolation in topomap plot arguments #10617

Uh oh!

Conversation

alexrockhill commented May 9, 2022

Uh oh!

alexrockhill commented May 9, 2022

Uh oh!

larsoner commented May 9, 2022

Uh oh!

alexrockhill commented May 9, 2022

Uh oh!

agramfort commented May 10, 2022

Uh oh!

mmagnuski commented May 10, 2022

Uh oh!

alexrockhill commented May 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

mmagnuski commented May 10, 2022

Uh oh!

alexrockhill commented May 10, 2022

Uh oh!

Uh oh!

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!

mmagnuski commented May 10, 2022

Uh oh!

alexrockhill commented May 10, 2022

Uh oh!

alexrockhill commented May 10, 2022

Uh oh!

mmagnuski commented May 10, 2022

Uh oh!

alexrockhill commented May 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mmagnuski commented May 10, 2022

Uh oh!

alexrockhill commented May 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

larsoner commented May 11, 2022

Uh oh!

alexrockhill commented May 11, 2022

Uh oh!

agramfort left a comment

Choose a reason for hiding this comment

Uh oh!

mmagnuski commented May 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alexrockhill commented May 10, 2022 •

edited

Loading

alexrockhill commented May 10, 2022 •

edited

Loading

alexrockhill commented May 10, 2022 •

edited

Loading