Repository: sktime/sktime
Pull request: [ENH] `NeuralForecastRNN` should auto-detect `freq`
Enhances `NeuralForecastRNN` to interpret `freq` from `ForecastingHorizon` when passed as `"auto"`
<!--
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 #6003.

#### What does this implement/fix? Explain your changes.
<!--
A clear and concise description of what you have implemented.
-->
The `NeuralForecastRNN` constructor previously required a `freq` argument, which is now proposed to default to `"auto"` in which case it interprets `freq` from `ForecastingHorizon`, leveraging `fh.freq` in the `fit` method.

#### 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 -->
I have run the tests with the updated estimator
```py
results = check_estimator(NeuralForecastRNN) # All tests PASSED!
```
`freq` can now be passed like this:
```py
y, X = load_longley()
y_train, y_test, X_train, X_test = temporal_train_test_split(y, X, test_size=4)

model = NeuralForecastRNN(
	"auto",	# interprets to be "A-DEC"
	futr_exog_list=["ARMED", "POP"], max_steps=5)

model.fit(y_train, X=X_train, fh=[1, 2, 3, 4])

model.predict(X=X_test)
# Seed set to 1
# 1959    66241.984375
# 1960    66700.132812
# 1961    66550.195312
# 1962    67310.007812
# Freq: A-DEC, Name: TOTEMP, dtype: float64
```

#### PR checklist
<!--
Please go through the checklist below. Please feel free to remove points if they are not applicable.
-->

##### For all contributions
- [x] 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).
- [x] 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.

<!--
Thanks for contributing!
-->


Problem statement:
[ENH] `NeuralForecastRNN` should auto-detect `freq`
Right now, a `freq` argument needs to be passed to the constructor of `NeuralForecastRNN`, and it will raise exception if not.

To my understanding, this must always be identical with the `freq` of the data (as `pandas`  coerced),
Therefore, this information is available in `fit`, so why not have a default setting of `"auto"`, in which case `fh.freq` is used? This would also be deprecation safe, as an arg without default would get one.

Hints from issue discussion:
FYI @yarnabrina 
I replied to your comment in the PR, but I think it got lost in later conversations.

I'm not sure if I understand how to use `fh.freq` always. If user passes a relative `fh`, say `[1, 6, 12]`, will that object always have a `freq` attribute? May be that is handled in `BaseForecaster` or in `ForecastingHorizon`, but I'm not very familiar with those.
`ForecastingHorizon` is designed to behave "greedy for `freq`", i.e., all objects it interacts with it queries for `freq` and sets to itself if not already set.

As a consequence, once `_fit` is reached and `fh` has been passed, it has a `freq` if the data had the attribute.

Therefore, trying to get `freq` from `fh` should be "safe" in a case where `fh` was passed. If it does not have `freq`, it means it cannot be inferred from the data.

In consequence, a sensible `"auto"` setting could get it from `fh`, and when that is not available, it could complain to the user.

Commit message: [ENH] `NeuralForecastRNN` should auto-detect `freq` (#6003)

Enhances `NeuralForecastRNN` to interpret `freq` from `ForecastingHorizon` when passed as `"auto"`

Patch to review:
diff --git a/.all-contributorsrc b/.all-contributorsrc
index 0f7a286d3e4..a3da25d70df 100644
--- a/.all-contributorsrc
+++ b/.all-contributorsrc
@@ -2604,6 +2604,15 @@
         "bug",
         "code"
       ]
+    },
+    {
+      "login": "geetu040",
+      "name": "Armaghan",
+      "avatar_url": "https://avatars.githubusercontent.com/u/90601662?s=96&v=4",
+      "profile": "https://github.com/geetu040",
+      "contributions": [
+        "code"
+      ]
     }
   ]
 }
diff --git a/sktime/forecasting/base/adapters/_neuralforecast.py b/sktime/forecasting/base/adapters/_neuralforecast.py
index dcf09acc54a..80583ce574d 100644
--- a/sktime/forecasting/base/adapters/_neuralforecast.py
+++ b/sktime/forecasting/base/adapters/_neuralforecast.py
@@ -17,8 +17,10 @@ class _NeuralForecastAdapter(BaseForecaster):
 
     Parameters
     ----------
-    freq : str
+    freq : str (default="auto")
         frequency of the data, see available frequencies [1]_ from ``pandas``
+
+        default ("auto") interprets freq from ForecastingHorizon in ``fit``
     local_scaler_type : str (default=None)
         scaler to apply per-series to all features before fitting, which is inverted
         after predicting
@@ -66,7 +68,7 @@ class _NeuralForecastAdapter(BaseForecaster):
 
     def __init__(
         self: "_NeuralForecastAdapter",
-        freq: str,
+        freq: str = "auto",
         local_scaler_type: typing.Optional[
             typing.Literal["standard", "robust", "robust-iqr", "minmax", "boxcox"]
         ] = None,
@@ -179,6 +181,16 @@ def _fit(
         if not fh.is_all_out_of_sample(cutoff=self.cutoff):
             raise NotImplementedError("in-sample prediction is currently not supported")
 
+        if self.freq == "auto":
+            if fh.freq:
+                # interpret freq from ForecastingHorizon
+                self.freq = fh.freq
+            else:
+                # when freq is not interpreted from ForecastingHorizon
+                raise ValueError(
+                    "Could not interpret freq, try passing freq in model initialization"
+                )
+
         train_indices = y.index
         if isinstance(train_indices, pandas.PeriodIndex):
             train_indices = train_indices.to_timestamp(freq=self.freq)
diff --git a/sktime/forecasting/neuralforecast.py b/sktime/forecasting/neuralforecast.py
index 8d2c452a0d0..0c291a1518b 100644
--- a/sktime/forecasting/neuralforecast.py
+++ b/sktime/forecasting/neuralforecast.py
@@ -22,8 +22,10 @@ class NeuralForecastRNN(_NeuralForecastAdapter):
 
     Parameters
     ----------
-    freq : str
+    freq : str (default="auto")
         frequency of the data, see available frequencies [4]_ from ``pandas``
+
+        default ("auto") interprets freq from ForecastingHorizon in ``fit``
     local_scaler_type : str (default=None)
         scaler to apply per-series to all features before fitting, which is inverted
         after predicting
@@ -156,7 +158,7 @@ class NeuralForecastRNN(_NeuralForecastAdapter):
 
     def __init__(
         self: "NeuralForecastRNN",
-        freq: str,
+        freq: str = "auto",
         local_scaler_type: typing.Optional[
             typing.Literal["standard", "robust", "robust-iqr", "minmax", "boxcox"]
         ] = None,


Code review comments:
diff_hunk: @@ -179,6 +181,16 @@ def _fit(
         if not fh.is_all_out_of_sample(cutoff=self.cutoff):
             raise NotImplementedError("in-sample prediction is currently not supported")
 
+        if self.freq == "auto":
+            if fh.freq:
+                # interpret freq from ForecastingHorizon
+                self.freq = fh.freq
+            else:
+                # when freq is not interpreted from ForecastingHorizon
+                raise ValueError(
+                    "Could not interpret freq, try passing freq in model initialization"
path: sktime/forecasting/base/adapters/_neuralforecast.py
text: @user1:
It's better if you edit the message to fail with model name. Something like `self.__class__.__name__` may work.

diff_hunk: @@ -179,6 +181,16 @@ def _fit(
         if not fh.is_all_out_of_sample(cutoff=self.cutoff):
             raise NotImplementedError("in-sample prediction is currently not supported")
 
+        if self.freq == "auto":
path: sktime/forecasting/base/adapters/_neuralforecast.py
text: @user1:
This one is a biased suggestion based on just ny personal preferences.

I suggest you first check for failure case, i.e. `if self.freq == "auto" and fh.freq is None` and raise if that is the case. Else proceed as usual.

@user2:
do we always need a `freq` in `neuralforecast`, or can it be `None` upstream?

@author:
from `neuralforecast.NeuralForecast`
```py
class NeuralForecast:
    def __init__(
        self, models: List[Any], freq: str, local_scaler_type: Optional[str] = None
    ):
```
I guess we always need `freq`, it can't be `None`

diff_hunk: @@ -179,6 +181,16 @@ def _fit(
         if not fh.is_all_out_of_sample(cutoff=self.cutoff):
             raise NotImplementedError("in-sample prediction is currently not supported")
 
+        if self.freq == "auto":
+            if fh.freq:
+                # interpret freq from ForecastingHorizon
+                self.freq = fh.freq
path: sktime/forecasting/base/adapters/_neuralforecast.py
text: @user1:
This is not in line with sktime/sklearn principle that init attributes should not be modified afterwards. To handle this, you can initiate a new attribute in init, say `_freq` as `None` and here override with self.freq or fh.freq as applicable.

@author:
Are you suggesting that I initialize `_freq` like
```py
self.freq = freq
self._freq = None
```
later in `fit()` update it like this
```py
if self.freq == "auto":
    # interpret freq from ForecastingHorizon
    self._freq = fh.freq
```
and then use `_freq` in `fit()` and other methods instead of `freq`

Also, is this a conventional good practice or there exists some guidelines that I can read to avoid such mistakes in future.

@user2:
> Also, is this a conventional good practice or there exists some guidelines that I can read to avoid such mistakes in future.

The extension templates, normally, though I noticed that the "do not mutate/overwrite" contract is not discussed very clearly. I added some clarifying explanations in this PR: https://github.com/sktime/sktime/pull/6053

Thanks for bringing this up!

@user2:
> Are you suggesting

That suggestion would satisfy the contract - whether that's what @user1 had in mind, of course only he can say (I'd guess: probably yes).

@user1:
Yes, I meant the same. You can do similar to how self.loss is handled when None is provided but MAE needs to be used etc.