Repository: sktime/sktime
Pull request: [ENH] forecasting tuners, `return_n_best_forecasters=-1` to return performances of all forecasters
<!--
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
-->
Fixes #6027

#### What does this implement/fix? Explain your changes.
<!--
A clear and concise description of what you have implemented.
-->
If user input `return_n_best_forecasters=-1` the function should add all forecasters to `n_best_forecasters_` list
#### Does your contribution introduce a new dependency? If yes, which one?
No
<!--
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?
* I didn't make any error handle `return_n_best_forecasters=-2`

<!-- 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. I added `test_return_n_best_forecasters`
<!-- 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, I've added myself and possibly others to the [CODEOWNERS](https://github.com/sktime/sktime/blob/main/CODEOWNERS) file - 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!
-->


Problem statement:
[ENH] Allow `return_n_best_forecasters` in  `ForecastingGridSearchCV ` to be -1 (select all forecasters)
**Is your feature request related to a problem? Please describe.**
I would like the class `ForecastingGridSearchCV ` to have a possibility to return ALL forecasters. Right now we have the `return_n_best_forecasters ` argument in the API which lets you choose how many forecasters you want to return

**Describe the solution you'd like**
I would like to pass -1 to `return_n_best_forecasters ` and automatically get back all the forecasters fit.

This is just an idea that i think would be helpful.

Hints from issue discussion:
sounds like a good idea to me - we should do it in all the other forecasting tuners too, and not forget to update docstrings.

Marking this a good first issue since I think it's a nicely defined entry contribution.
Can I try to work on this?

I'm new here, so I don't know how assignments work here, or can anyone work on any issues?
> I'm new here, so I don't know how assignments work here

Semi-formal - you can consider yourself assigned if you post on the issue and a core dev comments.

We track assignments more strictly (kanban board, assigned developers) only for intensive workstreams, but these last typically only through the summer period, which starts with GSoC and internships.

So: all yours!
PS: you can even make a contribution or PR without announcement if you want, of course it is "safer" to open an issue and describe the proposed work before, to avoid things like duplication or design incompatibilities.

Where this concerns contribution of estimators, sktime has a very open "library contribution" policy https://www.sktime.net/en/stable/get_involved/governance.html#algorithm-inclusion-guidelines
this is streamlined as long as you adhere to the extension templates and documentation requirements.

Commit message: Add testing for `ForecastingRandomizedSearchCV`

Patch to review:
diff --git a/sktime/forecasting/model_selection/_tune.py b/sktime/forecasting/model_selection/_tune.py
index 3b73b9454ba..71555226339 100644
--- a/sktime/forecasting/model_selection/_tune.py
+++ b/sktime/forecasting/model_selection/_tune.py
@@ -267,7 +267,10 @@ def evaluate_candidates(candidate_params):
         # Select n best forecaster
         self.n_best_forecasters_ = []
         self.n_best_scores_ = []
-        for i in range(self.return_n_best_forecasters):
+        _forecasters_to_return = self.return_n_best_forecasters
+        if _forecasters_to_return == -1:
+            _forecasters_to_return = len(results.index)
+        for i in range(_forecasters_to_return):
             params = results["params"].iloc[i]
             rank = results[f"rank_{scoring_name}"].iloc[i]
             rank = str(int(rank))
@@ -445,7 +448,9 @@ class ForecastingGridSearchCV(BaseGridSearch):
     verbose: int, optional (default=0)
     return_n_best_forecasters : int, default=1
         In case the n best forecaster should be returned, this value can be set
-        and the n best forecasters will be assigned to n_best_forecasters_
+        and the n best forecasters will be assigned to n_best_forecasters_.
+        Set return_n_best_forecasters to -1 to return all forecasters.
+
     error_score : numeric value or the str 'raise', optional (default=np.nan)
         The test score returned when a forecaster fails to be fitted.
     return_train_score : bool, optional (default=False)
@@ -773,7 +778,9 @@ class ForecastingRandomizedSearchCV(BaseGridSearch):
     verbose : int, optional (default=0)
     return_n_best_forecasters: int, default=1
         In case the n best forecaster should be returned, this value can be set
-        and the n best forecasters will be assigned to n_best_forecasters_
+        and the n best forecasters will be assigned to n_best_forecasters_.
+        Set return_n_best_forecasters to -1 to return all forecasters.
+
     random_state : int, RandomState instance or None, default=None
         Pseudo random number generator state used for random uniform sampling
         from lists of possible values instead of scipy.stats distributions.
