search

aeye-lab / pymovements

A python package for processing eye movement data
https://pymovements.readthedocs.io
MIT License
61 stars 12 forks source link

Add deg2pix() #533

Closed dkrako closed 6 months ago

dkrako commented 1 year ago

Description of the problem

We currently only have pix2deg() but it would be nice to have a transform from degrees of visual angle to pixel units.

Description of a solution

Similar to pymovements.gaze.transforms.pix2deg(), degrees can be transformed using trigonometric functions.

Moreover, an offset tuple parameter in cm would be nice, as some they eyes are not centered in some experiment setups.

Minimum viable acceptance criteria

OmerShubi commented 7 months ago

@dkrako I would be happy to have a go at dealing with this.

OmerShubi commented 7 months ago

A few questions:

  1. pix2deg first applies the center_origin function which changes the origin in case it is _lowerleft. Should deg2pix apply the inverse operation? i.e. if origin is not center, should (deg2pix(pix2deg(pixel)) be equal to pixel or should it be equal to center_origin(pixel)?
  2. Relatedly, currently if pix2deg is applied then position is centered but pixels is not. I think this could be confusing as the origin is still supposedly e.g. lower_left.
  3. Should I add a similar function to transforms_numpy.py?
OmerShubi commented 7 months ago

By the way, for pix2deg, pl.arctan2(.) * (180 / np.pi) in https://github.com/aeye-lab/pymovements/blob/b097c93e966e8f0dbdc460708a77b692f3da3149/src/pymovements/gaze/transforms.py#L278-L284

could be replaced with pl.arctan2d(.) which converts to degrees.

dkrako commented 7 months ago

Great questions!

1. `pix2deg` first applies the `center_origin` function which changes the origin in case it is _lower_left_. Should `deg2pix` apply the inverse operation? i.e. if origin is not _center_, should (deg2pix(pix2deg(pixel)) be equal to pixel or should it be equal to center_origin(pixel)?

I think regardless of the value of origin, deg2pix(pix2deg(pixel)) should result in the original pixel, provided that both functions get the same origin value.

2. Relatedly, currently if pix2deg is applied then _position_ is centered but _pixels_ is not. I think this could be confusing as the origin is still supposedly e.g. lower_left.

I tend to think centering both could lead to another confusion:

  1. the pixel coordinates refer to a specific pixel in the screen coordinate system. AOIs are usually also given as screen coordinates which don't have a centered origin.
  2. the position coordinates on the other hand refer to the eye coordinate system in degrees of visual angle, where the centered origin is more adequate, i.e. looking straight ahead as being the null position.

So I would see as the two columns of different projections of the data with different reference systems.

3. Should I add a similar function to transforms_numpy.py?

We are currently not sure how to proceed with the transforms_numpy and the functions are there mainly for backwards compatibility to some very old scripts in our group. You don't need to add any functionality there.

dkrako commented 7 months ago

By the way, for pix2deg, pl.arctan2(.) * (180 / np.pi) in

https://github.com/aeye-lab/pymovements/blob/b097c93e966e8f0dbdc460708a77b692f3da3149/src/pymovements/gaze/transforms.py#L278-L284

could be replaced with pl.arctan2d(.) which converts to degrees.

Nice catch, didn't know of it yet! You can refactor to use that in your PR if you want

OmerShubi commented 7 months ago

I tend to think centering both could lead to another confusion:

  1. the pixel coordinates refer to a specific pixel in the screen coordinate system. AOIs are usually also given as screen coordinates which don't have a centered origin.
  2. the position coordinates on the other hand refer to the eye coordinate system in degrees of visual angle, where the centered origin is more adequate, i.e. looking straight ahead as being the null position.

So I would see as the two columns of different projections of the data with different reference systems.

Thanks for the clarification, I agree with you.

We are currently not sure how to proceed with the transforms_numpy and the functions are there mainly for backwards compatibility to some very old scripts in our group. You don't need to add any functionality there.

Okay.

By the way, for pix2deg, pl.arctan2(.) * (180 / np.pi) in https://github.com/aeye-lab/pymovements/blob/b097c93e966e8f0dbdc460708a77b692f3da3149/src/pymovements/gaze/transforms.py#L278-L284

could be replaced with pl.arctan2d(.) which converts to degrees.

Nice catch, didn't know of it yet! You can refactor to use that in your PR if you want

Done.