[R-package] Fix off-by-one bug: nrounds=0 resulted in 2 iterations#11856
Conversation
Added a check for nrounds=0 to prevent an invalid sequence in the iteration loop.
Fix handling of nrounds=0 to ensure 'iteration' is defined. But left a trailing whitespace by mistake , now fixed it .
whitespace/style error in previous commit
Added test for xgb.train with nrounds set to 0 to ensure it results in 0 iterations.
Updated test for xgb.train with nrounds = 0 to handle potential NULL return for empty models.
Removed unnecessary blank line in test case.
Enhanced test for xgb.train with nrounds = 0, including checks for serialization, continuation, and callbacks.
|
@david-cortes Hello , I’ve updated test_basic.R to cover all the scenarios you requested.
One small detail: I noticed that when continuing training from an empty model with early stopping, the niter attribute sometimes stays NULL (likely because the callback can't find a previous "best iteration"). I updated the test to explicitly check that the predictions have diverged/updated in this case, which proves the training is working correctly even if the attribute is missing. Let me know if there are any other changes to be made! |
|
@trivialfis I have updated the tests to cover all the requested scenarios (serialization, continuation, and callbacks). Note on Case 3 (Callbacks): Ready for review. |
There was a problem hiding this comment.
Thank you for the fix, @Sanidhyavijay24 ! Looks good to me.
cc @david-cortes I would like to separate the issue of callbacks with this fix, the Python package can use some more tests as well. I tested the empty model primarily for weird distributed environments and the tests are not as comprehensive.
We will backport this PR to the 3.1 branch for the next CRAN patch release.
david-cortes
left a comment
There was a problem hiding this comment.
LGTM, just a few comments.
Updated test cases to use global 'train' variable instead of 'agaricus.train'. Adjusted parameters to include 'nthread' for consistency in xgb.train calls.
|
Thanks @david-cortes for pointing these out to me. I have updated the code to:
Ready for review! |
Co-authored-by: sanidhya <[email protected]>
This fixes issue #11854.
The Bug ->In R, the expression 1:0 evaluates to c(1, 0). When nrounds=0, the training loop begin_iteration:end_iteration was executing twice instead of zero times.
Fix:
->Replaced the loop range with seq(from = begin_iteration, length.out = nrounds) to correctly handle the zero case.
->Added if (nrounds == 0) iteration <- end_iteration to ensure the iteration variable exists for downstream callbacks (specifically cb_outputs), preventing an "object not found" error when the loop is skipped.