[MRG] FIX: read easycap-M10 montage #7224

christianbrodbeck · 2020-01-17T03:43:58Z

There was a bug in the standard montage reader that read easycap-M1 even when easycap-M10 was specified.

While investigating I also noticed that the DigMontage.plot default parameters don't agree with the documentation, below I changed the default parameters to match the docs (since the docs do agree with the default parameters of plot_montage()).

larsoner · 2020-01-17T05:17:54Z

mne/channels/_standard_montage_utils.py

+        rpa = np.mean([ch_pos['FT10'], ch_pos['TP10']], axis=0)
+        rpa *= head_size / np.linalg.norm(rpa)
+    elif basename == 'easycap-M10.txt':
+        nasion = lpa = rpa = None


This change does not look like reading one versus the other but rather whether or not the nasion/LPA/RPA are defined for the montage. Can you explain a bit?

Selecting the right cap happens on line 130/133 in the next hunk below. This here is a secondary change -- the M10 montage has different channel names so inferring the nasion can't be done with the same code. Based in this mapping (p. 5) the same heuristic could be applied though.

Without inferring fiducials:

With inferring fiducials:

Is it worth shifting the inferred LPA/RPA to fit the head more precisely?

The best thing to do would be to figure out where they are supposed to be in the montage, rather than judge based on the plot

At least for M1 I think that based on a standard 1020 layout definition we should be able to figure it out. It looks like like T3 and T4 being 10% of the way between LPA/RPA and Cz, and Fpz / Oz are 10% up from Nasion/Inion:

http://chgd.umich.edu/wp-content/uploads/2014/06/10-20_system_positioning.pdf

I'd first verify that the spacings in that doc are (at least approximately) accurate for M1 and then go from there.

For M10 we might want to see if the numerical names have readable equivalents (e.g., "10" is "Cz" or whatever) and go from there

The first plot looks much better - that seems to be the exact problem I have in plots in #7193.
@larsoner, @christianbrodbeck - I have digitized positions for this cap, I think, I can take a look.

(ok, the problem is different in the PR, there it is yet another eeg montage. But I should have easycap digitized too)

That would be helpful if you can check. And we should check that the percentage arc length math works, too. If it doesn't we should figure out how much they differ and choose which one seems correct.

Thanks @larsoner! These are the new montages after adding fiducials based on the spherical coordinate system:

codecov · 2020-01-17T11:17:58Z

Codecov Report

Merging #7224 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #7224   +/-   ##
=======================================
  Coverage   89.77%   89.77%           
=======================================
  Files         445      445           
  Lines       80134    80134           
  Branches    12820    12820           
=======================================
  Hits        71938    71938           
  Misses       5378     5378           
  Partials     2818     2818

larsoner

Looks reasonable to me assuming 115 is the correct theta

mne/channels/_standard_montage_utils.py

mmagnuski · 2020-01-18T17:13:36Z

These look good, if you still need a digitized coordinates I paste them for the M1 cap here (the file is actually a .csv file but github doesn't allow droping such files to posts, so I renamed to .txt):

digitization_rotated_coords.txt

This was done with photogrammetry from video frames, without control points in physical units, so the units do not mean anything.
I should have the M10 digitized positions somewhere too - so I can drop them too if they'd be useful.

christianbrodbeck

@mmagnuski in that case you have digitized fiducials so this should be a different issue right?

christianbrodbeck · 2020-01-18T17:24:06Z

Trying to Resolve conflicts but haven't used that feature before, the top says "committing changes -> christianbrodbeck:easycap" but the Commit merge button looks scary, will this commmit into the PR or merge the PR into master?

agramfort · 2020-01-18T17:28:49Z

Merge with master manually if easier

christianbrodbeck · 2020-01-18T17:40:23Z

Resolving the conflicts is easy enough but I did not want to merge into master before it's approved.

mmagnuski · 2020-01-18T19:08:01Z

@christianbrodbeck I'm not sure I understand. Eric wrote it would be useful if I posted the digitized positions in resolving the issue here, so I did.

larsoner · 2020-01-19T01:32:12Z

will this commmit into the PR or merge the PR into master?

This would just make a "merge commit" in your PR, it would not merge your changes into master. Eventually we'll squash and merge as usual.

in that case you have digitized fiducials so this should be a different issue right?

These look good, if you still need a digitized coordinates I paste them for the M1 cap here... Eric wrote it would be useful if I posted the digitized positions in resolving the issue here, so I did.

Indeed it would be nice to see at least how close the spherical representation is to the digitized one.

I say we merge this once the conflicts are fixed (via the GitHub interface with a merge commit or via rebasing, whatever you find easiest), and then I'm happy to take a quick look at some point at the digitized file (it's not a blocker here). I would say don't bother with the M10 for now unless it's trivial or you are motivated to find it.

