Topo plots for EEG data, even when no digitization points are present #1619

wmvanvliet · 2014-10-21T12:47:34Z

Many EEG-only labs (including mine) don't bother with source localization and so don't mark EEG electrode locations. When plotting, relying on the approximate electrode positions reported by the cap manufacturer is good enough. In practice, this means ch['eeg_loc'] and ch['loc'] are specified, but no digitization points are present in the data. This breaks make_eeg_layout, which is currently very picky about which digitization points should be present. This PR makes the function a lot less picky:

Add flag to select which digitization points to use in
fit_sphere_to_headshape. This defaults to only FIFF.FIFFV_POINT_CARDINAL, which was the default
before I started messing with it in Fix EEG electrode positions in EDF/BDF import. #1541. When calling the function from make_eeg_layout, more points
are considered. I've noticed that Neuromag data adds FIFF.FIFFV_POINT_EEG, while the EDF loader adds
FIFF.FIFFV_POINT_EXTRA, so it looks at both in addition to FIFF.FIFFV_POINT_CARDINAL landmarks.
Make make_eeg_layout still work even if no digitization points are present. Generate a warning though.

wmvanvliet · 2014-10-21T12:49:57Z

@dengemann's montage branch #1390 also doesn't add any digitization points, so we're going to need this in order to have a smooth workflow.

agramfort · 2014-10-21T12:54:31Z

looks clean to me. Thanks @wmvanvliet

@dengemann please review

dengemann · 2014-10-21T12:57:59Z

mne/preprocessing/maxfilter.py

this is not a good idea. funky things can and will happen with mutable default parameters (google python mutable defaults). The pattern we're using in such cases is default to None and substitute for the value. Also in this case there does not seem to be a reason to use a list. A tuple would be more appropriate. A tuple with one value will howeve not look that nice in a default parameter. So None as default and the tuple for substitution.

Tuples are immutable, so this could just be dig_kinds=(FIFF.FIFFV_POINT_CARDINAL,).

However, this is an API change. I think the default needs to be (FIFFV.FIFFV_POINT_CARDINAL, FIFFV_FIFFV_POINT_EXTRA).

dengemann · 2014-10-21T12:59:31Z

@wmvanvliet besided my comments LGTM!

larsoner · 2014-10-21T14:04:38Z

mne/preprocessing/maxfilter.py

You don't need this check

wmvanvliet · 2014-10-21T15:13:29Z

Since I'm changing things in fit_sphere_to_headshape I added some tests.
One test case that failed was when only the nasion and left/right auricular points are digitized. In the default case, the function only takes cardinal points and I guess that means it commonly only takes these three landmarks. It fails, because these three points lie in the same plane, leaving the z coordinate of the center of the sphere undefined.

@agramfort you are mentioned as one of the authors of this function. I've taken the liberty of modifying it to detect this case and handle it gracefully. It forces the center of the sphere to be in the same plane as the other points in this case. Is this acceptable? Will it horribly brake everyone's maxfilter results?

larsoner · 2014-10-21T15:47:38Z

I mentioned these two points already, but it must have been missed:

A tuple like (1, 2) is immutable, so if there is a default that is just a list of integers, you can just use that. So I don't see a reason to have None be the default choice...
The default point types should not change, because it constitutes an API change -- it should stay whatever it was before, which I think was to use the EXTRA points. In this particular case, the API change would indeed bad for most users, since most standard Neuromag recording setups only have 3 cardinal points (L/R auriculars and nasion). This means you'd get errors when trying to fit, and if somehow you didn't, your fit would be (much) worse than when using the extra points because you'd be fitting four parameters (XYZ+radius) using only a handful of points instead of 100+ (the typical choice for the number of extra points to digitize).

Make sense?

wmvanvliet · 2014-10-21T20:14:28Z

@Eric89GXL that makes perfect sense, you are right. If EXTRA points are the default, the case that only nasion/auricular points are taken should hardly ever occur. This makes my patch to fit_sphere_to_headshape just add pointless complexity.

wmvanvliet · 2014-10-21T20:31:57Z

Ok, the PR should now be in a usable state. What do you think?

larsoner · 2014-10-21T20:33:55Z

mne/layouts/layout.py

FWIW maxfilter assumes an origin of [0, 0, 40] (in mm) by default, not sure if we want to adopt the same here for layout generation

The general use case for not having any digitization points is to use the electrode positions specified by the cap manufacturer, which place the head center at (0, 0, 0).

Ahh, makes sense. I wonder if it would be better to have center=None be a kwarg. If None, then estimate, throwing error if it doesn't exist. If not None, should be a 3-element array-like specifying the position. It would add a little bit of overhead for people without digitization, but it has the nice effect of forcing them to explicitly choose the head center. WDYT?

that is a very good idea. That nicely avoids having to spit out an ugly warning too.

larsoner · 2014-10-21T20:35:50Z

Other than my comments, looks reasonable to me.

dengemann · 2014-10-21T20:55:16Z

@wmvanvliet I feel we have some redundant code paths here. Already on master for the use case you describe topomaps can be generated when passing the ch_type ... We should investigate and think about unifying the different code paths if they are really redundant.

