ENH: Create a Calibrations class for eyetracking data #11719

scott-huberty · 2023-06-03T20:34:30Z

Reference issue

I forgot to open a ticket before creating a PR (apologies!):

This PR is based on my discussions with @larsoner and @britta-wstnr regarding my current GSoC project. For the first week, the goal was to extend functionality to reading and storing some meta-information from eye-tracking recordings, that will be needed for work down the line in this GSoC project.

What does this implement/fix?

This PR creates a Calibration class.

Here is the updated eye-tracking tutorial.
Here is the documentation for the Calibration class.
Here is the documentation for read_eyelink_calibration
Here is the codecov report.

Minimally Working Example:

fname = mne.datasets.testing.datapath() / "eyetrack" / "test_eyelink.asc"
calibrations = mne.preprocessing.eyetracking..read_eyelink_calibration(fname, 
                                               screen_size=(.531, .298),
                                               screen_distance=.065,
                                               screen_resolution=(1920, 1080))

print(calibrations[0]) # show info for the left eye Calibration

Calibration |
  onset: 0 seconds
  model: HV13
  eye: left
  average error: 0.3 degrees
  max error: 0.9 degrees
  screen size: (0.531, 0.298) meters
  screen distance: 0.065 meters
  screen resolution: (1920, 1080) pixels

Additional information

A couple of notes:

screen_size, screen_distance, and screen_resolution are not available in the ASCII files but can be optionally provided by the user.
This draft from the WIP eye-tracking BIDS specificaton was used to guide some of the decisions regarding the fields to include by default in the Calibration class.

TODOS:

Fix failing tests
increase codecov of the changes introduce in this PR
move mne.io.eyelink.calibration to mne.preprocessing.eyetrack.calibration, since the Calibrations class is not specific to eyelink.
create a plot method for the Calibration class.
Add the a demonstration of reading and plotting the calibration to the current eye-tracking tutorial
Take a look at Calibration['points'] to see if it can be split into separate keys such that it is easier to work with / extend to future I/O functions for other manufacturers.

…for eyelink calibration This commit creates a Calibrations class to store eyetracking calibration info. Calibrations subclasses list and contains a list of Calibration instances. Calibration instances sublcass OrderedDict.

Added read_eyelink_calibration function to mne namespace, because IMO thematically it fits in with other file I/O funcs like mne.read_epochs_eeglab, mne.read_evoked_besa, that are not mne.io.read_raw_[] funcs. Added read_eyelink_calibration to mne.docs.file_io.rst

mne/io/eyelink/calibration.py

After some deliberation, I believe that it is better to always return a Calibration instance for a single eye. This means that, for a recording in binocular mode, 2 Calibration instances will be returned for each "calibration" run during the session. For example if a single calibration was collected in binocular mode, the new API is just: Calibrations[0] for the left eye calibration, and Calibrations[1] for the right eye calibration. The two Calibration instances can be linked because they have the same exact onset. my reasoning is: Conceptual: even in binocular mode, a SINGLE calibration is done for each eye (though simultaneously). The calibration info for each eye is returned. API: Users can expect a consistent structure in the Calibration class. For example, calibrations[0]["avg_error"] will alwasy be a single float (where before, if binocular, it was a dict). Code Design: This makes the routines of read_eyelink_calibration much simpler, as we don't need a lot of conditional logic that depends on the recording mode being binocular or monocular.

britta-wstnr · 2023-06-05T15:14:56Z

Great, @scott-huberty ! Let me know once this is ready to be tested on additional data.

britta-wstnr

Some small stuff my gaze tripped upon when looking at the PR - will do a more thorough review after tests pass 🙂

mne/io/eyelink/_utils.py

mne/io/eyelink/calibration.py

…eview. Instead of appending Calibration instances to a special Calibrations class, just append them to a list object. added Calibration class to mne namespace added Calibration class to doc/fileio.rst

Added a plot method to the Calibration class, and added a test for this method to mne.io.eyelink.tests.test_eyelink

That were suggested via the github browser.

This code simplifies the routine a bit, and makes it easier for a user to create a Calibration object manually (Now they would just pass in a list of tuples, instead of having to create a structured array).. Even though I think it is unlikely that someone will create one of these Calibration instances manually, it's good to make it easy if they do..

scott-huberty · 2023-06-05T19:16:10Z

Some small stuff my gaze tripped upon when looking at the PR - will do a more thorough review after tests pass 🙂

Thanks! since I made quite a bit of changes locally, I just made your suggested changes locally on top of my changes, (instead of comitting them here and rebasing). See the commit with title FIX: Incorporated Britta's suggested changes

