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

feat: add deg2pix transformation #699

Merged
merged 25 commits into from
Mar 12, 2024
Merged

Conversation

OmerShubi
Copy link
Collaborator

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)
  • 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:

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

@github-actions github-actions bot added the enhancement New feature or request label Feb 25, 2024
@OmerShubi
Copy link
Collaborator Author

OmerShubi commented Feb 25, 2024

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

Copy link

codecov bot commented Feb 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

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.
📢 Have feedback on the report? Share it here.

@OmerShubi
Copy link
Collaborator Author

OmerShubi commented Feb 25, 2024

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
Copy link
Contributor

dkrako commented Feb 26, 2024

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:

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

@dkrako dkrako linked an issue Feb 26, 2024 that may be closed by this pull request
2 tasks
@OmerShubi
Copy link
Collaborator Author

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.

Replace double negative
Copy link
Contributor

@dkrako dkrako left a 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 dkrako changed the title feature 533 - Add deg2pix feat: add deg2pix transformation Feb 29, 2024
@OmerShubi
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

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.

if 'origin' in method_kwargs and 'origin' not in kwargs:
self._check_experiment()
assert self.experiment is not None
kwargs['origin'] = self.experiment.screen.origin

@dkrako
Copy link
Contributor

dkrako commented Mar 1, 2024

Regarding the missing tests, these three are missing:

  • 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

  • test_gaze_dataframe_deg2pix_exceptions() in gaze_transfom_test.py: add an deg2pix equivalent of this to check exceptions:

    @pytest.mark.parametrize(
    ('init_kwargs', 'exception', 'expected_msg'),
    [
    pytest.param(
    {
    'data': pl.DataFrame(schema={'x': pl.Float64, 'y': pl.Float64}),
    'experiment': pm.Experiment(1024, 768, 38, 30, 60, 'center', 1000),
    },
    AttributeError,
    'n_components must be either 2, 4 or 6 but is None',
    id='no_column_components',
    ),
    pytest.param(
    {
    'data': pl.from_dict({'x': [0.1], 'y': [0.2]}),
    'experiment': pm.Experiment(1024, 768, 38, 30, 60, 'center', 1000),
    'acceleration_columns': ['x', 'y'],
    },
    pl.exceptions.ColumnNotFoundError,
    (
    "Neither is 'pixel' in the dataframe columns, "
    'nor is a pixel column explicitly specified. '
    'You can specify the pixel column via: '
    'pix2deg(pixel_column="name_of_your_pixel_column"). '
    "Available dataframe columns are: ['time', 'acceleration']"
    ),
    id='no_pixel_column',
    ),
    pytest.param(
    {
    'data': pl.DataFrame(schema={'x': pl.Float64, 'y': pl.Float64}),
    'pixel_columns': ['x', 'y'],
    },
    AttributeError,
    'experiment must not be None for this method to work',
    id='no_experiment',
    ),
    pytest.param(
    {
    'data': pl.DataFrame(schema={'x': pl.Float64, 'y': pl.Float64}),
    'experiment': pm.Experiment(1024, 768, 38, 30, None, 'center', 1000),
    'pixel_columns': ['x', 'y'],
    },
    AttributeError,
    'Neither eye-to-screen distance is in the columns of the dataframe '
    'nor experiment eye-to-screen distance is specified.',
    id='no_distance_column_no_experiment_distance',
    ),
    pytest.param(
    {
    'data': pl.DataFrame(schema={'x': pl.Float64, 'y': pl.Float64}),
    'experiment': pm.Experiment(1024, 768, 38, 30, None, 'center', 1000),
    'pixel_columns': ['x', 'y'],
    'distance_column': 'distance',
    },
    AttributeError,
    'Neither eye-to-screen distance is in the columns of the dataframe '
    'nor experiment eye-to-screen distance is specified.',
    id='distance_column_not_in_dataframe',
    ),
    ],
    )
    def test_gaze_dataframe_pix2deg_exceptions(init_kwargs, exception, expected_msg):
    gaze_df = pm.GazeDataFrame(**init_kwargs)
    with pytest.raises(exception) as excinfo:
    gaze_df.pix2deg()
    msg, = excinfo.value.args
    assert msg == expected_msg

  • test_deg2pix() in dataset_test.py, similar to this:

    def test_pix2deg(dataset_configuration):
    dataset = pm.Dataset(**dataset_configuration['init_kwargs'])
    dataset.load()
    original_schema = dataset.gaze[0].schema
    dataset.pix2deg()
    expected_schema = {**original_schema, 'position': pl.List(pl.Float64)}
    for result_gaze_df in dataset.gaze:
    print(result_gaze_df.schema)
    print(expected_schema)
    assert result_gaze_df.schema == expected_schema

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
Copy link
Contributor

dkrako commented Mar 1, 2024

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.

if 'origin' in method_kwargs and 'origin' not in kwargs:
self._check_experiment()
assert self.experiment is not None
kwargs['origin'] = self.experiment.screen.origin

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

@dkrako
Copy link
Contributor

dkrako commented Mar 1, 2024

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
Copy link
Collaborator Author

  • 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
Copy link
Collaborator Author

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

Copy link
Contributor

@dkrako dkrako left a 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!

@dkrako dkrako enabled auto-merge (squash) March 12, 2024 14:03
@dkrako dkrako merged commit 6542c4c into aeye-lab:main Mar 12, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add deg2pix()
2 participants