-
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 customizing Dataset creation in Booster.refit() (fixes #3038) #4894
[python-package] support customizing Dataset creation in Booster.refit() (fixes #3038) #4894
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 for picking this up! I left some initial comments based on the format of what you're proposing, but I'm not the best person to comment on whether or not #3038 should be adopted at all.
For example, I don't know if the comment mentioned at #3038 (comment) means that init_score
, weight
, and group
is still true.
Hopefully @guolinke @shiyu1994 or @StrikerRUS can comment on whether we should proceed with this feature.
I think it is useful that we want to change the weights of data points when refit. If we see For example, a new ranking dataset can have totally different |
@TremaMiguel Thanks for working on this! |
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 testing changes. Please see a few more suggestions I've provided.
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. Please see some additional suggestions to make the tests a bit stronger.
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 to me, thanks very much!
I've edited the PR description so it will be a bit more informative when it's used as a bullet point in the release notes.
Before this is merged, I'd like another Python maintainer like @jmoralez or @StrikerRUS to review.
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 working on this! Generally LGTM, just few minor fixes for the docstring.
@StrikerRUS @jameslamb @jmoralez is there any pending change to close this PR? |
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! Thank you so much for your contribution!
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. |
Context:
This PR aims to close #3038
Changes
kwargs_for_dataset
parameter to pass weight parameter to Dataset initialization insiderefit()
.kwargs_for_predict
parameter to pass original params topredict
method.Tests:
Based on @jtilly example on issue #3038 test that
refit
method returns a valid prediction value when passingkwargs_for_dataset
orkwargs_for_predict
.