Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH: plot_topo layout creation #3987

Closed
larsoner opened this issue Feb 14, 2017 · 23 comments · Fixed by #7066
Closed

ENH: plot_topo layout creation #3987

larsoner opened this issue Feb 14, 2017 · 23 comments · Fixed by #7066
Assignees
Milestone

Comments

@larsoner
Copy link
Member

It seems like we keep hitting issues with topo scaling confusing people. I think we probably shouldn't change where (0., 0., 0.) is in any given channel layout, and instead ensure that our internal layouts are actually in head coordinates. I wonder if we could also just assume a head is e.g. 90 cm. That would make subselections of electrodes appear correct. A normalize=True (default) | False argument in functions that do such topologies could do it.

@jaeilepp WDYT?

@jaeilepp
Copy link
Contributor

Yeah, that sounds like a good idea. I was already experimenting with this, but it seems that _check_outlines may require some tweaking to make it work well with topomaps.

@larsoner
Copy link
Member Author

This would fix #3814

@jaeilepp
Copy link
Contributor

I'm thinking that maybe the head coordinates should only apply when the layout is custom made. After all, the locations are only used in layout plots and they're not actual sensor locations. I noticed that vectorview locations for instance are not centered to 0. So some kind of adjustment would be needed anyway.
legend

@agramfort
Copy link
Member

agramfort commented Feb 22, 2017 via email

@larsoner
Copy link
Member Author

larsoner commented Feb 22, 2017 via email

@jaeilepp
Copy link
Contributor

Why would it go away? These are just the sensor locations from the layout file.

@larsoner
Copy link
Member Author

larsoner commented Feb 22, 2017 via email

@jaeilepp
Copy link
Contributor

But these are static locations from the layout file. Why would it help to transform them?

@larsoner
Copy link
Member Author

If we are plotting sensors on top of a head, it would be best to plot the sensors with the same (or at
least similar) geometrical relationship they actually had with the head during the recording. For MEG sensors this means using dev_head_t to get them to head coordinates.

@larsoner
Copy link
Member Author

... so maybe not for plot_topo (and even then I'm not sure), but for plot_topomap it seems like we should always do things in head coordinates. Concretely the proposal would be:

  1. Convert MEG sensors to head coords
  2. Assume (0, 0, 0) is meaningful: don't re-center
  3. Assume the head is about 90cm in diameter: don't rescale sensor positions
  4. Any built-in EEG layouts that don't give "sensor locations in head coordinates" should be put through a function that effectively makes them be in head coordinates

This will give us much more realistic geometry than our current procedure, which is just to center based on extrema and scale everything to hit the boundaries.

@jaeilepp
Copy link
Contributor

jaeilepp commented Mar 2, 2017

Any built-in EEG layouts that don't give "sensor locations in head coordinates" should be put through a function that effectively makes them be in head coordinates

Even some of our built-in MEG layouts don't give head coordinates (like KIT-AD). I'm not sure it's worth the effort to change this. After all, in most cases our layouts work fine and the change would require overhaul of all the plotting functions that use layouts. Also, topomaps use layouts so it might be dangerous to change this.

@larsoner
Copy link
Member Author

larsoner commented Mar 2, 2017 via email

@jaeilepp
Copy link
Contributor

jaeilepp commented Mar 2, 2017

The layouts should still be used at least for topo-plots since the sensor triplets have identical positions.

@larsoner
Copy link
Member Author

larsoner commented Mar 2, 2017

Ahh right. Forgot about that :)

And I guess there is some logic to the "unwrapping" transformation we normally do, since it's just one way (of many possibilities) to do a projection of spherical data.

Maybe just disabling norm and/or centering is enough

@larsoner larsoner added this to the 0.15 milestone Apr 18, 2017
@larsoner
Copy link
Member Author

@jaeilepp I think you have more or less fixed this, right?

@jaeilepp
Copy link
Contributor

Yeah, more or less. Maybe eventually it could be good to shift to convention you suggested, but I don't see a lot to be gained. Closing.

@larsoner larsoner reopened this May 3, 2017
@larsoner
Copy link
Member Author

larsoner commented May 3, 2017

This is essentially resurrected by #4239.

My proposal is that:

  1. Any time we just want to select sensors we can use a layout with scaling, centering, etc. For example, this would affect sensor selection in raw.plot and plotting done with plot_topo.

  2. Any time we want to show sensors in relationship to a head (topomap, joint plots, legends like in [MRG] Change legend to use absolute positions. #4239) we should always put sensors in head coordinates. We should thus use real sensor positions, and not center or scale them. In the case of MEG sensors, this means using the dev_head_t and chs[n]['loc']. Otherwise we are mis-representing the relationship between the sensors and the head. We should choose the head sphere fit to digitization or fiducials, either centered on the head coord frame center or fit to points (not sure whiche).

This will fix issues like evoked.plot_topo() and evoked.pick_channels(evoked.ch_names(:10)) not having the 10 sensors in the same position (which is a bit crazy IMO).

Edit: To clarify, layout is then used for (1) and montage / DigMontage / physical positions are used for (2).

@larsoner
Copy link
Member Author

larsoner commented May 3, 2017

... so the distinction would be that Layout is only ever used for interactive "sensor selection" stuff, and never shown on a head. Only sensor positions in head coordinates ever get shown on a head (which could easily tie in eventually to the proposed Digitization class).

@agramfort
Copy link
Member

agramfort commented May 3, 2017 via email

@larsoner larsoner self-assigned this Aug 6, 2017
@larsoner larsoner modified the milestones: 0.15, 0.16 Sep 14, 2017
@larsoner
Copy link
Member Author

This one won't happen for 0.15, moving to 0.16

@larsoner
Copy link
Member Author

larsoner commented Apr 5, 2018

I don't see this as a blocker for 0.16 and whatever solution we come up with should be tested thoroughly, so moving to 0.17

@larsoner
Copy link
Member Author

Superceded by #5472 and #5471

@larsoner
Copy link
Member Author

larsoner commented May 6, 2019

Not fixed yet, so reopening

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment