-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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-package] support saving and loading CVBooster (fixes #3556) #5160
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.
Thanks very much for taking this on!
I will provide a more thorough review in the next few days, for now please see my initial round of suggestions.
Also note that this does not totally resolve #3556. As mentioned in the discussion there, it should also be possible to pickle a CVBooster
and load one from a pickle file. I think that work should be picked up in a follow-up PR after this one.
Co-authored-by: James Lamb <jaylamb20@gmail.com>
Co-authored-by: James Lamb <jaylamb20@gmail.com>
Co-authored-by: James Lamb <jaylamb20@gmail.com>
This can be solved by simply defining |
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.
Thanks for the test changes! I'd originally suggested splitting pickling into a separate PR to keep this one small, but now that I see how small the changes are I think it's ok to leave all in this PR.
Please see a few more recommendations on the tests. I'll provide a more thorough review once I have a chance to run this code myself and inspect the outputs.
Co-authored-by: James Lamb <jaylamb20@gmail.com>
I signed the CLA and once the status was OK, but at some point, the bot status changed again to "not signed yet". |
@nyanp Sorry about this inconvenience! |
@jameslamb |
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.
Thanks a lot for this PR!
Please consider checking some my comments below.
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.
Thanks very much for this! I agree with all of @StrikerRUS 's suggestions, so please address those before we can merge this.
I have no further comments right now.
Kindly ping @nyanp |
@nyanp are you interested in continuing this pull request? This is a really useful addition to the library and we're very grateful for your work so far. I'd love to see this completed. |
@jameslamb |
Co-authored-by: Nikita Titov <nekit94-08@mail.ru>
I believe all review content has now been resolved. |
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.
Great job, thank you!
LGTM!
Just some minor comments below.
Co-authored-by: Nikita Titov <nekit94-08@mail.ru>
Co-authored-by: Nikita Titov <nekit94-08@mail.ru>
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 just re-reviewed. All of my and other reviewers' comments have been addressed, and I don't have any other suggestions.
EXCELLENT work @nyanp 🎉 🎉 🎉
We really appreciate your hard work on this very impactful feature, and your patience with the review process. I hope you'll consider contributing to LightGBM more in the future.
@jameslamb |
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 resolves #3556 by implementing the following method in
CVBooster
: