-
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] ensure that callbacks respect verbosity from params #5199
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.
Very thorough tests!
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! 👍
# Set validation as oneself | ||
if (verbose > 0L) { | ||
if (params[["verbosity"]] > 0L) { | ||
train_args[["valids"]][["train"]] <- dtrain | ||
} |
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.
Quite strange behavior, to be honest...
Maybe remove it in v4.0.0
?
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.
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 doing this!
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 #4862.
Fixes an issue introduced by #4899 .
In the R package,
lightgbm()
,lgb.train()
, andlgb.cv()
have a keyword argumentverbose
. #4899 added changes to ensure that that verbosity parameters passed throughparams
took precedence over that keyword argument.However, evaluation and and early stopping callbacks still reference that keyword argument, which can lead to inconsistent behavior where some printed messages' verbosity can only be controlled by the keyword argument.
While working on this, I also discovered another closely-related bug...parameter
verbose
in calls tocb_early_stop()
is being passed an integer, but that value is then checked withisTRUE()
...which means that no messages are ever printed from that callback.LightGBM/R-package/R/callback.R
Line 207 in dad3915
This PR proposes changes to ensure that it's possible to silence early stopping messages and evaluation messages using
params
.Notes for Reviewers
This PR reduces the number of logs printed by the R package's tests from 2095 lines to 1616 lines.