Skip to content

[R-package] Fix off-by-one bug: nrounds=0 resulted in 2 iterations#11856

Merged
trivialfis merged 16 commits into
dmlc:masterfrom
Sanidhyavijay24:master
Dec 13, 2025
Merged

[R-package] Fix off-by-one bug: nrounds=0 resulted in 2 iterations#11856
trivialfis merged 16 commits into
dmlc:masterfrom
Sanidhyavijay24:master

Conversation

@Sanidhyavijay24

Copy link
Copy Markdown
Contributor

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.

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
Comment thread R-package/R/xgb.train.R
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.
@trivialfis trivialfis mentioned this pull request Dec 11, 2025
10 tasks
Comment thread R-package/tests/testthat/test_basic.R Outdated
@Sanidhyavijay24

Copy link
Copy Markdown
Contributor Author

@david-cortes Hello , I’ve updated test_basic.R to cover all the scenarios you requested.
The new tests now verify:

  • Serialization: Saving and loading an nrounds=0 model preserves the state correctly (and keeps niter at 0).
  • Continuation: I confirmed that initializing with 0 rounds and then continuing for 10 rounds produces numerically identical predictions to training 10 rounds from scratch (using fixed seeds and a strict tolerance).
  • Callbacks: I verified that early_stopping and evals work fine with empty models.

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!
Thanks!!!

@Sanidhyavijay24

Copy link
Copy Markdown
Contributor Author

@trivialfis I have updated the tests to cover all the requested scenarios (serialization, continuation, and callbacks).

Note on Case 3 (Callbacks):
I noticed that the existing R wrapper has a separate issue where predict() crashes on 0-round models with early stopping (due to missing best_iteration). To work around this while still proving that continuation works, the test now validates that the continued model produces learned predictions (non-uniform standard deviation) rather than checking the niter attribute directly.
The R-package checks are passing now (the Mac/Python failure seems unrelated to these changes).

Ready for review.
Thanks!!!!

@trivialfis trivialfis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 david-cortes left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, just a few comments.

Comment thread R-package/tests/testthat/test_basic.R Outdated
Comment thread R-package/tests/testthat/test_basic.R Outdated
Comment thread R-package/tests/testthat/test_basic.R Outdated
Updated test cases to use global 'train' variable instead of 'agaricus.train'. Adjusted parameters to include 'nthread' for consistency in xgb.train calls.
@Sanidhyavijay24

Copy link
Copy Markdown
Contributor Author

Thanks @david-cortes for pointing these out to me.

I have updated the code to:

  • Set nthread = 1 in all xgb.train calls to satisfy CRAN requirements.
  • Use xgb.save.raw for serialization testing.
  • Reuse the global train variable instead of reloading the dataset.

Ready for review!
If anything else left let me know.
Thanks!!

@trivialfis
trivialfis merged commit fe5a6a3 into dmlc:master Dec 13, 2025
66 of 69 checks passed
trivialfis pushed a commit to trivialfis/xgboost that referenced this pull request Jan 3, 2026
trivialfis added a commit that referenced this pull request Jan 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants