Repository: aeye-lab/pymovements
Pull request #314 (2023-04-06 14:06:13)

Events: Fix non-terminating bug in I-DT
## Description

Fixes issue #284
Additionally this fixes a bug where the function did not terminate correctly.

## Implemented changes

- [x] pass `Series` instead of `DataFrame` as `timesteps` in `Dataset.detect_events()`
- [x] flatten `timesteps` in `idt()`
- [x] add check that timesteps must be of type int or not have any fraction
- [x] require `minimum_duration` to result in a `minimum_sample_duration` of at least 2. (window of size 1 will always have `dispersion == 0`) 

## Type of change

- [x] Bug fix (non-breaking change which fixes an issue)

## How Has This Been Tested?

- [x] test that stacked timesteps are flattened correctly
- [x] test that float timesteps without fractions pass

## 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



Patch:
diff --git a/src/pymovements/dataset/dataset.py b/src/pymovements/dataset/dataset.py
index 417c5cb..3c97954 100644
--- a/src/pymovements/dataset/dataset.py
+++ b/src/pymovements/dataset/dataset.py
@@ -375,7 +375,7 @@ class Dataset:
 
             positions = gaze_df.frame.select(position_columns).to_numpy()
             velocities = gaze_df.frame.select(velocity_columns).to_numpy()