christianbrodbeck · 2020-01-19T13:28:16Z

Ah, I made the merge commit.

mmagnuski · 2020-01-19T13:33:38Z

@larsoner @christianbrodbeck
Sure, I can even take a look at the comparison once this PR is merged.

agramfort · 2020-01-19T15:12:51Z

can you touch examples that use this easycap-M10 file such as

https://mne.tools/dev/auto_examples/visualization/plot_montage.html#sphx-glr-auto-examples-visualization-plot-montage-py

to see how it looks like now?

christianbrodbeck · 2020-01-19T21:06:10Z

@agramfort This example's example build artifact does not seem to contain mayavi images, and a search for "easycap" does not yield any other examples...

mmagnuski · 2020-01-19T21:23:21Z

@christianbrodbeck You need to change something in an example for it to render. That's what Alex meant with "touch".

larsoner · 2020-01-19T22:54:00Z

I would say that the new one looks better than master, in that it looks more consistent with the other layouts

mmagnuski · 2020-01-19T23:04:24Z

https://17597-1301584-gh.circle-artifacts.com/0/dev/auto_examples/visualization/plot_montage.html#sphx-glr-auto-examples-visualization-plot-montage-py

@larsoner
I have one question: why do the channels that are on the circumference of the sphere (3d plot) do not get plotted on the head circle in the topomap? They show within the circle, while the last channel "ring" - the one below the circumference is plotted on the head circle.
Is this behavior expected? I much prefer the circumference to be translated to the head circle (this is also something that eeglab does, IIRC).

larsoner · 2020-01-19T23:19:09Z

In the general case you have to define what you mean by circumference. Relative to what sphere center and what radius?

In spherical montage cases his seems perhaps trivial to define, but that is a specific case of the more general (realistic dig / not true sphere) dig case. At some point we decided that anything at z=0 would be on the head sphere, where z we choose somehow suitably in head coordinates.

Basically I'm open to figuring out how to redefine how we plot things, but it's not a easy as saying "it should be done this way", because it's better if we have something that works for all cases rather than special casing the spherical montage case. So if we are going to change it, we need to understand what we do now and how it should change for all cases.

larsoner · 2020-01-19T23:54:35Z

where z we choose somehow suitably in head coordinates.

I remembered a detail, which is why this it didn't seem trivial for the cases like this. We want the sensor positions to stay the same regardless when and which channels you pick from the data, i.e., the location of channel X in the full montage should end up being plotted in the same location relative to the spherical head whether or not you've done raw.pick_channels(['X']). Because of how picking works, channel locations in info['chs'] are lost when picking. So we cannot choose/fit the sphere (origin and radius) to plot relative to using the channel positions, we have to use info['dig'] instead.

