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

[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

[R-package] Ensure print.lgb.Booster() works when objective is not explicitly set #6850

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7b1a308
Handling NULL objective when booster is printed, moving test function…
walkerjameschris Feb 28, 2025
bad407c
Correcting whitespace
walkerjameschris Mar 1, 2025
57a121f
Adding types for integer slices
walkerjameschris Mar 2, 2025
60277c7
Merge branch 'master' into fix/lgb-train-print
walkerjameschris Mar 2, 2025
58ae5f7
Merge branch 'master' into fix/lgb-train-print
walkerjameschris Mar 6, 2025
8e22722
Moving test functions nearer to relevant tests, setting print to be d…
walkerjameschris Mar 6, 2025
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
3 changes: 3 additions & 0 deletions R-package/R/lgb.Booster.R
View file
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 <- "(default)"
}
if (obj == "none") {
obj <- "custom"
}
151 changes: 85 additions & 66 deletions R-package/tests/testthat/test_lgb.Booster.R
View file
Original file line number Diff line number Diff line change
@@ -1518,74 +1518,74 @@ 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
)
}
.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_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)))
}
.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)
}
.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(
@@ -1616,9 +1616,9 @@ test_that("Booster's print, show, and summary work correctly", {
, nrounds = 5L
)
.check_methods_work(model)
})


# with custom objective
test_that("Booster's print, show, and summary work correctly for custom objective", {
.logregobj <- function(preds, dtrain) {
labels <- get_field(dtrain, "label")
preds <- 1.0 / (1.0 + exp(-preds))
@@ -1638,6 +1638,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 +1654,24 @@ 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[, seq_len(3L)])
, label = iris[, 4L]
)
, verbose = .LGB_VERBOSITY
, nrounds = 5L
, params = list(num_threads = .LGB_MAX_THREADS)
)

log_txt <- capture.output(print(model))
Copy link
Author

Choose a reason for hiding this comment

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

I am checking that the output of print() for a booster is "Objective: (default)" before .check_methods_work() is called. My rationale for this order of operations is that the model is finalized in .check_methods_work() and .has_expected_content_for_finalized_model() checks that the printout is correctly modified to "Booster handle is invalid" which removes "Objective: (default)" from the output. So we can first check that "Objective: (default)" is reported and then run .check_methods_work().

Copy link
Collaborator

Choose a reason for hiding this comment

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

ohhhhh I forgot about that. Ok makes sense to me! Thanks for the explanation.

expect_true(any("Objective: (default)" == log_txt))

.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(