-
Notifications
You must be signed in to change notification settings - Fork 23
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
feat: add deg2pix transformation #699
Conversation
for more information, see https://pre-commit.ci
I didn't want to make too many changes but I used the distance conversion from cm to px as is as in pix2deg. def distance_cm_to_px(
distance: float | str,
screen_resolution: tuple[int, int],
screen_size: tuple[float, float],
n_components: int,) -> pl.Expr:
if isinstance(distance, (float, int)):
_check_distance(distance)
distance_series = pl.lit(distance)
elif isinstance(distance, str):
# True division by 10 is needed to convert distance from mm to cm
distance_series = pl.col(distance).truediv(10)
else:
raise TypeError(
f'`distance` must be of type `float`, `int` or `str`, but is of type'
f'`{type(distance).__name__}`',
)
distance_pixels = pl.concat_list([
distance_series.mul(screen_resolution[component % 2] / screen_size[component % 2])
for component in range(n_components)
])
return distance_pixels |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #699 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 55 55
Lines 2500 2527 +27
Branches 633 643 +10
=========================================
+ Hits 2500 2527 +27 ☔ View full report in Codecov by Sentry. |
…movements into feature/533-deg2pix
for more information, see https://pre-commit.ci
I tried adding a test for deg2pix in GazeDataFrame but am getting I'm not sure how to go about this. @dkrako any suggestions? |
Thank you for checking in before changing the existing testing. I don't think that it's necessary to enforce a column ordere here and the different column ordering makes sense actually:
|
You mean Anyways, added a suggested fix. |
Replace double negative
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.
This is excellent! Thank you so much for the implementation.
There are some very minor changes I would like to request which shouldn't take you long.
We'll discuss the naming of origin
vs pixel_origin
in deg2pix()
on our friday meeting.
@dkrako I added deg2pix tests in
Where would be relevant places to add deg2pix tests? |
Also, renaming origin -> pixel_origin leads to failed test:
Should I make changes in transform (see below)? or to the test? I am not sure how to handle this. pymovements/src/pymovements/gaze/gaze_dataframe.py Lines 351 to 354 in 53aa819
|
Regarding the missing tests, these three are missing:
As the dataset.pix2deg()
dataset.deg2pix(pixel_column='new_pixel')
expected_schema = {**original_schema, 'new_pixel': pl.List(pl.Float64)}
for result_gaze_df in dataset.gaze:
assert result_gaze_df.schema == expected_schema You don't need to check the values for correctness at the dataset level. |
Would this be resolved if we set the default |
As an addition: I think the (the This should be the signature for both def deg2pix(
self,
pixel_origin: str = 'lower left',
position_column: str = 'position',
pixel_column: str = 'pixel',
) All in all you just need to forward the arguments to the If you look at |
Okay, added the missing tests, except an equivalent for
Not really, but it seems reasonable to add it so I added it. The main issue, which I solved by passing the parameter directly, |
I think the latest commit covers all the comments you had @dkrako |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
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.
Great! Thank you so much for putting effort into the issue, clean code and quick development!
Description
Adds
deg2pix
method.Fixes issue #533.
Implemented changes
Added a deg2pix method.
Type of change
How Has This Been Tested?
Added the same tests as in
tests/unit/gaze/transforms/pix2deg_test.py
with the following changes:Checklist: