FIX: do not project to sphere; DOC - explain how to get EEGLAB-like topoplots #7455

mmagnuski · 2020-03-16T11:39:51Z

Fixes #7341
Currently all channels are projected to a sphere so the sphere radius does not take any effect. If we don't project to a sphere the sphere radius changes the channel positioning with respect to head outline:

However this means that channel positions that are not spherical, like EGI (or standard_1020) do not look so good because we no longer project to sphere.

One way to circumvent this would be to project to sphere and then scale channel positions by the average channel pos radius (currently each channel is scaled by its own radius, so the channels are no longer on a sphere).

agramfort · 2020-03-16T12:55:50Z

@cbrnr @larsoner thoughts?

larsoner · 2020-03-16T13:20:51Z

This already looks reasonable to me. I'm not too worried about the realistic positions looking bad (as above at least) on a sphere as long as they do so consistently, they probably shouldn't look perfect.

larsoner · 2020-03-16T13:21:23Z

If once you're done @mmagnuski can you push a commit with [circle full] in the message so that CircleCI builds the entire doc? It's useful for catching problems

cbrnr · 2020-03-16T14:21:54Z

My priority is that theoretical locations look good on the 2D topomap projection. I don't think it is necessary to make realistic locations look good on a sphere, or the other way round (theoretical locations on a realistic head).

mmagnuski · 2020-03-16T15:00:35Z

@larsoner @cbrnr
Good, that's reassuring :)
Then I'd only complain that the current default sphere radius value does not seem particularily good, at least in the cases that I've tested (see the first figure in the first post here, left topo - that's biosemi128). I'll take a look at the CI errors in the evening and push [circle full] later so we'll have a wider perspective.

mmagnuski · 2020-03-17T18:29:50Z

just letting you know that I'll be back to it tomorrow, I'll have to figure out what should be happening now in the lasso selection test (some of the steps in the test are not super-clear).

agramfort · 2020-03-24T21:14:48Z

@mmagnuski any chance to complete this quickly before the imminent 0.20 release?

mmagnuski · 2020-03-25T14:02:11Z

@agramfort Depends how imminent is that release :)
I can try looking into this today - but then we'll have to look through CI rendered docs and see if we like current sphere default etc. This might take a few days.

agramfort · 2020-03-25T14:03:12Z

reallly imminent like by the end of the week for sure

…

agramfort · 2020-03-25T14:03:29Z

how can we help?

…

larsoner · 2020-03-25T14:30:22Z

I can try looking into this today

That would be great, when you're happy please push a commit with [circle full] in it

but then we'll have to look through CI rendered docs and see if we like current sphere default etc. This might take a few days.

It only took about 20 minutes to look through last time I think. I would say if nothing is obviously broken we merge, if it breaks a lot of stuff we push to 0.21 (or maybe 0.20.1 via backport).

mmagnuski · 2020-03-25T14:33:00Z

@agramfort I just have to fix tests, I had a problem with lasso selection test - but I have solved it now, thanks for reminding me about this issue!

@larsoner Ok, that sounds good. :)

I'm not sure about test_channels.test_find_ch_connectivity, the "Silly test for checking the number of neighbors" fails for eeg ch_type:

raw = read_raw_fif(raw_fname, preload=True)
sizes = {'mag': 828, 'grad': 1700, 'eeg': 386}
nchans = {'mag': 102, 'grad': 204, 'eeg': 60}
ch_type = 'eeg'
conn, ch_names = find_ch_connectivity(raw.info, ch_type)
# Silly test for checking the number of neighbors.
assert_equal(conn.getnnz(), sizes[ch_type])

