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] Ensure print.lgb.Booster() works when objective is not explicitly set #6850

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Changes from 1 commit
Commits
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
Next Next commit
Handling NULL objective when booster is printed, moving test function…
…s out of method tests, creating unique tests for different method situations
  • Loading branch information
walkerjameschris committed Feb 28, 2025
commit 7b1a3084fbeb062f3d40c951f6790bb05bfcf4df
3 changes: 3 additions & 0 deletions R-package/R/lgb.Booster.R
Original file line number Diff line number Diff line change
@@ -1235,6 +1235,9 @@ print.lgb.Booster <- function(x, ...) {

if (!handle_is_null) {
obj <- x$params$objective
if (is.null(obj)) {
obj <- x$.__enclos_env__$private$get_eval_info()
}
if (obj == "none") {
obj <- "custom"
}
157 changes: 88 additions & 69 deletions R-package/tests/testthat/test_lgb.Booster.R
Original file line number Diff line number Diff line change
@@ -1,3 +1,70 @@
.have_same_handle <- function(model, other_model) {
expect_equal(
model$.__enclos_env__$private$handle
, other_model$.__enclos_env__$private$handle
)
}

.has_expected_content_for_fitted_model <- function(printed_txt) {
expect_true(any(startsWith(printed_txt, "LightGBM Model")))
expect_true(any(startsWith(printed_txt, "Fitted to dataset")))
}

.has_expected_content_for_finalized_model <- function(printed_txt) {
expect_true(any(printed_txt == "LightGBM Model"))
expect_true(any(grepl("Booster handle is invalid", printed_txt, fixed = TRUE)))
}

.check_methods_work <- function(model) {

#--- should work for fitted models --- #

# print()
log_txt <- capture.output({
ret <- print(model)
})
.have_same_handle(ret, model)
.has_expected_content_for_fitted_model(log_txt)

# show()
log_txt <- capture.output({
ret <- show(model)
})
expect_null(ret)
.has_expected_content_for_fitted_model(log_txt)

# summary()
log_txt <- capture.output({
ret <- summary(model)
})
.have_same_handle(ret, model)
.has_expected_content_for_fitted_model(log_txt)

#--- should not fail for finalized models ---#
model$.__enclos_env__$private$finalize()

# print()
log_txt <- capture.output({
ret <- print(model)
})
.has_expected_content_for_finalized_model(log_txt)

# show()
.have_same_handle(ret, model)
log_txt <- capture.output({
ret <- show(model)
})
expect_null(ret)
.has_expected_content_for_finalized_model(log_txt)

# summary()
log_txt <- capture.output({
ret <- summary(model)
})
.have_same_handle(ret, model)
.has_expected_content_for_finalized_model(log_txt)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for pulling these out! But could you please move them down to right before the first test case using them?

If they were more generically useful or used in more places I'd agree with putting helper functions like this at the top of the file, but in this case they're highly specific to the {print,show,summary}.lgb.Booster() test cases.

Copy link
Author

Choose a reason for hiding this comment

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

For sure! I will get on that.


test_that("Booster's finalizer should not fail", {
X <- as.matrix(as.integer(iris[, "Species"]), ncol = 1L)
y <- iris[["Sepal.Length"]]
@@ -1518,74 +1585,7 @@ test_that("boosters with linear models at leaves can be written to RDS and re-lo
expect_identical(preds, preds2)
})

test_that("Booster's print, show, and summary work correctly", {
.have_same_handle <- function(model, other_model) {
expect_equal(
model$.__enclos_env__$private$handle
, other_model$.__enclos_env__$private$handle
)
}

.has_expected_content_for_fitted_model <- function(printed_txt) {
expect_true(any(startsWith(printed_txt, "LightGBM Model")))
expect_true(any(startsWith(printed_txt, "Fitted to dataset")))
}

.has_expected_content_for_finalized_model <- function(printed_txt) {
expect_true(any(printed_txt == "LightGBM Model"))
expect_true(any(grepl("Booster handle is invalid", printed_txt, fixed = TRUE)))
}

.check_methods_work <- function(model) {

#--- should work for fitted models --- #

# print()
log_txt <- capture.output({
ret <- print(model)
})
.have_same_handle(ret, model)
.has_expected_content_for_fitted_model(log_txt)

# show()
log_txt <- capture.output({
ret <- show(model)
})
expect_null(ret)
.has_expected_content_for_fitted_model(log_txt)

# summary()
log_txt <- capture.output({
ret <- summary(model)
})
.have_same_handle(ret, model)
.has_expected_content_for_fitted_model(log_txt)

#--- should not fail for finalized models ---#
model$.__enclos_env__$private$finalize()

# print()
log_txt <- capture.output({
ret <- print(model)
})
.has_expected_content_for_finalized_model(log_txt)

# show()
.have_same_handle(ret, model)
log_txt <- capture.output({
ret <- show(model)
})
expect_null(ret)
.has_expected_content_for_finalized_model(log_txt)

# summary()
log_txt <- capture.output({
ret <- summary(model)
})
.have_same_handle(ret, model)
.has_expected_content_for_finalized_model(log_txt)
}

test_that("Booster's print, show, and summary work correctly for built-in objectives", {
data("mtcars")
model <- lgb.train(
params = list(
@@ -1604,7 +1604,7 @@ test_that("Booster's print, show, and summary work correctly", {
, nrounds = 5L
)
.check_methods_work(model)

data("iris")
model <- lgb.train(
params = list(objective = "multiclass", num_class = 3L, num_threads = .LGB_MAX_THREADS)
@@ -1616,7 +1616,9 @@ test_that("Booster's print, show, and summary work correctly", {
, nrounds = 5L
)
.check_methods_work(model)
})

test_that("Booster's print, show, and summary work correctly for custom objective", {

# with custom objective
.logregobj <- function(preds, dtrain) {
@@ -1638,6 +1640,7 @@ test_that("Booster's print, show, and summary work correctly", {
))
}

data("iris")
model <- lgb.train(
data = lgb.Dataset(
as.matrix(iris[, -5L])
@@ -1653,6 +1656,22 @@ test_that("Booster's print, show, and summary work correctly", {
.check_methods_work(model)
})

test_that("Booster's print, show, and summary work correctly when objective is not provided", {

data("iris")
model <- lgb.train(
data = lgb.Dataset(
as.matrix(iris[, 1:3])
, label = iris[, 4]
)
, verbose = .LGB_VERBOSITY
, nrounds = 5L
, params = list(num_threads = .LGB_MAX_THREADS)
)

.check_methods_work(model)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
.check_methods_work(model)
.check_methods_work(model)
log_txt <- capture.output({
ret <- print(model)
})
expect_true(any(printed_txt == "Objective: (default)"))

.check_methods_work() is great and tests a lot of things, but for this particular PR I think it'd be helpful to also test that print.lgb.Booster() prints exactly what we expect for the objective.

Could you add something like this here?

Copy link
Author

Choose a reason for hiding this comment

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

Sure thing!

})

test_that("LGBM_BoosterGetNumFeature_R returns correct outputs", {
data("mtcars")
model <- lgb.train(
Loading
Oops, something went wrong.