add ch_type and coils for eye-tracker data

[dominikwelke]

hi all,

I'm working on basic eyetracker support for MNE (see https://github.com/mne-tools/mne-python/issues/10751 ).

therefor we should probably add a specific channel type to the FIF standard, as @larsoner mentioned..

are these the right changes, or did I miss a relevant location? also, what is the logic behind the numbering system? so far I inserted ????

[dominikwelke]

@larsoner @drammock

[drammock]

also, what is the logic behind the numbering system?

I'm not certain, but I think it's basically "increment by 1 for similar/related things (e.g., channel types related to HPI, sensor kinds from the same manufacturer) and increment by 10 or 100 to add new groups while leaving plenty of numerical room to expand old groups.

[larsoner]

MNE-Python gets constants from / stays in sync with https://github.com/mne-tools/fiff-constants/ . So we technically need things there before we add them here.

In practice, to make progress in MNE-Python, you can pick/guess some reasonable values based on where other stuff is in this PR. I would go based on the fnirs_ constants ended up since those were added most recently.

Concurrently, open a PR to https://github.com/mne-tools/fiff-constants/blob/master/DictionaryTypes.txt#L276 for example to actually add them. You could open an issue first instead, but it's easy enough to open a PR-as-proposal and have people comment directly.

[drammock]

MNE-Python gets constants from / stays in sync with https://github.com/mne-tools/fiff-constants/ . So we technically need things there before we add them here.

this PR is into the fiff-constants repo already... or am I misunderstanding?

[dominikwelke]

this PR is into the fiff-constants repo already... or am I misunderstanding?

yes, this is the fiff-constatns repo.

another question: how is alignment/indentation working here.. is it important at all? my editor shows me a wild mixture of tabs and spaces for the previous entries..

[larsoner]

I just completely missed that this was to fiff-constants :)

The numbers so far look reasonable, but let's iterate on a PR to MNE-Python to figure out the right scope for variables. This seems likely to be correct already with

1. ch_type being pupillometry
2. coil_type being the different measures (pupil diameter, position, etc.)
3. ch['loc'] storing additional information, such as "left eye" vs "right eye", "x direction" vs "y direction", etc. -- we wouldn't set this in this repo but we can do it in MNE-Python. Probably it makes sense to store left vs right eye in ch['loc'][3] (setting the first 3 as zeros as these are always a location in space for other channel types), then direction type (for the position) in ch['loc'][4] or so

[scott-huberty]

Hi @dominikwelke , we'll need to finish this PR before completing the read_raw_eyelink code, so that we don't have to change this line in our branches

https://github.com/mne-tools/mne-python/blob/6feb7091cf7ce7fb88325e5e9c1535370fa9c004/mne/io/tests/test_constants.py#L23-L24

(as a reminder, in our local branches, the repo is pointing to your commit):

REPO = 'dominikwelke'
COMMIT = '2019d4d564eac3313d906c4d8dc77c4c6a06a6df'

let me know if there is anything I can do to do help out on this PR

[dominikwelke]

hi @scott-huberty i dont think that there's anything to do on our side..

i guess the devs here will wait until the MNE PR is more or less ready before accepting changes to the constants. this avoids potential need for multiple patches if something changes during development..

having the dev branch point to a local copy of constants doesnt hurt, as long as we change it back in the end :)

[larsoner]

Agreed we can converge in the MNE PR first then once it's ready to go, merge this one, then fix the ref in the MNE PR as a final step before merging that one

[scott-huberty]

OK I am very confused :face_with_spiral_eyes: ...

@dominikwelke If i'm not mistaken, at some point recently you merged main into your branch. This meant a change from PR #40 is now in this branch with some additional fnirs channel types:

fnirs_td_gated_amplitude   306 "fNIRS time domain gated amplitude"
fnirs_td_moments_amplitude 307 "fNIRS time domain moments amplitude"

As far as I can tell, there are not constants for these in mne-python/main mne.io.constants: https://github.com/mne-tools/mne-python/blob/7262ad94b1c1e80d2cd63662a2f99aed49e74beb/mne/io/constants.py#L999-L1005

I can only see mention of these in an open mne-python PR

We are hitting an error in mne.io.tests.test_constants, I think because these new fnirs fif types exist in this repo but they don't exist in mne.io.constants. The error is at the assert line here:

missing_from_fif = sorted(set(this_fif.keys()) -
                          set(this_con.keys()))
assert missing_from_fif == [], key

missing_from_fif should be an empty list, but right now it is [306, 307]. Which correspond to the new entries for fnirs_td_gated_amplitude and fnirs_td_moments_amplitude

