Repository: Aarhus-Psychiatry-Research/timeseriesflattener

Issue:
fix: change type hints in specs to allow for floats in interval_days
Currenty, float inputs to interval_days args in predictor and outcome specs are coerced into integers. Thus, it is not possible to generate predictors/outcomes with non-integer lookbehind/ahead windows. 

- [ ] Add test 

Discussion:

Patch:
diff --git a/.github/workflows/static_type_checks.yml b/.github/workflows/static_type_checks.yml
index abf1bb5..620427d 100644
--- a/.github/workflows/static_type_checks.yml
+++ b/.github/workflows/static_type_checks.yml
@@ -32,7 +32,7 @@ jobs:
         uses: actions/setup-python@v4
         id: setup_python
         with:
-          python-version: "3.9"
+          python-version: "3.8"
 
       - name: Install dependencies
         shell: bash
diff --git a/pyproject.toml b/pyproject.toml
index 50efc2b..38bd66d 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -194,6 +194,7 @@ commands =
 [testenv:type]
 description: run static type checking
 extras = test, text, dev
+basepython = py38
 use_develop = true
 allowlist_externals = ls
 commands =
diff --git a/src/timeseriesflattener/feature_spec_objects.py b/src/timeseriesflattener/feature_spec_objects.py
index bedcc13..eac7535 100644
--- a/src/timeseriesflattener/feature_spec_objects.py
+++ b/src/timeseriesflattener/feature_spec_objects.py
@@ -356,7 +356,7 @@ class TemporalSpec(_AnySpec):
             or timestamp_col_name.
         output_col_name_override (Optional[str]):
             Override the generated column name after flattening the time series
-        interval_days (Union[int, float, NoneType]):
+        interval_days (Optional[float]):
             How far to look in the given direction (ahead for outcomes,
             behind for predictors)
         resolve_multiple_fn (Union[Callable, str]):
@@ -381,7 +381,7 @@ class TemporalSpec(_AnySpec):
         short_description = """The minimum specification required for collapsing a temporal
         feature, whether looking ahead or behind. Mostly used for inheritance below."""
 
-    interval_days: Optional[Union[int, float]] = Field(
+    interval_days: Optional[float] = Field(
         description="""How far to look in the given direction (ahead for outcomes,
             behind for predictors)""",
     )
@@ -444,6 +444,7 @@ class TemporalSpec(_AnySpec):
 
     def get_col_str(self, additional_feature_name: Optional[str] = None) -> str:
         """Generate the column name for the output column.
+        If interval days is a float, the decimal point is changed to an underscore.
 
         Args:
             additional_feature_name (Optional[str]): additional feature name to
@@ -452,7 +453,14 @@ class TemporalSpec(_AnySpec):
         feature_name = self.feature_name
         if additional_feature_name:
             feature_name = feature_name + "-" + str(additional_feature_name)
-        col_str = f"{self.prefix}_{feature_name}_within_{self.interval_days}_days_{self.key_for_resolve_multiple}_fallback_{self.fallback}"
+
+        interval_days_str = (  # This is required because pydantic coerces the int 2 to float 2.0
+            int(self.interval_days)  # type: ignore
+            if self.interval_days.is_integer()  # type: ignore
+            else str(self.interval_days).replace(".", "-")
+        )
+
+        col_str = f"{self.prefix}_{feature_name}_within_{interval_days_str}_days_{self.key_for_resolve_multiple}_fallback_{self.fallback}"
         return col_str
 
 
@@ -483,7 +491,7 @@ class PredictorSpec(TemporalSpec):
             or timestamp_col_name.
         output_col_name_override (Optional[str]):
             Override the generated column name after flattening the time series
-        interval_days (Union[int, float, NoneType]):
+        interval_days (Optional[float]):
             How far to look in the given direction (ahead for outcomes,
             behind for predictors)
         resolve_multiple_fn (Union[Callable, str]):
@@ -502,7 +510,7 @@ class PredictorSpec(TemporalSpec):
             resolution, raise an error. Defaults to: 0.0.
         entity_id_col_name (str):
             Col name for ids in the input dataframe. Defaults to: entity_id.
-        lookbehind_days (Union[int, float]):
+        lookbehind_days (float):
             How far behind to look for values
     """
 
@@ -517,7 +525,7 @@ class PredictorSpec(TemporalSpec):
             <prefix>_<feature_name>.""",
     )
 
-    lookbehind_days: Union[int, float] = Field(
+    lookbehind_days: float = Field(
         description="""How far behind to look for values""",
     )
 
@@ -561,7 +569,7 @@ class TextPredictorSpec(PredictorSpec):
             or timestamp_col_name.
         output_col_name_override (Optional[str]):
             Override the generated column name after flattening the time series
-        interval_days (Union[int, float, NoneType]):
+        interval_days (Optional[float]):
             How far to look in the given direction (ahead for outcomes,
             behind for predictors)
         resolve_multiple_fn (Union[Callable, str]):
@@ -582,7 +590,7 @@ class TextPredictorSpec(PredictorSpec):
             resolution, raise an error. Defaults to: 0.0.
         entity_id_col_name (str):
             Col name for ids in the input dataframe. Defaults to: entity_id.
-        lookbehind_days (Union[int, float]):
+        lookbehind_days (float):
             How far behind to look for values
         embedding_fn (Callable):
             A function used for embedding the text. Should take a
@@ -653,7 +661,7 @@ class OutcomeSpec(TemporalSpec):
             or timestamp_col_name.
         output_col_name_override (Optional[str]):
             Override the generated column name after flattening the time series
-        interval_days (Union[int, float, NoneType]):
+        interval_days (Optional[float]):
             How far to look in the given direction (ahead for outcomes,
             behind for predictors)
         resolve_multiple_fn (Union[Callable, str]):
@@ -677,7 +685,7 @@ class OutcomeSpec(TemporalSpec):
             I.e., incident outcomes are outcomes you can only experience once.
             For example, type 2 diabetes is incident. Incident outcomes can be handled
             in a vectorised way during resolution, which is faster than non-incident outcomes.
-        lookahead_days (Union[int, float]):
+        lookahead_days (float):
             How far ahead to look for values
     """
 
@@ -699,7 +707,7 @@ class OutcomeSpec(TemporalSpec):
             in a vectorised way during resolution, which is faster than non-incident outcomes.""",
     )
 
-    lookahead_days: Union[int, float] = Field(
+    lookahead_days: float = Field(
         description="""How far ahead to look for values""",
     )
 
@@ -903,7 +911,7 @@ class PredictorGroupSpec(_MinGroupSpec):
             Prefix for column name, e,g, <prefix>_<feature_name>. Defaults to: pred.
         loader_kwargs (Optional[List[Dict[str, Any]]]):
             Optional kwargs for the values_loader.
-        lookbehind_days (List[Union[int, float]]):
+        lookbehind_days (List[float]):
             How far behind to look for values
     """
 
@@ -915,7 +923,7 @@ class PredictorGroupSpec(_MinGroupSpec):
         description="""Prefix for column name, e,g, <prefix>_<feature_name>.""",
     )
 
-    lookbehind_days: List[Union[int, float]] = Field(
+    lookbehind_days: List[float] = Field(
         description="""How far behind to look for values""",
     )
 
@@ -961,7 +969,7 @@ class OutcomeGroupSpec(_MinGroupSpec):
         can experience it more than once. For example, type 2 diabetes is incident.
         Incident outcomes can be handled in a vectorised way during resolution,
          which is faster than non-incident outcomes.
-    lookahead_days (List[Union[int, float]]):
+    lookahead_days (List[float]):
         How far ahead to look for values
     """
 
@@ -980,7 +988,7 @@ class OutcomeGroupSpec(_MinGroupSpec):
              which is faster than non-incident outcomes.""",
     )
 