@@ -1037,7 +1044,8 @@ class ForecastingSkoptSearchCV(BaseGridSearch):
         FitFailedWarning is raised.
     return_n_best_forecasters: int, default=1
         In case the n best forecaster should be returned, this value can be set
-        and the n best forecasters will be assigned to n_best_forecasters_
+        and the n best forecasters will be assigned to n_best_forecasters_.
+        Set return_n_best_forecasters to -1 to return all forecasters.
 
     backend : {"dask", "loky", "multiprocessing", "threading"}, by default "loky".
         Runs parallel evaluate if specified and `strategy` is set as "refit".
diff --git a/sktime/forecasting/model_selection/tests/test_tune.py b/sktime/forecasting/model_selection/tests/test_tune.py
index 7c1d604e21d..c7c74ddbacd 100644
--- a/sktime/forecasting/model_selection/tests/test_tune.py
+++ b/sktime/forecasting/model_selection/tests/test_tune.py
@@ -4,6 +4,9 @@
 
 __author__ = ["mloning", "fkiraly"]
 
+
+from functools import reduce
+
 import numpy as np
 import pytest
 from sklearn.model_selection import ParameterGrid, ParameterSampler
@@ -348,3 +351,87 @@ def test_gscv_backends(backend_set):
         backend_params=backend_params,
     )
     gscv.fit(y, X)
+
+
+def test_BaseGridSearch(
+    Forecaster,
+    *,
+    as_distribution=False,
+    **kwargs,
+):
+    if "param_distributions" not in kwargs and "param_grid" not in kwargs:
+        params = (
+            [
+                {
+                    "window_length": [1, 2, 3],
+                    "strategy": ["last", "mean"],
+                    "transformer__degree": [1, 2, 3],
+                    "forecaster__strategy": ["last", "mean", "seasonal_last"],
+                },
+                {
+                    "window_length": [4, 5, 6],
+                    "forecaster__strategy": ["last", "mean"],
+                },
+            ],
+        )
+        kwargs.setdefault(
+            "param_distributions" if as_distribution else "param_grid", params
+        )
+    return Forecaster, {
+        **kwargs,
+    }
+
+
+def test_ForecastingGridSearchCV(**kwargs):
+    return test_BaseGridSearch(ForecastingGridSearchCV, **kwargs)
+
+
+def test_ForecastingRandomizedSearchCV(**kwargs):
+    return test_BaseGridSearch(
+        ForecastingRandomizedSearchCV, as_distribution=True, **kwargs
+    )
+
+
+def test_ForecastingSkoptSearchCV(**kwargs):
+    return test_BaseGridSearch(ForecastingSkoptSearchCV, as_distribution=True, **kwargs)
+
+
+@pytest.mark.parametrize("return_n_best_forecasters", [-1, 0, 1, 2])
+@pytest.mark.parametrize(
+    "Forecaster, kwargs",
+    [
+        test_ForecastingGridSearchCV(),
+        test_ForecastingRandomizedSearchCV(),
+        test_ForecastingRandomizedSearchCV(n_iter=3),
+        test_ForecastingRandomizedSearchCV(n_iter=5),
+        # test_ForecastingSkoptSearchCV(n_iter=5),  # Not working
+    ],
+)
+def test_return_n_best_forecasters(Forecaster, return_n_best_forecasters, kwargs):
+    y, X = load_longley()
+    gscv = Forecaster(
+        **kwargs,
+        forecaster=PIPE,
+        cv=CVs[0],
+        return_n_best_forecasters=return_n_best_forecasters,
+    )
+    gscv.fit(y, X)
+    if return_n_best_forecasters == -1:
+
+        def calculate_total_combinations(param_grid):
+            if isinstance(param_grid, dict):
+                return reduce(lambda x, y: x * y, [len(x) for x in param_grid.values()])
+            elif isinstance(param_grid, list):
+                return sum(calculate_total_combinations(i) for i in param_grid)
+            else:
+                error_message = "`param_grid` must be a dict or a list[dict]"
+                raise ValueError(error_message)
+
+        if "param_grid" in kwargs:
+            total_combinations = calculate_total_combinations(kwargs["param_grid"])
+            assert len(gscv.n_best_forecasters_) == total_combinations
+        else:
+            assert hasattr(gscv, "n_iter")
+            assert len(gscv.n_best_forecasters_) == gscv.n_iter
+    else:
+        assert len(gscv.n_best_forecasters_) == return_n_best_forecasters


