ENH: Read eyetracking data (Eyelink) (Fork of #10855 ) #11152

scott-huberty · 2022-09-09T21:33:50Z

refactored code initially pushed in PR #10855 by @dominikwelke and reviewed by @larsoner and @drammock. Starting a new PR as suggested by @larsoner

briefly: When plotting the test_eyelink.asc file, The eye channel traces now appear at the beginning of the time series as expected. I also added a dataframes attribute to the RawEyelink class, which returns the pandas dataframes that are created while loading the file.

@dominikwelke I'll leave some comments on the code in your PR to explain some of the changes I made in the code refactoring, and you can let me know if you agree, and we can discuss what needs to be worked on next!

@sportnoah14 also cc'ing you here so that you can stay in the loop.

drammock · 2022-09-09T21:51:02Z

@scott-huberty looks like this needs a rebase or a merge-main. If it were me, I'd probably

git rebase -i 4a390cc^

and then change pick to s (or squash) for all the commits @dominikwelke did. That way you're only applying 3 commits on top of current main instead of 27 so resolving the merge conflicts might go much more smoothly.

scott-huberty · 2022-09-09T22:20:02Z

git rebase -i 4a390cc^

Thanks @drammock - I think I did that correctly.. I had to force push because my local branch was behind remote after rebasing.

drammock · 2022-09-09T23:03:54Z

I think I did that correctly.. I had to force push because my local branch was behind remote after rebasing.

Force-push is expected here (you have now 2 commits where GitHub had 27), and the monster multiline commit message for 5efcf53 suggests that you did it correctly... except that this was only the first step. Now you have to fetch upstream main and then git rebase upstream/main, to then actually fix the merge conflicts.

scott-huberty · 2022-09-16T15:59:25Z

I think I did that correctly.. I had to force push because my local branch was behind remote after rebasing.

Force-push is expected here (you have now 2 commits where GitHub had 27), and the monster multiline commit message for 5efcf53 suggests that you did it correctly... except that this was only the first step. Now you have to fetch upstream main and then git rebase upstream/main, to then actually fix the merge conflicts.

Sorry for the delay - While doing git rebase upstream/main I needed to manually resolve conflicts that couldn't be merged, it was the first time I've done this but I hopefully did it correctly.

…om jun-28-2022 (4a390cc) - jul-01-2022 (91516c) [ci skip][skip azp][skip actions]

dominikwelke · 2023-01-06T13:15:48Z

hi @scott-huberty
i think ill finally find some time to work on this again, while working on pupil-size data..
what is the status on your branch?

i saw you did quite some reorganisation of the eyelink parser code.. generally ok with me, i am not bound to my initial layout
yet, maybe worksplitting makes sense to avoid double structures?
i am currently working with data that has already been synchronized with EEG, so i can focus on basic integration (e.g. the ch_types FIFF stuff) and (pre)processing after having it loaded, while you further work on the parser?

what do you think?

scott-huberty · 2023-01-06T14:34:26Z

@dominikwelke I think this is a great idea!

I should wrap up the code for the parser next week. Then I can start advancing the test-suite that you started.

I can also create a new local branch and cherry-pick your recent commits to make sure the parser code remains compatible with the development on channel types etc you are doing right now.

I'll follow up with you next week when the parser code is finalized, and maybe we can start to merge our work then.

scott-huberty · 2023-01-16T22:47:31Z

Hey @dominikwelke , I think the parser is looking good!

I haven't really touched your code on setting channel and FIFF types, but I see you did some refactoring of that last week. So I will try to pull your recent commits to a copy of this branch.

On that note, this Parser should be robust to reading all the various eyelink channel types that can be present in an eyelink file (Gaze, Pupil, Resolution, Velocity, Head target (remote mode), and DINS). But, I'm not sure what coils/units etc we should give these channels, what do you think?

I've started a github repo that contains various eyelink files that I've been using to test code during development, including monocular files and files with resolution/head target/DIN channels. Feel free to check it out.

scott-huberty · 2023-01-16T23:07:22Z

One comment on the DIN (digital input port) data in Eyelink files.. Eyelink tracks the pin downs/ups of a DIN port for every sample of the file, so if I'm not mistaken, I think that we could load this data in as a stim channel. In my lab, we are sending info to the eyelink host PC's DIN port, and get integer values between 0-3 in our files (0 being pin down, and 1,2, or 3 depending on what pin is up).

…k Welke (4a390cc) . co-authored by Dominik Welke <[email protected]> [ci skip][skip azp][skip actions] refactored code for reading eyelink eyetracker data, initially by Dominik Welke (4a390cc) . co-authored by Dominik Welke <[email protected]> [ci skip][skip azp][skip actions] account for missing timestamps between recording blocks [ci skip][skip azp][skip actions] refactored code and removed get_data_spec function (To be done on another branch) [ci skip][skip azp][skip actions]

scott-huberty · 2023-01-18T13:58:12Z

One comment on the DIN (digital input port) data in Eyelink files.. Eyelink tracks the pin downs/ups of a DIN port for every sample of the file, so if I'm not mistaken, I think that we could load this data in as a stim channel.

Following up on that, if we read in the DIN data as a stim channel, this is an example of what it would look like.

for this example file, a photodiode was connected to Eyelinks DIN port. This is a pupillary light reflex task, so a white flash on the screen triggers the photodiode (and thus a change in the pins of the DIN port). This is why, in this file, just after the DIN value changes at around 2-seconds, you see the pupil size constrict (in the pupil channel).

dominikwelke · 2023-01-18T16:01:59Z

nice, great progress @scott-huberty !
i'll test it with some of my files soon and give feedback.

immediate comments:

my initial parser function was based on an already existing project.
if you kept any of it, we should acknowledge the author (and the copyright note, as it's MIT licensed)

this Parser should be robust to reading all the various eyelink channel types that can be present in an eyelink file (Gaze, Pupil, Resolution, Velocity, Head target (remote mode), and DINS). But, I'm not sure what coils/units etc we should give these channels, what do you think?

treating DINS as stim channel makes sense, imo.
for the other channel types:
we can define new coil types, if needed.
but what do you think is the priority of it? do ppl actually use these data?
i think setting specific channel info makes sense IF there will be methods for these data types. otherwise we can simply give them the misc channel type and leave it to the users to treat the data as wanted.
an alternative/addition could be a not implemented warning

how should we proceed? (also @drammock @larsoner )
if you want @scott-huberty - and your work on the parser is more or less done - i can deal with the integration of our branches.
i would do so by merging your branch into mine, as the associated PR has had a bit more public discussion than yours here.
of course your branch can stay active and news can be merged again down the line

drammock · 2023-01-18T18:52:07Z

treating DINS as stim channel makes sense, imo.

Agreed

for the other channel types: we can define new coil types, if needed. [...] otherwise we can simply give them the misc channel type

misc makes sense to me unless there is a compelling reason to add a more specific channel type

scott-huberty · 2023-01-19T00:15:48Z

treating DINS as stim channel makes sense, imo.

Agreed

for the other channel types: we can define new coil types, if needed. [...] otherwise we can simply give them the misc channel type

misc makes sense to me unless there is a compelling reason to add a more specific channel type

Ok, let's start with misc, I can look at some test files after to make sure everything looks okay.

my initial parser function was based on an already existing project.
if you kept any of it, we should acknowledge the author (and the copyright note, as it's MIT licensed)

The code from that project isn't in this branch anymore. The code in this branch I developed along with @christian-oreilly , (who I've listed as a 3rd contributor in the eyelink.py file). The code you wrote for initializing the RawEyelink class, creating the channel/coil types, starting the test-suite etc. is in this branch too.

how should we proceed? (also @drammock @larsoner )
if you want @scott-huberty - and your work on the parser is more or less done - i can deal with the integration of our branches.
i would do so by merging your branch into mine, as the associated PR has had a bit more public discussion than yours here.
of course your branch can stay active and news can be merged again down the line

What do you think about the idea of finishing up this PR (basically wrapping up setting the channel types as discussed, and completing the test-suite and docs), which won't include the preprocessing functions that you've been working on in your branch. Then, assuming the PR is approved, you could merge main from your branch and carry forward the preprocessing functions in a second PR (the one currently open). IMO, this might create a nice "separation of concerns" as far as testing, documenting, and reviewing read_raw_eyelink and the additional preprocessing functions. If so, you could pull this branch now and make your own commits to it too, which we could merge into the PR.

Alternatively, yes we could merge this into your branch, and continue working on it from there. In that case, there's a small amount of refactoring I wanted to do, but I can do it before or after the merge. just let me know what you're thoughts are! (Others please feel free to weigh in on this too if you don't like my proposed idea).

larsoner · 2023-01-19T00:18:40Z

What do you think about the idea of finishing up this PR (basically wrapping up setting the channel types as discussed, and completing the test-suite and docs) [... and then] carry forward the preprocessing functions in a second PR

I have not read all previous responses, but I will say from a reviewer perspective ideally the I/O and preprocessing could go in separate, sequential PRs. It will make reviewing much easier for us!

dominikwelke · 2023-01-20T15:58:19Z

i tested with some of my files and everything seems to work swiftly! thanks for moving this forward! :)

also thanks for updating me on the contributions.
even better this way!

concerning the choice of branch/PR:
moving the preprocessing code from my current branch into a separate branch+pr would be a matter of seconds, as it happens in entriely different files. also no real discussion of this code has happened in the PR so far.

my further arguments for merging into my branch were:

it would probably generate the cleaner git history (your commits stay your's of course)
related: tbh your history of force-pushing altered commits made me a bit wary of having your branch as the "main" because this can lead to nasty git pains (of course it's necessary some times, no offense!)
and as mentioned: the next integration steps are largely mine (fiddling with the FIFF types) and it seemed very convoluted to me to generate a copy of your branch to merge parts of my branch into, to then merge into yours again :)

but we've discussed this long enough - you have a preference yor your branch and the reviewers dont seem to care so lets take your branch :)

i'll integrate the latest status of my FIFF and channel type settings into a copy of it, and then we can merge these branches.
so please, no changing of the git history in your branch from now on. just have a new commit for it if you change smth - this also makes changes more transparent and enables review :)
(plus the final merge into MNE main will collapse everything to a single commit anyway.. )

scott-huberty · 2023-01-20T18:51:45Z

Concerning the choice of branch/PR: moving the preprocessing code from my current branch into a separate branch+pr would be a matter of seconds, as it happens in entriely different files. also no real discussion of this code has happened in the PR so far.

my further arguments for merging into my branch were: [...but ] we've discussed this long enough - you have a preference yor your branch and the reviewers dont seem to care so lets take your branch :)

It's up to you - Either way, I'm just glad we've progressed to the stage that we can start working on a common branch!

so please, no changing of the git history in your branch from now on.

Perhaps I made a faulty assumption that no one had fetched this remote branch - of course, we'll keep the history linear from now on!

i tested with some of my files and everything seems to work swiftly!

Awesome!

scott-huberty · 2023-03-16T12:44:42Z

I'm not sure why - the minimal 3.8 CI is returning a module import error for: nibabel on many of the beamformer tests.

dominikwelke · 2023-03-16T14:09:19Z

this doesnt seem like our fault; all the other CIs are fine

but test_radianseems to have failed too: no pandas?!
seems more like the testing environment is outdated or broken

larsoner · 2023-03-16T14:22:25Z

Argh I introduced those regressions and will fix them in a separate PR. Don't know why they're only being caught now, though!

scott-huberty · 2023-03-16T14:22:36Z

this doesnt seem like our fault; all the other CIs are fine

but test_radianseems to have failed too: no pandas?! seems more like the testing environment is outdated or broken

ahh ya thanks for pointing that out -- I'll add the requires_pandas decorator and push.

dominikwelke · 2023-03-16T16:00:16Z

🎉🎉🎉 100 commits 🎉🎉🎉

larsoner · 2023-03-25T20:34:47Z

mne-tools/fiff-constants#39 is in, feel free to update this PR and ping me when it's green -- I think we can merge it then!

dominikwelke · 2023-03-27T14:31:16Z

it's green and LGTM

dominikwelke · 2023-03-27T14:34:18Z

do we need to update any whatsnew or contributors log @larsoner

larsoner · 2023-03-27T15:07:06Z

Yes please add an entry @dominikwelke then I think we can merge!

scott-huberty · 2023-03-27T15:10:53Z

@dominikwelke unless you're already on top of this I'll update the changelog now

scott-huberty · 2023-03-27T16:18:40Z

argh.. getting another nibabel import error in mne/simulation/tests/test_raw.py::test_simulate_raw_bem[testing_data]

larsoner · 2023-03-27T16:22:00Z

Weird that it happens here but not main... but just add a pytest.importorskip('nibabel') at the start of that test

larsoner · 2023-03-27T16:27:05Z

Pushed a commit for the importorskip and marked for merge when green. Thanks in advance @dominikwelke @scott-huberty , this will be very useful!

scott-huberty · 2023-03-27T16:28:19Z

Pushed a commit for the importorskip and marked for merge when green. Thanks in advance @dominikwelke @scott-huberty , this will be very useful!

Thank you!!!

Big thanks to everyone for all the work on this!!!

* upstream/main: (50 commits) BUG: Fix bug with paths (mne-tools#11639) MAINT: Report download time and size (mne-tools#11635) MRG: Allow retrieval of channel names via make_1020_channel_selections() (mne-tools#11632) Fix index name in to_data_frame()'s docstring (mne-tools#11457) MAINT: Use VTK prerelease wheels in pre jobs (mne-tools#11629) ENH: Allow gradient compensated data in maxwell_filter (mne-tools#10554) make test compatible with future pandas (mne-tools#11625) Display SVG figures correctly in Report (mne-tools#11623) API: Port ieeg gui over to mne-gui-addons and add tfr gui example (mne-tools#11616) MAINT: Add token [ci skip] (mne-tools#11622) API: One cycle of backward compat (mne-tools#11621) MAINT: Use git rather than zipball (mne-tools#11620) ENH: Speed up code a bit (mne-tools#11614) [BUG, MRG] Don't modify info in place for transform points (mne-tools#11612) [BUG, MRG] Fix topomap extra plot generated, add util to check a range (mne-tools#11607) ENH: Add mne-bids-pipeline to mne sys_info (mne-tools#11606) MAINT: `coding: utf-8` is implicit in Python 3 (mne-tools#11599) ENH: Read eyetracking data (Eyelink) (Fork of mne-tools#10855 ) (mne-tools#11152) MAINT: In Python 3, do not prefix literals with `u` (mne-tools#11604) MAINT: object is an implicit base for all classes (mne-tools#11601) ...

scott-huberty marked this pull request as draft September 9, 2022 21:35

scott-huberty force-pushed the mne-eyetrack_revisions branch from a0b1090 to 8a3d4bc Compare September 9, 2022 22:19

scott-huberty force-pushed the mne-eyetrack_revisions branch from 8a3d4bc to b654cef Compare September 16, 2022 15:56

scott-huberty force-pushed the mne-eyetrack_revisions branch from b654cef to c4fdf49 Compare September 20, 2022 15:22

rebased upstream/main on top of Dominik Welkes eyetracking commits fr…

a79807a

…om jun-28-2022 (4a390cc) - jul-01-2022 (91516c) [ci skip][skip azp][skip actions]

scott-huberty force-pushed the mne-eyetrack_revisions branch from 78a50af to a865640 Compare December 1, 2022 22:06

scott-huberty force-pushed the mne-eyetrack_revisions branch 5 times, most recently from c8bb4e8 to 32bffe5 Compare January 15, 2023 13:57

scott-huberty force-pushed the mne-eyetrack_revisions branch from 32bffe5 to 0c28fa4 Compare January 17, 2023 22:34

scott-huberty added 2 commits January 19, 2023 16:19

minor refactor

1f239fd

removed preprocessing funcs, to be moved to separate PR [ci skip]

3fe784d

FIX: test_radian requires pandas

286b95b

Merge branch 'main' into mne-eyetrack_revisions

4ae1d80

larsoner mentioned this pull request Mar 16, 2023

basic eyetracker functionality #10855

Closed

larsoner added this to the 1.4 milestone Mar 16, 2023

larsoner mentioned this pull request Mar 17, 2023

ENH: Support TD data #11064

Draft

11 tasks

Update test_constants.py

c709c36

scott-huberty added 2 commits March 27, 2023 11:30

DOC: Update changelog

70ccc1c

Merge branch 'main' into mne-eyetrack_revisions

c13db88

larsoner approved these changes Mar 27, 2023

View reviewed changes

FIX: Skip

f011d7a

larsoner enabled auto-merge (squash) March 27, 2023 16:26

larsoner merged commit 62af4ac into mne-tools:main Mar 27, 2023

scott-huberty mentioned this pull request Sep 27, 2023

BUG: triage_eyetrack_pick does not work #12018

Closed

scott-huberty mentioned this pull request Jan 22, 2024

Function to convert eyegaze units to radians #12237

Merged

scott-huberty mentioned this pull request Jul 31, 2024

Inconsistent handling and documentation of pupil size units #12756

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Read eyetracking data (Eyelink) (Fork of #10855 ) #11152

ENH: Read eyetracking data (Eyelink) (Fork of #10855 ) #11152

scott-huberty commented Sep 9, 2022

drammock commented Sep 9, 2022

scott-huberty commented Sep 9, 2022 •

edited

Loading

drammock commented Sep 9, 2022

scott-huberty commented Sep 16, 2022 •

edited

Loading

dominikwelke commented Jan 6, 2023

scott-huberty commented Jan 6, 2023 •

edited

Loading

scott-huberty commented Jan 16, 2023

scott-huberty commented Jan 16, 2023 •

edited

Loading

scott-huberty commented Jan 18, 2023

dominikwelke commented Jan 18, 2023

drammock commented Jan 18, 2023

scott-huberty commented Jan 19, 2023

larsoner commented Jan 19, 2023

dominikwelke commented Jan 20, 2023 •

edited

Loading

scott-huberty commented Jan 20, 2023

scott-huberty commented Mar 16, 2023

dominikwelke commented Mar 16, 2023 •

edited

Loading

larsoner commented Mar 16, 2023

scott-huberty commented Mar 16, 2023

dominikwelke commented Mar 16, 2023

larsoner commented Mar 25, 2023

dominikwelke commented Mar 27, 2023 •

edited

Loading

dominikwelke commented Mar 27, 2023

larsoner commented Mar 27, 2023

scott-huberty commented Mar 27, 2023

scott-huberty commented Mar 27, 2023

larsoner commented Mar 27, 2023

larsoner commented Mar 27, 2023

scott-huberty commented Mar 27, 2023

ENH: Read eyetracking data (Eyelink) (Fork of #10855 ) #11152

ENH: Read eyetracking data (Eyelink) (Fork of #10855 ) #11152

Conversation

scott-huberty commented Sep 9, 2022

drammock commented Sep 9, 2022

scott-huberty commented Sep 9, 2022 • edited Loading

drammock commented Sep 9, 2022

scott-huberty commented Sep 16, 2022 • edited Loading

dominikwelke commented Jan 6, 2023

scott-huberty commented Jan 6, 2023 • edited Loading

scott-huberty commented Jan 16, 2023

scott-huberty commented Jan 16, 2023 • edited Loading

scott-huberty commented Jan 18, 2023

dominikwelke commented Jan 18, 2023

drammock commented Jan 18, 2023

scott-huberty commented Jan 19, 2023

larsoner commented Jan 19, 2023

dominikwelke commented Jan 20, 2023 • edited Loading

scott-huberty commented Jan 20, 2023

scott-huberty commented Mar 16, 2023

dominikwelke commented Mar 16, 2023 • edited Loading

larsoner commented Mar 16, 2023

scott-huberty commented Mar 16, 2023

dominikwelke commented Mar 16, 2023

larsoner commented Mar 25, 2023

dominikwelke commented Mar 27, 2023 • edited Loading

dominikwelke commented Mar 27, 2023

larsoner commented Mar 27, 2023

scott-huberty commented Mar 27, 2023

scott-huberty commented Mar 27, 2023

larsoner commented Mar 27, 2023

larsoner commented Mar 27, 2023

scott-huberty commented Mar 27, 2023

scott-huberty commented Sep 9, 2022 •

edited

Loading

scott-huberty commented Sep 16, 2022 •

edited

Loading

scott-huberty commented Jan 6, 2023 •

edited

Loading

scott-huberty commented Jan 16, 2023 •

edited

Loading

dominikwelke commented Jan 20, 2023 •

edited

Loading

dominikwelke commented Mar 16, 2023 •

edited

Loading

dominikwelke commented Mar 27, 2023 •

edited

Loading