Update extension template for predict_quantiles #1780
Conversation
extension_templates/forecasting.py
Outdated
| @@ -90,8 +90,10 @@ class MyForecaster(BaseForecaster): | |||
| "requires-fh-in-fit": True, # is forecasting horizon already required in fit? | |||
| "X-y-must-have-same-index": True, # can estimator handle different X/y index? | |||
| "enforce_index_type": None, # index type that needs to be enforced in X/y | |||
| "capability:pred_int": False, # does forecaster implement predict_quantiles? | |||
| # in the future can be changed to capability:predict_quantiles | |||
There was a problem hiding this comment.
bit unclear what this means to the person who wants to implement the forecaster. Perhaps say sth like "deprecated and will be renamed to capability:predict_quantiles in 0.11.0"?
kejsitake
force-pushed
the
template-quantiles
branch
from
e7b8d9a
to
276dd36
Compare
|
Hi! Thank you for the comments. I made these changes and also removed the |
extension_templates/forecasting.py
Outdated
| alpha : float or list, optional (default=0.95) | ||
| A significance level or list of significance levels. | ||
| fh : int, list, np.array or ForecastingHorizon | ||
| Forecasting horizon, default = y.index (in-sample forecast) |
There was a problem hiding this comment.
there is no y here, so the default does not seem to make sense - or does it and I'm missing sth?
If this is a typo, please remove the "default" part.
kejsitake
force-pushed
the
template-quantiles
branch
from
276dd36
to
ded6853
Compare
|
Fixed the typo! Thank you for the comments! |
There was a problem hiding this comment.
Thanks, looks great!
I have one more request, looking through this.
I don't think we should import from private modules here, i.e., the DEFAULT_ALPHA.
How I would avoid it is to set the default in the _predict_quantiles to None or, perhaps better, 0.5. In the body of the function, explain that alpha is guaranteed to be a float or list of float by predict_quantiles calling this function, which is by default the DEFAULT_ALPHA coming from predict_quantiles.
|
I believe I made this changes. I will keep in mind to include |
There was a problem hiding this comment.
Great, thanks!
I noticed another thing, unfortunately, sorry for the bits-and-pieces here.
The docstring of _predict_quantiles is incorrect in describign the return, pred_quantiles. This is incorrectly referring to coverage, i.e., to the input of _predict_interval. Instead, it shoudl be referring to alpha.
|
Thank you for the comments! |
What does this implement/fix? Explain your changes.
Updates the extension template to account for changes introducing predict_quantiles.
Does your contribution introduce a new dependency? If yes, which one?
No
Any other comments?
This was included in another PR #1579, but I am separating to make review easier. I have fixed some of the issues mentioned there.