Repository: sktime/sktime
Pull request #7053 (2024-08-29 23:08:32)

[BUG] fix `ExpandingCutoffSplitter` for case where `fh` is not continuous
<!--
Welcome to sktime, and thanks for contributing!
Please have a look at our contribution guide:
https://www.sktime.net/en/latest/get_involved/contributing.html
-->

#### Reference Issues/PRs
<!--
Example: Fixes #1234. See also #3456.

Please use keywords (e.g., Fixes) to create link to the issues or pull requests
you resolved, so that they will automatically be closed when your pull request
is merged. See https://github.com/blog/1506-closing-issues-via-pull-requests.
If no issue exists, you can open one here: https://github.com/sktime/sktime/issues
-->


#### What does this implement/fix? Explain your changes.
<!--
A clear and concise description of what you have implemented.
-->
Fixes bug where existing ExpandingCutoffSplitter assumed `fh` was continuous. 

See: https://github.com/sktime/sktime/blob/58615b694cc2928fbbc7fd5e82bdf0adb337d1d1/sktime/split/expandingcutoff.py#L127




#### Does your contribution introduce a new dependency? If yes, which one?

<!--
Only relevant if you changed pyproject.toml.
We try to minimize dependencies in the core dependency set. There
are also further specific instructions to follow for soft dependencies.
See here for handling dependencies in sktime: https://www.sktime.net/en/latest/developer_guide/dependencies.html
-->

#### What should a reviewer concentrate their feedback on?

<!-- This section is particularly useful if you have a pull request that is still in development. You can guide the reviews to focus on the parts that are ready for their comments. We suggest using bullets (indicated by * or -) and filled checkboxes [x] here -->

#### Did you add any tests for the change?
Yes. See the following pytest added to `sktime/split/tests/test_expandingcutoff.py`:
```python3
def test_expandingcutoff_fh():
    """Test different forecast horizon configurations"""
    # Datetime cutoff
    y = _make_series(n_timepoints=10, index_type="period", random_state=42)
    fh_configs = ([1], [1, 2, 3], [4], [2, 4])
    for _fh in fh_configs:
        fh = ForecastingHorizon(_fh)
        index_num = 3
        cutoff = y.index[index_num]

        # Behavior of cutoff and window splitter should be identical in this scenario
        cv1 = ExpandingCutoffSplitter(cutoff=cutoff, fh=fh, step_length=1)
        cv2 = ExpandingWindowSplitter(initial_window=index_num, fh=fh, step_length=1)

        splits1 = list(cv1.split(y))
        splits2 = list(cv2.split(y))

        for (split_train_a, split_test_a), (split_train_b, split_test_b) in zip(
            splits1, splits2
        ):
            np.testing.assert_array_equal(split_train_a, split_train_b)
            np.testing.assert_array_equal(split_test_a, split_test_b)
```
The existing `ExpandingCutoffSplitter` fails this test, as it only accounts for continuous forecast horizons that start at 1.

<!-- This section is useful if you have added a test in addition to the existing ones. This will ensure that further changes to these files won't introduce the same kind of bug. It is considered good practice to add tests with newly added code to enforce the fact that the code actually works. This will reduce the chance of introducing logical bugs.
-->

#### Any other comments?
<!--
We value all user contributions, no matter how small or complex they are. If you have any questions, feel free to post
in the dev-chat channel on the sktime discord https://discord.com/invite/54ACzaFsn7. If we are slow to review (>3 working days), likewise feel free to ping us on discord. Thank you for your understanding during the review process.
-->

#### PR checklist
<!--
Please go through the checklist below. Please feel free to remove points if they are not applicable.
-->

##### For all contributions
- [ ] I've added myself to the [list of contributors](https://github.com/sktime/sktime/blob/main/CONTRIBUTORS.md) with any new badges I've earned :-)
  How to: add yourself to the [all-contributors file](https://github.com/sktime/sktime/blob/main/.all-contributorsrc) in the `sktime` root directory (not the `CONTRIBUTORS.md`). Common badges: `code` - fixing a bug, or adding code logic. `doc` - writing or improving documentation or docstrings. `bug` - reporting or diagnosing a bug (get this plus `code` if you also fixed the bug in the PR).`maintenance` - CI, test framework, release.
  See here for [full badge reference](https://allcontributors.org/docs/en/emoji-key)
- [ ] Optionally, for added estimators: I've added myself and possibly to the `maintainers` tag - do this if you want to become the owner or maintainer of an estimator you added.
  See here for further details on the [algorithm maintainer role](https://www.sktime.net/en/latest/get_involved/governance.html#algorithm-maintainers).
- [ ] The PR title starts with either [ENH], [MNT], [DOC], or [BUG]. [BUG] - bugfix, [MNT] - CI, test framework, [ENH] - adding or improving code, [DOC] - writing or improving documentation or docstrings.

##### For new estimators
- [ ] I've added the estimator to the API reference - in `docs/source/api_reference/taskname.rst`, follow the pattern.
- [ ] I've added one or more illustrative usage examples to the docstring, in a pydocstyle compliant `Examples` section.
- [ ] If the estimator relies on a soft dependency, I've set the `python_dependencies` tag and ensured
  dependency isolation, see the [estimator dependencies guide](https://www.sktime.net/en/latest/developer_guide/dependencies.html#adding-a-soft-dependency).

<!--
Thanks for contributing!
-->


ninedigits: > Though, it looks like it accidentally reverts your fixes to the docstring formatting...

This was actually intentional. Looks like it didn't render even with the triple back ticks. 
https://www.sktime.net/en/latest/api_reference/auto_generated/sktime.split.ExpandingCutoffSplitter.html

What I did instead was copy the format of ExandingWindowSplitter.

https://github.com/sktime/sktime/blob/4ec47811a42c24e2d6b0020944d8f2524871b704/sktime/split/expandingwindow.py#L37-L46
ninedigits: > I see - this is removing the "c" indicator though, somehow?

It is, I wasn't sure if that was creating more problems that it was solving. You think we should keep it? I suppose it's useful to point out visually where the cutoff value is. 
fkiraly: yes, I think it is helpful to the reader - I hope we can restore it somehow, and also manage to display this properly...
ninedigits: Added it back in. Is there a way to tell if it will render before committing?
fkiraly: > Added it back in. Is there a way to tell if it will render before committing?

Yes, you can see a full build of the docs if you go on "details" of the docs job:

![image](https://github.com/user-attachments/assets/26addbf0-c04c-47f3-9ca3-f2c38077a387)


Patch:
diff --git a/sktime/split/expandingcutoff.py b/sktime/split/expandingcutoff.py
index d9a01c225..741616f0c 100644
--- a/sktime/split/expandingcutoff.py
+++ b/sktime/split/expandingcutoff.py
@@ -54,14 +54,18 @@ class ExpandingCutoffSplitter(BaseSplitter):
     functions as an iloc indexer. When an int index is paired with a positive int
     cutoff, the cutoff serves as a loc indexer. If the int cutoff is negative, it
     functions as an iloc indexer.
+
+    For example for ``cutoff = 10``, ``step_length = 1`` and ``fh = [1, 2, 3, 4, 5, 6]``
+    here is a representation of the folds:
+
     ```
                           c
-                          |
     |---------------------|----fh----|------|
     | * * * * * * * * * * x x x x x x - - - |
     | * * * * * * * * * * * x x x x x x - - |
     | * * * * * * * * * * * * x x x x x x - |
     | * * * * * * * * * * * * * x x x x x x |
+
     ```
 
     ``c`` = cutoff date or index.
@@ -121,10 +125,9 @@ class ExpandingCutoffSplitter(BaseSplitter):
         """
         if fh is None:
             fh = self._fh
-        offset = fh.to_numpy().max() + 1
         for cutoff in self.get_cutoffs(y):
             train_window = np.arange(0, cutoff + 1, step=1)
-            test_window = np.arange(cutoff + 1, cutoff + offset, step=1)
+            test_window = cutoff + fh
             yield train_window, test_window
 
     def _validate_y(self, y):


Test patch:
diff --git a/sktime/split/tests/test_expandingcutoff.py b/sktime/split/tests/test_expandingcutoff.py
index 9d0261281..4e320dd9e 100644
--- a/sktime/split/tests/test_expandingcutoff.py
+++ b/sktime/split/tests/test_expandingcutoff.py
@@ -6,7 +6,7 @@ import pandas as pd
 import pytest
 
 from sktime.forecasting.base import ForecastingHorizon
-from sktime.split import ExpandingCutoffSplitter
+from sktime.split import ExpandingCutoffSplitter, ExpandingWindowSplitter
 from sktime.split.tests.test_split import _check_cv
 from sktime.tests.test_switch import run_test_for_class
 from sktime.utils._testing.hierarchical import _make_hierarchical
@@ -256,3 +256,31 @@ def test_expanding_cutoff_docstring_examples():
         == _make_splits_listlike(splits2)
         == _make_splits_listlike(splits3)
     )
+
+
+@pytest.mark.skipif(
+    not run_test_for_class(ExpandingCutoffSplitter),
+    reason="run test only if softdeps are present and incrementally (if requested)",
+)
+def test_expandingcutoff_fh():
+    """Test different forecast horizon configurations"""
+    # Datetime cutoff
+    y = _make_series(n_timepoints=10, index_type="period", random_state=42)
+    fh_configs = ([1], [1, 2, 3], [4], [2, 4])
+    for _fh in fh_configs:
+        fh = ForecastingHorizon(_fh)
+        index_num = 3
+        cutoff = y.index[index_num]
+
+        # Behavior of cutoff and window splitter should be identical in this scenario
+        cv1 = ExpandingCutoffSplitter(cutoff=cutoff, fh=fh, step_length=1)
+        cv2 = ExpandingWindowSplitter(initial_window=index_num, fh=fh, step_length=1)
+
+        splits1 = list(cv1.split(y))
+        splits2 = list(cv2.split(y))
+
+        for (split_train_a, split_test_a), (split_train_b, split_test_b) in zip(
+            splits1, splits2
+        ):
+            np.testing.assert_array_equal(split_train_a, split_train_b)
+            np.testing.assert_array_equal(split_test_a, split_test_b)