[ENH] in `BaseForecaster`, move check for `capability:insample` to `_check_fh` boilerplate #6593

Xinyu-Wu-0000 · 2024-06-14T07:28:08Z

For issue [ENH] in sample fh should raise NotImplementedError in _check_fh #6579

fkiraly

Thanks!

This approach will not work, because at the time the code is added:

fh could be None, so the code will fail
cutoff may not have been seen yet, which is required to know whether fh is in or out of sample relative to the training data

My recommendation is, kindly test the code locally, together with some forecasters, some of which can forecast in-sample, some that cannot.

Xinyu-Wu-0000 · 2024-06-20T06:57:59Z

I am adding more changes than I originally thought and I am not sure if there is a better way to implement it with less impact. @fkiraly could you take a look?

fkiraly · 2024-06-20T08:51:04Z

sktime/forecasting/base/_base.py

@@ -380,7 +380,7 @@ def fit(self, y, X=None, fh=None):
        self._update_y_X(y_inner, X_inner)

        # check forecasting horizon and coerce to ForecastingHorizon object
-        fh = self._check_fh(fh)


_check_fh is not a pure/static method, it is already using parameters from self. So instead of passing here, I would suggest to simply get self._cutoff at the start, _cutoff = self._cutoff, and add it in the docstring on the list of accessed attributes if not already present there. That way, you need to change much less of the lines.

fkiraly · 2024-06-20T08:51:58Z

sktime/forecasting/base/_base.py

+            and not self._fh.is_all_out_of_sample(cutoff)
+        ):
+            msg = (
+                f"in-sample prediction is currently not supported: "


I would put the forecaster name more early in the error message.

fkiraly · 2024-06-20T08:53:05Z

sktime/forecasting/base/_base.py

@@ -616,7 +617,8 @@ def predict_quantiles(self, fh=None, X=None, alpha=None):
        # input checks and conversions

        # check fh and coerce to ForecastingHorizon, if not already passed in fit
-        fh = self._check_fh(fh)
+        cutoff = get_cutoff(self._y, self.cutoff, return_index=True)


can you explain why you are recomputing the cutoff, can we not get self._cutoff?

I am not sure if self._cutoff is updated before _chech_fh. In some functions like update_predict_single, _check_cutoff is actully called before _update_y_X. I moved _check_cutoff and removed passing cutoff now.

fkiraly · 2024-06-20T08:53:56Z

sktime/forecasting/base/_base.py

+            else self.get_tag("capability:pred_int:insample")
+        )
+        if (
+            not in_sample_pred


are these conditions all needed?

E.g., is there an example in which we do know fh but do not know the cutoff?

I was not sure so I put them all. I tested it without cutoff, it worked. cutoff is removed now.

fkiraly

Added some review comments.

passing of self._cutoff could be replaced by inspection of self, is that correct?
why are we recomputing the cutoff in proba methods instead of looking it up? Explanation would be appreciated, if you think it's necessary
comment about the condition in the block added, is that too complex or is it necessary (please explain)

Further, we should pick one or two forecasters that raise this message manually currently, and remove the private raise there. Otherwise we have no example where the message would be correctly raised.

Short-term, possibly in this PR, we would like to do this for all forecasters that do not support in-sample predictions.

fkiraly · 2024-06-20T08:56:03Z

sktime/forecasting/tests/test_all_forecasters.py

@@ -227,9 +227,12 @@ def test_predict_time_index(

        # if estimator cannot forecast in-sample and fh is in-sample, terminate
        # if the tag correctly states this, we consider this fine as per contract
-        # todo: check that estimator raises error message when fitting instead
+        # check that estimator raises error message when fitting


use with pytest.raises instead, that is the idiomatic check for an exception being raised. Also check that the error message agrees.

Xinyu-Wu-0000 · 2024-06-21T04:33:18Z

Further, we should pick one or two forecasters that raise this message manually currently, and remove the private raise there.

All forecasters with capability:insample=False or capability:pred_int:insample=False are checked to remove private raise.

                                         name                                             object
0                              CINNForecaster  <class 'sktime.forecasting.conditional_inverti... (cleared in parent class)
1           DirRecTabularRegressionForecaster  <class 'sktime.forecasting.compose._reduce.Dir... (cleared)
2        DirRecTimeSeriesRegressionForecaster  <class 'sktime.forecasting.compose._reduce.Dir... (cleared)
3           DirectTabularRegressionForecaster  <class 'sktime.forecasting.compose._reduce.Dir... (cleared)
4        DirectTimeSeriesRegressionForecaster  <class 'sktime.forecasting.compose._reduce.Dir... (cleared)
5                               DynamicFactor  <class 'sktime.forecasting.dynamic_factor.Dyna... (no NotImplementedError)
6                    HFTransformersForecaster  <class 'sktime.forecasting.hf_transformers_for... (cleared)
7                       LTSFDLinearForecaster  <class 'sktime.forecasting.ltsf.LTSFDLinearFor... (cleared in parent class)
8                        LTSFLinearForecaster  <class 'sktime.forecasting.ltsf.LTSFLinearFore... (cleared in parent classr)
9                       LTSFNLinearForecaster  <class 'sktime.forecasting.ltsf.LTSFNLinearFor... (cleared in parent class)
10     MultioutputTabularRegressionForecaster  <class 'sktime.forecasting.compose._reduce.Mul... (cleared)
11  MultioutputTimeSeriesRegressionForecaster  <class 'sktime.forecasting.compose._reduce.Mul... (cleared)
12                         NeuralForecastLSTM  <class 'sktime.forecasting.neuralforecast.Neur... (cleared)
13                          NeuralForecastRNN  <class 'sktime.forecasting.neuralforecast.Neur... (cleared)
14                            PyKANForecaster  <class 'sktime.forecasting.pykan_forecaster.Py... (no NotImplementedError)
15       RecursiveTabularRegressionForecaster  <class 'sktime.forecasting.compose._reduce.Rec... (cleared)
16    RecursiveTimeSeriesRegressionForecaster  <class 'sktime.forecasting.compose._reduce.Rec... (cleared)
17                          SquaringResiduals  <class 'sktime.forecasting.squaring_residuals.... (no NotImplementedError)

                        name                                             object
0             CINNForecaster  <class 'sktime.forecasting.conditional_inverti... (cleared in parent class)
1         ConformalIntervals  <class 'sktime.forecasting.conformal.Conformal... (no NotImplementedError)
2   HFTransformersForecaster  <class 'sktime.forecasting.hf_transformers_for... (cleared in parent class)
3      LTSFDLinearForecaster  <class 'sktime.forecasting.ltsf.LTSFDLinearFor... (cleared in parent class)
4       LTSFLinearForecaster  <class 'sktime.forecasting.ltsf.LTSFLinearFore... (cleared in parent class)
5      LTSFNLinearForecaster  <class 'sktime.forecasting.ltsf.LTSFNLinearFor... (cleared in parent class)
6            PyKANForecaster  <class 'sktime.forecasting.pykan_forecaster.Py... (no NotImplementedError)
7          SquaringResiduals  <class 'sktime.forecasting.squaring_residuals.... (no NotImplementedError)
8          StatsForecastMSTL  <class 'sktime.forecasting.statsforecast.Stats... (no NotImplementedError)
9                        VAR               <class 'sktime.forecasting.var.VAR'> (no NotImplementedError)
10                    VARMAX         <class 'sktime.forecasting.varmax.VARMAX'> (no NotImplementedError)
11                      VECM             <class 'sktime.forecasting.vecm.VECM'> (no NotImplementedError)

Xinyu-Wu-0000 · 2024-06-21T06:32:23Z

All change requests are addressed and no error in the CI now. @fkiraly could you take a look if they are resovled?

fkiraly · 2024-06-21T23:53:17Z

Nice!

Quick question: why do VAR, VARMAX, VECM etc not fail currently?

fkiraly

Points addressed, from my side.

We should understand why VAR etc are not failing currently or did not need action, before we merge. I do not understand that, any ideas, @Xinyu-Wu-0000?

Xinyu-Wu-0000 · 2024-06-22T03:43:20Z

We should understand why VAR etc are not failing currently or did not need action

In test_predict_time_index, if a forecaster does not support insample predict, the test will simple return None and skip the test, so the forecaster actully doesn't see any insample input in the test, therefor no forecasters should fail.

In test_predict_time_index_in_sample_full, if a forecaster does not support probabilistic insample predict, the test will simple return None and skip the predict_interval and predict_quantiles, so the forecaster actully doesn't see any insample input for the predict_interval and predict_quantiles functions. ConformalIntervals, StatsForecastMSTL, VAR, VARMAX, VECM support insample point prediction but not insample probabilistic prediction, so they should not fail.

However I am not sure why DynamicFactor, PyKANForecaster and SquaringResiduals do not fail in test_predict_time_index_in_sample_full as they are taking insample input for point prediction.

Actully, DynamicFactor and PyKANForecaster in main branch failed in my local environment with check_estimator but I am not sure why they didn't fail in CI. SquaringResiduals in main branch passed in my local environment with check_estimator.

When I search "SquaringResiduals insample" with Google Search, I found potential bug with fh for in-sample prediction with forecasters that require fh in fit. In this implementation, "capability:insample": False, is actully remove from the tag list of SquaringResiduals.

fkiraly · 2024-06-22T23:29:41Z

hm, odd. Could you open an issue with the failure so we do not forget? I think this can be merged, and that issue is separate.

…check_fh` boilerplate (sktime#6593) For issue [ENH] in sample fh should raise NotImplementedError in _check_fh sktime#6579

Xinyu-Wu-0000 added 2 commits June 14, 2024 15:14

in sample check in _check_fh

c15876b

check in sample error handle

b8a20ea

Xinyu-Wu-0000 requested review from achieveordie, benHeid, fkiraly and yarnabrina as code owners June 14, 2024 07:28

fkiraly requested changes Jun 14, 2024

View reviewed changes

fkiraly changed the title ~~[ENH] In sample check~~ [ENH] in BaseForecaster, move check for capability:insample to _check_fh boilerplate Jun 14, 2024

fkiraly added module:forecasting forecasting module: forecasting, incl probabilistic and hierarchical forecasting enhancement Adding new functionality labels Jun 14, 2024

Xinyu-Wu-0000 force-pushed the in_sample_check branch from 7a2e91b to b8a20ea Compare June 15, 2024 02:25

Xinyu-Wu-0000 added 6 commits June 20, 2024 11:35

add cutoff to _check_fh

6576d70

Merge branch 'main' into in_sample_check

06b41eb

remove in sample check in predict_residuals

2e1a18e

docstring

5278153

pred_int

b0e6df8

regex match in neuralforecast

015c340

fkiraly assigned Xinyu-Wu-0000 Jun 20, 2024

fkiraly reviewed Jun 20, 2024

View reviewed changes

fkiraly requested changes Jun 20, 2024

View reviewed changes

fkiraly reviewed Jun 20, 2024

View reviewed changes

Xinyu-Wu-0000 added 4 commits June 21, 2024 10:31

with pytest.raises

8f99f8a

remove passing cutoff

46a1620

remove private error raise

bd08df7

move _check_fh in update_predict_single

fa2d976

fkiraly approved these changes Jun 21, 2024

View reviewed changes

Xinyu-Wu-0000 mentioned this pull request Jun 23, 2024

[BUG] Unexpected behaviors with DynamicFactor, PyKANForecaster, and SquaringResiduals #6652

Open

fkiraly merged commit 04644f7 into sktime:main Jun 25, 2024
42 of 45 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENH] in `BaseForecaster`, move check for `capability:insample` to `_check_fh` boilerplate #6593

[ENH] in `BaseForecaster`, move check for `capability:insample` to `_check_fh` boilerplate #6593

Xinyu-Wu-0000 commented Jun 14, 2024

fkiraly left a comment •

edited

Loading

Xinyu-Wu-0000 commented Jun 20, 2024

fkiraly Jun 20, 2024

Xinyu-Wu-0000 Jun 21, 2024

fkiraly Jun 20, 2024

Xinyu-Wu-0000 Jun 21, 2024

fkiraly Jun 20, 2024

Xinyu-Wu-0000 Jun 21, 2024

fkiraly Jun 20, 2024

Xinyu-Wu-0000 Jun 21, 2024

fkiraly left a comment •

edited

Loading

fkiraly Jun 20, 2024

Xinyu-Wu-0000 Jun 21, 2024

Xinyu-Wu-0000 commented Jun 21, 2024 •

edited

Loading

Xinyu-Wu-0000 commented Jun 21, 2024

fkiraly commented Jun 21, 2024

fkiraly left a comment

Xinyu-Wu-0000 commented Jun 22, 2024 •

edited

Loading

fkiraly commented Jun 22, 2024

[ENH] in BaseForecaster, move check for capability:insample to _check_fh boilerplate #6593

[ENH] in BaseForecaster, move check for capability:insample to _check_fh boilerplate #6593

Conversation

Xinyu-Wu-0000 commented Jun 14, 2024

fkiraly left a comment • edited Loading

Choose a reason for hiding this comment

Xinyu-Wu-0000 commented Jun 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fkiraly left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xinyu-Wu-0000 commented Jun 21, 2024 • edited Loading

Xinyu-Wu-0000 commented Jun 21, 2024

fkiraly commented Jun 21, 2024

fkiraly left a comment

Choose a reason for hiding this comment

Xinyu-Wu-0000 commented Jun 22, 2024 • edited Loading

fkiraly commented Jun 22, 2024

[ENH] in `BaseForecaster`, move check for `capability:insample` to `_check_fh` boilerplate #6593

[ENH] in `BaseForecaster`, move check for `capability:insample` to `_check_fh` boilerplate #6593

fkiraly left a comment •

edited

Loading

fkiraly left a comment •

edited

Loading

Xinyu-Wu-0000 commented Jun 21, 2024 •

edited

Loading

Xinyu-Wu-0000 commented Jun 22, 2024 •

edited

Loading