feat: add deg2pix transformation

OmerShubi commented 4 months ago

Description

Adds deg2pix method. Fixes issue #533.

Implemented changes

Added a deg2pix method.

Type of change

[ ] Bug fix (non-breaking change which fixes an issue)
[x] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
[ ] This change is or requires a documentation update

How Has This Been Tested?

Added the same tests as in tests/unit/gaze/transforms/pix2deg_test.py with the following changes:

Replaced pixel<-->position in output<-->input.
Added atol=1-e4 in equality assertions. @dkrako Is this okay? Alternative is to refine the input values.
Added two digits after zero for input position value in isosceles_triangle_origin_lowerleft_returns_45 and isosceles_triangle_left_origin_lowerleft_returns_neg45.

Checklist:

[x] My code follows the style guidelines of this project
[x] I have performed a self-review of my own code
[x] I have commented my code, particularly in hard-to-understand areas
[x] I have made corresponding changes to the documentation
[x] My changes generate no new warnings
[x] I have added tests that prove my fix is effective or that my feature works
[x] New and existing unit tests pass locally with my changes
[x] Any dependent changes have been merged and published in downstream modules
[x] I have checked my code and corrected any misspellings

OmerShubi commented 4 months ago

I didn't want to make too many changes but I used the distance conversion from cm to px as is as in pix2deg. Can consider extracting a function like:

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[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 100.00%. Comparing base (53aa819) to head (fb5152b).

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #699 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 55 55 Lines 2500 2527 +27 Branches 633 643 +10 ========================================= + Hits 2500 2527 +27 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

OmerShubi commented 4 months ago

I tried adding a test for deg2pix in GazeDataFrame but am getting FAILED tests/unit/gaze/apply_test.py::test_gaze_apply[deg2pix_origin_center] - AssertionError: DataFrames are different (columns are not in the same order). I think that the column order for the expected df is hardcoded in the creation of the GazeDataFrame and in deg2pix it appends the pixel column after position.

I'm not sure how to go about this. @dkrako any suggestions?

dkrako commented 4 months ago

I tried adding a test for deg2pix in GazeDataFrame but am getting FAILED tests/unit/gaze/apply_test.py::test_gaze_apply[deg2pix_origin_center] - AssertionError: DataFrames are different (columns are not in the same order). I think that the column order for the expected df is hardcoded in the creation of the GazeDataFrame and in deg2pix it appends the pixel column after position.

I'm not sure how to go about this. @dkrako any suggestions?

Thank you for checking in before changing the existing testing. I thought about it a bit and I think it's safe to add check_column_order=True when calling assert_frame_equal().

I don't think that it's necessary to enforce a column ordere here and the different column ordering makes sense actually:

when we have only a position column and add a pixel column via deg2pix() I would expect a FIFO column order.
when we create a new GazeDataFrame the columns are in a default initialization order.

OmerShubi commented 4 months ago

Thank you for checking in before changing the existing testing. I thought about it a bit and I think it's safe to add check_column_order=True when calling assert_frame_equal().

You mean check_column_order=False right?

Anyways, added a suggested fix.

OmerShubi commented 4 months ago

@dkrako I added deg2pix tests in deg2pix_test and apply_test but I see also pix2deg tests in

dataset_processing_test.py
gaze_file_processing_test
public_dataset_processing_test
dataset_test
gaze_transform_test
transforms_library_test
and traceplot_test and tsplot_test

Where would be relevant places to add deg2pix tests?

OmerShubi commented 4 months ago

Also, renaming origin -> pixel_origin leads to failed test:

            'deg2pix',
            {},
            pm.GazeDataFrame(
                data=pl.from_dict(
                    {
                        'time': [1000, 1000],
                        'x_dva': [26.335410003881346, 26.335410003881346],
                        'y_dva': [0.0, 0.0],
                    },
                ),
                experiment=pm.Experiment(100, 100, 100, 100, 100, 'center', 1000),
                position_columns=['x_dva', 'y_dva'],
            ),

Should I make changes in transform (see below)? or to the test? I am not sure how to handle this. https://github.com/aeye-lab/pymovements/blob/53aa81926f91ded4990dc04906e0fd40e41cefd7/src/pymovements/gaze/gaze_dataframe.py#L351-L354

dkrako commented 4 months ago

Regarding the missing tests, these three are missing:

[x] test_gaze_transform_expected_frame() in gaze_transforms_test.py misses a few cases: add deg2pix equivalents of the ids pix2deg, pix2deg_origin_center, pix2deg_origin_lower_left and pix2deg_origin_center_distance_column
[x] test_gaze_dataframe_deg2pix_exceptions() in gaze_transfom_test.py: add an deg2pix equivalent of this to check exceptions: https://github.com/aeye-lab/pymovements/blob/53aa81926f91ded4990dc04906e0fd40e41cefd7/tests/unit/gaze/gaze_transform_test.py#L778-L847
[x] test_deg2pix() in dataset_test.py, similar to this: https://github.com/aeye-lab/pymovements/blob/53aa81926f91ded4990dc04906e0fd40e41cefd7/tests/unit/dataset/dataset_test.py#L536-L548

As the dataset_configuration fixture already has a pixel column, let's adjust the new test to something like this:

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.

dkrako commented 4 months ago

Also, renaming origin -> pixel_origin leads to failed test:
            'deg2pix',
            {},
            pm.GazeDataFrame(
                data=pl.from_dict(
                    {
                        'time': [1000, 1000],
                        'x_dva': [26.335410003881346, 26.335410003881346],
                        'y_dva': [0.0, 0.0],
                    },
                ),
                experiment=pm.Experiment(100, 100, 100, 100, 100, 'center', 1000),
                position_columns=['x_dva', 'y_dva'],
            ),
Should I make changes in transform (see below)? or to the test? I am not sure how to handle this.

https://github.com/aeye-lab/pymovements/blob/53aa81926f91ded4990dc04906e0fd40e41cefd7/src/pymovements/gaze/gaze_dataframe.py#L351-L354

Would this be resolved if we set the default pixel_origin: str = 'lower left' in transforms.py::deg2pix()?

dkrako commented 4 months ago

As an addition: I think the deg2pix() implementations for Dataset and GazeDataFrame are missing the forwarding of some keyword arguments (especially pixel_origin, position_column, pixel_column).

(the pix2deg() implementation is currently also missing this, I'll create a separate issue for improving the pix2deg implemtation so you don't need to worry about that)

This should be the signature for both Dataset and GazeDataFrame (you can leave out n_components as that can be handled internally):

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 apply/transform method and you're good.

If you look at pos2vel() and pos2acc() in Dataset and GazeDataFrame you can see examples how we did that before. I would prefer to have the keyword arguments explicit and not passing them implicitly via **kwargs like it's currently implemented in the pos2vel() method.

OmerShubi commented 3 months ago

[x] test_gaze_transform_expected_frame() in gaze_transforms_test.py misses a few cases: add deg2pix equivalents of the ids pix2deg, pix2deg_origin_center, pix2deg_origin_lower_left and pix2deg_origin_center_distance_column

Okay, added the missing tests, except an equivalent for pix2deg which I did not find.

Would this be resolved if we set the default pixel_origin: str = 'lower left' in transforms.py::deg2pix()?

Not really, but it seems reasonable to add it so I added it. The main issue, which I solved by passing the parameter directly, was that the experiment doesn't have a pixel_origin parameter. Maybe this is okay.

OmerShubi commented 3 months ago

I think the latest commit covers all the comments you had @dkrako

aeye-lab / pymovements