search

mne-tools / fiff-constants

Bookkeeping and documentation of FIFF file format constants
4 stars 12 forks source link

FIX: Add X/Y point mags #12

Closed larsoner closed 5 years ago

larsoner commented 5 years ago

Okay my next PR wasn't to add coil_def.dat but rather to add X/Y point mags (2002, 2003; used in fine cal for Maxwell filtering).

There are a bunch of accompanying whitespace changes, but really only two lines are meaningfully added here.

Ready for review/merge from my end. @jnenonen this one is pretty straightforward if you want to experience the joy of pressing the big green button :)

jnenonen commented 5 years ago

Agree with MSH: We should just use the point sets which are in the standard coil_def.dat file generated by MNE-C.

larsoner commented 5 years ago

These are required for fine calibration in Maxwell filtering, though, so I think we need @mshamalainen to add these to coil_def.dat and for us to have them in DictionaryTypes.txt.

larsoner commented 5 years ago

@LorenzE could you look into potentially adding these to coil_def.dat? They are defined (almost trivially) here:

https://github.com/mne-tools/mne-python/blob/master/mne/data/coil_def_Elekta.dat

jnenonen commented 5 years ago

MaxFilter and fine-calibration are independent of the integration point defs, and I expect both the 9- and 16-point formulas to give very similar results. Hence I will leave it up to you to decide what to use in coil_def.dat in mne.

larsoner commented 5 years ago

MaxFilter and fine-calibration are independent of the integration point defs, and I expect both the 9- and 16-point formulas to give very similar results. Hence I will leave it up to you to decide what to use in coil_def.dat in mne.

The reason I'd like to merge these is ~MaxFilter~ Maxwell filterting needs and uses coil definitions for these the same way that forward models in MNE do. In MNE we also have FIFFV_COIL_POINT_MAGNETOMETER_X and ..._Y in mne.io.constants, which makes sense to go along with FIFFV_COIL_POINT_MAGNETOMETER, which is already in coil_def.dat. So it seems most consistent to add the _X and _Y variants for completeness.

Regarding the 9 vs 16-point definition, it sounds like we're going to stick with what MNE currently uses, which makes a slight inconsistency in MNE-Python but it's easy enough to work around. But the fewer of these inconsistencies, the better -- and it looks like adding the X and Y point-mags to coil_def.dat is a pretty low-effort solution to gain consistency here. @LorenzE WDYT?