Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] fix forecaster default predict_quantiles for multivariate data #3106

Merged
merged 3 commits into from Jul 30, 2022

Conversation

fkiraly
Copy link
Collaborator

@fkiraly fkiraly commented Jul 26, 2022

This fixes a bug reported by @lbventura in #3105: the default implementation of _predict_quantiles, when _predict_interval is implemented, would return quantiles only for the first variable - which is fine in univariate forecasting, but produces too few columns in multivariate forecasting.

This has now been fixed, the correct columns are now returned in both cases, under the condition that _predict_quantiles is implemented but _predict_interval is not.

Odd that this was not tested so far, I still need to investigate why.

To the bets of my knowledge, we should be testing the proba predict methods with multivariate scenarios?

@fkiraly fkiraly added module:forecasting forecasting module: forecasting, incl probabilistic and hierarchical forecasting bugfix Fixes a known bug or removes unintended behavior labels Jul 26, 2022
@fkiraly
Copy link
Collaborator Author

fkiraly commented Jul 27, 2022

Reason why tests did not catch this:

vectorization takes priority over the default fill in, so forecasters that are not genuinely multivariate will call the _predict_interval from _predict_quantiles via vectorization, which is fine.

The problem with the logic therefore occurs only for genuinely multivariate, probabilistic forecasters, of which there were, so far, none in the code base.

Somewhat surprising since VAR is in the code base, but that does not have probabilistic prediction implemented.

AurumnPegasus added a commit to AurumnPegasus/sktime that referenced this pull request Jul 28, 2022
@fkiraly
Copy link
Collaborator Author

fkiraly commented Jul 30, 2022

Included as part of #3105 and #2925

@fkiraly fkiraly merged commit 9e306d5 into main Jul 30, 2022
@fkiraly fkiraly deleted the fix-forecaster-default-predict-quantiles branch July 30, 2022 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fixes a known bug or removes unintended behavior module:forecasting forecasting module: forecasting, incl probabilistic and hierarchical forecasting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant