add ch_type and coils for eye-tracker data #39

dominikwelke · 2022-06-27T12:41:17Z

hi all,

I'm working on basic eyetracker support for MNE (see mne-tools/mne-python#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 · 2022-06-27T12:46:04Z

@larsoner @drammock

drammock · 2022-06-27T13:09:02Z

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 · 2022-06-27T13:09:08Z

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 · 2022-06-27T13:11:33Z

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 · 2022-06-27T13:15:53Z

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 · 2022-06-27T15:15:15Z

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

ch_type being pupillometry
coil_type being the different measures (pupil diameter, position, etc.)
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 · 2023-01-05T23:11:08Z

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 · 2023-01-06T11:18:26Z

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 · 2023-01-06T19:38:07Z

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 · 2023-01-30T21:40:56Z

DictionaryTypes.txt

@@ -274,6 +277,7 @@ enum(ch_type) "Type of the channel"
    dipole_wave    1000		     "Dipole time curve"
    goodness_fit   1001		     "Goodness of fit"
    fnirs	   1100		     "Functional near-infrared spectroscopy"
+    eyetrack   1200          "Eye-tracking"


Hey @dominikwelke can we update this file to be in line with the code in mne/io/constants.py on our PR?

Basically, PR's on adding skin temp and galvonic skin response channels got merged after you started the eyetracking work.

So Eyetrack channels got bumped from 1200 to 1400, because temperature channels are in the 1200's, gsr in the 1300's:

From mne/io/constants.py:

FIFF.FIFFV_FNIRS_CH = 1100 # Functional near-infrared spectroscopy FIFF.FIFFV_TEMPERATURE_CH = 1200 # Functional near-infrared spectroscopy FIFF.FIFFV_GALVANIC_CH = 1300 # Galvanic skin response FIFF.FIFFV_EYETRACK_CH = 1400 # Eye-tracking

Once we update this, mne/io/tests/test_constants.py should pass 🙂

done @scott-huberty

here's the commit if you want to update it in your pr branch (mne/io/tests/test_constants.py):

e300d7a

awesome thanks. And now there are no conflicts between this branch and the base branch. 2 birds 1 stone..

scott-huberty · 2023-01-31T13:39:18Z

OK I am very confused 😵‍💫 ...

@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 · 2023-01-31T14:06:19Z

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

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

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

dominikwelke · 2023-01-31T14:27:33Z

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 · 2023-01-31T14:40:42Z

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 · 2023-01-31T15:27:42Z

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 · 2023-02-01T16:53:12Z

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 · 2023-02-01T17:31:04Z

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 · 2023-02-01T19:39:48Z

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 · 2023-02-02T15:14:54Z

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 ...

scott-huberty · 2023-02-03T18:48:47Z

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..

DictionaryTypes.txt

larsoner · 2023-03-13T20:45:26Z

@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 · 2023-03-17T11:40:49Z

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

dominikwelke mentioned this pull request Jun 28, 2022

basic eyetracker functionality mne-tools/mne-python#10855

Closed

dominikwelke force-pushed the add-eyetracking branch from 0204967 to 3da188c Compare July 1, 2022 06:54

dominikwelke force-pushed the add-eyetracking branch 2 times, most recently from 833d487 to 9734da9 Compare January 6, 2023 11:42

scott-huberty reviewed Jan 30, 2023

View reviewed changes

dominikwelke added 3 commits January 30, 2023 23:15

add ch_type and coils for eyetracker data

7a49ac9

add mumbers

bc35f9d

add units for eyetracking data

e300d7a

dominikwelke force-pushed the add-eyetracking branch from 9734da9 to e300d7a Compare January 30, 2023 22:21

scott-huberty reviewed Mar 13, 2023

View reviewed changes

DictionaryTypes.txt Outdated Show resolved Hide resolved

dominikwelke force-pushed the add-eyetracking branch from 7cf3d9c to e300d7a Compare March 13, 2023 16:58

remove non-SI eyetracker units

cc6c5d5

larsoner approved these changes Mar 13, 2023

View reviewed changes

larsoner merged commit e27f68c into mne-tools:master Mar 25, 2023

larsoner mentioned this pull request Mar 25, 2023

ENH: Read eyetracking data (Eyelink) (Fork of #10855 ) mne-tools/mne-python#11152

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add ch_type and coils for eye-tracker data #39

add ch_type and coils for eye-tracker data #39

dominikwelke commented Jun 27, 2022

dominikwelke commented Jun 27, 2022

drammock commented Jun 27, 2022

larsoner commented Jun 27, 2022

drammock commented Jun 27, 2022

dominikwelke commented Jun 27, 2022

larsoner commented Jun 27, 2022

scott-huberty commented Jan 5, 2023 •

edited

Loading

dominikwelke commented Jan 6, 2023

larsoner commented Jan 6, 2023

scott-huberty Jan 30, 2023

dominikwelke Jan 30, 2023

scott-huberty Jan 30, 2023

scott-huberty commented Jan 31, 2023 •

edited

Loading

scott-huberty commented Jan 31, 2023 •

edited

Loading

dominikwelke commented Jan 31, 2023 •

edited

Loading

larsoner commented Jan 31, 2023

scott-huberty commented Jan 31, 2023

scott-huberty commented Feb 1, 2023 •

edited

Loading

larsoner commented Feb 1, 2023

scott-huberty commented Feb 1, 2023

larsoner commented Feb 2, 2023

scott-huberty commented Feb 3, 2023

larsoner commented Mar 13, 2023

larsoner commented Mar 17, 2023

add ch_type and coils for eye-tracker data #39

add ch_type and coils for eye-tracker data #39

Conversation

dominikwelke commented Jun 27, 2022

dominikwelke commented Jun 27, 2022

drammock commented Jun 27, 2022

larsoner commented Jun 27, 2022

drammock commented Jun 27, 2022

dominikwelke commented Jun 27, 2022

larsoner commented Jun 27, 2022

scott-huberty commented Jan 5, 2023 • edited Loading

dominikwelke commented Jan 6, 2023

larsoner commented Jan 6, 2023

scott-huberty Jan 30, 2023

Choose a reason for hiding this comment

dominikwelke Jan 30, 2023

Choose a reason for hiding this comment

scott-huberty Jan 30, 2023

Choose a reason for hiding this comment

scott-huberty commented Jan 31, 2023 • edited Loading

scott-huberty commented Jan 31, 2023 • edited Loading

dominikwelke commented Jan 31, 2023 • edited Loading

larsoner commented Jan 31, 2023

scott-huberty commented Jan 31, 2023

scott-huberty commented Feb 1, 2023 • edited Loading

larsoner commented Feb 1, 2023

scott-huberty commented Feb 1, 2023

larsoner commented Feb 2, 2023

scott-huberty commented Feb 3, 2023

larsoner commented Mar 13, 2023

larsoner commented Mar 17, 2023

scott-huberty commented Jan 5, 2023 •

edited

Loading

scott-huberty commented Jan 31, 2023 •

edited

Loading

scott-huberty commented Jan 31, 2023 •

edited

Loading

dominikwelke commented Jan 31, 2023 •

edited

Loading

scott-huberty commented Feb 1, 2023 •

edited

Loading