Repository: aeye-lab/pymovements
Pull request #922 (2025-01-12 19:33:26)

feat: add `GazeDataFrame.compute_event_properties()`
## Description

issue 871: https://github.com/aeye-lab/pymovements/issues/871

Fixes #

## Implemented changes

Insert a description of the changes implemented in the pull request.

- [x] made compute_events work at the gaze df level; the dataset calls each gaze_df individually
- [x] added trial columns at the Dataset level, these are extracted from `Dataset.fileinfo['gaze'].columns` and set on each subsequent GazeDataFrame when `dataset.load(preprocessed=True, events=True, set_trial_columns=True)` is called with the parameter `set_trial_columns=True`; this seemed necessary because the fixtures for tests that call `compute_event_properties` (e.g. `test_event_dataframe_add_property_has_expected_height`) are loaded from temporary preprocessed feather files (when preprocessed=True); the temporary feather files lose the "subject_id" column and are unable to load `trial_columns` using `io.from_ipc`
- [x] changed the `Dataset.load` function to set for each  subsequent GazeDataFrame the events that are loaded from separate files
- [x] fixed some linters

Observation:
- there is a huge difference between [io.from_csv](https://github.com/aeye-lab/pymovements/blob/c0c11cddfbdf16829656713da2220b7e55051515/src/pymovements/gaze/io.py#L257) and [io.from_ipc](https://github.com/aeye-lab/pymovements/blob/c0c11cddfbdf16829656713da2220b7e55051515/src/pymovements/gaze/io.py#L539) in terms of how the `GazeDataFrame` is constructed such that feather files seem to miss a lot of metadata from the `Dataset.definition`


## 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)
- [x] This change is or requires a documentation update

## 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

- [x] tests remain mostly the same since the cases are covered from `dataset.py`
- [ ] some tests will have to be added

## 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
- [ ] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [ ] 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
- [ ] Any dependent changes have been merged and published in downstream modules
- [x] I have checked my code and corrected any misspellings


codecov[bot]: ## [Codecov](https://app.codecov.io/gh/aeye-lab/pymovements/pull/922?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aeye-lab) Report
Attention: Patch coverage is `87.50000%` with `2 lines` in your changes missing coverage. Please review.
> Project coverage is 99.73%. Comparing base [(`c0c11cd`)](https://app.codecov.io/gh/aeye-lab/pymovements/commit/c0c11cddfbdf16829656713da2220b7e55051515?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aeye-lab) to head [(`80b662c`)](https://app.codecov.io/gh/aeye-lab/pymovements/commit/80b662c9eb0b56c2326b27744ad7ba3699fb7d9d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aeye-lab).

| [Files with missing lines](https://app.codecov.io/gh/aeye-lab/pymovements/pull/922?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aeye-lab) | Patch % | Lines |
|---|---|---|
| [src/pymovements/gaze/gaze\_dataframe.py](https://app.codecov.io/gh/aeye-lab/pymovements/pull/922?src=pr&el=tree&filepath=src%2Fpymovements%2Fgaze%2Fgaze_dataframe.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aeye-lab#diff-c3JjL3B5bW92ZW1lbnRzL2dhemUvZ2F6ZV9kYXRhZnJhbWUucHk=) | 77.77% | [1 Missing and 1 partial :warning: ](https://app.codecov.io/gh/aeye-lab/pymovements/pull/922?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aeye-lab) |

<details><summary>Additional details and impacted files</summary>


```diff
@@             Coverage Diff             @@
##              main     #922      +/-   ##
===========================================
- Coverage   100.00%   99.73%   -0.27%     
===========================================
  Files           74       74              
  Lines         3422     3425       +3     
  Branches       613      614       +1     
===========================================
- Hits          3422     3416       -6     
- Misses           0        7       +7     
- Partials         0        2       +2     
```

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/aeye-lab/pymovements/pull/922?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aeye-lab).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aeye-lab).

dkrako: >    there is a huge difference between [io.from_csv](https://github.com/aeye-lab/pymovements/blob/c0c11cddfbdf16829656713da2220b7e55051515/src/pymovements/gaze/io.py#L257) and [io.from_ipc](https://github.com/aeye-lab/pymovements/blob/c0c11cddfbdf16829656713da2220b7e55051515/src/pymovements/gaze/io.py#L539) in terms of how the GazeDataFrame is constructed such that feather files seem to miss a lot of metadata from the Dataset.definition

I have created an issue with further info here #927 
dkrako: > added trial columns at the Dataset level, these are extracted from Dataset.fileinfo['gaze'].columns and set on each subsequent GazeDataFrame when dataset.load(preprocessed=True, events=True, set_trial_columns=True) is called with the parameter set_trial_columns=True; this seemed necessary because the fixtures for tests that call compute_event_properties (e.g. test_event_dataframe_add_property_has_expected_height) are loaded from temporary preprocessed feather files (when preprocessed=True); the temporary feather files lose the "subject_id" column and are unable to load trial_columns using io.from_ipc

I think there's a confusion between terms as they are not well documented.

`trial_columns` are columns in a gaze file that provide information on the current trial. For example there can be a column in the gaze data file which specifies the particular page that is presented at each time step. This is relevant for processing, as each trial is treated as a distinct time series.

We have further metadata available in `Dataset.fileinfo` which is parsed from the filename. This metadata is related to a single file and is usually session wide, for example `subject_id` and `session_id`. A session usually spans multiple trials encoded in the `trial_columns` of each `GazeDataFrame`.

These columns in `Dataset.fileinfo` are the ominous `identifier_columns` used in the code of `Dataset.compute_event_properties()` that you elegantly replaced. Currently, these `identifier_columns` are copied to a new `GazeDataFrame` on `Dataset.load()` (or `load_gaze_file()`) via `add_columns`. So they are set as additional `trial_columns`, hence the confusion.

https://github.com/aeye-lab/pymovements/blob/c0c11cddfbdf16829656713da2220b7e55051515/src/pymovements/dataset/dataset_files.py#L306-L309

Honestly, I would like to get rid of copying fileinfo metadata into the `GazeDataFrame`, at least not by default as it doesn't provide much value and created headaches on saving and loading. We will probably have to figure out how to generally improve handling metadata first.

Your workaround circumvents the actual problem but the additional parameters are not really needed. But nevermind I implemented a fix in #928 where the `trial_columns` are now correctly passed when initializing a `GazeDataFrame`. Your workaround is thus not needed anymore. Please revert the respective changes.

dkrako: Hi @senisioi I just wanted to check in if you're planning to finish this PR or if someone else should take over.

There's not much work left, so either way is fine.
dkrako: Please be aware that this PR and #1041 will lead to a merge conflict.

If #1041 is merged first, please move the exceptions into `GazeDataFrame`.
saeub: @senisioi I could take over and finish the last bits if you don't mind (I keep running into this and would really like to see this merged soon)
senisioi: sure thing! I'm so overwhelmed with work right now 🙏

Patch:
diff --git a/src/pymovements/dataset/dataset.py b/src/pymovements/dataset/dataset.py
index 49acf60..728e950 100644
--- a/src/pymovements/dataset/dataset.py
+++ b/src/pymovements/dataset/dataset.py
@@ -36,7 +36,6 @@ from pymovements.dataset.dataset_library import DatasetLibrary
 from pymovements.dataset.dataset_paths import DatasetPaths
 from pymovements.events import EventDataFrame
 from pymovements.events.precomputed import PrecomputedEventDataFrame
-from pymovements.events.processing import EventGazeProcessor
 from pymovements.gaze import GazeDataFrame
 from pymovements.reading_measures import ReadingMeasures
 
@@ -159,6 +158,8 @@ class Dataset:
                 events_dirname=events_dirname,
                 extension=extension,
             )
+            for loaded_gaze_df, loaded_events_df in zip(self.gaze, self.events):
+                loaded_gaze_df.events = loaded_events_df
 
         return self
 
@@ -224,6 +225,7 @@ class Dataset:
             preprocessed_dirname=preprocessed_dirname,
             extension=extension,
         )
+
         return self
 
     def load_precomputed_events(self) -> None:
@@ -765,22 +767,8 @@ class Dataset:
         Dataset
             Returns self, useful for method cascading.
         """
-        processor = EventGazeProcessor(event_properties)
-
-        identifier_columns = [
-            column
-            for column in self.fileinfo['gaze'].columns
-            if column != 'filepath'
-        ]
-
-        disable_progressbar = not verbose
-        for events, gaze in tqdm(zip(self.events, self.gaze), disable=disable_progressbar):
-            new_properties = processor.process(
-                events, gaze, identifiers=identifier_columns, name=name,
-            )
-            join_on = identifier_columns + ['name', 'onset', 'offset']
-            events.add_event_properties(new_properties, join_on=join_on)
-
+        for gaze in tqdm(self.gaze, disable=not verbose):
+            gaze.compute_event_properties(event_properties, name=name)
         return self
 
     def compute_properties(
diff --git a/src/pymovements/gaze/gaze_dataframe.py b/src/pymovements/gaze/gaze_dataframe.py
index 3db904f..5246cd0 100644
--- a/src/pymovements/gaze/gaze_dataframe.py
+++ b/src/pymovements/gaze/gaze_dataframe.py
@@ -32,6 +32,7 @@ import polars as pl
 from tqdm import tqdm
 
 import pymovements as pm  # pylint: disable=cyclic-import
+from pymovements.events.processing import EventGazeProcessor
 from pymovements.gaze import transforms
 from pymovements.gaze.experiment import Experiment
 from pymovements.utils.checks import check_is_mutual_exclusive
@@ -885,6 +886,46 @@ class GazeDataFrame:
                 how='diagonal',
             )
 
+    def compute_event_properties(
+            self,
+            event_properties: str | tuple[str, dict[str, Any]]
+            | list[str | tuple[str, dict[str, Any]]],
+            name: str | None = None,
+    ) -> None:
+        """Calculate event properties for given events.
+
+        The calculated event properties are added as columns to
+        :py:attr:`~pymovements.gaze.GazeDataFrame.events`.
+
+        Parameters
+        ----------
+        event_properties: str | tuple[str, dict[str, Any]] | list[str | tuple[str, dict[str, Any]]]
+            The event properties to compute.
+        name: str | None
+            Process only events that match the name. (default: None)
+
+        Raises
+        ------
+        InvalidProperty
+            If ``property_name`` is not a valid property. See
+            :py:mod:`pymovements.events.event_properties` for an overview of supported properties.
+        RuntimeError
+            If specified event name ``name`` is missing from ``events``.
+        """
+        if len(self.events) == 0:
+            warnings.warn(
+                'No events available to compute event properties. '
+                'Did you forget to use detect()?',
+            )
+
+        identifiers = self.trial_columns if self.trial_columns is not None else []
+        processor = EventGazeProcessor(event_properties)
+        new_properties = processor.process(
+            self.events, self, identifiers=identifiers, name=name,
+        )
+        join_on = identifiers + ['name', 'onset', 'offset']
+        self.events.add_event_properties(new_properties, join_on=join_on)
+
     def measure_samples(
             self,
             method: str | Callable[..., pl.Expr],


Test patch:
diff --git a/tests/unit/gaze/gaze_dataframe_test.py b/tests/unit/gaze/gaze_dataframe_test.py
index 33eb701..8a667b2 100644
--- a/tests/unit/gaze/gaze_dataframe_test.py
+++ b/tests/unit/gaze/gaze_dataframe_test.py
@@ -221,3 +221,17 @@ def test_gaze_dataframe_split():
     assert_frame_equal(gaze.frame.filter(pl.col('trial_id') == 0), split_gaze[0].frame)
     assert_frame_equal(gaze.frame.filter(pl.col('trial_id') == 1), split_gaze[1].frame)
     assert_frame_equal(gaze.frame.filter(pl.col('trial_id') == 2), split_gaze[2].frame)
+
+
+def test_gaze_dataframe_compute_event_properties_no_events():
+    gaze = pm.GazeDataFrame(
+        pl.DataFrame(schema={'x': pl.Float64, 'y': pl.Float64, 'trial_id': pl.Int8}),
+        position_columns=['x', 'y'],
+        trial_columns=['trial_id'],
+    )
+
+    with pytest.warns(
+        UserWarning,
+        match='No events available to compute event properties. Did you forget to use detect()?',
+    ):
+        gaze.compute_event_properties('amplitude')