the expected number of non-zero elements for eeg is 386 while we get 384. Probably due to different channel 2d positioning two neighbours are "lost". Can I just change the eeg value to 384 (that's what I temporarily did)?

codecov · 2020-03-25T14:33:42Z

Codecov Report

Merging #7455 into master will decrease coverage by 0.33%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #7455      +/-   ##
==========================================
- Coverage   90.07%   89.73%   -0.34%     
==========================================
  Files         454      454              
  Lines       82353    81536     -817     
  Branches    13016    13015       -1     
==========================================
- Hits        74180    73170    -1010     
- Misses       5350     5503     +153     
- Partials     2823     2863      +40

larsoner · 2020-03-25T15:18:47Z

Yes I think that's reasonable

mmagnuski · 2020-03-26T12:00:32Z

Ok, one test is still broken, I'll fix it in the evening and then we'll see [circle full]

cbrnr · 2020-03-26T13:22:33Z

I've created comparisons for standard_1020 and biosemi64 montages:

I think the real problem is not how we project, but that the standard_XXXX montages do not contain template locations. IMO standard_1020 looks even worse with the changes in this PR.

larsoner · 2020-03-26T13:26:21Z

My vote is to push this to 0.21, then. I don't see this as a critical release blocker and we don't have consensus on a quick solution

agramfort · 2020-03-26T13:35:03Z

I agree move to 0.21 unless there is something we agree on in the next 24h before the release

…

mmagnuski · 2020-03-26T22:39:14Z

I agree, I was actually expecting this, that's why I thought this might take longer.

@cbrnr

I think the real problem is not how we project, but that the standard_XXXX montages do not contain template locations.

These are partly overlapping issues, but the main thing this PR fixes is not the problems you previously mentioned about the layout of standard montages but #7341. The problem was that using sphere argument to change sphere radius did not change how topomap was plotted. This was because channels were always projected to the sphere, so even when sphere radius was very small compared to the channel positions it was not reflected in the topomap. All other changes are not intended.

IMO standard_1020 looks even worse with the changes in this PR.

This is what I previously meant about choosing the default sphere radius so that channel layouts look good by default. This definitely requires some thought and time.
Could you see @cbrnr if using a different sphere radius (passing sphere=sphere_radius) makes the plots better? You can also change the sphere origin by sphere=(sphere_x, sphere_y, sphere_z, sphere_radius).

mmagnuski · 2020-03-26T22:43:08Z

@cbrnr
But to be honest I prefer the standard_1020 layout on the left. :) But it could be clearly better.

mmagnuski · 2020-03-26T22:48:52Z

@larsoner @agramfort
We should add a short note to the sphere argument then that currently the sphere radius does not affect the plotting.

agramfort · 2020-03-26T22:55:51Z

Ok

…rial

larsoner · 2020-09-02T12:58:42Z

tutorials/intro/plot_40_sensor_locations.py

+# In mne-python the head center and therefore the sphere center are calculated
+# using fiducial points. Because of this the head circle represents head


This seems a bit misleading. The center of the head coordinate frame is defined by the fiducial points. The automated sphere center is calculated using all EEG + fiducials + extra digitization points. With this wording, people might think both of these are calculated just from the three fiducial points. Maybe something like:

Suggested change

# In mne-python the head center and therefore the sphere center are calculated

# using fiducial points. Because of this the head circle represents head

# In mne-python the head coordinate frame is defined by the fiducial points.

# (LPA, Nasion, RPA). Because of this the head circle represents head

Yes, that's better, thanks. But it seems I may still not understand something here - I'm not sure what you are referring to as "the automated sphere center". The default sphere center is (0, 0, 0), right?

The docs say:

The sphere parameters to use for the cartoon head. Can be array-like of shape (4,) to give the X/Y/Z origin and radius in meters, or a single float to give the radius (origin assumed 0, 0, 0). Can also be a spherical ConductorModel, which will use the origin and radius. Can be “auto” to use a digitization-based fit. Can also be None (default) to use ‘auto’ when enough extra digitization points are available, and 0.095 otherwise. Currently the head radius does not affect plotting.

So for mon.plot in theory there should be enough points (EEG points can be used) and so the origin should not be (0, 0, 0). If it is, we either have an implementation (probably this) or a documentation bug.

at least I think it will try to use the EEG dig points, even though it says "extra digitization points"...

Okay I was wrong, it does just look for extra:

mne-python/mne/utils/check.py

Line 650 in af1120c

get_fitting_dig(info, 'extra', verbose='error')

IIRC the motivation was that when you apply a montage, missing channels are not included from info['dig']. I think if we kept them all in there (maybe with the others as extra points?) then checked for EEG+extra, we could make it always fit an origin.

We can look into enabling this -- I actually don't think it would be too much work in DigMontage -- but if what you have here works well enough already, then we should just stick with it (especially right before release) rather than try to make DigMontage do more.

Defining head center (and sphere origin) based on fiducials is simple and makes sense. However, I'm not so sure about using all available dig and channel points for that: because then, depending on the distribution of these points, the head outline has a different meaning. I think it is good to have a consistent meaning for what the head outline represents. But I may still be missing something here.

larsoner · 2020-09-02T13:00:54Z

tutorials/intro/plot_40_sensor_locations.py

+# calculate the sphere origin from position of Oz, Fpz, T3/T7 or T4/T8
+# channels. This is easier once the montage has been applied to the data and
+# channel positions are in head space - see this example
+# (all the code below should be a separate example):


Feel free to make a separate examples/visualization/plot_eeglab_head_sphere.py or so, then link to it here