Code review comments:
diff_hunk: @@ -353,35 +353,72 @@ def test_gscv_backends(backend_set):
     gscv.fit(y, X)
 
 
+def test_BaseGridSearch(
path: sktime/forecasting/model_selection/tests/test_tune.py
text: @user1:
I don't know if it's just me, but this seems a bit strange way to set up the different instances for testing. Won't setting up 3 fixtures, and choose between them, or choose a common parameter grid and passing that by position and then initiate selected class instance be simpler? 

(This is completely subjective, so non-blocking)

@author:
You are right. I was trying not to put anything in the gloabal namespace just in case any other module imported this one, but I ended up overengineering it.

diff_hunk: @@ -353,35 +353,72 @@ def test_gscv_backends(backend_set):
     gscv.fit(y, X)
 
 
+def test_BaseGridSearch(
+    Forecaster,
+    *,
+    as_distribution=False,
+    **kwargs,
+):
+    if "param_distributions" not in kwargs and "param_grid" not in kwargs:
+        params = (
+            [
+                {
+                    "window_length": [1, 2, 3],
+                    "strategy": ["last", "mean"],
+                    "transformer__degree": [1, 2, 3],
+                    "forecaster__strategy": ["last", "mean", "seasonal_last"],
+                },
+                {
+                    "window_length": [4, 5, 6],
+                    "forecaster__strategy": ["last", "mean"],
+                },
+            ],
+        )
+        kwargs.setdefault(
+            "param_distributions" if as_distribution else "param_grid", params
+        )
+    return Forecaster, {
+        **kwargs,
+    }
+
+
+def test_ForecastingGridSearchCV(**kwargs):
+    return test_BaseGridSearch(ForecastingGridSearchCV, **kwargs)
+
+
+def test_ForecastingRandomizedSearchCV(**kwargs):
+    return test_BaseGridSearch(
+        ForecastingRandomizedSearchCV, as_distribution=True, **kwargs
+    )
+
+
+def test_ForecastingSkoptSearchCV(**kwargs):
+    return test_BaseGridSearch(ForecastingSkoptSearchCV, as_distribution=True, **kwargs)
+
+
 @pytest.mark.parametrize("return_n_best_forecasters", [-1, 0, 1, 2])
 @pytest.mark.parametrize(
-    "param_grid",
+    "Forecaster, kwargs",
     [
-        PIPE_GRID,
-        [
-            {
-                "forecaster__strategy": ["last", "mean"],
-                "transformer__forecaster__degree": [3, 4],
-            },
-            {
-                "forecaster__strategy": ["mean"],
-                "transformer__forecaster__degree": [5, 6, 7],
-            },
-        ],
+        test_ForecastingGridSearchCV(),
+        test_ForecastingRandomizedSearchCV(),
+        test_ForecastingRandomizedSearchCV(n_iter=3),
+        test_ForecastingRandomizedSearchCV(n_iter=5),
+        # test_ForecastingSkoptSearchCV(n_iter=5),  # Not working
     ],
 )
-def test_return_n_best_forecasters(return_n_best_forecasters, param_grid):
+def test_return_n_best_forecasters(Forecaster, return_n_best_forecasters, kwargs):
     y, X = load_longley()
-    gscv = ForecastingGridSearchCV(
+    gscv = Forecaster(
+        **kwargs,
         forecaster=PIPE,
-        param_grid=param_grid,
         cv=CVs[0],
         return_n_best_forecasters=return_n_best_forecasters,
     )
     gscv.fit(y, X)
     if return_n_best_forecasters == -1:
 
-        def calculate_total_combinations(param_grid: dict | list[dict]) -> int:
+        def calculate_total_combinations(param_grid):
path: sktime/forecasting/model_selection/tests/test_tune.py
text: @user1:
If you want, you could use Union and List. The | shirtcut is unavailable before 3.10, and list not before 3.9 I think.

@author:
We need type ~script~ python

@user1:
Sorry, couldn't follow you. Can you please explain? 

I meant you can do this at start:

```python
from typing import Dict, List, Union
```

@author:
It was a joke. I was trying to be funny, but it didn't work sorry 😨

Sure I'll add typing.

@user2:
> It was a joke. I was trying to be funny, but it didn't work sorry

I thought it was funny 😆 

@user3:
You should be able to add `from __future__ import annotations` at the top of the file and use PEP 585 style annotations for earlier versions of Python (incl Python 3.8)

@user3:
Since sktime uses flake8, you could consider using this plugin: https://github.com/tyleryep/flake8-future-annotations

I don't personally use it but I use an [equivalent feature](https://docs.astral.sh/ruff/settings/#lint_isort_required-imports) with `ruff`.

@author:
I already used `typing` module as instructed [here](https://github.com/sktime/sktime/pull/6031#discussion_r1509146401). Should I change it?

@user3:
Ultimately it's up to the maintainers. My take is that this should probably be tackled in a different PR if sktime decides to adopt PEP 585 (`List[int] -> list[int]`) and PEP 604 (`Union[str, int] -> str | int`) annotations. There are tools that could help automatically apply this migration to the entire codebase.

@user2:
the current problem with the mentioned PEPs is lack of python 3.8 compatibility, and afaik a good number of users are still on 3.8. Though, 3.8 reaches EoL this year, after which we can in-principle add such annotations.

@user1:
The 2nd option will need 3.10, so one more year I suppose.

Or we can add typing-extensions as a dependency.

@user3:
@user2 @user1  the `from __future__ import annotations` import makes these compatible with Python 3.8. 

I just checked in two of our apps running in production w/ Python 3.8 and they both work with PEP 585 (`List[int] -> list[int]`) and PEP 604 (`Union[str, int] -> str | int`) style annotations when using in combination with `from __future__ import annotations`

@user1:
I understand, my comment was towards @user2 's comment above.

I've nothing against future imports, but it doesn't enable all of the newer features. Addition of typing extensions will allow everything at a cost of very minimal, if any (it may already be a indirect dependency), dependency changes.

@user2:
I think it's better to err on the side of compatibility, future imports feel risky.

Apart from that - and this is not a strong opinion but a matter of taste - I prefer to emphasize good docstrings over type annotations, since:

* assumptions on input are typically stronger than just the type
* truthful type annotations are often hard to read
* docstrings are much lower entry barrier for new contributors

Example, from skbase, would you prefer

```python
    def _coerce_to_named_object_tuples(
        self,
        objs: Union[
            Sequence[Union[BaseObject, Tuple[str, BaseObject]]], Dict[str, BaseObject]
        ],
        clone: bool = False,
        make_unique: bool = True,
    ) -> List[Tuple[str, BaseObject]]:
```

or would you prefer reading the docstring?

```python
        """Coerce sequence of objects or named objects to list of (str, obj) tuples.

        Input that is sequence of objects, list of (str, obj) tuples or
        dict[str, object] will be coerced to list of (str, obj) tuples on return.

        Parameters
        ----------
        objs : list of objects, list of (str, object tuples) or dict[str, object]
            The input should be coerced to list of (str, object) tuples. Should
            be a sequence of objects, or follow named object API.
        clone : bool, default=False.
            Whether objects in the returned list of (str, object) tuples are
            cloned (True) or references (False).
        make_unique : bool, default=True
            Whether the str names in the returned list of (str, object) tuples
            should be coerced to unique str values (if str names in input
            are already unique they will not be changed).

        Returns
        -------
        list[tuple[str, BaseObject]]
            List of tuples following named object API.

            - If `objs` was already a list of (str, object) tuples then either the
              same named objects (as with other cases cloned versions are
              returned if ``clone=True``).
            - If `objs` was a dict[str, object] then the named objects are unpacked
              into a list of (str, object) tuples.
            - If `objs` was a list of objects then string names were generated based
               on the object's class names (with coercion to unique strings if
               necessary).
        """
```

@user1:
May be my personal preference, but both. They are not replacement for one another.

If you have tools like pylance or etc. installed, or even without them sometimes, type hints are very useful during development to get auto completions or suggestions that I am not passing objects of correct types. I'll of course prefer to use through a more meaningful type alias though.

But for first time use (not just just first, but more often at start of using a new package), just argument names or argument types may not be enough to get full idea what is an argument and then docstrings are much more helpful, when present. The possible issue with docstrings is that people (including me) often make a change in signature to try new things to do quick stuff, and then completely forget to update the docstring. Over time, docstrings and signature are barely aligned together, and since no linters (so far what I have used, may @user3 can suggest something new?) check if docstrings note correctly as signature or not it's very difficult to detect.