scott-huberty · 2023-06-05T19:29:59Z

Maintenance note: After some refactoring and adding a plot method, I edited my initial message at the top to reflect this. I've included an example of the calibration plot, there.

…eters

mscheltienne · 2023-06-05T20:29:52Z

The plot looks great, it could be nice to have the average and max error written in a corner.

scott-huberty · 2023-06-05T20:40:45Z

The plot looks great, it could be nice to have the average and max error written in a corner.

Good idea. I'll give it a shot!

Removed read_eyelink_calibration, Calibration, from mne namespace moved Calibration.py to mne.preprocessing.eyetracking read_eyelink_calibration is accessed via mne.io namespace DOC: added Calibration to doc.preprocessing.rst DOC: added read_eyelink_calibration to reading_raw_data.rst

scott-huberty · 2023-06-06T13:48:38Z

Hey @britta-wstnr and @larsoner :

In my latest commit ec13a2b, I moved mne.io.eyelink.calibration.py to mne.preprocessing.eyetrack (Since the Calibration class will not be specific to EyeLink). I updated the __init__ files accordingly (I think).

However, now when I try to import the Calibration class in mne.io.eyelink._utils with:

from ...preprocessing.eyetracking.calibration import Calibration (previously it was from .calibration import Calibration):

I trigger a circular import error, and I cannot figure out how to fix it!

I pushed that commit to this PR knowing the tests will fail, so that I can share the circular import error: For example see this test failure

larsoner · 2023-06-06T13:52:56Z

I would nest the from ...preprocessing.eyetracking.calibration import Calibration in whatever function actually needs the Calibration class

mne/io/eyelink/_utils.py

Co-authored-by: Eric Larson <[email protected]>

scott-huberty · 2023-06-06T13:57:51Z

I would nest the from ...preprocessing.eyetracking.calibration import Calibration in whatever function actually needs the Calibration class

That did it, thanks!

scott-huberty · 2023-06-06T15:26:34Z

It seems like inherited methods of Calibration are being built by sphinx autosummary: here is the Calibration doc page

And this is triggering some sphinx warnings during the build.

In mne/doc/preprocessing.rst we specify :no-inherited-members:, but this doesn't seem to solve the problem:

:py:mod:`mne.preprocessing.eyetracking`:

.. currentmodule:: mne.preprocessing.eyetracking

.. automodule:: mne.preprocessing.eyetracking
   :no-members:
   :no-inherited-members:

.. autosummary::
   :toctree: generated/

   Calibration
   set_channel_types_eyetrack

@larsoner / @britta-wstnr do you have any idea of what I'm doing wrong here?

larsoner · 2023-06-06T17:08:52Z

Add "None. Remove all items from od." to nitpick ignore in doc/conf.py. You can see some other examples from dict subclassing in there

larsoner · 2023-06-06T17:12:57Z

... and also "a shallow copy of od"

The short explanation is that warnings in doc build are treated as errors. So if you search "warning" in the circle log you'll see these. And we can ignore them.

For the copy one it would actually be better to override the method using a deep copy like we do for Info and other objects that subclass __dict__. Basically in MNE there is an expectation that .copy() operates like a deep copy so we should probably continue that behavior here

scott-huberty · 2023-06-06T17:57:42Z

... and also "a shallow copy of od"

The short explanation is that warnings in doc build are treated as errors. So if you search "warning" in the circle log you'll see these. And we can ignore them.

For the copy one it would actually be better to override the method using a deep copy like we do for Info and other objects that subclass __dict__. Basically in MNE there is an expectation that .copy() operates like a deep copy so we should probably continue that behavior here

Ok! Adding those phrases to conf.py and I will over ride the copy method.

What do you think we should do about the warning triggered by the move_to_end inherited method?:

WARNING: [numpydoc] Validation warnings while processing docstring for 'mne.preprocessing.eyetracking.Calibration.move_to_end':
  GL02: Closing quotes should be placed in the line after the last text in the docstring (do not close the quotes in the same line as the text, or leave a blank line between the last text and the quotes)
  PR01: Parameters {'key', 'last'} not documented

scott-huberty · 2023-06-15T19:10:32Z

Looks like a great start! Got partway through and think I'm at a good stopping spot -- let me know what you think of my ideas and I'll keep looking after any additional changes (tests and tutorial might need to be updated)

Thanks @larsoner! I integrated your suggestions in 6ede0af and updated the docs / tests to reflect the changes.

It looks like the CI's are good. Let me know when you've had a chance to continue your review

doc/changes/latest.inc

Co-authored-by: Richard Höchenberger <[email protected]>

- described that we are deprecating the gap_description kwarg of read_raw_eyelink