larsoner · 2020-09-02T13:01:16Z

tutorials/intro/plot_40_sensor_locations.py

+
+n_channels = len(biosemi_montage.ch_names)
+fake_info = mne.create_info(ch_names=biosemi_montage.ch_names, sfreq=250.,
+                            ch_types=['eeg'] * n_channels)


ch_types='eeg' works and is shorter

tutorials/intro/plot_40_sensor_locations.py

larsoner · 2020-09-02T13:02:57Z

tutorials/intro/plot_40_sensor_locations.py

+fake_raw.set_montage(biosemi_montage)
+
+check_ch = ['Oz', 'Fpz', 'T7', 'T8']
+pos = fake_raw._get_channel_positions(picks=check_ch)


shouldn't use a private function in an example so we have to wait for #7667

Oh, yes, the example part is sketchy for now. Great to know .get_ch_positions() will be added, I was using my own function get_ch_pos() for years! :)

tutorials/intro/plot_40_sensor_locations.py

mne/viz/topomap.py

tutorials/intro/plot_40_sensor_locations.py

Co-authored-by: Eric Larson <larson.eric.d@gmail.com>

sappelhoff · 2020-09-02T13:40:16Z

tutorials/intro/plot_40_sensor_locations.py

+# positions onto a sphere. Because ``'standard_1020'`` montage contains
+# realistic, not spherical, channel positions, we will use a different montage
+# to demonstrate controlling how channels are projected to 2d space.


just a fly-by comment --> many users (including myself) will want to "fix" the plotting of 'standard_1020' and not the biosemi montage. Can that be included in this brief example? :)

This PR seems 98% ready to go (right @mmagnuski ?), in which case let's save that for #7141 unless it's trivial to add here

Unfortunatelly the standard_1020 is not spherical, it contains realistic channel positions, I don't think it can be easily fixed. The easiest fix would be to distribute the spherical 1020 locations, which is discussed here: #7141 (as you know, for sure, cause you took part in the discussions :) ). Once this is done I'd happily change the tutorial to use the spherical 1020.

alright, thanks - I guess I was hoping against all odds to find a ready-made solution here. Thanks for your PR!

larsoner · 2020-09-02T14:18:58Z

tutorials/intro/plot_40_sensor_locations.py

+
+fig.texts[0].remove()
+ax[0].set_title('MNE channel projection', fontweight='bold')
+ax[1].set_title('EEGLAB channel projection', fontweight='bold')


Also looks like these did not render, probably because the axes are turned off

https://21995-1301584-gh.circle-artifacts.com/0/dev/auto_tutorials/intro/plot_40_sensor_locations.html#sphx-glr-auto-tutorials-intro-plot-40-sensor-locations-py

hm, strange, when I tested it in the notebook it looked ok:

Try in native Python instead? I would try tight_layout, subplots_adjust, or if you're brave, constrained_layout

The images on circle look cropped at the top compared with what I get locally in the notebook. Maybe the titles go outside figure - IIRC notebook usually extends the rendered space in such cases, but the saved image is still cropped (meaning: not extended). I'll add top gridspec_kws in the example then.

Co-authored-by: Eric Larson <larson.eric.d@gmail.com>

mmagnuski · 2020-09-03T14:06:53Z

tutorials/intro/plot_40_sensor_locations.py

+# ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+#
+# Channel positions in 2d space are obtained by projecting their actual 3d
+# positions using a sphere as a reference. Because ``'standard_1020'`` montage


@larsoner I changed this to "using a sphere as a reference" - this may be less clear, but is closer to what we do. After all it was funny to mention that channels are projected to a sphere in a PR titled "do not project to sphere" 😀

mmagnuski · 2020-09-03T15:23:37Z

The links to the tutorial and the example:
https://22069-1301584-gh.circle-artifacts.com/0/dev/auto_tutorials/intro/plot_40_sensor_locations.html
https://22069-1301584-gh.circle-artifacts.com/0/dev/auto_examples/visualization/plot_eeglab_head_sphere.html

The remaining TODOs (feel free to add your comments to be adressed here):

change the sphinx_gallery_thumbnail_number in the tutorial to point to 8th image
[circle full]

mmagnuski · 2020-09-03T15:44:56Z

@larsoner @agramfort
I'm also thinking about how the Layout is discussed in the tutorial. The tutorial says:

Layouts are idealized 2-D representations of sensor positions, and are primarily used for arranging individual sensor subplots in a topoplot (...)

