-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
[python] add return_cvbooster flag to cv func and publish _CVBooster (#283,#2105,#1445) #3204
Conversation
Hmm... At a glance, I think this change did not trigger breaking CI.
|
Random network error. Fixed. Sorry for the inconvenience. |
@momijiame Please add new public class to the documentation: https://github.com/microsoft/LightGBM/blob/master/docs/Python-API.rst. @matsuken92 Can you please help to review this PR? |
@StrikerRUS Thank you quick response! I added the CVBooster to the documentation. |
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.
@momijiame Thanks for your quick fixes! Please take a look at some my comments below:
- Add some clarifications to the documentation - Rename CVBooster.append to make private - Decrease iteration rounds of testing to save CI time - Use CVBooster as root member of lgb
@StrikerRUS Thank you so much for the feedback! I reflected the comments. |
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.
Please add some more checks in tests and I believe we should document best_iteration
and boosters
attributes to let users know about them. (You can simply Commit suggestion
if you agree with it.)
Co-authored-by: Nikita Titov <[email protected]>
Co-authored-by: Nikita Titov <[email protected]>
Co-authored-by: Nikita Titov <[email protected]>
Thank you for the great suggestion. I committed. |
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.
@momijiame Thank you very much for this enhancement! LGTM!
I'd like to get one more review before merging (preferably from @matsuken92 or @henry0312).
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
Gently ping @matsuken92 as you wanted to give your review for the general idea of this PR. |
I noticed that this PR also solves #1445, so I updated the title. NOTE: It would be more useful to support |
@momijiame : thank you for this much needed feature! After running For example:
On the other hand, Without best iteration it is not possible to obtain predictions for each fold nor feature importances (which rely on the iterations number). In fact I suspect they may be incorrect now, because they are the same regardless of the value their
For individual folds it is also not possible to perform the usual
|
@mirekphd Thank you for your feedback. We could consider setting best_iteration of Boosters to all the same value, but that would break backwards compatibility. Even before return_cvbooster option was added, the technique of retrieving CVBooster (_CVBooster) by using Callback was exist. Also, if we want to train Boosters on each folds, we can simply use train() function. The documentation could be improved on the above, but I do not think the behavior should be changed. |
Not in this case:). Please note that we are talking only about early stopping here, where To see that each fold uses a different number of iterations it's enough to see that we have only one model training pass per each CV model (as opposed to two passes), and also from my example above, where training stopped at iterations from 150 to 155 rounds (as reported by The information on the @momijiame suggested that ligthgbm uses average iterations number computed across all folds in the first modeling pass (or somehow avoids two passes by synchronizing between folds to avoid performance penalty of two passes and knowing average after a single pass). That would be possibly inefficient (if two passes are needed), but more importantly could lead to lower predictive accuracy, with some folds underfitted and some overfitted, with respectively too few or too many boosting rounds (depending on their to their relation to the average). We have been always using separate numbers of boosting rounds for each CV fold as far as I can remember in our internal library, and this may explain its accuracy differences as compared to @guolinke - could you find time for a few words clarification here? Are all folds in |
@mirekphd |
This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this. |
This PR allows users to get trained boosters from cv function directly.
This feature is useful for using ensemble techniques and OOF predictions.
Add the changes that proposed in #283 discussion.
return_cvbooster
flag to cv function_CVBooster
to make public