Repository: aeye-lab/pymovements
Pull request #140 (2023-02-14 18:12:30)

Transforms: Fix padding bug in cut_into_subsequences
Thanks to the examples in #95 from @theDebbister we could find a bug for the padding in `cut_into_subsequences()`.

Fortunately I never used `keep_padded=True`, but I have added the respective tests to prevent regressions.

codecov-commenter: # [Codecov](https://codecov.io/gh/aeye-lab/pymovements/pull/140?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aeye-lab) Report
Base: **53.22**% // Head: **56.25**% // Increases project coverage by **`+3.03%`** :tada:
> Coverage data is based on head [(`b5700a2`)](https://codecov.io/gh/aeye-lab/pymovements/pull/140?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aeye-lab) compared to base [(`6b23ff2`)](https://codecov.io/gh/aeye-lab/pymovements/commit/6b23ff27d169f8a3e0a470cb4e28b3fe04c2fabd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aeye-lab).
> Patch coverage: 100.00% of modified lines in pull request are covered.

> :exclamation: Current head b5700a2 differs from pull request most recent head 6b09f81. Consider uploading reports for the commit 6b09f81 to get more accurate results

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


```diff
@@            Coverage Diff             @@
##             main     #140      +/-   ##
==========================================
+ Coverage   53.22%   56.25%   +3.03%     
==========================================
  Files          18       18              
  Lines         635      631       -4     
  Branches      150      150              
==========================================
+ Hits          338      355      +17     
+ Misses        284      263      -21     
  Partials       13       13              
```


| [Impacted Files](https://codecov.io/gh/aeye-lab/pymovements/pull/140?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aeye-lab) | Coverage Δ | |
|---|---|---|
| [src/pymovements/transforms.py](https://codecov.io/gh/aeye-lab/pymovements/pull/140?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aeye-lab#diff-c3JjL3B5bW92ZW1lbnRzL3RyYW5zZm9ybXMucHk=) | `76.85% <100.00%> (+17.92%)` | :arrow_up: |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aeye-lab). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aeye-lab)

</details>

[:umbrella: View full report at Codecov](https://codecov.io/gh/aeye-lab/pymovements/pull/140?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aeye-lab).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](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: The function itself is implemented very ineffienctly, so I have created an issue for this #144

Patch:
diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
index d350de3..e3baef2 100644
--- a/.pre-commit-config.yaml
+++ b/.pre-commit-config.yaml
@@ -47,6 +47,10 @@ repos:
     rev: v2.0.1
     hooks:
     -   id: autopep8
+-   repo: https://github.com/PyCQA/autoflake
+    rev: v2.0.1
+    hooks:
+    -   id: autoflake
 -   repo: https://github.com/PyCQA/doc8
     rev: v1.1.1
     hooks:
diff --git a/pyproject.toml b/pyproject.toml
index a656f6f..6996144 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -5,6 +5,11 @@ requires = [
 ]
 build-backend = "setuptools.build_meta"
 
+[tool.autoflake]
+in-place = true
+remove-all-unused-imports = true
+ignore-init-module-imports = true
+
 [tool.autopep8]
 max_line_length = 100
 in-place = true
diff --git a/src/pymovements/transforms.py b/src/pymovements/transforms.py
index cfac2a5..36bd398 100644
--- a/src/pymovements/transforms.py
+++ b/src/pymovements/transforms.py
@@ -256,24 +256,22 @@ def norm(arr: np.ndarray, axis: int | None = None) -> np.ndarray | Any:
 def cut_into_subsequences(
     arr: np.ndarray, window_size: int, keep_padded: bool = True,
 ) -> np.ndarray:
-    """
-    Example: if old seq len was 7700, window_size=1000:
-    Input arr has: 144 x 7700 x n_channels
-    Output arr has: 144*8 x 1000 x n_channels
-    The last piece of each trial 7000-7700 gets padded with first 300 of this piece to be 1000 long
+    """Cut sequence into subsequences of equal length.
 
     Parameters
     ----------
     arr: np.ndarray
-        uncut sequence
+        Input sequence of shape (N, L, C).
     window_size: int
         size of subsequences
     keep_padded: bool
-        If True, last subsequence (which is padded) is kept in the output array.
+        If True, last subsequence (if not of length `window_size`) is kept in the output array and
+        padded with np.nan.
 
     Returns
     -------
     np.ndarray
+        Output sequence with new window length of shape (N', L', C)
     """
     n, rest = np.divmod(arr.shape[1], window_size)
 
@@ -287,30 +285,23 @@ def cut_into_subsequences(
     idx = 0
     for t in range(0, arr.shape[0]):
         for i in range(0, n):
-            # cut out 1000 ms piece of trial t
+            # get part of the sequence with window_size
             arr_tmp = np.expand_dims(arr[t, i * window_size: (i + 1) * window_size, :], axis=0)
 
-            # concatenate pieces
+            # fill row with respective part of the sequence
             arr_cut[idx, :, :] = arr_tmp
 
             idx = idx + 1
 
         if rest > 0 and keep_padded:
-            # concatenate last one with pad
+            # get end point of last part of the sequence
             start_idx_last_piece = window_size * n
-            len_pad_to_add = window_size - rest
-            # piece to pad:
-            arr_incomplete = np.expand_dims(arr[t, start_idx_last_piece:arr.shape[1], :], axis=0)
-            # padding piece:
-            start_idx_last_piece = window_size * (n - 1)
-            arr_pad = np.expand_dims(
-                arr[t, start_idx_last_piece:start_idx_last_piece + len_pad_to_add, :], axis=0,
-            )
-
-            arr_tmp = np.concatenate((arr_incomplete, arr_pad), axis=1)
-
-            # concatenate last piece of original row t
-            arr_cut[idx, :, :] = arr_tmp
+
+            # get last part of sequence which is shorter than window_size
+            arr_incomplete = arr[t, start_idx_last_piece:, :]
+
+            # insert last part, remaining samples are already np.nan
+            arr_cut[idx, :rest, :] = arr_incomplete
 
             idx = idx + 1
 


Test patch:
diff --git a/tests/test_transformations.py b/tests/test_transformations.py
index ca93a71..19faed9 100644
--- a/tests/test_transformations.py
+++ b/tests/test_transformations.py
@@ -4,9 +4,11 @@ Test all functions in pymovements.transforms.
 import numpy as np
 import pytest
 
+from pymovements.transforms import cut_into_subsequences
 from pymovements.transforms import pix2deg
 from pymovements.transforms import pos2vel
 
+
 n_coords = 100
 screen_px_1d = 100
 screen_cm_1d = 100
@@ -485,3 +487,46 @@ def test_pos2vel_stepped_input_returns(params, expected_value):
 
     lpad, rpad = 1, -1
     assert np.allclose(actual_value[lpad:rpad], expected_value[lpad:rpad])
+
+
+@pytest.mark.parametrize(
+    'params, expected',
+    [
+        pytest.param(
+            {'arr': np.ones((1, 10, 2)), 'window_size': 5, 'keep_padded': False},
+            {'value': np.ones((2, 5, 2))},
+            id='length_double_window_size_keep_padded_returns_two_instances',
+        ),
+        pytest.param(
+            {'arr': np.ones((1, 10, 2)), 'window_size': 5, 'keep_padded': True},
+            {'value': np.ones((2, 5, 2))},
+            id='length_double_window_size_not_keep_padded_returns_two_instances',
+        ),
+        pytest.param(
+            {'arr': np.ones((1, 11, 2)), 'window_size': 5, 'keep_padded': False},
+            {'value': np.ones((2, 5, 2))},
+            id='length_double_window_size+1_keep_padded_returns_two_instances',
+        ),
+        pytest.param(
+            {'arr': np.ones((1, 11, 2)), 'window_size': 5, 'keep_padded': True},
+            {
+                'value': np.concatenate([
+                    np.ones((2, 5, 2)),
+                    np.expand_dims(np.concatenate([np.ones((1, 2)), np.ones((4, 2)) * np.nan]), 0),
+                ]),
+            },
+            id='length_double_window_size+1_not_keep_padded_returns_three_instances',
+        ),
+    ],
+)
+def test_cut_into_subsequences(params, expected):
+    if 'exception' in expected:
+        with pytest.raises(expected['exception']):
+            cut_into_subsequences(**params)
+        return
+
+    arr = cut_into_subsequences(**params)
+
+    assert np.array_equal(arr, expected['value'], equal_nan=True), (
+        f"arr = {arr}, expected = {expected['value']}"
+    )