scott-huberty · 2023-06-16T12:53:11Z

Just FYI The Test windows 3.10 CI is returning an error "no matching distribution found for pandas" during the pip install action.

Not sure if that's a fluke, Let me know if you want me to re-trigger the CI's.

larsoner

Almost there! Just some minor cleanups here and there then we should be good

doc/changes/latest.inc

mne/io/eyelink/eyelink.py

larsoner · 2023-06-16T17:51:44Z

mne/io/eyelink/eyelink.py

@@ -338,16 +344,17 @@ def read_raw_eyelink(
    --------
    mne.io.Raw : Documentation of attribute and methods.


Add Notes section mentioning that gaps between recording blocks will be annotated with BAD_ACQ_SKIP

larsoner · 2023-06-16T17:52:25Z

mne/io/eyelink/eyelink.py

@@ -418,7 +430,8 @@ class RawEyelink(BaseRaw):
        Whether whether a single eye was tracked ('monocular'), or both
        ('binocular').
    _gap_desc : str


No _-prefixed attributes should be documented in the class doc. Feel free to move these to code comments if you want, though.

Okay, I'll go through and remove these from the read_raw_eyelink docstring.

mne/preprocessing/eyetracking/calibration.py

larsoner · 2023-06-16T17:56:32Z

mne/preprocessing/eyetracking/calibration.py

+
+    def __repr__(self):
+        """Return a summary of the Calibration object."""
+        onset = self.get("onset", "N/A")


... then by making some (or all?) obligatory hopefully some of these can just be direct dict access within the f-str rather than this self.get business

Hmm yeah I see. Yeah, it's easy enough to make the values for most of those keys None or np.nan, and most of them are just informational and so having values of np.nan or None is not going to break anything.

Maybe making the 'positions', 'gaze', and 'offsets' keys obligatory could be a little annoying for users who do have the 'model' 'avg_error' and 'max_error' information but don't have the data for individual points, in which case they would have to do something like np.empty(n_calibration_points, 2).

But I'm not sure yet how often someone will even create one of these instances from scratch instead of using an available helper function like read_eyelink_calibration, so it's hard to say if it will be an issue for people?

Let's tell them not to create it from scratch and just make sure we do the right thing internally. This is how Forward, SourceSpaces, etc. have worked for the most part for years and it has been okay. So +1 for keeping as simple as possible and adding more checks later if needed

mne/preprocessing/eyetracking/calibration.py

Co-authored-by: Eric Larson <[email protected]>

2 more of Eric's suggestions. 1 remaining that has not been added just yet. Co-authored-by: Eric Larson <[email protected]>

more suggestions from Eric Co-authored-by: Eric Larson <[email protected]>

- don't use custom __set_item__ dunder method - make most Calibration params positional - and now just use f string formatting in __repr__ - add notes section to read_raw_eyelink about bad_acq_skip - remove private attribute docstring from read_raw_eyelink - updated tests - updated doc because show_offsets defaults to true

larsoner

Just one last idea then we can merge @scott-huberty !

mne/io/eyelink/_utils.py

Don't force onset to be 0, allow negative time if before recording start. Co-authored-by: Eric Larson <[email protected]>

scott-huberty · 2023-06-20T17:09:48Z

Just one last idea then we can merge @scott-huberty !

I think I need to update the docstring to scrub any mention of "will be zero if calibration was before recording start". Plus looks like there is some style complaint. push a commit in a moment.

mne/preprocessing/eyetracking/calibration.py

…and flake fixes

scott-huberty · 2023-06-20T20:33:39Z

@larsoner assuming the tests will run as smoothly here as they did locally, this is ready for your review again. Note that as discussed I added an axes kwarg to the plot method and removed the origin kwarg.

larsoner

Awesome, marking for merge-when-green once I eyeball the example (it looked good last I checked)! Thanks in advance @scott-huberty

EDIT: All good https://output.circle-artifacts.com/output/job/a37bd176-ceeb-4441-aeb6-44bf33b2d3b1/artifacts/0/html/auto_tutorials/preprocessing/90_eyetracking_data.html

* upstream/main: (24 commits) Allow int-like as ID of make_fixed_length_events (mne-tools#11748) Easycap-M43 montage (mne-tools#11744) ENH: Create a Calibrations class for eyetracking data (mne-tools#11719) Fix alphabetical order in overview/people.rst, fix sphinx formatting in docstrings and set verbose to keyword-only (mne-tools#11745) Add Mathieu Scheltienne to MNE-Python Steering Council (mne-tools#11741) removed requirement for curv.*h files to create Brain object (mne-tools#11704) [BUG] Fix mne.viz.Brain.add_volume_labels matrix ordering bug (mne-tools#11730) Fix installer links (mne-tools#11729) MAINT: Update for PyVista deprecation (mne-tools#11727) MAINT: Update roadmap (mne-tools#11724) MAINT: Update download link [skip azp] [skip cirrus] [skip actions] fix case for chpi_info[1] == None (mne-tools#11714) Add cmap argument for mne.viz.utils.plot_sensors (mne-tools#11720) BUG: Fix one more PySide6 bug (mne-tools#11723) MAINT: Fix PySide6 and PyVista compat (mne-tools#11721) MRG: If _check_fname() cannot find a file, display the path in quotation marks to help spot accidental trailing spaces (mne-tools#11718) Add "array-like" to `_validate_type()` (mne-tools#11713) MAINT: Avoid problematic PySide6 (mne-tools#11715) Fix installer links (mne-tools#11709) Updating change log after PR mne-tools#11575 (mne-tools#11707) ...

britta-wstnr · 2023-06-27T11:55:58Z

Yay for merging @scott-huberty ! 🎉

scott-huberty added 2 commits June 3, 2023 15:47

larsoner reviewed Jun 5, 2023

View reviewed changes

mne/io/eyelink/calibration.py Outdated Show resolved Hide resolved

britta-wstnr reviewed Jun 5, 2023

View reviewed changes

mne/io/eyelink/_utils.py Outdated Show resolved Hide resolved

mne/io/eyelink/calibration.py Outdated Show resolved Hide resolved

mne/io/eyelink/calibration.py Outdated Show resolved Hide resolved

scott-huberty added 4 commits June 5, 2023 11:29

ENH, STY: Remove Calibrations class in favor of simple list. Please R…

000bc5c

…eview. Instead of appending Calibration instances to a special Calibrations class, just append them to a list object. added Calibration class to mne namespace added Calibration class to doc/fileio.rst

ENH: Add plot method and a test

1863b24

Added a plot method to the Calibration class, and added a test for this method to mne.io.eyelink.tests.test_eyelink

FIX: Incorporate Britta's suggested revisions.

e1115c1

That were suggested via the github browser.

scott-huberty added 2 commits June 5, 2023 15:37

FIX: nest matplotlib import under plot method

7fac97e

FIX, DOC: Fixes to docstring that were raised by test_docstring_param…

71ccbac

…eters

FIX, DOC: More docstring error fixes

2bfff19

scott-huberty marked this pull request as draft June 6, 2023 13:29

larsoner reviewed Jun 6, 2023

View reviewed changes

mne/io/eyelink/_utils.py Show resolved Hide resolved

mne/io/eyelink/_utils.py Outdated Show resolved Hide resolved

scott-huberty and others added 2 commits June 6, 2023 09:56

Update mne/io/eyelink/_utils.py

1d09b00

Co-authored-by: Eric Larson <[email protected]>

Update mne/io/eyelink/_utils.py

dd25b35

Co-authored-by: Eric Larson <[email protected]>

FIX: black error

ca01ed8

scott-huberty and others added 2 commits June 15, 2023 14:03

DOC: add bug fix to change log

b256454

Merge branch 'main' into et_calibration

cd3b34a

hoechenberger reviewed Jun 15, 2023

View reviewed changes

doc/changes/latest.inc Outdated Show resolved Hide resolved

scott-huberty and others added 2 commits June 15, 2023 16:44

Update doc/changes/latest.inc

c0f8bac

Co-authored-by: Richard Höchenberger <[email protected]>

DOC: Add API change to change log

48fdd2e

- described that we are deprecating the gap_description kwarg of read_raw_eyelink

larsoner reviewed Jun 16, 2023

View reviewed changes

scott-huberty and others added 5 commits June 16, 2023 14:03

Update doc/changes/latest.inc

5b437fa

Co-authored-by: Eric Larson <[email protected]>

Apply suggestions from code review

86d0138

2 more of Eric's suggestions. 1 remaining that has not been added just yet. Co-authored-by: Eric Larson <[email protected]>

FIX: add _check_option import

1184215

Apply suggestions from code review

5781a59

more suggestions from Eric Co-authored-by: Eric Larson <[email protected]>

larsoner reviewed Jun 20, 2023

View reviewed changes

mne/io/eyelink/_utils.py Outdated Show resolved Hide resolved

Update mne/io/eyelink/_utils.py

0e5133c

Don't force onset to be 0, allow negative time if before recording start. Co-authored-by: Eric Larson <[email protected]>

scott-huberty commented Jun 20, 2023

View reviewed changes

mne/preprocessing/eyetracking/calibration.py Outdated Show resolved Hide resolved

FIX, remove origin kwarg from plot method, add axes kwarg. doc upate …

3d5d5d7

…and flake fixes

scott-huberty requested a review from larsoner June 20, 2023 20:33

Merge branch 'main' into et_calibration

073fba9

larsoner approved these changes Jun 20, 2023

View reviewed changes

larsoner enabled auto-merge (squash) June 20, 2023 20:40

larsoner merged commit 35d3797 into mne-tools:main Jun 20, 2023

scott-huberty deleted the et_calibration branch June 20, 2023 21:53

drammock added a commit to drammock/mne-python that referenced this pull request Jul 21, 2023

add proper deprecation label missed in mne-tools#11719

16f6fbc

scott-huberty mentioned this pull request Aug 9, 2023

FIX: A couple of minor changes to eyetracking Calibrations #11872

Merged

scott-huberty mentioned this pull request Oct 16, 2023

Unit Conversion Helper function for eye-tracking data (pixels to radians) #12112

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Create a Calibrations class for eyetracking data #11719

ENH: Create a Calibrations class for eyetracking data #11719

scott-huberty commented Jun 3, 2023 •

edited

Loading

britta-wstnr commented Jun 5, 2023

britta-wstnr left a comment

scott-huberty commented Jun 5, 2023

scott-huberty commented Jun 5, 2023

mscheltienne commented Jun 5, 2023

scott-huberty commented Jun 5, 2023

scott-huberty commented Jun 6, 2023

larsoner commented Jun 6, 2023

scott-huberty commented Jun 6, 2023

scott-huberty commented Jun 6, 2023 •

edited

Loading

larsoner commented Jun 6, 2023

larsoner commented Jun 6, 2023

scott-huberty commented Jun 6, 2023

scott-huberty commented Jun 15, 2023 •

edited

Loading

scott-huberty commented Jun 16, 2023

larsoner left a comment

larsoner Jun 16, 2023

larsoner Jun 16, 2023

scott-huberty Jun 16, 2023

larsoner Jun 16, 2023

scott-huberty Jun 16, 2023

larsoner Jun 16, 2023

larsoner left a comment

scott-huberty commented Jun 20, 2023

scott-huberty commented Jun 20, 2023 •

edited

Loading

larsoner left a comment •

edited

Loading

britta-wstnr commented Jun 27, 2023

		@@ -338,16 +344,17 @@ def read_raw_eyelink(
		--------
		mne.io.Raw : Documentation of attribute and methods.

ENH: Create a Calibrations class for eyetracking data #11719

ENH: Create a Calibrations class for eyetracking data #11719

Conversation

scott-huberty commented Jun 3, 2023 • edited Loading

Reference issue

What does this implement/fix?

Minimally Working Example:

Additional information

A couple of notes:

TODOS:

britta-wstnr commented Jun 5, 2023

britta-wstnr left a comment

Choose a reason for hiding this comment

scott-huberty commented Jun 5, 2023

scott-huberty commented Jun 5, 2023

mscheltienne commented Jun 5, 2023

scott-huberty commented Jun 5, 2023

scott-huberty commented Jun 6, 2023

larsoner commented Jun 6, 2023

scott-huberty commented Jun 6, 2023

scott-huberty commented Jun 6, 2023 • edited Loading

larsoner commented Jun 6, 2023

larsoner commented Jun 6, 2023

scott-huberty commented Jun 6, 2023

scott-huberty commented Jun 15, 2023 • edited Loading

scott-huberty commented Jun 16, 2023

larsoner left a comment

Choose a reason for hiding this comment

larsoner Jun 16, 2023

Choose a reason for hiding this comment

larsoner Jun 16, 2023

Choose a reason for hiding this comment

scott-huberty Jun 16, 2023

Choose a reason for hiding this comment

larsoner Jun 16, 2023

Choose a reason for hiding this comment

scott-huberty Jun 16, 2023

Choose a reason for hiding this comment

larsoner Jun 16, 2023

Choose a reason for hiding this comment

larsoner left a comment

Choose a reason for hiding this comment

scott-huberty commented Jun 20, 2023

scott-huberty commented Jun 20, 2023 • edited Loading

larsoner left a comment • edited Loading

Choose a reason for hiding this comment

britta-wstnr commented Jun 27, 2023

scott-huberty commented Jun 3, 2023 •

edited

Loading

scott-huberty commented Jun 6, 2023 •

edited

Loading

scott-huberty commented Jun 15, 2023 •

edited

Loading

scott-huberty commented Jun 20, 2023 •

edited

Loading

larsoner left a comment •

edited

Loading