Skip to content
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] fix warnings in demos #4569

Merged
merged 5 commits into from
Aug 29, 2021
Merged

[R-package] fix warnings in demos #4569

merged 5 commits into from
Aug 29, 2021

Conversation

jameslamb
Copy link
Collaborator

Similar to the changes made to examples in the R package's API docs in #4568, this PR fixes warnings in demos related to the deprecation warnings added as part of #4226.

For example, if you run Rscript R-package/demo/basic_walkthrough.R on current master, you'll see the following on stdout.

Warning message:
In lgb.train(data = dtrain2, num_leaves = 4L, learning_rate = 1, :
lgb.train: Found the following passed through '...': num_leaves, learning_rate, nthread, objective. These will be used, but in future releases of lightgbm, this warning will become an error. Add these to 'params' instead. See ?lgb.train for documentation on how to call this function.

This PR fixes them and converts examples to the new pattern we want to encourage as of v3.3.0 (#4310), passing all parameters through params.

Notes for Reviewers

By release 4.0.0, the goal is to have these demos converted to R package vignettes (#1944). However, until then I think it's at least worth updating these to the new recommended pattern, in case users reference the demos as examples.

Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix duplicated min_data param here:

, min_data = 1L
, learning_rate = 0.1
, min_data = 0L

And update multiclass.R demo in the following two places:

, min_data = 1L
, learning_rate = 1.0

, min_data = 1L
, learning_rate = 1.0
, early_stopping_rounds = 10L
, objective = "multiclass"
, metric = "multi_error"
, num_class = 3L

model_builtin <- lgb.train(
list()
params = params
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to pass keyword arguments before positional ones in R?

Suggested change
params = params
params

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, R doesn't care about that difference. R's argument parsing will first match all values passed as keyword args, then pass remaining positional arguments to remaining unmatched parameters.

f <- function(x1, x2,  x3) {
    print(paste0("x1: ", x1, " | x2: ", x2, " | x3: ", x3))
}

f("a", "b", "c")
# [1] "x1: a | x2: b | x3: c"

f(x2 = "a", "b", "c")
# [1] "x1: b | x2: a | x3: c"

f("b", x3 = "a", "c")
# [1] "x1: b | x2: c | x3: a"

I'm fine with the suggestion, but just wanted to show R isn't opinionated about that the way Python is.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, interesting! Thanks for the explanation and sorry for the false alarm.

R-package/demo/multiclass_custom_objective.R Outdated Show resolved Hide resolved
R-package/demo/weight_param.R Outdated Show resolved Hide resolved
@jameslamb
Copy link
Collaborator Author

Please fix duplicated min_data param here...and update multiclass.R

Fixed in 3838d33, thanks.

Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thank you! Just one minor suggestion below:

R-package/demo/multiclass.R Outdated Show resolved Hide resolved
Co-authored-by: Nikita Titov <nekit94-08@mail.ru>
@jameslamb jameslamb merged commit 846e895 into master Aug 29, 2021
@jameslamb jameslamb deleted the r/demo-warnings branch August 29, 2021 01:01
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants