[R-package] respect 'verbose' argument in lgb.cv() (fixes #4667) (#4903)

* fixes * revert debugging code * add test * check for LightGBM explicitly * empty commit * revert unnecessary line deletion * respect verbose everywhere and update params for constructted dataset * Update R-package/R/lgb.Dataset.R Co-authored-by: Nikita Titov <[email protected]> Co-authored-by: Nikita Titov <[email protected]>
microsoft · Jan 6, 2022 · 1d8bfd8 · 1d8bfd8
1 parent 100a10d
commit 1d8bfd8
Show file tree

Hide file tree

Showing 2 changed files with 62 additions and 5 deletions.
diff --git a/R-package/R/lgb.Dataset.R b/R-package/R/lgb.Dataset.R
@@ -109,7 +109,7 @@ Dataset <- R6::R6Class(
                             params = list()) {
 
       # the Dataset's existing parameters should be overwritten by any passed in to this call
-      params <- modifyList(self$get_params(), params)
+      params <- modifyList(private$params, params)
 
       # Create new dataset
       ret <- Dataset$new(
@@ -535,7 +535,7 @@ Dataset <- R6::R6Class(
       return(
         Dataset$new(
           data = NULL
-          , params = self$get_params()
+          , params = private$params
           , reference = self
           , colnames = private$colnames
           , categorical_feature = private$categorical_feature
@@ -554,15 +554,17 @@ Dataset <- R6::R6Class(
       if (length(params) == 0L) {
         return(invisible(self))
       }
+      new_params <- utils::modifyList(private$params, params)
       if (lgb.is.null.handle(x = private$handle)) {
-        private$params <- utils::modifyList(private$params, params)
+        private$params <- new_params
       } else {
         tryCatch({
           .Call(
             LGBM_DatasetUpdateParamChecking_R
             , lgb.params2str(params = private$params)
-            , lgb.params2str(params = params)
+            , lgb.params2str(params = new_params)
           )
+          private$params <- new_params
         }, error = function(e) {
           # If updating failed but raw data is not available, raise an error because
           # achieving what the user asked for is not possible
@@ -572,14 +574,19 @@ 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 <- utils::modifyList(private$params, params)
+          private$params <- new_params
           self$finalize()
         })
       }
       return(invisible(self))
 
     },
 
+    # [description] Get only Dataset-specific parameters. This is primarily used by
+    #               Booster to update its parameters based on the characteristics of
+    #               a Dataset. It should not be used by other methods in this class,
+    #               since "verbose" is not a Dataset parameter and needs to be passed
+    #               through to avoid globally re-setting verbosity.
     get_params = function() {
       dataset_params <- unname(unlist(.DATASET_PARAMETERS()))
       ret <- list()

diff --git a/R-package/tests/testthat/test_basic.R b/R-package/tests/testthat/test_basic.R
@@ -2155,6 +2155,56 @@ test_that("early stopping works with lgb.cv()", {
   )
 })
 
+test_that("lgb.cv() respects changes to logging verbosity", {
+  dtrain <- lgb.Dataset(
+    data = train$data
+    , label = train$label
+  )
+  # (verbose = 1) should be INFO and WARNING level logs
+  lgb_cv_logs <- capture.output({
+    cv_bst <- lgb.cv(
+      params = list()
+      , nfold = 2L
+      , nrounds = 5L
+      , data = dtrain
+      , obj = "binary"
+      , verbose = 1L
+    )
+  })
+  expect_true(any(grepl("\\[LightGBM\\] \\[Info\\]", lgb_cv_logs)))
+  expect_true(any(grepl("\\[LightGBM\\] \\[Warning\\]", lgb_cv_logs)))
+
+  # (verbose = 0) should be WARNING level logs only
+  lgb_cv_logs <- capture.output({
+    cv_bst <- lgb.cv(
+      params = list()
+      , nfold = 2L
+      , nrounds = 5L
+      , data = dtrain
+      , obj = "binary"
+      , verbose = 0L
+    )
+  })
+  expect_false(any(grepl("\\[LightGBM\\] \\[Info\\]", lgb_cv_logs)))
+  expect_true(any(grepl("\\[LightGBM\\] \\[Warning\\]", lgb_cv_logs)))
+
+  # (verbose = -1) no logs
+  lgb_cv_logs <- capture.output({
+    cv_bst <- lgb.cv(
+      params = list()
+      , nfold = 2L
+      , nrounds = 5L
+      , data = dtrain
+      , obj = "binary"
+      , verbose = -1L
+    )
+  })
+  # NOTE: this is not length(lgb_cv_logs) == 0 because lightgbm's
+  #       dependencies might print other messages
+  expect_false(any(grepl("\\[LightGBM\\] \\[Info\\]", lgb_cv_logs)))
+  expect_false(any(grepl("\\[LightGBM\\] \\[Warning\\]", lgb_cv_logs)))
+})
+
 test_that("lgb.cv() updates params based on keyword arguments", {
   dtrain <- lgb.Dataset(
     data = matrix(rnorm(400L), ncol =  4L)