Repository: sktime/sktime
Pull request: [ENH] Set `context_len` and `horizon_len` per default in `TimesFMForecaster`
<!--
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

- Fixes #7209.

#### What does this implement/fix? Explain your changes.

As discussed in #7209, this PR enhances the `TimesFMForecaster` by setting automatic defaults for `context_len` and `horizon_len`:
- `horizon_len`: Defaults to `fh-max` if not explicitly specified.  
- `context_len`: Defaults to the smallest multiple of `input_patch_len` exceeding `len(y)`.  

Explicitly specified values still override these defaults. 
Moreover, calculated values are stored in `self.horizon_len_` and `self.context_len_`.  

<!--
A clear and concise description of what you have implemented.
-->

#### Does your contribution introduce a new dependency? If yes, which one?
N/A
<!--
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
-->

#### Did you add any tests for the change?

<!-- 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.
-->

Yes, tests have been added in `get_test_params` to cover default and overridden values, including broadcasting.

#### Any other comments?

N/A
<!--
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
- [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 :-)
- [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] in `TimesFMForecaster`, `context_len` and `horizon_len` should be set automatically per default
`TimesFMForecaster` should set `context_len` and `horizon` len to a reasonable value, automatically per default.

The `horizon_len` can be obtained from `fh`, but it makes sense to give the user the option to override.

The `context_len` can also be set to an automatic choice, in the default. Again, giving the user the option to override makes sense.

FYI @geetu040.

As a recipe to change this, there needs to be an internal `self._context_len` and `self._horizon_len` which is determined by the settings.

Hints from issue discussion:
yes that sure sounds good. there are no specific values ideal for the defaults, but there are a few considerations
- `context_length` should be a multiple of `input_patch_len`, which by default is `input_patch_len=32` for loading the model checkpoint.
- `horizon_len` should ideally be a less than or equal to `context_len`

