mne-tools / mne-python

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

Bug in ctf readers #3071

Closed jaeilepp closed 8 years ago

jaeilepp commented 8 years ago

Ref mailing list.

Reading CTF data seems to have a bug in defining some of the coil types. I've isolated the issue to https://github.com/mne-tools/mne-python/blob/master/mne/io/ctf/info.py#L209. For some reason some extra bits are added to the coil_type if 'grad_order_no' is non-zero. This causes the coil types to become something outrageous (our spm faces dataset has coil_types of 201609). This seems to happen with both, the c conversion tool and python. What is this grad_order_no? cc @mshamalainen @Eric89GXL

jaeilepp commented 8 years ago

And 201609 = 5001 | 3 << 16 5001 is defined as FIFF.FIFFV_COIL_CTF_GRAD

larsoner commented 8 years ago

I don't think this is a bug in Matti's code but in how we are using coil_type (wherever that is in the original report). See how it's handled in forward:

https://github.com/mne-tools/mne-python/blob/master/mne/forward/_make_forward.py#L126

Which led me to do the same thing a couple of days back in my PR:

https://github.com/mne-tools/mne-python/blob/master/mne/viz/_3d.py#L472

My assumption when I saw this code was that the higher order bits were being used to store some form of additional information. Hopefully @mshamalainen can share some insight but in the meantime we can probably work on fixing wherever coil_type is used instead of changing this (likely correct) behavior.

jaeilepp commented 8 years ago

So this use of higher order bits is only for ctf data? Sounds like something very error prone to me. Any idea where the grad_order_no is used or what does it mean?

larsoner commented 8 years ago

Looking a bit deeper into the CTF code (mne_ctf_comp.c), the higher order bits indicate the compensation grade. Other systems don't have compensation, so they effectively have/use these bits but they are naturally always zero. I haven't looked into grad_order_no, but it could be related to the compensation grade.

There is possibly a CTF-related bug here in the sense that, when we apply compensation, we don't change these higher order bits -- it's possible we're supposed to do that, but I'm not sure.

larsoner commented 8 years ago

@mshamalainen it looks like mne_process_raw with a non-zero --grad option does not modify the coil_type, but I suspect it should. Any ideas?

larsoner commented 8 years ago

From a quick conversation with Matti, it looks like if we do write out data that has been compensated, we should set the higher order bits to the compensation grade.

larsoner commented 8 years ago

@jaeilepp do you remember where you actually saw coil_type being mis-used due to the compensation grade being present?

jaeilepp commented 8 years ago

No idea. I cannot find the mailing list post that had this issue. I think we can close this.