aeye-lab / pymovements

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

feat: Add suffixes argument to GazeDataFrame.unnest() #479

Closed theDebbister closed 1 year ago

theDebbister commented 1 year ago

Description

Please include a clear and concise description of the change and which issue is fixed. Please also include excepted improvements, relevant motivation and context. List any dependencies that are required for this change.

Fixes issue #

Implemented changes

Insert a description of the changes implemented in the pull request.

Type of change

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Checklist:

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (f6751fc) 100.00% compared to head (999fcf6) 100.00%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #479 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 48 48 Lines 2080 2079 -1 Branches 502 505 +3 ========================================= - Hits 2080 2079 -1 ``` | [Files Changed](https://app.codecov.io/gh/aeye-lab/pymovements/pull/479?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aeye-lab) | Coverage Δ | | |---|---|---| | [src/pymovements/dataset/dataset.py](https://app.codecov.io/gh/aeye-lab/pymovements/pull/479?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aeye-lab#diff-c3JjL3B5bW92ZW1lbnRzL2RhdGFzZXQvZGF0YXNldC5weQ==) | `100.00% <100.00%> (ø)` | | | [src/pymovements/dataset/dataset\_files.py](https://app.codecov.io/gh/aeye-lab/pymovements/pull/479?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aeye-lab#diff-c3JjL3B5bW92ZW1lbnRzL2RhdGFzZXQvZGF0YXNldF9maWxlcy5weQ==) | `100.00% <100.00%> (ø)` | | | [src/pymovements/gaze/gaze\_dataframe.py](https://app.codecov.io/gh/aeye-lab/pymovements/pull/479?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aeye-lab#diff-c3JjL3B5bW92ZW1lbnRzL2dhemUvZ2F6ZV9kYXRhZnJhbWUucHk=) | `100.00% <100.00%> (ø)` | |

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

dkrako commented 1 year ago

Regarding some of the failing tests, these are from the breaking change in the signature of unnest(). Here are three main issues:

  1. the tsplot tests in tests/plotting/tsplot_test.py can be fixed by using the correct keyword argument, so write gaze.unnest('pixel', output_columns=['x_pix', 'y_pix']) instead of gaze.unnest('pixel', ['x_pix', 'y_pix'])

  2. the save_preprocessed() function in dataset/dataset_files.py needs to be changed accordingly.

This is the current state: https://github.com/aeye-lab/pymovements/blob/4af4fc109b401458f8c7f2bf09119d0f6bfc0504/src/pymovements/dataset/dataset_files.py#L489-L527

But thanks to your work you can remove most of the stuff here, and reduce the entries to two lines each, e.g.:

if 'pixel' in gaze_df.frame.columns:
    gaze_df.unnest('pixel')

You don't need to keep the dictionary with unnested_columns. Actually there is some unexpected behaviour here, as calling the funtion modifies the dataframe (by calling unnest()). I will fix this in another issue so you don't need to worry about that.

  1. Usage of unnest needs to be adjusted in detect_events().

https://github.com/aeye-lab/pymovements/blob/4af4fc109b401458f8c7f2bf09119d0f6bfc0504/src/pymovements/dataset/dataset.py#L430-L431

and

https://github.com/aeye-lab/pymovements/blob/4af4fc109b401458f8c7f2bf09119d0f6bfc0504/src/pymovements/dataset/dataset.py#L473-L474

Simply use the output_columns keyword argument here as in the tests for tsplot. I am currently in the process of rewriting that method to use all the new functionality, so don't bother about the rest of the method for now.