This might be confusing to some people as topoplot outside of mne is commonly associated with what is known as topomap in mne. However usage of layouts has been deprecated in topomaps.
It might be enough to to just add a link to mne-topoplot example, but a side box with clarification on terminology might be helpful. I can deal with this in a separate PR.

drammock

this looks great. I've made a bunch of small nitpicky suggestions, but generally I think it's very clear and helpful.

examples/visualization/plot_eeglab_head_sphere.py

drammock · 2020-09-03T16:42:54Z

examples/visualization/plot_eeglab_head_sphere.py

+# make x and limits the same in both panels
+ylm = ax[1].get_ylim()
+xlm = ax[1].get_xlim()
+ax[0].set_ylim(ylm)
+ax[0].set_xlim(xlm)


passing sharex and sharey to plt.subplots above should make this unnecessary

Oh, right, thanks!

drammock · 2020-09-03T16:45:02Z

examples/visualization/plot_eeglab_head_sphere.py

+# make x and limits the same in both panels
+ylm = ax[1].get_ylim()
+xlm = ax[1].get_xlim()
+ax[0].set_ylim(ylm)
+ax[0].set_xlim(xlm)


sharex / sharey again

Co-authored-by: Daniel McCloy <dan@mccloy.info>

mmagnuski · 2020-09-03T17:00:43Z

Thanks for all the suggestions @drammock !

… thumbnail

larsoner

LGTM @drammock feel free to merge if you're happy

drammock · 2020-09-03T22:30:01Z

thanks @mmagnuski!

mmagnuski · 2020-09-05T15:28:04Z

Thanks @larsoner and @drammock! 🚀
@larsoner - I forgot about one of your suggestions and also didn't do the [circle-full], I'll prepare a small PR tomorrow (also trying to fix what I described about how Layout is discussed in the tutorial).

