-
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
[R-package] add deprecation warnings on uses of '...' in predict() and reset_parameter() #4548
Conversation
I'll look into these CI failures on
ref: https://github.com/microsoft/LightGBM/runs/3396977164?check_suite_focus=true Maybe |
I just pushed 799bd01 to address #4548 (comment) I'm still not sure why this NOTE is only showing up now, but that commit should fix it. |
TBH, I'm not sure what consistency is better: consistency with the Python-package or consistency across the R-package. Probably the latter, but will defer to other maintainers' opinion. |
@@ -469,8 +480,19 @@ Booster <- R6::R6Class( | |||
predcontrib = FALSE, | |||
header = FALSE, | |||
reshape = FALSE, | |||
params = list(), |
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 suppose we should update all our tests (and examples, but if I'm not mistaken, we don't have any yet) and use params
in them to silence new warning.
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.
Have you seen any such warnings or places where prediction parameters are passed through in our examples or tests? I checked through the tests and didn't find any uses of extra parameters that didn't match the other keyword arguments already.
To be clear, this would only apply for passing the parameters listed at https://lightgbm.readthedocs.io/en/latest/Parameters.html#predict-parameters, such as pred_early_stop
.
I searched for those parameters with git grep
and didn't find any uses in the R package. For example, with
git grep pred_early_stop R-package/
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.
Sorry, didn't know that you had already done it! Then everything is OK with this PR I think. But it's a good sign to add a test for those parameters in R-package 😉
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.
No problem, I should have mentioned it. And definitely agree that it's a sign of a missing test.
Ok thank you! I think consistency within the R package is probably more important, and consistency between R and Python is valuable but not AS important. So I think we should move forward with introducing 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.
LGTM, thanks!
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. |
Contributes to #4310 (based on #4226 (comment)).
This PR proposes adding some deprecation warnings on uses of
...
for parameters in the following functions:predict.lgb.Booster()
Booster$predict()
Booster$reset_parameter()
Notes for reviewers
Removing
...
fromreset_parameter()
in release 4.0.0 would make that method's signature identical to the Python package's implementation.LightGBM/python-package/lightgbm/basic.py
Line 2870 in 4db10d8
However, removing
...
from the signature ofBooster$predict()
andpredict.lgb.Booster()
would be different from the behavior in the Python package, which allows additional parameters passed through**kwargs
.LightGBM/python-package/lightgbm/basic.py
Lines 3399 to 3401 in c65a2e3
I think not allowing
...
, and instead encouraging the use ofparams
, for additional prediction parameters is a better interface for the R package. It makes that interface consistent with all the other parts of the R package which useparams
(like Dataset construction and training), and leaves open the possibility of an improved user experience with "other" prediction parameters by implementing something like a prediction-parameters-specific version of the feature described in #4195 (comment).But @StrikerRUS , if you feel strongly that consistency with the Python package is more important than enabling that eventual "generate a
params
list from a function" feature for the specific case ofpredict()
methods, I'd be ok with leaving the...
for them.