Repository: sktime/sktime
Pull request: [BUG] Fixed SARIMAX fail when X is passed to predict but not fit
<!--
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
-->
This fixes #5789 .

#### What does this implement/fix? Explain your changes.
<!--
A clear and concise description of what you have implemented.
-->
`SARIMAX` no longer gives an error when `X` is passed to `predict` but not to `fit`. The `X` passed to `predict` is ignored in this case i.e. it is changed to `None`.
#### 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?

<!-- 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?
No.
<!-- 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?
All check estimator tests have passed when I checked it locally. Do I have to add any additional tests? If yes, how do I go about it?
<!--
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] 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:
[BUG] `SARIMAX` fails when `X` is passed in `predict` but not used in `fit`
## Minimal Reproducible Example

```pycon
>>> 
>>> from sktime.datasets import load_longley
>>> from sktime.forecasting.sarimax import SARIMAX
>>> from sktime.split import temporal_train_test_split
>>> 
>>> y, X = load_longley()
>>> 
>>> y_train, _, _, X_test = temporal_train_test_split(y, X)
>>> 
>>> forecaster = SARIMAX()
>>> 
>>> forecaster.fit(y_train)
SARIMAX()
>>> 
>>> # works
>>> forecaster.predict(fh=[1, 2, 3, 4])
1959    66061.176439
1960    65682.034815
1961    65363.883253
1962    65096.910677
Freq: A-DEC, Name: TOTEMP, dtype: float64
>>> 
>>> # fails
>>> forecaster.predict(fh=[1, 2, 3, 4], X=X_test)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/anirban/sktime-fork/sktime/forecasting/base/_base.py", line 412, in predict
    y_pred = self._predict(fh=fh, X=X_inner)
  File "/home/anirban/sktime-fork/sktime/forecasting/base/adapters/_statsmodels.py", line 108, in _predict
    ind_drop = self._X.index
AttributeError: 'NoneType' object has no attribute 'index'
>>> 
```

## Expectation

I was expecting no failures, and identical behaviour in both cases. I do get that behaviour if I try with `sktime.forecasting.arima.ARIMA`.

## Version

Operating System: Ubuntu 22.04.3 LTS (WSL)
Python: 3.10.12
Sktime: e51ec2472a

Hints from issue discussion:
On looking into this issue, I found that the error was initially because of this code snippet
```python
if X is not None:
    ind_drop = self._X.index
    X = X.loc[~X.index.isin(ind_drop)]
    # Entire range of the forecast horizon is required
    X = X.iloc[: (fh_int[-1] + 1)]  # include end point
```
Since we are not passing any X in fit, `self._X` is None and it does not have an index attribute. This could be solved by simply adding an if condition to check if self._X is None or not and then dropping the indices accordingly.
However this fix throws another error when run :
```python
ValueError                                Traceback (most recent call last)
Cell In[3], line 1
----> 1 forecaster.predict(fh=[1, 2, 3,4],X=X_test)

File ~/sktime/sktime/forecasting/base/_base.py:444, in BaseForecaster.predict(self, fh, X)
    442 # we call the ordinary _predict if no looping/vectorization needed
    443 if not self._is_vectorized:
--> 444     y_pred = self._predict(fh=fh, X=X_inner)
    445 else:
    446     # otherwise we call the vectorized version of predict
    447     y_pred = self._vectorize("predict", X=X_inner, fh=fh)

File ~/sktime/sktime/forecasting/base/adapters/_statsmodels.py:121, in _StatsModelsAdapter._predict(self, fh, X)
    118     X = X.iloc[: (fh_int[-1] + 1)]  # include end point
    120 if "exog" in inspect.signature(self._forecaster.__init__).parameters.keys():
--> 121     y_pred = self._fitted_forecaster.predict(start=start, end=end, exog=X)
    122 else:
    123     y_pred = self._fitted_forecaster.predict(start=start, end=end)

File ~/anaconda3/envs/sktime-dev/lib/python3.10/site-packages/statsmodels/base/wrapper.py:113, in make_wrapper.<locals>.wrapper(self, *args, **kwargs)
    111     obj = data.wrap_output(func(results, *args, **kwargs), how[0], how[1:])
    112 elif how:
--> 113     obj = data.wrap_output(func(results, *args, **kwargs), how)
    114 return obj
...
    644     # endpoint, but does not know what the calling methods may
    645     # specifically require.
    646     updated_mat = np.asarray(kwargs[name])

ValueError: Model has time-invariant design matrix, so cannot provide an extended matrix.
```

This error is coming from a method within the `statsmodels` source code. I am not very familiar with what exactly happens in SARIMAX but are we sure that in order to pass exog args in predict, they do not have to be passed in fit first?


My expectation was that this should be handled by the base class itself. The expectation came from this:

https://github.com/sktime/sktime/blob/64e90aae0deb51ad89c2e29ef56607e533a7c984/extension_templates/forecasting.py#L114-L117

I interpreted this that even if user pass X only in predict, base will set that to None before forwarding it to _predict. And even for models supporting exogenous features in general but not in a particular case, it'll still set it as None.
It looks like the tag being False isn't setting the X passed in predict to None conditionally when X is not passed in fit. On the other hand it cannot be set to True as exogeneous features cannot be ignored in all cases. Simply adding an if check just before passing the exog X to the predict method seems to work.
```python
if "exog" in inspect.signature(self._forecaster.__init__).parameters.keys():
    if (self._X is None):
        X = None
    y_pred = self._fitted_forecaster.predict(start=start, end=end, exog=X)
else:
    y_pred = self._fitted_forecaster.predict(start=start, end=end)
```
Is this solution satisfactory or should we go about it some other way?
> My expectation was that this should be handled by the base class itself. The expectation came from this:
> 
> https://github.com/sktime/sktime/blob/64e90aae0deb51ad89c2e29ef56607e533a7c984/extension_templates/forecasting.py#L114-L117
> 
> I interpreted this that even if user pass X only in predict, base will set that to None before forwarding it to _predict. And even for models supporting exogenous features in general but not in a particular case, it'll still set it as None.

Also, how do you display code in this manner?
silly question: why should passing `X_test` work if `X_train` has not been passed? Are there models that can make use of `X_test` if they have not seen an `X_train`?
My expectation is it should not fail. Obviously the fitted model will not be able to use that as a feature, but my expectation is that it should not fail and it should work without error.

As of now, the signature of the function allows users to pass X always as an optional argument, irrespective of signature of fit. So I think predict should not fail if user's call is compatible with that signature.

(I do not remember what exactly I was trying to face this, so can not give an actual situation.)

Commit message: Merge branch 'sktime:main' into Sarimax_exog_fix

Patch to review:
diff --git a/sktime/forecasting/base/adapters/_statsmodels.py b/sktime/forecasting/base/adapters/_statsmodels.py
index 1f00fe1066f..07479944c7e 100644
--- a/sktime/forecasting/base/adapters/_statsmodels.py
+++ b/sktime/forecasting/base/adapters/_statsmodels.py
@@ -111,12 +111,15 @@ def _predict(self, fh, X):
         # statsmodels exog must contain test indices only.
         # For discussion see https://github.com/sktime/sktime/issues/3830
         if X is not None:
-            ind_drop = self._X.index
-            X = X.loc[~X.index.isin(ind_drop)]
+            if self._X is not None:
+                ind_drop = self._X.index
+                X = X.loc[~X.index.isin(ind_drop)]
             # Entire range of the forecast horizon is required
             X = X.iloc[: (fh_int[-1] + 1)]  # include end point
 
         if "exog" in inspect.signature(self._forecaster.__init__).parameters.keys():
+            if self._X is None:
+                X = None  # change X passed in predict to None if X wasn't passed to fit
             y_pred = self._fitted_forecaster.predict(start=start, end=end, exog=X)
         else:
             y_pred = self._fitted_forecaster.predict(start=start, end=end)


Code review comments:
diff_hunk: @@ -111,12 +111,15 @@ def _predict(self, fh, X):
         # statsmodels exog must contain test indices only.
         # For discussion see https://github.com/sktime/sktime/issues/3830
         if X is not None:
-            ind_drop = self._X.index
-            X = X.loc[~X.index.isin(ind_drop)]
+            if self._X is not None:
path: sktime/forecasting/base/adapters/_statsmodels.py
text: @user1:
Optional: may be merge with previous check with `and`?

@author:
I will check this out

@author:
Can be done ,I'll make the change.

diff_hunk: @@ -111,12 +111,15 @@ def _predict(self, fh, X):
         # statsmodels exog must contain test indices only.
         # For discussion see https://github.com/sktime/sktime/issues/3830
         if X is not None:
-            ind_drop = self._X.index
-            X = X.loc[~X.index.isin(ind_drop)]
+            if self._X is not None:
+                ind_drop = self._X.index
path: sktime/forecasting/base/adapters/_statsmodels.py
text: @user1:
This is the question I asked yesterday. Is it guaranteed that `ind_drop` only contains indices from training range and never from prediction range?

@author:
Here `ind_drop` contains the indices present in the `X` passed to `fit` (which should be the training set ideally).

@user1:
Ideally and more common, yes. Not sure if it's a guarantee. For example, I personally have definitely passed full `X` sometimes (not necessarily to SARIMAX, but I don't remember exactly).

@author:
Ok so in the event that a user passes full `X` to SARIMAX in fit , what will they pass to predict? If it is a subset of this `X`, (say `test_X` obtained by splitting `X`) it will result in all it's indices being dropped and the code will run without error but the exog features will not be used in this case. However ideally the user should be passing different X to fit and predict in order for it to serve it's purpose.
This code to drop indices was added as a fix for issue #3830 as mentioned in the comment.
https://github.com/sktime/sktime/blob/14c88c69d2a0e961390e749278b5349470d35332/sktime/forecasting/base/adapters/_statsmodels.py#L110-L115
We technically cannot do anything about `full X` being passed to `fit` because we do not know what the user will pass to `predict` next.

@user1:
> it will result in all it's indices being dropped and the code will run without error

Can you please validate this? My intinct says that the underlying model (from `statsmodels`) will complain if `X` is not passed during `predict` but passed during `fit`.

@author:
>it will result in all it's indices being dropped and the code will run without error

Small correction here, I meant this in the case where `full X` is passed to `fit` and a `subset of X` is passed to `predict`. However I was wrong in saying that code runs without error, rather all indices get dropped and it returns an `error` saying the received exog is not in the correct shape.
```python
from sktime.datasets import load_longley
from sktime.split import temporal_train_test_split
from sktime.forecasting.sarimax import SARIMAX

y, X = load_longley()
y_train, _, X_train, X_test = temporal_train_test_split(y, X)
forecaster = SARIMAX()
forecaster.fit(y,X=X)
forecaster.predict(fh=[1, 2, 3, 4],X=X_train)
```
```python
ValueError: Provided exogenous values are not of the appropriate shape. Required (4, 5), got (0, 5).
```
Also, if X is passed to `fit` but not `predict`, it returns the following error asking for exog features to be passed in predict as well.
```python
ValueError: Out-of-sample operations in a model with a regression component require additional exogenous values via the `exog` argument.
```



@author:
@user1 so do we mark this conversation as resolved or are we waiting for @user2 /@user3-g to review it as well

@user1:
I'd wait for at least 2 days. Otherwise, we can go ahead and merge.

@user4:
Not 100% sure on the tag behaviour myself unfortunately, but the changes look ok and sensible to me 👍