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.

Sign up for GitHub

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

Jump to bottom

[R-package] add deprecation warnings on uses of '...' in predict() and reset_parameter() #4548

Merged
merged 2 commits into from
Aug 25, 2021
Merged

[R-package] add deprecation warnings on uses of '...' in predict() and reset_parameter() #4548

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e8232fa
[R-package] add deprecation warnings on uses of '...' in predict()
jameslamb Aug 23, 2021
799bd01
add importFrom(utils, modifyList) in NAMESPACE
jameslamb Aug 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions R-package/NAMESPACE
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,5 +48,6 @@ importFrom(graphics,par)
importFrom(jsonlite,fromJSON)
importFrom(methods,is)
importFrom(stats,quantile)
importFrom(utils,modifyList)
importFrom(utils,read.delim)
useDynLib(lib_lightgbm , .registration = TRUE)
60 changes: 53 additions & 7 deletions R-package/R/lgb.Booster.R
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#' @importFrom R6 R6Class
#' @importFrom utils modifyList
Booster <- R6::R6Class(
classname = "lgb.Booster",
cloneable = FALSE,
Expand Down Expand Up @@ -38,7 +39,7 @@ Booster <- R6::R6Class(
stop("lgb.Booster: Can only use lgb.Dataset as training data")
}
train_set_handle <- train_set$.__enclos_env__$private$get_handle()
params <- modifyList(params, train_set$get_params())
params <- utils::modifyList(params, train_set$get_params())
params_str <- lgb.params2str(params = params)
# Store booster handle
handle <- .Call(
Expand Down Expand Up @@ -176,11 +177,21 @@ Booster <- R6::R6Class(

reset_parameter = function(params, ...) {

additional_params <- list(...)
if (length(additional_params) > 0L) {
warning(paste0(
"Booster$reset_parameter(): Found the following passed through '...': "
, paste(names(additional_params), collapse = ", ")
, ". These will be used, but in future releases of lightgbm, this warning will become an error. "
, "Add these to 'params' instead."
))
}

if (methods::is(self$params, "list")) {
params <- modifyList(self$params, params)
params <- utils::modifyList(self$params, params)
}

params <- modifyList(params, list(...))
params <- utils::modifyList(params, additional_params)
params_str <- lgb.params2str(params = params)

.Call(
Expand Down Expand Up @@ -469,8 +480,19 @@ Booster <- R6::R6Class(
predcontrib = FALSE,
header = FALSE,
reshape = FALSE,
params = list(),
Copy link
Collaborator

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.

Copy link
Collaborator Author

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/

Copy link
Collaborator

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 😉

Copy link
Collaborator Author

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.

...) {

additional_params <- list(...)
if (length(additional_params) > 0L) {
warning(paste0(
"Booster$predict(): Found the following passed through '...': "
, paste(names(additional_params), collapse = ", ")
, ". These will be used, but in future releases of lightgbm, this warning will become an error. "
, "Add these to 'params' instead. See ?predict.lgb.Booster for documentation on how to call this function."
StrikerRUS marked this conversation as resolved.
Show resolved Hide resolved
))
}

if (is.null(num_iteration)) {
num_iteration <- self$best_iter
}
Expand All @@ -480,7 +502,7 @@ Booster <- R6::R6Class(
}

# Predict on new data
params <- list(...)
params <- utils::modifyList(params, additional_params)
predictor <- Predictor$new(
modelfile = private$handle
, params = params
Expand Down Expand Up @@ -699,8 +721,11 @@ Booster <- R6::R6Class(
#' @param header only used for prediction for text file. True if text file has header
#' @param reshape whether to reshape the vector of predictions to a matrix form when there are several
#' prediction outputs per case.
#' @param ... Additional named arguments passed to the \code{predict()} method of
#' the \code{lgb.Booster} object passed to \code{object}.
#' @param params a list of additional named parameters. See
#' \href{https://lightgbm.readthedocs.io/en/latest/Parameters.html#predict-parameters}{
#' the "Predict Parameters" section of the documentation} for a list of parameters and
#' valid values.
#' @param ... Additional prediction parameters. NOTE: deprecated as of v3.3.0. Use \code{params} instead.
#' @return For regression or binary classification, it returns a vector of length \code{nrows(data)}.
#' For multiclass classification, either a \code{num_class * nrows(data)} vector or
#' a \code{(nrows(data), num_class)} dimension matrix is returned, depending on
Expand Down Expand Up @@ -728,7 +753,17 @@ Booster <- R6::R6Class(
#' , learning_rate = 1.0
#' )
#' preds <- predict(model, test$data)
#'
#' # pass other prediction parameters
#' predict(
#' model,
#' test$data,
#' params = list(
#' predict_disable_shape_check = TRUE
#' )
#' )
#' }
#' @importFrom utils modifyList
#' @export
predict.lgb.Booster <- function(object,
data,
Expand All @@ -739,12 +774,23 @@ predict.lgb.Booster <- function(object,
predcontrib = FALSE,
header = FALSE,
reshape = FALSE,
params = list(),
...) {

if (!lgb.is.Booster(x = object)) {
stop("predict.lgb.Booster: object should be an ", sQuote("lgb.Booster"))
}

additional_params <- list(...)
if (length(additional_params) > 0L) {
warning(paste0(
"predict.lgb.Booster: Found the following passed through '...': "
, paste(names(additional_params), collapse = ", ")
, ". These will be used, but in future releases of lightgbm, this warning will become an error. "
, "Add these to 'params' instead. See ?predict.lgb.Booster for documentation on how to call this function."
))
}

return(
object$predict(
data = data
Expand All @@ -755,7 +801,7 @@ predict.lgb.Booster <- function(object,
, predcontrib = predcontrib
, header = header
, reshape = reshape
, ...
, params = utils::modifyList(params, additional_params)
)
)
}
Expand Down
5 changes: 3 additions & 2 deletions R-package/R/lgb.Dataset.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

#' @importFrom methods is
#' @importFrom R6 R6Class
#' @importFrom utils modifyList
Dataset <- R6::R6Class(

classname = "lgb.Dataset",
Expand Down Expand Up @@ -535,7 +536,7 @@ Dataset <- R6::R6Class(
return(invisible(self))
}
if (lgb.is.null.handle(x = private$handle)) {
private$params <- modifyList(private$params, params)
private$params <- utils::modifyList(private$params, params)
} else {
tryCatch({
.Call(
Expand All @@ -552,7 +553,7 @@ Dataset <- R6::R6Class(

# If updating failed but raw data is available, modify the params
# on the R side and re-set ("deconstruct") the Dataset
private$params <- modifyList(private$params, params)
private$params <- utils::modifyList(private$params, params)
self$finalize()
})
}
Expand Down
4 changes: 3 additions & 1 deletion R-package/R/lgb.cv.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@ CVBooster <- R6::R6Class(
return(invisible(NULL))
},
reset_parameter = function(new_params) {
for (x in boosters) { x$reset_parameter(new_params) }
for (x in boosters) {
x$reset_parameter(params = new_params)
}
return(invisible(self))
}
)
Expand Down
18 changes: 16 additions & 2 deletions R-package/man/predict.lgb.Booster.Rd
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 7 additions & 7 deletions R-package/tests/testthat/test_basic.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,7 @@ test_that("lgb.cv() fit on linearly-relatead data improves when using linear lea
cv_bst_linear <- lgb.cv(
data = dtrain
, nrounds = 10L
, params = modifyList(params, list(linear_tree = TRUE))
, params = utils::modifyList(params, list(linear_tree = TRUE))
, nfold = 5L
)
expect_is(cv_bst_linear, "lgb.CVBooster")
Expand Down Expand Up @@ -1767,7 +1767,7 @@ test_that("lgb.train() fit on linearly-relatead data improves when using linear
bst_linear <- lgb.train(
data = dtrain
, nrounds = 10L
, params = modifyList(params, list(linear_tree = TRUE))
, params = utils::modifyList(params, list(linear_tree = TRUE))
, valids = list("train" = dtrain)
)
expect_true(lgb.is.Booster(bst_linear))
Expand Down Expand Up @@ -1798,7 +1798,7 @@ test_that("lgb.train() w/ linear learner fails already-constructed dataset with
bst_linear <- lgb.train(
data = dtrain
, nrounds = 10L
, params = modifyList(params, list(linear_tree = TRUE))
, params = utils::modifyList(params, list(linear_tree = TRUE))
)
}, regexp = "Cannot change linear_tree after constructed Dataset handle")
})
Expand Down Expand Up @@ -1839,7 +1839,7 @@ test_that("lgb.train() works with linear learners even if Dataset has missing va
bst_linear <- lgb.train(
data = dtrain
, nrounds = 10L
, params = modifyList(params, list(linear_tree = TRUE))
, params = utils::modifyList(params, list(linear_tree = TRUE))
, valids = list("train" = dtrain)
)
expect_true(lgb.is.Booster(bst_linear))
Expand Down Expand Up @@ -1887,7 +1887,7 @@ test_that("lgb.train() works with linear learners, bagging, and a Dataset that h
bst_linear <- lgb.train(
data = dtrain
, nrounds = 10L
, params = modifyList(params, list(linear_tree = TRUE))
, params = utils::modifyList(params, list(linear_tree = TRUE))
, valids = list("train" = dtrain)
)
expect_true(lgb.is.Booster(bst_linear))
Expand Down Expand Up @@ -1925,7 +1925,7 @@ test_that("lgb.train() works with linear learners and data where a feature has o
bst_linear <- lgb.train(
data = dtrain
, nrounds = 10L
, params = modifyList(params, list(linear_tree = TRUE))
, params = utils::modifyList(params, list(linear_tree = TRUE))
)
expect_true(lgb.is.Booster(bst_linear))
})
Expand Down Expand Up @@ -1964,7 +1964,7 @@ test_that("lgb.train() works with linear learners when Dataset has categorical f
bst_linear <- lgb.train(
data = dtrain
, nrounds = 10L
, params = modifyList(params, list(linear_tree = TRUE))
, params = utils::modifyList(params, list(linear_tree = TRUE))
, valids = list("train" = dtrain)
)
expect_true(lgb.is.Booster(bst_linear))
Expand Down