&SmirnGregHM [BUG] ensure forecasting tuners do not vectorize over columns (variables)

[fkiraly]

This PR introduces parameters to forecasting tuners such as ForecastingGridSearchCV to control whether parameters are tuned overall, or for instances or variables.

The current behaviour is retained for the default, which is tuning per variable and for all instances.

As this is somewhat inconsistent, this PR also starts a deprecation period with an end state where the default is a single parameter tuned for all instances and variables.

Fixes #5143, see there for a discussion why this is a (logic) bug - the solution is now to move to the most intuitive setting as a default, but leave the option to tune by instance or variable to an user, and expose this cleaerly in the docstrings.

Also extends the tests as follows:

• adds test cases where the multivariate case is tested
• adds checks for presence of best_params in all relevant tests

[SmirnGregHM]

I have checked, it works as expected. Are you sure it can go without the deprecation warning though?

[fkiraly]

I have checked, it works as expected. Are you sure it can go without the deprecation warning though?

Not entirely, that's why I would like to hear from @yarnabrina.

Whether we need one hinges on how many users would think this is a bug, and how many users have actually used it as-is, accessing the best_params_ via forecasters_ (despite that not being clearly documented).

If there's even a small number of the latter, we ought to run this through a deprecation cycle as it would break their code.

What do you think, @SmirnGregHM?

[SmirnGregHM]

@fkiraly it is hard for me to estimate the number of people who would use it as it is now. Can you maybe leave something like

class ForecastingGridSearchCV(...):
    ...
    
    @property
    def _forecasters(self):
        raise AttributeError(
            "`_forecasters` property was removed and the behaviour of `ForecastingGridSearchCV`"
            " for multivariate series changed in 0.22.1. Access `best_params_`, `cv_results_`,"
            " etc. directly from `ForecastingGridSearchCV` instead."
        )

Would it break something else?

[fkiraly]

Can you maybe leave something like

Hm, that sounds like a good compromise. I don't think it will break anything, since broadcasting doesn't happen after this PR.

[fkiraly]

added the error message, slightly reworded and with more pointers. Review appreciated, @SmirnGregHM. Also added a credit to you to this PR due to your contributions.

[yarnabrina]

I'll prefer this to go through deprecation, possibly through a temporary legacy_behaviour argument similar to predict_interval/predict_quantiles methods.

(As it's probably obvious from my recent questions in Discord and Github, I don't use this yet, and please feel free to consider this with low to zero priority as just a random opinion.)

[yarnabrina]

Does it make sense to add examples documenting how to access best model/score/parameters for both univariate/multivariate(/ForecastX ?) cases?

(In the github discussion, I noticed if passed forecaster if ForecastX with same model for both, y and X forecasters get treated separately, which may not be obvious to users.)

[fkiraly]

@yarnabrina, point taken. FYI @SmirnGregHM

I now went with a deprecation solution after all - which is close to @SmirnGregHM's original suggestion to have additional parameters.

Why:

• following @yarnabrina's comment, I infer that some users may use the estimators with their current interface
• wrapping the grid search in ForecastByLevel or ColumnEnsembleForecaster to achieve the effect is not as intuitive as a dedicated parameter, and incurs more nested estimators.
• deprecation is better managed by a dedicated parameter that remains rather than a more unintuitive transition with temporary legacy_interface parameter, and it is much simpler (switch of a default, single cycle)

[SmirnGregHM]

Great, @fkiraly! I only suggest you to make the warning explicitly DeprecationWarning rather than the generic UserWarning. I also did not test the recent version yet, I will test later today. Codewise all seems good.

[SmirnGregHM]

    "to version 0.24.0.",
    DeprecationWarning

[fkiraly]

thanks, changed

[SmirnGregHM]

Good, works as expected, and old behavior is kept with a warning that it will change soon