In the [latest code](https://github.com/google-research/timesfm/blob/master/src/timesfm/timesfm_base.py#L79), the authors have by default kept these values to be
```py
context_len = 512
horizon_len = 128
```
Is there an algorithmic difference in the output, from wetting `horizon_len` to 42 and then taking the first 12 values, or setting `horizon_len` to 12 in the first place?
> Is there an algorithmic difference in the output, from wetting `horizon_len` to 42 and then taking the first 12 values, or setting `horizon_len` to 12 in the first place?

No, there is no difference in the output
It produces forecasts for any horizon using auto-regressive decoding
then why should we not set it to max `fh` always, at least in the default?
Can I work on this issue?

sure, just make sure you have agreed on clear defaults for the parameters before you implement them. What would you suggest?
Hey, I am interested in working on this issue ..

Setting the default value of horizon_len to max fh sounds good to me,
Or I can set it to whatever else you suggest ..
@geetu040, does default `horizon_len` = max-fh sound reasonable?

Also, what should `context_len` be? We can work on that separately of course.
> @geetu040, does default `horizon_len` = max-fh sound reasonable?

I designed it the way it is, because this allows the model to be flexible towards different `fh` during predict. If any of the `fh` is greator than `horizon_len` during fit, `horizon_len` is overridden and during predict, exception is raised.

Now by default `horizon_len=None` and interpret it from `fh` makes sense, but I would prefer that `max(fh)` overrides `horizon_len` only when `horizon_len=None` (new condition) or `horizon_len < max(fh)` (already there).

> Also, what should `context_len` be? We can work on that separately of course.

I am not too sure about `context_len`, it has be to `>=horizon_len`, we can set that automatically from the horizon len, that is not a bad approach, but I would prefer to take it from the user instead of setting that automatically because there is not good standard to creating context_len from horizon_len.

> Now by default `horizon_len=None` and interpret it from `fh` makes sense, but I would prefer that `max(fh)` overrides `horizon_len` only when `horizon_len=None` (new condition) or `horizon_len < max(fh)` (already there).

Makes sense, I would agree with that.

> I am not too sure about `context_len`, it has be to `>=horizon_len`, we can set that automatically from the horizon len, that is not a bad approach, but I would prefer to take it from the user instead of setting that automatically because there is not good standard to creating context_len from horizon_len.

What about setting it equal to `horizon_len` by default, or a constant multiple of it?

Commit message: [ENH] Set context_len and horizon_len per default in TimesFMForecaster

Patch to review:
diff --git a/.all-contributorsrc b/.all-contributorsrc
index b779b3f9767..d91d19e4bae 100644
--- a/.all-contributorsrc
+++ b/.all-contributorsrc
@@ -3260,10 +3260,10 @@
       ]
     },
     {
-      "login": "TanviPooranmal",
+      "login": "tanvincible",
       "name": "Tanvi Pooranmal Meena",
-      "avatar_url": "https://avatars.githubusercontent.com/TanviPooranmal",
-      "profile": "https://github.com/TanviPooranmal",
+      "avatar_url": "https://avatars.githubusercontent.com/tanvincible",
+      "profile": "https://github.com/tanvincible",
       "contributions": [
         "code",
         "doc"
diff --git a/sktime/forecasting/timesfm_forecaster.py b/sktime/forecasting/timesfm_forecaster.py
index cd5cf2b8682..c8a319922d4 100644
--- a/sktime/forecasting/timesfm_forecaster.py
+++ b/sktime/forecasting/timesfm_forecaster.py
@@ -35,8 +35,11 @@ class TimesFMForecaster(_BaseGlobalForecaster):
 
     Parameters
     ----------
-    context_len : int
+    context_len : int, optional (default=None)
         The length of the input context sequence.
+        If set to None, the context length is automatically computed as the smallest
+        multiple of ``input_patch_len`` that is larger than the length
+        of the input time series `y`.
         It should be a multiple of ``input_patch_len`` (32).
         The maximum context length currently supported is 512, but this can be
         increased in future releases.
@@ -44,8 +47,11 @@ class TimesFMForecaster(_BaseGlobalForecaster):
         and padding or truncation will
         be handled by the model's inference code if necessary.
 
-    horizon_len : int
-        The length of the forecast horizon. This can be set to any value, although it
+    horizon_len : int, optional (default=None)
+        The length of the forecast horizon.
+        If set to None, the forecast horizon is dynamically determined based on the
+        provided forecasting horizon `fh`, if available.
+        This can be set to any value, although it
         is generally recommended to keep it less than or equal to ``context_len`` for
         optimal performance. The model will still function
         if ``horizon_len`` exceeds ``context_len``.
@@ -180,8 +186,8 @@ class TimesFMForecaster(_BaseGlobalForecaster):
 
     def __init__(
         self,
-        context_len,
-        horizon_len,
+        context_len=None,
+        horizon_len=None,
         freq=0,
         repo_id="google/timesfm-1.0-200m",
         input_patch_len=32,
@@ -196,8 +202,10 @@ def __init__(
         ignore_deps=False,
     ):
         self.context_len = context_len
+        self._context_len = None
         self.horizon_len = horizon_len
         self._horizon_len = None
+        self._context_len = None
         self.freq = freq
         self.repo_id = repo_id
         self.input_patch_len = input_patch_len
@@ -242,12 +250,27 @@ def __init__(
         super().__init__()
 
     def _fit(self, y, X, fh):
-        if fh is not None:
+        if fh is None and self.horizon_len is None:
+            raise ValueError(
+                "Both 'fh' and 'horizon_len' cannot be None. Provide at least one."
+            )
+        elif fh is not None and self.horizon_len is not None:
             fh = fh.to_relative(self.cutoff)
             self._horizon_len = max(self.horizon_len, *fh._values.values)
+        elif fh is not None:
+            fh = fh.to_relative(self.cutoff)
+            self._horizon_len = max(*fh._values.values)
         else:
             self._horizon_len = self.horizon_len
 
+        if self.context_len is not None:
+            self._context_len = self.context_len
+        else:
+            # Compute context_len as the smallest multiple of input_patch_len
+            # that is larger than the length of y.
+            context_multiple = (len(y) // self.input_patch_len) + 1
+            self._context_len = context_multiple * self.input_patch_len
+
         self.tfm = _CachedTimesFM(
             key=self._get_unique_timesfm_key(),
             timesfm_kwargs=self._get_timesfm_kwargs(),
@@ -258,7 +281,7 @@ def _fit(self, y, X, fh):
     def _get_timesfm_kwargs(self):
         """Get the kwargs for TimesFM model."""
         return {
-            "context_len": self.context_len,
+            "context_len": self._context_len,
             "horizon_len": self._horizon_len,
             "input_patch_len": self.input_patch_len,
             "output_patch_len": self.output_patch_len,
@@ -286,10 +309,10 @@ def _predict(self, fh, X, y=None):
             fh = self.fh
         fh = fh.to_relative(self.cutoff)
 
-        if max(fh._values.values) > self.horizon_len:
+        if max(fh._values.values) > self._horizon_len:
             raise ValueError(
                 f"Error in {self.__class__.__name__}, the forecast horizon exceeds the"
-                f" specified horizon_len of {self.horizon_len}. Change the horizon_len"
+                f" specified horizon_len of {self._horizon_len}. Change the horizon_len"
                 " when initializing the model or try another forecasting horizon."
             )
 
@@ -370,6 +393,12 @@ def get_test_params(cls, parameter_set="default"):
             `create_test_instance` uses the first (or only) dictionary in `params`
         """
         test_params = [
+            {
+                "context_len": None,
+                "horizon_len": None,
+                "freq": 0,
+                "verbose": False,
+            },
             {
                 "context_len": 64,
                 "horizon_len": 32,


Code review comments:
diff_hunk: @@ -35,17 +35,23 @@ class TimesFMForecaster(_BaseGlobalForecaster):
 
     Parameters
     ----------
-    context_len : int
+    context_len : int, optional (default=None)
         The length of the input context sequence.
+        If set to None, the context length is automatically computed as the smallest
+        multiple of ``input_patch_len`` that is larger than the length
+        of the input time series `y`.
         It should be a multiple of ``input_patch_len`` (32).
         The maximum context length currently supported is 512, but this can be
         increased in future releases.
         The input time series can have any context length,
         and padding or truncation will
         be handled by the model's inference code if necessary.
 
-    horizon_len : int
-        The length of the forecast horizon. This can be set to any value, although it
+    horizon_len : int, optional (default=None)
path: sktime/forecasting/timesfm_forecaster.py
text: @user1:
```suggestion
    horizon_len : int, optional (default=128)
```