-            timesteps = gaze_df.frame.select('time').to_numpy()
+            timesteps = gaze_df.frame.get_column('time').to_numpy()
 
             event_df = method(
                 positions=positions, velocities=velocities, timesteps=timesteps, **kwargs,
diff --git a/src/pymovements/events/detection/idt.py b/src/pymovements/events/detection/idt.py
index 4a98822..057cd81 100644
--- a/src/pymovements/events/detection/idt.py
+++ b/src/pymovements/events/detection/idt.py
@@ -94,6 +94,8 @@ def idt(
 
     Raises
     ------
+    TypeError
+        If minimum_duration is not of type ``int`` or timesteps
     ValueError
         If positions is not shaped (N, 2)
         If dispersion_threshold is not greater than 0
@@ -105,13 +107,25 @@ def idt(
 
     if timesteps is None:
         timesteps = np.arange(len(velocities), dtype=np.int64)
-    timesteps = np.array(timesteps)
+    timesteps = np.array(timesteps).flatten()
+
+    # Check that timesteps are integers or are floats without a fractional part.
+    timesteps_int = timesteps.astype(int)
+    if np.any((timesteps - timesteps_int) != 0):
+        raise TypeError('timesteps must be of type int')
+    timesteps = timesteps_int
+
     checks.check_is_length_matching(velocities=velocities, timesteps=timesteps)
 
     if dispersion_threshold <= 0:
-        raise ValueError('dispersion threshold must be greater than 0')
+        raise ValueError('dispersion_threshold must be greater than 0')
     if minimum_duration <= 0:
-        raise ValueError('minimum duration must be greater than 0')
+        raise ValueError('minimum_duration must be greater than 0')
+    if not isinstance(minimum_duration, int):
+        raise TypeError(
+            'minimum_duration must be of type int'
+            f' but is of type {type(minimum_duration)}',
+        )
 
     onsets = []
     offsets = []
@@ -127,27 +141,32 @@ def idt(
         raise ValueError(
             'minimum_duration must be divisible by the constant interval between timesteps',
         )
-    minimum_sample_duration = minimum_duration // timesteps_diff[0]
+    minimum_sample_duration = int(minimum_duration // timesteps_diff[0])
+    if minimum_sample_duration < 2:
+        raise ValueError('minimum_duration must be longer than the equivalent of 2 samples')
 
     # Initialize window over first points to cover the duration threshold
     win_start = 0
     win_end = minimum_sample_duration
 
-    while win_end < len(positions):
+    while win_start < len(timesteps) and win_end <= len(timesteps):
 
         # Initialize window over first points to cover the duration threshold.
         # This automatically extends the window to the specified minimum event duration.
         win_end = max(win_start + minimum_sample_duration, win_end)
+        win_end = min(win_end, len(timesteps))
+        if win_end - win_start < minimum_sample_duration:
+            break
 
         if dispersion(positions[win_start:win_end]) <= dispersion_threshold:
             # Add additional points to the window until dispersion > threshold.
             while dispersion(positions[win_start:win_end]) < dispersion_threshold:
-                win_end += 1
-
                 # break if we reach end of input data
-                if win_end == len(positions):
+                if win_end == len(timesteps):
                     break
 
+                win_end += 1
+
             # check for np.nan values
             if np.sum(np.isnan(positions[win_start:win_end - 1])) > 0:
                 tmp_candidates = [np.arange(win_start, win_end - 1, 1)]


Test patch:
diff --git a/tests/events/detection/idt_test.py b/tests/events/detection/idt_test.py
index 7acee35..1c63d78 100644
--- a/tests/events/detection/idt_test.py
+++ b/tests/events/detection/idt_test.py
@@ -22,10 +22,7 @@ import numpy as np
 import pytest
 from polars.testing import assert_frame_equal
 
-from pymovements.events.detection.idt import idt
-from pymovements.events.events import EventDataFrame
-from pymovements.gaze.transforms import pos2vel
-from pymovements.synthetic import step_function
+import pymovements as pm
 
 
 @pytest.mark.parametrize(
@@ -116,7 +113,7 @@ from pymovements.synthetic import step_function
                 'positions': [[1, 2], [1, 2]],
                 'velocities': [[1, 2], [1, 2]],
                 'dispersion_threshold': 1,
-                'minimum_duration': 1.0,
+                'minimum_duration': 1.1,
             },
             TypeError,
             id='duration_threshold_not_integer_raises_type_error',
@@ -126,7 +123,7 @@ from pymovements.synthetic import step_function
 def test_idt_raises_error(kwargs, expected_error):
     """Test if idt raises expected error."""
     with pytest.raises(expected_error):
-        idt(**kwargs)
+        pm.events.idt(**kwargs)
 
 
 @pytest.mark.parametrize(
@@ -138,16 +135,16 @@ def test_idt_raises_error(kwargs, expected_error):
                 'dispersion_threshold': 1,
                 'minimum_duration': 10,
             },
-            EventDataFrame(),
+            pm.events.EventDataFrame(),
             id='constant_velocity_no_fixation',
         ),
         pytest.param(
             {
-                'positions': step_function(length=100, steps=[0], values=[(0, 0)]),
+                'positions': pm.synthetic.step_function(length=100, steps=[0], values=[(0, 0)]),
                 'dispersion_threshold': 1,
-                'minimum_duration': 1,
+                'minimum_duration': 2,
             },
-            EventDataFrame(
+            pm.events.EventDataFrame(
                 name='fixation',
                 onsets=[0],
                 offsets=[99],
@@ -156,16 +153,16 @@ def test_idt_raises_error(kwargs, expected_error):
         ),
         pytest.param(
             {
-                'positions': step_function(
+                'positions': pm.synthetic.step_function(
                     length=100,
                     steps=[49, 50],
                     values=[(9, 9), (1, 1)],
                     start_value=(0, 0),
                 ),
                 'dispersion_threshold': 1,
-                'minimum_duration': 1,
+                'minimum_duration': 2,
             },
-            EventDataFrame(
+            pm.events.EventDataFrame(
                 name='fixation',
                 onsets=[0, 50],
                 offsets=[49, 99],
@@ -174,7 +171,7 @@ def test_idt_raises_error(kwargs, expected_error):
         ),
         pytest.param(
             {
-                'positions': step_function(
+                'positions': pm.synthetic.step_function(
                     length=100, steps=[10, 20, 90],
                     values=[
                         (np.nan, np.nan), (0, 0),
@@ -182,9 +179,9 @@ def test_idt_raises_error(kwargs, expected_error):
                     ],
                 ),
                 'dispersion_threshold': 1,
-                'minimum_duration': 1,
+                'minimum_duration': 2,
             },
-            EventDataFrame(
+            pm.events.EventDataFrame(
                 name='fixation',
                 onsets=[0, 20],
                 offsets=[9, 89],
@@ -193,7 +190,7 @@ def test_idt_raises_error(kwargs, expected_error):
         ),
         pytest.param(
             {
-                'positions': step_function(
+                'positions': pm.synthetic.step_function(
                     length=100, steps=[10, 20, 90],
                     values=[
                         (np.nan, np.nan), (0, 0),
@@ -201,10 +198,10 @@ def test_idt_raises_error(kwargs, expected_error):
                     ],
                 ),
                 'dispersion_threshold': 1,
-                'minimum_duration': 1,
+                'minimum_duration': 2,
                 'include_nan': True,
             },
-            EventDataFrame(
+            pm.events.EventDataFrame(
                 name='fixation',
                 onsets=[0],
                 offsets=[89],
@@ -213,24 +210,54 @@ def test_idt_raises_error(kwargs, expected_error):
         ),
         pytest.param(
             {
-                'positions': step_function(length=100, steps=[0], values=[(0, 0)]),
+                'positions': pm.synthetic.step_function(length=100, steps=[0], values=[(0, 0)]),
                 'timesteps': np.arange(1000, 1100, dtype=int),
                 'dispersion_threshold': 1,
-                'minimum_duration': 1,
+                'minimum_duration': 2,
             },
-            EventDataFrame(
+            pm.events.EventDataFrame(
                 name='fixation',
                 onsets=[1000],
                 offsets=[1099],
             ),
             id='constant_position_single_fixation_with_timesteps',
         ),
+        pytest.param(
+            {
+                'positions': pm.synthetic.step_function(length=100, steps=[0], values=[(0, 0)]),
+                'timesteps': np.reshape(np.arange(1000, 1100, dtype=int), (100, 1)),
+                'dispersion_threshold': 1,
+                'minimum_duration': 2,
+            },
+            pm.events.EventDataFrame(
+                name='fixation',
+                onsets=[1000],
+                offsets=[1099],
+            ),
+            id='constant_position_single_fixation_with_timesteps_extra_dim',
+        ),
+        pytest.param(
+            {
+                'positions': pm.synthetic.step_function(length=100, steps=[0], values=[(0, 0)]),
+                'timesteps': np.arange(1000, 1100, dtype=float),
+                'dispersion_threshold': 1,
+                'minimum_duration': 2,
+            },
+            pm.events.EventDataFrame(
+                name='fixation',
+                onsets=[1000],
+                offsets=[1099],
+            ),
+            id='constant_position_single_fixation_with_timesteps_float',
+        ),
     ],
 )
 def test_idt_detects_fixations(kwargs, expected):
     """Test if idt detects fixations."""
-    velocities = pos2vel(kwargs['positions'], sampling_rate=10, method='preceding')
-    events = idt(velocities=velocities, **kwargs)
+    velocities = pm.gaze.transforms.pos2vel(
+        kwargs['positions'], sampling_rate=10, method='preceding',
+    )
+    events = pm.events.idt(velocities=velocities, **kwargs)
 
     assert_frame_equal(events.frame, expected.frame)
 
@@ -240,7 +267,7 @@ def test_idt_detects_fixations(kwargs, expected):
     [
         pytest.param(
             {
-                'positions': step_function(length=10, steps=[0], values=[(0, 0)]),
+                'positions': pm.synthetic.step_function(length=10, steps=[0], values=[(0, 0)]),
                 'timesteps': np.concatenate([
                     np.arange(0, 5, dtype=int), np.arange(7, 12, dtype=int),
                 ]),
@@ -252,7 +279,7 @@ def test_idt_detects_fixations(kwargs, expected):
         ),
         pytest.param(
             {
-                'positions': step_function(length=10, steps=[0], values=[(0, 0)]),
+                'positions': pm.synthetic.step_function(length=10, steps=[0], values=[(0, 0)]),
                 'timesteps': np.arange(0, 30, step=3, dtype=int),
                 'dispersion_threshold': 1,
                 'minimum_duration': 2,
@@ -260,12 +287,33 @@ def test_idt_detects_fixations(kwargs, expected):
             ValueError, ('interval', 'timesteps', 'divisible', 'minimum_duration'),
             id='minimum_duration_not_divisible_by_timesteps_interval',
         ),
+        pytest.param(
+            {
+                'positions': pm.synthetic.step_function(length=100, steps=[0], values=[(0, 0)]),
+                'timesteps': np.linspace(0, 1, 100),
+                'dispersion_threshold': 1,
+                'minimum_duration': 1,
+            },
+            TypeError, ('timesteps', 'int'),
+            id='constant_position_single_fixation_with_timesteps_float_with_fractions',
+        ),
+        pytest.param(
+            {
+                'positions': pm.synthetic.step_function(length=100, steps=[0], values=[(0, 0)]),
+                'dispersion_threshold': 1,
+                'minimum_duration': 1,
+            },
+            ValueError, ('minimum_duration', '2'),
+            id='minimum_duration_1_sample',
+        ),
     ],
 )
 def test_idt_timesteps_exceptions(kwargs, exception, msg_substrings):
-    velocities = pos2vel(kwargs['positions'], sampling_rate=10, method='preceding')
+    velocities = pm.gaze.transforms.pos2vel(
+        kwargs['positions'], sampling_rate=10, method='preceding',
+    )
     with pytest.raises(exception) as excinfo:
-        idt(velocities=velocities, **kwargs)
+        pm.events.idt(velocities=velocities, **kwargs)
 
     msg, = excinfo.value.args
     for msg_substring in msg_substrings: