feat: add deg2pix transformation

[OmerShubi]

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

[OmerShubi]

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]

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]

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]

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.

[OmerShubi]

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.

[dkrako]

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.

[OmerShubi]

@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]

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.

pymovements/src/pymovements/gaze/gaze_dataframe.py

Lines 351 to 354 in 53aa819

	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]

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:

  pymovements/tests/unit/gaze/gaze_transform_test.py

  Lines 778 to 847 in 53aa819

  	@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:

  pymovements/tests/unit/dataset/dataset_test.py

  Lines 536 to 548 in 53aa819

  	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]

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.

pymovements/src/pymovements/gaze/gaze_dataframe.py

Lines 351 to 354 in 53aa819

	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]

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]

• 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]

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

[dkrako]

Great! Thank you so much for putting effort into the issue, clean code and quick development!