Repository: aeye-lab/pymovements
Pull request #1084 (2025-04-09 07:51:33)

feat: use locale encoding by default in from_asc()
## Description

This PR sets the default value of `gaze.from_asc()` to `None` (was: `ascii`) .

The main reason is that `None` defaults reduce headaches when implementing #1004 

The [`open()`](https://docs.python.org/3/library/functions.html#open) function has the following documentation on the `encoding` argument:

> In text mode, if encoding is not specified the encoding used is platform-dependent: [locale.getencoding()](https://docs.python.org/3/library/locale.html#locale.getencoding) is called to get the current locale encoding.

So I guess it would also be more in line with the builtins to have `None` as the default.

This is not a breaking change, as `ascii` is a small charset that is fully included in both `utf8` (locale on mac and linux) and `latin1` (locale on windows).

For eyelink-ascii datasets the encoding should now always be specified to make sure it is independent of platform configurations.

## Implemented changes

- [x] change `encoding` default in `gaze/io.py::from_asc()`
- [x] change `encoding` default in `gaze/_utils/parsing.py::parse_eyelink()`
- [x] add `encoding` to `ToyDatasetEyeLink`

## How Has This Been Tested?

- [x] test encoding results in expected umlauts in metadata pattern
- [x] test exception if ascii encoding but umlauts in file

## 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


codecov[bot]: ## [Codecov](https://app.codecov.io/gh/aeye-lab/pymovements/pull/1084?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aeye-lab) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 99.94%. Comparing base [(`4ae7f21`)](https://app.codecov.io/gh/aeye-lab/pymovements/commit/4ae7f2167c0357cf75385706556577765c27f2f8?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aeye-lab) to head [(`2bd151f`)](https://app.codecov.io/gh/aeye-lab/pymovements/commit/2bd151f15e6490e50b34b67cd42ba6c4c8223a71?dropdown=coverage&el=desc&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    #1084      +/-   ##
===========================================
- Coverage   100.00%   99.94%   -0.06%     
===========================================
  Files           87       87              
  Lines         3717     3717              
  Branches       639      639              
===========================================
- Hits          3717     3715       -2     
- Misses           0        1       +1     
- Partials         0        1       +1     
```

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/aeye-lab/pymovements/pull/1084?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).

<details><summary> :rocket: New features to boost your workflow: </summary>

- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</details>
dkrako: @saeub I also added you as a reviewer as you added the encoding argument.
you might possibly see some shortcomings of this PR that I didn't think of
saeub: > To avoid this, it should be sufficient to specify the encoding in every DatasetDefinition that uses eyelink asc files, right?

Yes. I think it would be enough to do it for the datasets where non-ASCII characters occur, but it might be easier just to do it for all of them.
dkrako: If I'm correct, then currently we only have `ToyDatasetEyeLink`, which has only ascii characters.
We would just need to make sure for future datasets that encoding is specified to guarantee locale platform independence.
saeub: RaCCooNS (https://github.com/aeye-lab/pymovements/pull/961) will be the first one with non-ASCII characters

Patch:
diff --git a/src/pymovements/datasets/toy_dataset_eyelink.py b/src/pymovements/datasets/toy_dataset_eyelink.py
index 01cefaf..a7d1e5e 100644
--- a/src/pymovements/datasets/toy_dataset_eyelink.py
+++ b/src/pymovements/datasets/toy_dataset_eyelink.py
@@ -247,6 +247,7 @@ class ToyDatasetEyeLink(DatasetDefinition):
                     'point_id': pl.Int64,
                     'task': pl.Utf8,
                 },
+                'encoding': 'ascii',
             },
         },
     )
diff --git a/src/pymovements/datasets/toy_dataset_eyelink.yaml b/src/pymovements/datasets/toy_dataset_eyelink.yaml
index c87e2bf..29d8fb6 100644
--- a/src/pymovements/datasets/toy_dataset_eyelink.yaml
+++ b/src/pymovements/datasets/toy_dataset_eyelink.yaml
@@ -85,3 +85,4 @@ custom_read_kwargs:
       screen_id: !polars.Int64
       point_id: !polars.Int64
       task: !polars.Utf8
+    encoding: 'ascii'
diff --git a/src/pymovements/gaze/_utils/parsing.py b/src/pymovements/gaze/_utils/parsing.py
index 8153446..4afbb45 100755
--- a/src/pymovements/gaze/_utils/parsing.py
+++ b/src/pymovements/gaze/_utils/parsing.py
@@ -182,7 +182,7 @@ def parse_eyelink(
         patterns: list[dict[str, Any] | str] | None = None,
         schema: dict[str, Any] | None = None,
         metadata_patterns: list[dict[str, Any] | str] | None = None,
-        encoding: str = 'ascii',
+        encoding: str | None = None,
 ) -> tuple[pl.DataFrame, dict[str, Any]]:
     """Parse EyeLink asc file.
 
@@ -196,8 +196,8 @@ def parse_eyelink(
         Dictionary to optionally specify types of columns parsed by patterns. (default: None)
     metadata_patterns: list[dict[str, Any] | str] | None
         list of patterns to match for additional metadata. (default: None)
-    encoding: str
-        Text encoding of the file. (default: 'ascii')
+    encoding: str | None
+        Text encoding of the file. If None, the locale encoding is used. (default: None)
 
     Returns
     -------
diff --git a/src/pymovements/gaze/io.py b/src/pymovements/gaze/io.py
index 31166da..ea92d93 100644
--- a/src/pymovements/gaze/io.py
+++ b/src/pymovements/gaze/io.py
@@ -282,7 +282,7 @@ def from_asc(
         trial_columns: str | list[str] | None = None,
         add_columns: dict[str, str] | None = None,
         column_schema_overrides: dict[str, Any] | None = None,
-        encoding: str = 'ascii',
+        encoding: str | None = None,
 ) -> GazeDataFrame:
     """Initialize a :py:class:`pymovements.gaze.GazeDataFrame`.
 
@@ -311,8 +311,8 @@ def from_asc(
     column_schema_overrides: dict[str, Any] | None
         Dictionary containing types for columns.
         (default: None)
-    encoding: str
-        Text encoding of the file. (default: 'ascii')
+    encoding: str | None
+        Text encoding of the file. If None, the locale encoding is used. (default: None)
 
     Returns
     -------


Test patch:
diff --git a/tests/files/eyelink_monocular_no_dummy_example.asc b/tests/files/eyelink_monocular_no_dummy_example.asc
index ddbc19b..4fa90fa 100644
--- a/tests/files/eyelink_monocular_no_dummy_example.asc
+++ b/tests/files/eyelink_monocular_no_dummy_example.asc
@@ -11,6 +11,7 @@
 **
 
 MSG	220960 DISPLAY_COORDS 0 0 1919 1079
+MSG 229999 ENCODING TEST ÄÖÜ
 INPUT	234411	127
 INPUT	406205	127
 INPUT	509350	127
diff --git a/tests/unit/gaze/io/asc_test.py b/tests/unit/gaze/io/asc_test.py
index e870026..1e0272d 100644
--- a/tests/unit/gaze/io/asc_test.py
+++ b/tests/unit/gaze/io/asc_test.py
@@ -156,6 +156,7 @@ def test_from_asc_has_frame_equal(kwargs, expected_frame):
             },
             id='eyelink_asc_mono_pattern_eyelink',
         ),
+
         pytest.param(
             {
                 'file': 'tests/files/eyelink_monocular_example.asc',
@@ -178,6 +179,7 @@ def test_from_asc_has_frame_equal(kwargs, expected_frame):
             },
             id='eyelink_asc_mono_pattern_list',
         ),
+
         pytest.param(
             {
                 'file': 'tests/files/eyelink_monocular_2khz_example.asc',
@@ -191,6 +193,7 @@ def test_from_asc_has_frame_equal(kwargs, expected_frame):
             },
             id='eyelink_asc_mono_2khz_pattern_eyelink',
         ),
+
         pytest.param(
             {
                 'file': 'tests/files/eyelink_monocular_no_dummy_example.asc',
@@ -214,7 +217,7 @@ def test_from_asc_has_shape_and_schema(kwargs, shape, schema):
 
 
 @pytest.mark.parametrize(
-    ('kwargs', 'exception', 'message'),
+    ('kwargs', 'exception', 'message_prefix'),
     [
         pytest.param(
             {
@@ -225,14 +228,27 @@ def test_from_asc_has_shape_and_schema(kwargs, shape, schema):
             "unknown pattern key 'foobar'. Supported keys are: eyelink",
             id='unknown_pattern',
         ),
+
+        pytest.param(
+            {
+                'file': 'tests/files/eyelink_monocular_no_dummy_example.asc',
+                'metadata_patterns': [
+                    {'pattern': r'ENCODING TEST (?P<foobar>.+)'},
+                ],
+                'encoding': 'ascii',
+            },
+            UnicodeDecodeError,
+            'ascii',
+            id='eyelink_monocular_no_dummy_example_encoding_ascii',
+        ),
     ],
 )
-def test_from_asc_raises_exception(kwargs, exception, message):
+def test_from_asc_raises_exception(kwargs, exception, message_prefix):
     with pytest.raises(exception) as excinfo:
         pm.gaze.from_asc(**kwargs)
 
-    msg, = excinfo.value.args
-    assert msg == message
+    msg = excinfo.value.args[0]
+    assert msg.startswith(message_prefix)
 
 
 @pytest.mark.parametrize(
@@ -376,3 +392,43 @@ def test_from_asc_detects_mismatches_in_experiment_metadata(experiment_kwargs, i
     expected_msg = 'Experiment metadata does not match the metadata in the ASC file:\n'
     expected_msg += '\n'.join(f'- {issue}' for issue in issues)
     assert msg == expected_msg
+
+
+@pytest.mark.parametrize(
+    ('kwargs', 'expected_metadata'),
+    [
+        pytest.param(
+            {
+                'file': 'tests/files/eyelink_monocular_no_dummy_example.asc',
+                'metadata_patterns': [
+                    {'pattern': r'ENCODING TEST (?P<foobar>.+)'},
+                ],
+                'encoding': 'utf8',
+            },
+            {
+                'foobar': 'ÄÖÜ',
+            },
+            id='eyelink_monocular_no_dummy_example_encoding_utf8',
+        ),
+
+        pytest.param(
+            {
+                'file': 'tests/files/eyelink_monocular_no_dummy_example.asc',
+                'metadata_patterns': [
+                    {'pattern': r'ENCODING TEST (?P<foobar>.+)'},
+                ],
+                'encoding': 'latin1',
+            },
+            {
+                'foobar': 'Ã\x84Ã\x96Ã\x9c',
+            },
+            id='eyelink_monocular_no_dummy_example_encoding_latin1',
+        ),
+    ],
+)
+def test_from_asc_has_expected_metadata(kwargs, expected_metadata):
+    gaze = pm.gaze.from_asc(**kwargs)
+
+    for key, value in expected_metadata.items():
+        assert key in gaze._metadata
+        assert gaze._metadata[key] == value