wmvanvliet · 2014-10-21T21:07:17Z

@dengemann yes we should

agramfort · 2014-10-22T06:47:49Z

mne/layouts/tests/test_layout.py

you should still check the RuntimeError is properly raised.

agramfort · 2014-10-22T06:51:03Z

besides LGTM

wmvanvliet · 2014-10-22T06:59:53Z

@dengemann as far as I can tell, there is a duplication in code when no layout can be found for the data.

both _auto_topomap_coords and make_eeg_layout take the ['loc'] property of the channels and project them into 2D space, and do so in different ways. This should be merged. What's interesting is that _auto_topomap_coords doesn't bother fitting a sphere to the digitization points at all and instead relies on the head origin being at (0, 0, 0).

Maybe we should rely on the code in _auto_topomap_coords in both cases, but add the fitting of the sphere if usable digitization points are present. This should also improve the accuracy of the topomaps.

agramfort · 2014-10-22T07:07:07Z

good catch... I remember opening an issue on this a while ago...

dengemann · 2014-10-22T07:07:54Z

I think I've added the code that does it without fitting a sphere. It more or less follows fieldtrip I think. Would be good to merge both the way you propose.

On 22 Oct 2014, at 08:59, Marijn van Vliet notifications@github.com wrote:

@dengemann as far as I can tell, there is a duplication in code when no layout can be found for the data.

both _auto_topomap_coords and make_eeg_layout take the ['loc'] property of the channels and project them into 2D space, and do so in different ways. This should be merged. What's interesting is that _auto_topomap_coords doesn't bother fitting a sphere tot he digitization points at all and instead relies on the head origin being at (0, 0, 0).

—
Reply to this email directly or view it on GitHub.

wmvanvliet · 2014-10-22T13:24:30Z

There we go! The codepaths are now merged, tests are added and are passing.

There is a small API issue I would like your opinion on. When no digitization points are present in the data, I like the idea of manually having to specify the head origin (for example to (0, 0, 0)) and raising an error if no origin was specified. For make_eeg_layout, the function would fail in this case anyway, so noone's code will break because of this. However, the topomap functionality silently did this and in the current state of the PR, it still does, overruling fitting with a sphere. The behavior should be consistent when no head origin can be estimated. Some options are:

Fail loud (might break some existing scripts of the users)
Generate a warning and fall back to (0, 0, 0) (might change electrode positions in some existing scripts, users have to notice warning in order to know why)
Silently fall back to (0, 0, 0) (might change electrode positions in some existing scripts, users wonder why)
For topomaps, always fall back to (0, 0, 0) (perfect backwards compatibility)

wmvanvliet · 2014-10-22T13:27:03Z

now that I read this back, option 2 should be a clear winner.

dengemann · 2014-10-22T13:31:20Z

@wmvanvliet I would prefer option 2. It kepps things compatible but exposes the issue. Nice work!

agramfort · 2014-10-22T13:33:26Z

let me know when to review

wmvanvliet · 2014-10-23T10:46:00Z

I've rebased, but Travis is still not happy. Some error in a test that exports a source estimate to a Pandas dataset. A test that succeeds on my machine.

agramfort · 2014-10-23T12:24:56Z

you have a rebase issue as it contains commits already in master.

travis will fail due to pandas update. Don't bother about it if it's just a pandas failure.

wmvanvliet · 2014-10-23T13:16:10Z

OK, this PR is ready for review.

wmvanvliet · 2014-11-08T20:32:01Z

@dengemann better?

wmvanvliet · 2014-11-08T20:33:39Z

@t3on thanks for the review

wmvanvliet · 2014-11-10T08:20:48Z

I think I've addressed everything now

agramfort · 2014-11-10T10:02:25Z

@wmvanvliet can you rebase on master?

wmvanvliet · 2014-11-10T11:06:59Z

Here you go, I squashed everything up to now into a single commit to keep it manageable.

wmvanvliet · 2014-11-10T12:02:18Z

hrm, the imports are failing. What's going on here?

agramfort · 2014-11-10T12:30:23Z

circular import?

try using nested imports at some location to see if it fixes the pb.

agramfort · 2014-11-10T12:30:42Z

mne/channels/layout.py

this import could need a nested import

it does! Had the same issue. It seems like your rebasing un-did some of my changes we just merged. Actually in layuot this import has been removed by myself.

dengemann · 2014-11-10T17:03:02Z

mne/channels/layout.py

@wmvanvliet ping defaults are still missing. Add: "Defaults to XXX." everywhere where XXX is the default

agramfort · 2014-11-10T17:06:31Z

besides LGTM

@christianbrodbeck last chance to take a look :)

christianbrodbeck · 2014-11-10T17:10:19Z

Thanks @agramfort I trust that it's fine :) , doesn't touch any code that I've been recently working with

wmvanvliet · 2014-11-11T07:45:54Z

@dengemann could you take a look at the documentation for the _find_topomap_coods function? It's unclear to me what the layout parameter does exactly and what happens if it's not supplied. Other than that, I've fixed up the rest of the documentation in the file a bit.

wmvanvliet · 2014-11-11T09:42:03Z