* upstream/master: (489 commits) MRG, DOC: Fix ICA docstring, add whitening (mne-tools#8227) MRG: Extract measurement date and age for NIRX files (mne-tools#7891) Nihon Kohden EEG file reader WIP (mne-tools#6017) BUG: Fix scaling for src_mri_t in coreg (mne-tools#8223) MRG: Set pyvista as default 3d backend (mne-tools#8220) MRG: Recreate our helmet graphic (mne-tools#8116) [MRG] Adding get_montage for montage to BaseRaw objects (mne-tools#7667) ENH: Allow setting tqdm backend (mne-tools#8177) [MRG, IO] Persyst reader into Raw object (mne-tools#8176) MRG, BUG: Fix errors in IO/loading/projectors (mne-tools#8210) MAINT: vectorize _read_annotations_edf (mne-tools#8214) FIX : events_from_annotation when annotations.orig_time is None and f… (mne-tools#8209) FIX: do not project to sphere; DOC - explain how to get EEGLAB-like topoplots (mne-tools#7455) [MRG, DOC] Added linear algebra of transform to doc (mne-tools#7087) FIX: Travis failure on python3.8.1 (mne-tools#8207) BF: String formatting in exception message (mne-tools#8206) BUG: Fix STC limit bug (mne-tools#8202) MRG, DOC: fix ica tutorial (mne-tools#8175) CSP component order selection (mne-tools#8151) MRG, ENH: Add on_missing to plot_events (mne-tools#8198) ...

@drammock

…opoplots (mne-tools#7455) * FIX: do not project to sphere * FIX: fix tests * FIX: check if border is correct before _get_extra_points * DOC: add controlling channel topo projection to sensor locations tutorial * better option checking Co-authored-by: Eric Larson <larson.eric.d@gmail.com> * fix missing space Co-authored-by: Eric Larson <larson.eric.d@gmail.com> * use Eric's suggestion - format string Co-authored-by: Eric Larson <larson.eric.d@gmail.com> * use Eric's suggestion - random seed Co-authored-by: Eric Larson <larson.eric.d@gmail.com> * DOC: add example and link the tutorial and the example * FIX: fix syntax and spelling in the example * FIX: fix example * FIX: more fixes, the example and one topomap test * FIX: and some more fixes to the example * Apply suggestions from Daniel's code review Co-authored-by: Daniel McCloy <dan@mccloy.info> * DOC: use sharex, sharey, as suggested by @drammock, also fix tutorial thumbnail * FIX: fix links in the example Co-authored-by: Eric Larson <larson.eric.d@gmail.com> Co-authored-by: Daniel McCloy <dan@mccloy.info>

cbrnr mentioned this pull request Mar 16, 2020

Default standard_1020 montage contains realistic, not template positions #7141

Open

cbrnr mentioned this pull request Mar 18, 2020

Create canonical template channel locations #7472

Open

larsoner added this to the 0.20 milestone Mar 19, 2020

larsoner modified the milestones: 0.20, 0.21 Mar 27, 2020

larsoner mentioned this pull request Mar 27, 2020

MRG, DOC: Add note about head radius [ci skip] #7524

Merged

mmagnuski mentioned this pull request Apr 3, 2020

VIZ: plot_topomap outlines 'head' and 'skirt' have the same output #7397

Closed

larsoner mentioned this pull request Apr 13, 2020

MRG, BUG: Use auto extrap and mean borders #7603

Merged

2 tasks

DOC: add controlling channel topo projection to sensor locations tuto…

5b5a261

…rial

larsoner reviewed Sep 2, 2020

View reviewed changes

tutorials/intro/plot_40_sensor_locations.py Outdated Show resolved Hide resolved

mmagnuski and others added 2 commits September 2, 2020 15:31

better option checking

f305967

Co-authored-by: Eric Larson <larson.eric.d@gmail.com>

fix missing space

bc2beb8

Co-authored-by: Eric Larson <larson.eric.d@gmail.com>

sappelhoff reviewed Sep 2, 2020

View reviewed changes

larsoner mentioned this pull request Sep 2, 2020

ENH: When using set_montage additional channels as extra points #8195

Open

larsoner reviewed Sep 2, 2020

View reviewed changes

mmagnuski and others added 3 commits September 3, 2020 14:42

use Eric's suggestion - format string

3c87ef9

Co-authored-by: Eric Larson <larson.eric.d@gmail.com>

use Eric's suggestion - random seed

05419be

Co-authored-by: Eric Larson <larson.eric.d@gmail.com>

DOC: add example and link the tutorial and the example

4a10491

mmagnuski commented Sep 3, 2020

View reviewed changes

FIX: fix syntax and spelling in the example

ea7517c

mmagnuski changed the title ~~[WIP] FIX: do not project to sphere~~ FIX: do not project to sphere; DOC - explain how to get EEGLAB-like topoplots Sep 3, 2020

mmagnuski added 3 commits September 3, 2020 16:24

FIX: fix example

d065aae

FIX: more fixes, the example and one topomap test

15e531f

FIX: and some more fixes to the example

4d99321

drammock reviewed Sep 3, 2020

View reviewed changes

Apply suggestions from Daniel's code review

3ec6dd4

Co-authored-by: Daniel McCloy <dan@mccloy.info>

mmagnuski added 2 commits September 3, 2020 19:06

DOC: use sharex, sharey, as suggested by @drammock, also fix tutorial…

fda0f1f

… thumbnail

FIX: fix links in the example

6b85132

larsoner approved these changes Sep 3, 2020

View reviewed changes

drammock merged commit f6cb511 into mne-tools:master Sep 3, 2020

		# In mne-python the head center and therefore the sphere center are calculated
		# using fiducial points. Because of this the head circle represents head

Uh oh!

FIX: do not project to sphere; DOC - explain how to get EEGLAB-like topoplots #7455

FIX: do not project to sphere; DOC - explain how to get EEGLAB-like topoplots #7455

Uh oh!

Conversation

mmagnuski commented Mar 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agramfort commented Mar 16, 2020

Uh oh!

larsoner commented Mar 16, 2020

Uh oh!

larsoner commented Mar 16, 2020

Uh oh!

cbrnr commented Mar 16, 2020

Uh oh!

mmagnuski commented Mar 16, 2020

Uh oh!

mmagnuski commented Mar 17, 2020

Uh oh!

agramfort commented Mar 24, 2020

Uh oh!

mmagnuski commented Mar 25, 2020

Uh oh!

agramfort commented Mar 25, 2020 via email

Uh oh!

agramfort commented Mar 25, 2020 via email

Uh oh!

larsoner commented Mar 25, 2020

Uh oh!

mmagnuski commented Mar 25, 2020

Uh oh!

codecov bot commented Mar 25, 2020

Codecov Report

Uh oh!

larsoner commented Mar 25, 2020

Uh oh!

mmagnuski commented Mar 26, 2020

Uh oh!

cbrnr commented Mar 26, 2020

Uh oh!

larsoner commented Mar 26, 2020

Uh oh!

agramfort commented Mar 26, 2020 via email

Uh oh!

mmagnuski commented Mar 26, 2020

Uh oh!

mmagnuski commented Mar 26, 2020

Uh oh!

mmagnuski commented Mar 26, 2020

Uh oh!

agramfort commented Mar 26, 2020 via email

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

mmagnuski commented Mar 16, 2020 •

edited

Loading