search

mne-tools / mne-python

MNE: Magnetoencephalography (MEG) and Electroencephalography (EEG) in Python
https://mne.tools
BSD 3-Clause "New" or "Revised" License
2.66k stars 1.31k forks source link

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

Open larsoner opened 3 years ago

larsoner commented 3 years ago

It might be better if we make it so that doing inst.set_montage, rather than discarding EEG channels that aren't in the instance, instead added them to info['dig'] as extra / headshape points. It should help with coreg, and make it so that we get consistent sphere fits any time the same montage is used. It would allow us to update the sphere argument of our topomap plotting functions to allow looking for EEG+extra dig points to enable 'auto' fit-based mode.

After #7455 is in, I might try a quick PR to see how this affects the topomaps.

agramfort commented 3 years ago

I am not enough an EEG user to have a strong opinion here.

mmagnuski commented 3 years ago

I'm a bit torn here - fitting a sphere to all the channels might improve how topomaps look, but then the head outline may loose its objective meaning (z level of fiducials). However, for caps with ideal spherical channel positions fitting a sphere to all the channels should produce topomaps closer to what EEGLAB displays, which would be nice.

larsoner commented 3 years ago

The nice thing about fitting to all the points is that you can always override it with your own sphere=(0., 0., 0., ...). So really we're giving people the option of fitting. To me it brings the digitized-locations case and the standard-montage case closer in terms of how they are treated. And as a nice side effect it should make it so that EEG people get more EEG-standard plots.

I say we try it and see how things look with a circle full. I don't think it's going to take too many lines to make this functionality work. Maybe something to do early after 0.21 comes out so that we don't cause ourselves too many release-related headaches?

mmagnuski commented 3 years ago

Sure, as an additional option it is good, I am just not sure it should be the default. But you are right that it might be better to take a more informed decision based on circle full. :)

larsoner commented 3 years ago

We can do 'auto', 'auto_radius', and 'auto_origin' if need be

mmagnuski commented 3 years ago

After some time I'd have to agree that it would be good to automatically fit the sphere by default. I have multiple datasets where I have to specify sphere=... all the time because otherwise the head is almost the size of one channel (the issue is that channel positions were read from eeglab and are in different units, cm most likely - something I could fix for all files, but I use sphere=... all the time because it's faster :) ).

larsoner commented 3 years ago

something I could fix for all files, but I use sphere=... all the time because it's faster :) ).

I think it's a better idea to fix it properly to be in the correct units. Algorithms in MNE -- at the very least the forward solution ones, and possibly things like CSD, too -- depend on the position units being correct and in a reasonable range to work properly. So in this case I'd rather make it easier for you to fix your units, e.g., with montage.reset_units_si(from='cm') or something to reset from whatever they actually are to SI units (m).

mmagnuski commented 3 years ago

good point, I'll fix it then, thanks for reset_units_si(from='cm') - I didn't know about it.

larsoner commented 3 years ago

We don't have such a function, but we could add it. Would it make your life easier?

larsoner commented 3 years ago

... and I think in at least some we have a units option in the read_montage function. If we added it in more of those would it help?

larsoner commented 3 years ago

or we used to. I guess read_custom_montage does not have it but we could add it:

https://mne.tools/dev/generated/mne.channels.read_custom_montage.html#mne.channels.read_custom_montage

If you can share a brief code snippet and (ideally but not 100% necessary) some data we could figure out the best API to get the units set properly.

agramfort commented 3 years ago

+1 fot fix the readers rather than hacking sphere

mmagnuski commented 3 years ago

@larsoner Ah, I misundertood. :)

We don't have such a function, but we could add it. Would it make your life easier?

In this specific case - no, I will have to figure out the scale myself and then dividing would be trivial. :) But its no big deal, and it actually might be my fault - for some of these datasets I have simple reading functions that may predate the times when mne read eeglab channel positions - or there might have been other problems and I was reading channels myself.