Skip to content
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

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

Closed
scott-huberty opened this issue Oct 16, 2023 · 4 comments · Fixed by #12237
Closed

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

scott-huberty opened this issue Oct 16, 2023 · 4 comments · Fixed by #12237
Labels

Comments

@scott-huberty
Copy link
Contributor

scott-huberty commented Oct 16, 2023

Describe the new feature or enhancement

This Ticket is related to meta-issue #11879

I am at a point in one of my projects where I'd like to convert my eye-tracking position data from pixels-on-screen to a unit of angle. Since 1) this is a fairly common conversion in eye-tracking research; 2) it was highlighted as a TODO in #11879 ; and 3) this would help users convert their position data into an SI unit (radians), I think It would be good to build a helper function for this.

CC @britta-wstnr @larsoner

Describe your proposed implementation

Possible API (borrowing from @drammock):

mne.preprocessing.eyetracking.convert_units(calibration: Calibration, from: str = 'px', to: str = 'radian')

Where Calibration is an instance of mne.preprocessing.eyetracking.Calibration that contains necessary metadata: screen_size, distance, screen_resolution.

Or we could have a more specific function like:

mne.preprocessing.eyetracking.convert_pixels_to_radians(calibration: Calibration)

and a similar function for radian to pixel

Describe possible alternatives

We could also include screen_size, screen_resolution, distance as parameters of the function, so the user can either pass a Calibration instance with this info, or specify them directly via the function call (this was an idea brought up in the summer and documented in this comment)

Additional context

This was a feature that I proposed during my GSoC but didn't get to by the end of summer.

@mscheltienne
Copy link
Member

mne.preprocessing.eyetracking.convert_units(calibration: Calibration, from: str = 'px', to: str = 'radian')

with optional screen_size, screen_resolution and distance sounds good to me.

@britta-wstnr
Copy link
Member

britta-wstnr commented Oct 19, 2023

Yeah I agree that distance and such should probably be parameters.
One thing to think about then: if a parameter is specified in Calibration but also gets passed and numbers don't match, what should be expected behavior? Overwrite by parameters and throw warning? Throw error?

EDIT: second thought: alternative of course could be to only allow setting of those measurements in Calibration. In that respect it would be cleaner.

@scott-huberty
Copy link
Contributor Author

with optional screen_size, screen_resolution and distance sounds good to me.

Thanks!

One thing to think about then: if a parameter is specified in Calibration but also gets passed and numbers don't match, what should be expected behavior? Overwrite by parameters and throw warning?

Yeah, I think allowing explicit parameters to over-ride Calibration makes sense, and so does throwing a warning.

EDIT: second thought: alternative of course could be to only allow setting of those measurements in Calibration. In that respect it would be cleaner.

I'm also okay with this. I offered the option to add optional keyword args for screen_distance/screen_size etc. because IIRC someone suggested this in the summer. But we could also add these later if people ask for it?

@larsoner
Copy link
Member

+1 for adding screen_distance/size later if needed and starting out with just Calibration. As it says in @britta-wstnr's note eventually we could allow setting those in the Calibration itself which would be cleaner. So let's start from the most common case where the user has a Calibration they want to use to convert units and add things later as we hit use cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants