[BUG] pass user passed parameters to ForecastX to underlying estimators

[yarnabrina]

Reference Issues/PRs

Fixes #4386

What does this implement/fix? Explain your changes.

Before this PR, predict_interval or predict_quantiles of ForecastX did not pass coverage or alpha parameters passed by user to the corresponding methods of self.forecaster_y_. This PR addresses this bug.

fkiraly's comment:
Test coverage will be added by other PR, see discussion there:

• [ENH] cleaned up probabilistic forecasting tests for quantile and interval predictions #4393
• [ENH] cover list input case for test_predict_interval/coverage and test_predict_quantiles/alpha in forecaster contract tests #4394

(FK comment end)

before

>>> pipe.predict_interval(fh=fh, X=X_test.drop(columns=columns), coverage=0.95)
          Coverage              
               0.9              
             lower         upper
1960  69583.430473  70587.653223
1961  69569.814972  70576.864647
1962  72161.834476  73168.900067
>>> pipe.predict_quantiles(fh=fh, X=X_test.drop(columns=columns), alpha=[0.25, 0.75])
         Quantiles              
              0.05          0.95
1960  69583.430473  70587.653223
1961  69569.814972  70576.864647
1962  72161.834476  73168.900067

after

>>> pipe.predict_interval(fh=fh, X=X_test.drop(columns=columns), coverage=0.95)
          Coverage              
              0.95              
             lower         upper
1960  69487.239242  70683.844453
1961  69473.352959  70673.326660
1962  72065.370939  73265.363604
>>> pipe.predict_quantiles(fh=fh, X=X_test.drop(columns=columns), alpha=[0.25, 0.75])
         Quantiles              
              0.25          0.75
1960  69879.645730  70291.437965
1961  69866.864086  70279.815532
1962  72458.888285  72871.846258

Does your contribution introduce a new dependency? If yes, which one?

No.

What should a reviewer concentrate their feedback on?

There are two other methods of ForecastX which does not use passed parameters, viz. cov in _predict_var and marginal in _predict_proba. However, these are not used even in BaseForecaster. I've made changes (similar to above two) so that these parameters are always passed, whether or not used by any estimator.

Did you add any tests for the change?

No.

Any other comments?

The setup for the code snippets shared above is as follows:

Setup Codes

# imports
from sktime.datasets import load_longley
from sktime.forecasting.arima import ARIMA
from sktime.forecasting.base import ForecastingHorizon
from sktime.forecasting.compose import ForecastX
from sktime.forecasting.model_selection import temporal_train_test_split
from sktime.forecasting.var import VAR

# data
y, X = load_longley()
y_train, _, X_train, X_test = temporal_train_test_split(y, X, test_size=3)
fh = ForecastingHorizon([1, 2, 3])
columns = ["ARMED", "POP"]

# training
pipe = ForecastX(
    forecaster_X=VAR(),
    forecaster_y=ARIMA(),
    columns=columns,
)
pipe = pipe.fit(y_train, X=X_train, fh=fh)

PR checklist

For all contributions

• I've added myself to the list of contributors.
• Optionally, I've updated sktime's CODEOWNERS to receive notifications about future changes to these files.
• The PR title starts with either [ENH], [MNT], [DOC], or [BUG] indicating whether the PR topic is related to enhancement, maintenance, documentation, or bug.

For new estimators

• I've added the estimator to the online documentation.
• I've updated the existing example notebooks or provided a new one to showcase how my estimator works.

[fkiraly]

Excellent, thanks!

This should hopefully fix it.

Let us know if you also want to try to fix the tests (or find out why this was not detected), I won't merge until you had the chance so you can also do it in this PR if you want.

[fkiraly]

general rationale: every bugfix should optimally be covered by a test, so we would see if we accidentally reintroduce the bug later.
That is, you want to add a test that would have failed before the fix, and therefore "certifies" for the fix.

In this case, it's up to you if you want to add that test, if not, one of the developers will add it later.

Where, "adding" probably means fixing the existing test that was supposed to check for this but didn't, see the discussion in #4386.

[fkiraly]

I've been working on the probabilistic forecasting tests as well - I think this covers it now: #4393, so it should find the bug while this is not merged.

[fkiraly]

merged with update from main, still aprpove

[yarnabrina]

I found why the tests didn't capture the issue. It's because test_predict_quantiles never went till _check_predict_quantiles step for ForecastX, as the created estimator is always of this type, which doesn't support predict_quantiles:

ForecastX(forecaster_X=DirectReductionForecaster(X_treatment='shifted',
                                                 estimator=LinearRegression(),
                                                 pooling='global',
                                                 window_length=3),
          forecaster_y=DirectTabularRegressionForecaster(estimator=LinearRegression(),
                                                         window_length=3))

So, I think the tests should ensure that types of estimator do vary over different iterations, as currently all 8 cases use this same estimator.

---

Edit: Sorry, just saw #4393. So, I guess this is addressed there completely?

[yarnabrina]

Also, for both coverage and quantiles, tests are parameterized on [0.05, 0.1]. This means these tests can never run on a list input corresponding to these parameters, which is supported by the underlying methods. Is there a reason for this, or this can be modified? This makes the following code to never run.

sktime/sktime/forecasting/tests/test_all_forecasters.py

Lines 406 to 415 in 8332ece

	if isinstance(alpha, list):
	# sorts the columns that correspond to alpha values
	pred_quantiles = pred_quantiles.reindex(
	columns=pred_quantiles.columns.reindex(sorted(alpha), level=1)[0]
	)
	# check if values are monotonically increasing
	for var in pred_quantiles.columns.levels[0]:
	for index in range(len(pred_quantiles.index)):
	assert pred_quantiles[var].iloc[index].is_monotonic_increasing

And the above code really expects a non-list alpha, as otherwise expected = pd.MultiIndex.from_product([["Quantiles"], [alpha]]) will fail with TypeError: unhashable type: 'list'.

[fkiraly]

Sorry, just saw #4393. So, I guess this is addressed there completely?

Apologies for crossing over here.
Yes, I was working on the probabilistic prediction interface and also noticed this.
The change I made to ForecastX is add a case where the y-forecaster is probabilistic.

Is there a reason for this, or this can be modified? This makes the following code to never run.

I see - not sure what the reason is, the test might be left over as a refactor from the time where we did not allow multiple alpha to be passed.

Yes, we should indeed add list-like alpha and coverage too.
Would you like to make a PR?

We may have to think in which sequence we merge things - you could branch off my changes in #4393.