-
Notifications
You must be signed in to change notification settings - Fork 917
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
Feat/probabilistic-lgbm-linreg #831
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #831 +/- ##
==========================================
+ Coverage 91.27% 91.37% +0.09%
==========================================
Files 70 70
Lines 6978 7105 +127
==========================================
+ Hits 6369 6492 +123
- Misses 609 613 +4 ☔ View full report in Codecov by Sentry. |
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.
Looks good! Just got a few comments
return TimeSeries(new_xa) | ||
|
||
@staticmethod | ||
def _check_quantiles(quantiles): |
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.
This code is already present in utils.likelihood_models.py
. Could we extract this somewhere (e.g. in utils/utils.py
)?
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.
i moved it to utils/utils.py
|
||
return self | ||
|
||
super().fit( |
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.
This will also be called one last time when likelihood=quantile
. is this intended?
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.
does the return self
statement in line 171 not exit the method?
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.
Oh right, maybe you could add an else
clause, just for clarity here?
Co-authored-by: Julien Herzen <[email protected]>
Co-authored-by: Julien Herzen <[email protected]>
Co-authored-by: Julien Herzen <[email protected]>
Co-authored-by: Julien Herzen <[email protected]>
Co-authored-by: Julien Herzen <[email protected]>
Co-authored-by: Julien Herzen <[email protected]>
…quantiles to utils/utils.py
…to feat/probabilistic-lgbm
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.
LGTM, thanks!
Got a tiny suggestion to add in else
clause around line 170 of linear_regression.py
, then all good.
First step towards #616.
Summary
Implements quantile and poisson regression for the
LightGBMModel
andLinearRegressionModel
.