MRG, BUG: Fix M/EEG topomap plotting

[larsoner]

Closes #3987
Closes #4880
Closes #5190
Closes #5471
Closes #5472
Closes #6304

Todo:

• ensure our montages actually use the same head sizes (done by dig refactoring)
• Make the plot_sensors be in physical coordinates
• make all code assume a 0.095 m head radius
• check that all examples in gist are fixed
• deprecate params: transform in montage, outlines, ...
• get tests to pass again
• change to sphere rather than head_radius for consistency and flexibility
• change units in setup_volume_source_space for consistency
• move head relative to MEG sensors based on info['dev_head_t']
• check plot_topomap, plot_sensors, montage.plot examples via circle full
• add setup_volume_source_space param and layout deprecation to API section of latest.inc

cc @jona-sassenhagen @sappelhoff @cbrnr you might be interested in this plan.

After this, due to using physical units for the head, #5471 should be simpler.

[mmagnuski]

That is indeed a lot of refactoring! Great work, I will test it during the weekend!

[larsoner]

Also I realized the most consistent API is to have a "sphere" argument allowing you to specify the origin and radius of the sphere. We have this elsewhere and I think in auto mode for digitized EEG locations it will look better.

But feel free to test that things are fixed. The sphere functionality would be a superset of this functionality, and I don't think the defaults will change from what's there now (essentially 0, 0, 0 origin and 9.5cm radius).

[codecov]

Codecov Report

Merging #7066 into master will decrease coverage by 1.28%.
The diff coverage is 92.42%.

@@            Coverage Diff             @@
##           master    #7066      +/-   ##
==========================================
- Coverage   89.74%   88.46%   -1.29%     
==========================================
  Files         444      444              
  Lines       79229    79213      -16     
  Branches    12707    12676      -31     
==========================================
- Hits        71107    70078    -1029     
- Misses       5314     6340    +1026     
+ Partials     2808     2795      -13

[larsoner]

Here are the [circle full] outputs:

https://16870-1301584-gh.circle-artifacts.com/0/dev/index.html

One setup_volume_source_space example was broken on that run but fixed in 9c0525c (probably not interesting from an evaluate-whether-or-not-this-works perspective)

There are several examples that are broken, though, so I'll fix those. I think it has to do with loading a layout when it should use the _auto_topomap_coords function instead.

[larsoner]

Okay, docs built

https://16891-1301584-gh.circle-artifacts.com/0/dev/index.html

CIs happy other than a small problem with Travis that I just pushed a commit to fix

[mmagnuski]

this example looks a bit weird:

head outlies are not where they should be.

[mmagnuski]

BTW is extrapolate 'local' by default now? (it looks so in the example)

[larsoner]

BTW is extrapolate 'local' by default now? (it looks so in the example)

Even worse, I had some debugging cruft in there that made it always use local. Pushing a fix for projs topomap plotting now. But hopefully the rest of the examples look okay.

[drammock]

Many many thanks for tackling this. A few comments:

• in this image it seems like the EEG electrodes don't get as close to the edge of the head as they should. I thought the idea was that if a sensor were in the plane of the fiducials, it should fall on the edge of the head outline, yet even what looks like electrode Iz (which ought to fall below the fiducial plane right?) is still well away from the edge

• here in a few calls to Layout.plot() the outline is missing.

• A question: what is the new deciding factor for whether to do a scalp topography with or without skirt? For example in the ICA tutorial, the corrmap plots do not have skirts, but ICA.plot_components(), ICA.plot_properties(), and evoked.plot_joint() all do have skirts on this PR (whereas on current master only evoked.plot_joint() has skirts).

note that I didn't come close to looking at all the tutorials or examples so don't consider this a thorough review. I can make time to do that if it's needed.

[drammock]

also strangely this ERP plot is now flat, whereas it's not on master

[larsoner]

yet even what looks like electrode Iz (which ought to fall below the fiducial plane right?) is still well away from the edge

No electrodes have z<0:

Details

aud_epochs.average().pick_types(meg=False, eeg=True).plot_sensors(kind='3d')

I don't aesthetically like this contraction, but it seems to be a more true depiction of the channel positions.

Using sphere='auto' (which changes the flattening origin and radius) you see something probably closer to what you'd expect:

Details

here in a few calls to Layout.plot() the outline is missing.

This is intentional. The idea is that:

• head outlines are only shown only if info['chs'] positions are used, typically in topomap plots
• layouts are schematics to be used only in interactive sensor selection and also topo-style plots for better separation of sensors, and should never be plotted with a head outline because it's potentially misleading about the geometry / head-to-sensor relationship.

A question: what is the new deciding factor for whether to do a scalp topography with or without skirt?

Whatever old defaults there were should remain unchanged in this PR (that's the goal anyway!). What I did was that, regardless of which skirt you choose, it will always plot at least out to all the electrode positions. So for things like MEG sensors, even if you do skirt='head' this will be effectively ignored because the sensors are way outside the head boundary.

strangely this ERP plot is now flat

Fixed by commit I just pushed. It was actually unrelated to this PR, and had non-deterministic behavior. It seemed to have to do with doing get_ylim before the limits had actually been calculated properly (probably some deep mpl bug), adding a figure.draw_idle() seems to fix it locally. We'll see if CircleCI agrees.

I didn't come close to looking at all the tutorials or examples so don't consider this a thorough review. I can make time to do that if it's needed.

It would be helpful, even if you just opened each one and quickly flipped through to look to see if anything is obviously wrong.

[larsoner]

Updated full build:

https://16912-1301584-gh.circle-artifacts.com/0/dev/index.html

[agramfort]

often the topos are clipped the bottom. See https://16912-1301584-gh.circle-artifacts.com/0/dev/auto_examples/io/plot_read_proj.html#sphx-glr-auto-examples-io-plot-read-proj-py https://16912-1301584-gh.circle-artifacts.com/0/dev/_images/sphx_glr_plot_50_artifact_correction_ssp_016.png any idea why?

…

[mmagnuski]

It looks as if the center of the head outline was not where the center of the of the circular aperture is. Because of this we see where the extrapolation box ends - it is not ocvered by the aperture.

[drammock]

oops sorry I didn't mean to push directly to your branch @larsoner (though the changes are relevant to this PR)

[larsoner]

No problem, I was tweaking those a little bit so further adjustment is fine by me. In the meantime I'll look into the box-cutting-off-circle issue (I think @mmagnuski is right about the issue), hopefully that's the last one :)

[larsoner]

Okay, clipping should be fixed. Made code more DRY and modified a min/max check in the last commit I pushed to fix it. The Circle full build will currently fail because we need #7099 or some similar variant for it to succeed. Hopefully we can get one merged today and I'll rebase and force-push.

@agramfort I trust you'll have free time over the weekend that you want to dedicate to looking through every example :)

[agramfort]

maybe this one is still broken https://16984-1301584-gh.circle-artifacts.com/0/dev/auto_examples/visualization/plot_evoked_arrowmap.html#sphx-glr-auto-examples-visualization-plot-evoked-arrowmap-py

also the EEG topos now look weird on sample. I would like to know if it's still comparable topos to fieldtrip on the same data. @dengemann had spent a lot of time years ago to have the topomaps looking at fieldtrip results.

[larsoner]

See comment above, I think it's a more accurate representation of the 3D coordinates for sample. They are actually all 2cm or more above z=0 line. sphere='auto' is a better choice for actually digitized data (and it's better for sample), but it's not a good default because it behaves not so nicely for montages because it's affected by channel subsection in that case.

[agramfort]

thx @larsoner

[mmagnuski]

🎊 🚀 !