-    lookahead_days: List[Union[int, float]] = Field(
+    lookahead_days: List[float] = Field(
         description="""How far ahead to look for values""",
     )
 
diff --git a/tasks.py b/tasks.py
index 5cab18b..b9965f4 100644
--- a/tasks.py
+++ b/tasks.py
@@ -20,7 +20,7 @@ import platform
 import re
 from dataclasses import dataclass
 from pathlib import Path
-from typing import Optional
+from typing import List, Optional
 
 from invoke import Context, Result, task
 
@@ -236,24 +236,36 @@ def update(c: Context):
     c.run("pip install --upgrade -e '.[dev,tests,docs]'")
 
 
-@task
-def test(c: Context, run_all_envs: bool = False):
+@task(iterable="pytest_args")
+def test(
+    c: Context,
+    python_versions: str = "3.9",
+    pytest_args: List[str] = [],  # noqa
+):
     """Run tests"""
     echo_header(f"{Emo.TEST} Running tests")
-    pytest_args = "-n auto -rfE --failed-first -p no:cov --disable-warnings -q"
 
-    if not run_all_envs:
-        test_result: Result = c.run(
-            f"tox -e py311 -- {pytest_args}",
-            warn=True,
-            pty=True,
-        )
-    else:
-        test_result = c.run(
-            f"tox -- {pytest_args}",
-            warn=True,
-            pty=True,
-        )
+    if len(pytest_args) == 0:
+        pytest_args = [
+            "-n auto",
+            "-rfE",
+            "--failed-first",
+            "-p no:cov",
+            "--disable-warnings",
+            "-q",
+        ]
+
+    pytest_arg_str = " ".join(pytest_args)
+
+    python_version_list = python_versions.replace(".", "").split(",")
+    python_version_strings = [f"py{v}" for v in python_version_list]
+    python_version_arg_string = ",".join(python_version_strings)
+
+    test_result: Result = c.run(
+        f"tox -e {python_version_arg_string} -- {pytest_arg_str}",
+        warn=True,
+        pty=True,
+    )
 
     # If "failed" in the pytest results
     if "failed" in test_result.stdout:
@@ -302,7 +314,7 @@ def pr(c: Context, auto_fix: bool = False):
     """Run all checks and update the PR."""
     add_and_commit(c)
     lint(c, auto_fix=auto_fix)
-    test(c, run_all_envs=True)
+    test(c, python_versions="3.8,3.11")
     update_branch(c)
     update_pr(c)
 


Test patch:
diff --git a/.github/actions/test/action.yml b/.github/actions/test/action.yml
index 9567555..abe4801 100644
--- a/.github/actions/test/action.yml
+++ b/.github/actions/test/action.yml
@@ -10,32 +10,26 @@ runs:
   using: "composite"
   steps:
     ### Setup prerequisites
-    - name: Cache venv
+    - name: Cache tox
       uses: actions/cache@v3.2.6
-      id: cache_venv
+      id: cache_tox
       with:
         path: |
-          .venv
-        key: ${{ runner.os }}-venv-${{ matrix.python-version }}-${{ hashFiles('**/pyproject.toml') }}
+          .tox
+        key: ${{ runner.os }}-${{ matrix.python-version }}
 
     - name: Set up Python
       uses: actions/setup-python@v4
-      if: steps.cache_venv.outputs.cache-hit != 'true'
+      if: steps.cache_tox.outputs.cache-hit != 'true'
       with:
         python-version: ${{ inputs.python-version }}
-        cache: "pip"
-        cache-dependency-path: "**/pyproject.toml"
 
     - name: Install dependencies
       shell: bash
       run: |
-        python -m pip install --upgrade pip
-        pip install -e ".[dev,test,text]"
+        pip install invoke tox
 
-    ### Run tests
-    - name: Run pytest
+    - name: Run tests
       shell: bash
       run: |
-        set -o pipefail
-        ls -la
-        pytest --skiphuggingface --cov=src --cov-report term-missing --color=yes
+        inv test --python-versions=${{ matrix.python-version }} --pytest-args "-n auto"  --pytest-args "--skiphuggingface" --pytest-args "--cov=src" --pytest-args "--cov-report term-missing" --pytest-args="--color=yes"
diff --git a/tests/test_timeseriesflattener/test_feature_spec_objects.py b/tests/test_timeseriesflattener/test_feature_spec_objects.py
index 8f578f3..91ca3a5 100644
--- a/tests/test_timeseriesflattener/test_feature_spec_objects.py
+++ b/tests/test_timeseriesflattener/test_feature_spec_objects.py
@@ -145,6 +145,18 @@ def test_resolve_multiple_fn_to_str():
     assert "maximum" in pred_spec_batch[0].get_col_str()
 
 
+def test_lookbehind_days_handles_floats():
+    """Test that lookbheind days does not coerce floats into ints."""
+    pred_spec_batch = PredictorGroupSpec(
+        values_loader=["synth_predictor_float"],
+        lookbehind_days=[2, 0.5],
+        fallback=[np.nan],
+        resolve_multiple_fn=[maximum],
+    ).create_combinations()
+
+    assert pred_spec_batch[1].lookbehind_days == 0.5
+
+
 def get_lines_with_diff(text1: str, text2: str) -> List[str]:
     """Find all lines in text1 which are different from text2."""
     # Remove whitespace and periods