-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Function to convert eyegaze units to radians #12237
Conversation
- add requires_testing_data decorator - add convert units API to doc - fix mistake in calculation
- Make sure that plot_gaze works with eyegaze data that are in radians - add demonstration of the above to current plot_gaze example - add a bunch of tests to make sure no feature is covered - created a raw_eyetrack fixture for reuse across tests - add xref alias for calibration class
Hi @drammock , @britta-wstnr , @mscheltienne Happy new year! I think this PR is ready for review. To recap previous discussions, the major concerns were:
Consider simulating eyegaze data with fixations at three locations on a horizontal plane. The transformation becomes notably non-linear when the visual angle becomes too large (i.e the participant is too close to the screen or the screen is very large).
Fortunately, if the visual angle to the screen is within reasonable limits (< 30°), the relationship between pixel coordinates and visual angle is approximately linear (refer to the image below). Most eyetracker setups include a chin-rest to stabilize head position, and standard screens provided by manufacturers, so I think for most use cases the visual angle will be < 30° (but we could throw a warning if it is above this). Since reporting the visual angle is fairly common in eye-tracking literature, so I still think this is a useful feature to provide. Updated tutorial with demonstration of unit conversion |
pip-pre failures are unrelated, I opened #12372 to track it |
- ammend pre-commit yaml to see if it fixes strange formatting - add warnings and info to convert_units docstring - remove _1 from private pix_To_rad function name - eyetraacking plot heatmap style suggestions Co-authored-by: Eric Larson <[email protected]>
- update precommit yaml - docstring fixes - minor code tweaks Co-authored-by: Eric Larson <[email protected]> Co-authored-by: Daniel McCloy <[email protected]>
Co-authored-by: Daniel McCloy <[email protected]>
for more information, see https://pre-commit.ci
- wondering if there is just a precision error with ubuntu old CI, and if so, just pushing the visual angle value to be far above .52 (the threshold for warngin) will assure that the warning is thrown across all systems.
Head branch was pushed to by a user without write access
Hmm okay. I actually want the warning to be raised (AFAIK I added a test that simulated a scenario where the unit conversion resulted in values greater than .52 rad (30 deg), so a warning should be thrown. The simulated rad values should be 1.10 so it can't be a matter of numerical precision.. Trying to figure it out but I'm not sure why the error isn't thrown on the Ubuntu old VM. |
In this case I usually create a env in
then you can hopefully run the tests locally to replicate the error and then inspect etc. |
trying to figure out what is happening inside the ubuntu old runner causing the test to fail.. I will revert this commit after i am done.
OK, unfortunately I can't replicate the test failiure locally on a Linux machine with the same specs as the ubuntu old env: local environment specs
In 00ab0db I added a couple asserts to check that the values are what I expect during the test (they are), which should raise the warning. So to me this suggests that the issue is with |
Ahah.. interesting. It looks like the warning we want is getting reported by pytest: https://github.com/mne-tools/mne-python/actions/runs/7644559198/job/20829160758?pr=12237#step:16:3509 but it isn't getting captured by our test line |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes feel free to sneak in a fix for #12391
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for being so late to the radians party ... Only found some typos, otherwise LGTM!
Co-authored-by: Britta Westner <[email protected]>
all suggested revisions have been incorporated, are we okay to merge? |
* upstream/main: [pre-commit.ci] pre-commit autoupdate (mne-tools#12421)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just two tiny changes I pushed, marking for merge when green, thanks @scott-huberty !
.. Important:: | ||
There are important considerations to keep in mind when using this function, | ||
see the Notes section below. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@scott-huberty pushed a commit to add this to the top rather than have all the text indented in the Notes
. It should make Notes
more readable by itself and also call better attention to it by putting this at the top.
if ch_dict["coil_type"] != FIFF.FIFFV_COIL_EYETRACK_POS: | ||
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@scott-huberty this is the other change I made -- if not x: continue
rather than if x: <indent everything another 4 spaces
. I find it the same readability but less indented which is nice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea thx!
Co-authored-by: Eric Larson <[email protected]> Co-authored-by: Daniel McCloy <[email protected]> Co-authored-by: Britta Westner <[email protected]>
closes #12112
closes #12391
This PR implements a function
mne.preprocessing.eyetracking.convert_units
to help users convert pixel data to radians, which is nice for us because radians are an SI unit, and it may come in handy down the line because converting pixel data to degrees/radians of visual angle is often done prior to calculating eye-tracking related derivatives (saccade amplitude, saccade velocity, etc).Perhaps the best way to make sure this is behaving as expected is to plot a heatmap before and after unit conversion. But i'll need to refactor the
mne.viz.eyetracking.plot_gaze
to handle the scenario where data are in radians and not pixels.