[scott-huberty]

I think it's because mne-python/main mne.io.tests.test_constants points to commit 6d9ca9ce7fb44c63d429c2986a953500743dfb22 , which occured earlier than the commit that added the fnirs channel types ( d477427def717f9d77e1bbdd68463be2dcab9eb7).

REPO = 'mne-tools'
COMMIT = '6d9ca9ce7fb44c63d429c2986a953500743dfb22'

So it seems that this latter commit ( d477427def717f9d77e1bbdd68463be2dcab9eb7 ) is still a work in progress and shouldn't be in our branch of fiff-constants.

[dominikwelke]

ok, well spotted @scott-huberty !

i think we have two options:

• i can remove them from my branch (avoiding the errors, but only for now - see below)
• or we just live with the test error for now - expecting the other PR to be merged soon (we would then merge these changes, or rebase our stuff on top)

as the changes are already in fiff_constants/main and would persist after merging my PR here, i'd opt for the latter.

should the other mne-python PR not be merged in time, we can handle the fiff thing then (maybe i can rebase my changes to BEFORE the fNIRS additions and link this commit. yet, i am not sure this would work, as there would probably be only one merge commit AFTER the last merge - @larsoner ?!)

technically, you could also add the new fnirs units to our io.constants file to avoid the errors, but this might also get messy if the other PR isnt merged by the time we want to merge.

[larsoner]

FWIW I think your PR will end up being merged before the TD NIRS one.

technically, you could also add the new fnirs units to our io.constants file to avoid the errors, but this might also get messy if the other PR isnt merged by the time we want to merge.

If this is easy enough to do (just a few lines hopefully), then let's go this route. The TD NRIS PR can just fix any errors in naming or whatever.

[scott-huberty]

technically, you could also add the new fnirs units to our io.constants file to avoid the errors, but this might also get messy if the other PR isnt merged by the time we want to merge.

If this is easy enough to do (just a few lines hopefully), then let's go this route. The TD NRIS PR can just fix any errors in naming or whatever.

Sounds good - I'll give adding the units a shot!

[scott-huberty]

Hey @larsoner how does mne/data/coil_def.dat get created? .. the two new fnirs channel types are missing from it and it's throwing an error....

mne\io\tests\test_constants.py:325: in test_constants
    assert len(bad_list) == 0, \
E   AssertionError:
E     In fiff-constants, missing from coil_def:
E         306,    # fNIRS time domain gated amplitude
E         307,    # fNIRS time domain moments amplitude
E   assert 2 == 0
E    +  where 2 = len(['    306,    # fNIRS time domain gated amplitude', '    307,    # fNIRS time domain moments amplitude'])

edit: are they supposed to be?.

[larsoner]

coil_def.dat only matters for MEG, can you see if that test has some way of skipping this check for non-MEG sensors like fNIRS?

[scott-huberty]

coil_def.dat only matters for MEG, can you see if that test has some way of skipping this check for non-MEG sensors like fNIRS?

I'm at a loss here. your branch feature/TD-nirs_snirf didnt touch mne.io.test_constants, yet that test passes on your branch but not mne-eyetrack_revisions, even after I merge in the majority of the changes made on that nirs PR.

Maybe my branch is further behind main than yours and someone else touched mne.io.test_constants in the mean time, but not from what I can tell.

Maybe I can return to this after all the other tests pass on my PR and i've merged main into it.

[larsoner]

Sure, feel free to temporarily mark the failures as @pytest.mark.xfail # TODO: Remove before merge in your PR for now. That way you know if it's a failure you care about when you look at CIs.

And maybe in the meantime I can finish https://github.com/mne-tools/mne-python/pull/11064 ...

[scott-huberty]

Sure, feel free to temporarily mark the failures as @pytest.mark.xfail # TODO: Remove before merge in your PR for now. That way you know if it's a failure you care about when you look at CIs.

And maybe in the meantime I can finish mne-tools/mne-python#11064 ...

Finally figured it out.. There were indeed additions to mne.io.tests.test_constants from featre/TD-nirs_snirf that I missed. The test passes now, and I checked some other tests locally that still all look okay. i'll see if the change introduces any new failures in the full run through..

[larsoner]

@mkajola @jnenonen do these changes look reasonable to you now? The short version is that we want to support eye-tracking data in FIF and think we can get away with just a few new constants to do it:

https://github.com/mne-tools/fiff-constants/pull/39/files

[larsoner]

@mkajola @jnenonen let me know if you'd like more time to look, otherwise I'll merge Tuesday