Hold on. Removing the fitting of a head sphere and omitting transforming the eeg_loc into head coordinates may have broken things.

Where and when exactly are EEG digitization points transformed into head coordinates? What's the difference between ch['loc'] and ch['eeg_loc']?

agramfort · 2014-11-11T10:01:00Z

when reading the file from disk loc and eeg_loc contain the same info:

In [6]: evoked.info['chs'][350]['eeg_loc']
Out[6]:
array([[-0.0785085 ,  0.00235201],
       [-0.03574997,  0.11096951],
       [ 0.03009207, -0.03500458]])

In [7]: evoked.info['chs'][350]['loc'].reshape(4, 3).T
Out[7]:
array([[-0.0785085 ,  0.00235201,  0.        ,  0.        ],
       [-0.03574997,  0.11096951,  1.        ,  0.        ],
       [ 0.03009207, -0.03500458,  0.        ,  1.        ]], dtype=float32)

I don't know for the rest.

wmvanvliet · 2014-11-11T10:57:20Z

never mind, it's my data that has weird digitization points.

wmvanvliet · 2014-11-11T10:57:37Z

this PR is ready for yet another evaluation round :)

agramfort · 2014-11-11T11:27:21Z

+1 for merge.

dengemann · 2014-11-11T11:33:55Z

@wmvanvliet nice work! Thanks a lot! Merging.

Topo plots for EEG data, even when no digitization points are present

dengemann reviewed Oct 21, 2014
View reviewed changes

larsoner reviewed Oct 21, 2014
View reviewed changes

mne/preprocessing/maxfilter.py Outdated

Copy link

Member

larsoner Oct 21, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need this check

larsoner reviewed Oct 21, 2014
View reviewed changes

agramfort reviewed Oct 22, 2014
View reviewed changes

mne/layouts/tests/test_layout.py Outdated

Copy link

Member

agramfort Oct 22, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should still check the RuntimeError is properly raised.

wmvanvliet force-pushed the eeg-layout-fix branch from 8684639 to 5354c02 Compare October 23, 2014 13:09

This was referenced Nov 9, 2014

dev_head_t needed for plotting EEG data? #1645

Closed

ENH: add spatio-temporal clustering example for sensor data #1642

Merged

Rebase onto master

a8d3c2d

wmvanvliet force-pushed the eeg-layout-fix branch from cf42a26 to a8d3c2d Compare November 10, 2014 11:04

attempt to import fit_sphere_to_headshape

f81097c

agramfort reviewed Nov 10, 2014
View reviewed changes

nested import

de95915

dengemann reviewed Nov 10, 2014
View reviewed changes

mne/channels/layout.py

Copy link

Member

dengemann Nov 10, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wmvanvliet ping defaults are still missing. Add: "Defaults to XXX." everywhere where XXX is the default

Doc improvements + PEP8

fdb83af

dengemann added a commit that referenced this pull request Nov 11, 2014

Merge pull request #1619 from wmvanvliet/eeg-layout-fix

7ffa765

Topo plots for EEG data, even when no digitization points are present

dengemann merged commit 7ffa765 into mne-tools:master Nov 11, 2014

wmvanvliet deleted the eeg-layout-fix branch November 11, 2014 12:24

Uh oh!

Topo plots for EEG data, even when no digitization points are present #1619

Topo plots for EEG data, even when no digitization points are present #1619

Uh oh!

Conversation

wmvanvliet commented Oct 21, 2014

Uh oh!

wmvanvliet commented Oct 21, 2014

Uh oh!

agramfort commented Oct 21, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dengemann commented Oct 21, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wmvanvliet commented Oct 21, 2014

Uh oh!

larsoner commented Oct 21, 2014

Uh oh!

wmvanvliet commented Oct 21, 2014

Uh oh!

wmvanvliet commented Oct 21, 2014

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!

larsoner commented Oct 21, 2014

Uh oh!

dengemann commented Oct 21, 2014

Uh oh!

wmvanvliet commented Oct 21, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agramfort commented Oct 22, 2014

Uh oh!

wmvanvliet commented Oct 22, 2014

Uh oh!

agramfort commented Oct 22, 2014

Uh oh!

dengemann commented Oct 22, 2014

Uh oh!

wmvanvliet commented Oct 22, 2014

Uh oh!

wmvanvliet commented Oct 22, 2014

Uh oh!

dengemann commented Oct 22, 2014

Uh oh!

agramfort commented Oct 22, 2014

Uh oh!

wmvanvliet commented Oct 23, 2014

Uh oh!

agramfort commented Oct 23, 2014

Uh oh!

wmvanvliet commented Oct 23, 2014

Uh oh!

wmvanvliet commented Nov 8, 2014

Uh oh!

wmvanvliet commented Nov 8, 2014

Uh oh!

wmvanvliet commented Nov 10, 2014

Uh oh!

agramfort commented Nov 10, 2014

Uh oh!

wmvanvliet commented Nov 10, 2014

Uh oh!

wmvanvliet commented Nov 10, 2014

Uh oh!

agramfort commented Nov 10, 2014

Uh oh!

Choose a reason for hiding this comment