You could say that keeping the EEG locations in dig after picking (even if they disappear from info['chs'][ii]['loc'] could get around this problem. But then you run into a different problem, namely that doing:

raw.pick_channels(['X']).set_montage(std).plot_montage() would look different from doing
raw.set_montage(std).pick_channels(['X']).plot_montage()

because we currently only populate info['dig'] based on the channels that are actually there.

I guess we could get around this by always populating info['dig'] with all locations from the provided montage even when only a subset of locations are present. Then we could make it so that we always fit with extra+eeg locations in info['dig'] rather than only with extra points.

I'm not sure if this would have side effects but we could try it. It would at least probably produce the behavior that you were expecting. It's arguably more consistent with what we do in the true-digitization case, because we do not get modify info['dig'] when picking channels, so even if we are only left with one of the original 60 EEG channels, we will still have 60 (usually actually 61 because ref is in there) locations in info['dig']. But it will often break the relationship that originally exists in true files, which is the points in info['dig'] do correspond exactly to the locations of the (ref plus) channels in the data file.

Modifying the dig code is not easy, though, so if this seems like a reasonable approach I can look
-- I messed with that code recently. I think I'm +0.5 on trying it to see if it helps

mmagnuski · 2020-01-20T12:49:54Z

In spherical montage cases his seems perhaps trivial to define, but that is a specific case of the more general (realistic dig / not true sphere) dig case. At some point we decided that anything at z=0 would be on the head sphere, where z we choose somehow suitably in head coordinates.

Basically I'm open to figuring out how to redefine how we plot things, but it's not a easy as saying "it should be done this way", because it's better if we have something that works for all cases rather than special casing the spherical montage case. So if we are going to change it, we need to understand what we do now and how it should change for all cases.

Oh, I didn't mean to sound "it should be done this way". What you describe sounds ok - the user can easily change the coordinates so that the channels are plotted the way they prefer.
I was just wondering why the 3d sphere and channel locations in the rendered example look as if the last channel "ring" was <z but it was still plotted on the head circle in the examples posted by @christianbrodbeck.

So we cannot choose/fit the sphere (origin and radius) to plot relative to using the channel positions, we have to use info['dig'] instead.

I don't see a problem here actually (but I may be missing something) - the default sphere is in (0, 0) and if the center is somewhere else the user can change that using sphere= (and the same for radius).

I don't want to pollute this PR with my slightly off-topic questions, we can move the discussion somwehere else - I can for example open a PR to add the info about topomap plotting and sphere to the channel positions tutorial (or add this info to the topomap example) and we could continue there.

larsoner · 2020-01-20T13:21:28Z

Sounds good to me

christianbrodbeck · 2020-01-21T02:51:02Z

As far as I am concerned this is ready for merge, I don't think there was anything left to address?

larsoner · 2020-01-21T13:13:47Z

Failures are unrelated pip 0.20 errors, thanks @christianbrodbeck

* FIX: Easycap-M10 montage * FIX: DigMontage.plot() defaults * Infer fiducials * STY * ENH Easycap montages: add fiducials in spherical coordinates * DOC [ci skip] * STY: capitalization Co-authored-by: Eric Larson <larson.eric.d@gmail.com>

christianbrodbeck added 2 commits January 16, 2020 20:50

FIX: Easycap-M10 montage

03dbf32

FIX: DigMontage.plot() defaults

2037090

christianbrodbeck added the BUG label Jan 17, 2020

larsoner reviewed Jan 17, 2020

View reviewed changes

Infer fiducials

a7197a7

christianbrodbeck added 2 commits January 18, 2020 10:58

STY

a8bff3e

ENH Easycap montages: add fiducials in spherical coordinates

fce3d98

larsoner reviewed Jan 18, 2020

View reviewed changes

mne/channels/_standard_montage_utils.py Show resolved Hide resolved

DOC [ci skip]

3a30bbc

christianbrodbeck commented Jan 18, 2020

View reviewed changes

Merge branch 'master' into easycap

aca2d9e

STY: capitalization

ec16ddd

christianbrodbeck changed the title ~~FIX: read easycap-M10 montage~~ [MRG] FIX: read easycap-M10 montage Jan 21, 2020

Merge branch 'master' into easycap

235a66a

larsoner merged commit 1ef6ebe into mne-tools:master Jan 21, 2020

Uh oh!

[MRG] FIX: read easycap-M10 montage #7224

[MRG] FIX: read easycap-M10 montage #7224

Uh oh!

Conversation

christianbrodbeck commented Jan 17, 2020

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!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jan 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

larsoner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mmagnuski commented Jan 18, 2020

Uh oh!

christianbrodbeck left a comment

Choose a reason for hiding this comment

Uh oh!

christianbrodbeck commented Jan 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agramfort commented Jan 18, 2020 via email

Uh oh!

christianbrodbeck commented Jan 18, 2020

Uh oh!

mmagnuski commented Jan 18, 2020

Uh oh!

larsoner commented Jan 19, 2020

Uh oh!

christianbrodbeck commented Jan 19, 2020

Uh oh!

mmagnuski commented Jan 19, 2020

Uh oh!

agramfort commented Jan 19, 2020

Uh oh!

christianbrodbeck commented Jan 19, 2020

Uh oh!

mmagnuski commented Jan 19, 2020

Uh oh!

larsoner commented Jan 19, 2020

Uh oh!

mmagnuski commented Jan 19, 2020

Uh oh!

larsoner commented Jan 19, 2020

Uh oh!

larsoner commented Jan 19, 2020

Uh oh!

mmagnuski commented Jan 20, 2020

Uh oh!

larsoner commented Jan 20, 2020

Uh oh!

christianbrodbeck commented Jan 21, 2020

Uh oh!

larsoner commented Jan 21, 2020

Uh oh!

Reviewers

Assignees

Labels

codecov bot commented Jan 17, 2020 •

edited

Loading

christianbrodbeck commented Jan 18, 2020 •

edited

Loading