-
Notifications
You must be signed in to change notification settings - Fork 46
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
[ENH] multiple quantile regression #108
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice addition, thanks! This will be useful in sktime
!
Blocking comments below.
Docs:
- defaults should be stated in docstring
- the exact algorithm should be stated in docstring (even if it is simple)
Algorithm:
- can we implement something for
predict_proba
? I would simply put masses on the quantiles we predict, such that any quantiles closest to that will be supported at the same point.- let's say, we predict three quantiles, 0.1, 0.5, 0.9. Then, we put masses 0.3, 0.4, 0.3 on the respective predictions, and get a weighted empirical distribution with support at three points.
- this will allow sidestep the "no regressors for alpha" problem, and give the correct predictions for the correct
alpha
. - if this
predict_proba
is implemented, the remaining methods are filled in automatically, although one might think about efficiency.
Interface requirements:
get_params
should not be overwritten like this.- do you want to allow different quantile regressors per quantile point? If yes, we would need to use the heterogenous ensemble base class. Perhaps better in a separate PR since it's a bit fiddly?
get_test_params
should return two or more test parameter settings.
Many thanks for your very quick feedback, much appreciated!
Per probability level in alpha I intend to use one regressor. Since an instance of a QuantileRegressor is passed in init, it likely already has a quantile probability level assigned to it. The value assigned to the quantile probability parameter is however overridden by the logic in fit: per quantile probability in alpha, I make a copy of the quantile regressor and set the corresponding probability with set_params. This initial quantile probability level would also be exposed to the user with get/set deep params, although changing it with those methods wouldn't make any difference. Therefore, I thought it would be best if the quantile probability of the QuantileRegressor instance passed in init wouldn't be exposed to the user with get/set params methods. Can you think of a better way to handle this? I didn't know that the heterogenous ensemble forecaster base class exists and I've just had a look at it. It's nice that it has functionality for fitting and predicting with multiple estimators (similar to what I'm doing). Using a similar base class for a probabilistic regressor would resolve the aforementioned issue, however I can also think about a few cons:
Personally (with my current understanding), I tend towards not using the heterogenous ensemble base class as it seems like it would complicate things without significant benefit. What are your thoughts on this?
Something for I'm not really familiar with the implementation of distributions in Sktime/Skpro, but would it also be possible to generate and return empirical distributions from the predicted quantiles in |
I see. The important point here is, get/set params methods must return and access the same parameters as passed to But of course that still leaves up to discussion and choice the exact parameterization. So, regarding "better ways", I think your parameterization is probably the best and most user friendly. Overall I can think of:
|
Regarding the heteorogenous ensemble, I think you list its main drawbacks correctly.
Agreed from a general perspective - I would only suggest to use it if you want different regressor per alpha, but I think you have explained that that's not what you actually want to do, so we agree on the "what" and on the "how" both. |
If we put same weights, then the quantiles will in general not be the quantiles anymore. If you go with empirical with the same support, you need to put weights with the property so the cumulative weights at the chosen quantile match. There is in general an infinity of such choices, and the "vornoi/closest" by measure seems most canonical.
I have thought about the above as well, it was my first thought actually and I rejected it because:
|
PS: I'm happy to implement the
alternatively, list of row= |
here's another argument for implementing the variant with "empirical": Imagine a probability calibrator (a transformer distr to distr) acting on empiricals, applying a smoother or linear interpolator like you propose, @Ram0nB. Now your version can be easily obtained from my version, by appending. The other way round this is not true ("easy" or perhaps even "possible"), as the transformation is not invertible, or at least not easily so in the generic case. Therefore, the "@Ram0nB version" is actually a pipeline (transformed target regressor for proba) of the "@fkiraly version" with a probability calibrator that does the smoothing or interpolation of the empirical. From a general design perspective in sklearn-like libraries, it is a good idea (and one that has helped a lot in sktime) to implement "minimal components" rather than "all-in-one estimators". I.e., if it looks like X = SomePipeline(Y, Z), there should be a strong preference to implement three estimators - Y, Z, SomePipeline - rather than X. An even stronger preference if some of the three are already existing. |
Thanks for the clarification @fkiraly, I agree with all your points. I processed all the requested changes, looking forward to your feedback 😃 .
This would be great, thanks! The utility function is called |
ok, will look at it after work today - 3.12 compatible releases of skpro and sktime having priority first, but that should be out of the way quickly (I hope...) |
I added a draft for |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #108 +/- ##
==========================================
+ Coverage 70.00% 71.73% +1.73%
==========================================
Files 97 98 +1
Lines 5157 5264 +107
Branches 952 971 +19
==========================================
+ Hits 3610 3776 +166
+ Misses 1317 1220 -97
- Partials 230 268 +38 ☔ View full report in Codecov by Sentry. |
- prob pred params mandatory - docstring updated - predict_proba finalised
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Many thanks for the quick
Only the prob. prediction methods wouldn't work as I did an input check in init, but it indeed makes more sense to make all parameters for the prob prediction methods non-optional. The newest version has the following changes:
Do you think that the docstring explains the "algorithm" for all prob prediction methods extensive enought? |
ok, let's see if it runs! 😁 |
I suppose as good as it can be without math (perhaps instead of "nearest probability", "nearest quantile probability"). ALthough I think it can be further improved by adding a little math. |
linting fails because you have an empty notebook in the branch, kindly remove |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, I see you found an efficient algorithm for _predict_quantiles
too.
One thing that might be worth to check before we merge, if the user passes non-sorted alpha
either to predict_quantiles
, or to __init__
, is all the logic still correct? I have a nagging feeling that it might not be, and I'm not sure whether we check in the tests either.
- updated docstring with little math - handle unsorted alpha
I see that the tests are failing, I'll have a look at it tomorrow :) |
Is this readthedocs again - might be unrelated to you, I thought I fixed it: #122 |
ah, I think it's the docstring. If you use TeX, and hence the backslash character, the string needs to be preceded by an "r" character, i.e., r"""This is a docstring.
This is a docstring description with :math:`\pi`
""" |
updated the docstring with the |
FYI, the regressor was failing a test since the I made two small changes which should fix that - feel free to revert or change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works now, with my changes, see above.
Happy to merge if you are happy with my changes.
Many thanks for your changes, all looks great to me! 👍 |
Thanks for your contribution! Once released in 2.1.1, you should be able to use this with |
Reference Issues/PRs
Fixes #107
What does this implement/fix? Explain your changes.
For quantile regression, often more than one quantile probability is of interest. However, existing Sklearn compatible quantile regressors always fit and predict a single quantile probability. To the best of my knowdlegde, there is no standardized way to integrate multiple quantile regression with Sktime/Skpro probabilistic prediction methods such as predict_quantiles/predict_intervals. This PR adds new Skpro regressor that wraps multiple quantile regressors and supports probabilistic predictions from wrapped regressors.
Does your contribution introduce a new dependency? If yes, which one?
No
What should a reviewer concentrate their feedback on?
All added/changed files :)
Did you add any tests for the change?
No - I ran the Skpro tests locally without issues
Any other comments?
No
PR checklist
For all contributions
How to: add yourself to the all-contributors file in the
skpro
root directory (not theCONTRIBUTORS.md
). Common badges:code
- fixing a bug, or adding code logic.doc
- writing or improving documentation or docstrings.bug
- reporting or diagnosing a bug (get this pluscode
if you also fixed the bug in the PR).maintenance
- CI, test framework, release.See here for full badge reference
For new estimators
docs/source/api_reference/taskname.rst
, follow the pattern.Examples
section.python_dependencies
tag and ensureddependency isolation, see the estimator dependencies guide.