fix(xgboost): keep one row when subsample truncates the sample to zero (#444)

[teddytennant]

Description

XGRegressor::fit panics with attempt to subtract with overflow when subsample is small enough that the sampled index set comes out empty. sample_without_replacement sizes the sample as (population_size as f64 * subsample_ratio) as usize, which floors to 0. Three rows at subsample = 0.3 does it, and so does a single row at any ratio below 1.0. find_best_split then runs 0..sorted_idxs.len() - 1 on an empty slice.

The guard in fit only rejects subsample > 1.0 and subsample <= 0.0, so this input gets through.

Fixes #444

Current behaviour

thread 'main' panicked at src/xgboost/xgb_regressor.rs:323:21:
attempt to subtract with overflow

New expected behaviour

The sample keeps a minimum of one row, so the fit succeeds and returns a usable model. This is what scikit-learn does for its own subsample parameter (max(1, int(subsample * n_samples)) in sklearn/ensemble/_gb.py).

I went this way rather than returning Failed::because(FailedError::ParametersError, ...) because subsample is a regularisation hyperparameter, not a statement about the data. The parameter is valid on its own; only the pair (subsample, n_samples) is degenerate. Erroring would mean a subsample that works on the full dataset starts failing inside cross_validate as k-fold shrinks each fold, which is a surprising place to hit it.

Sample sizes of one row or more are byte-identical to before. .max(1) only engages when the floor is 0, and shuffle consumes the same RNG state either way.

Tests

Four tests, in the module's own mod tests. Three fail before this change and pass after: 3 rows at 0.3, 1 row at 0.9, and a table over the boundary. The fourth is the regression guard, checking that ratios which already yield one or more rows are untouched: (10, 0.8) -> 8, (100, 0.8) -> 80, indices unique and in range. (2, 0.4) and (2, 0.5) sit either side of the floor boundary, so the pair pins the clamp rather than just testing small numbers.

Lib tests go 428 to 432.

Change logs

CHANGELOG.md gets a ### Fixed bullet under a new ## [0.6.12], and Cargo.toml is bumped to match, following #443.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 63.93%. Comparing base (9eaae9e) to head (c31eebb).
⚠️ Report is 180 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #445       +/-   ##
===========================================
+ Coverage   43.97%   63.93%   +19.95%     
===========================================
  Files          85       95       +10     
  Lines        7281     8207      +926     
===========================================
+ Hits         3202     5247     +2045     
+ Misses       4079     2960     -1119     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Mec-iS]

thanks 🙏 i will look into this asap

[Mec-iS]

Review: ✅ Approve with minor notes

The fix is correct, minimal, and safe. The one-line change in sample_without_replacement is the right approach and matches the established scikit-learn convention.

---

The Core Fix

// Before
indices.truncate((population_size as f64 * subsample_ratio) as usize);

// After
indices.truncate(((population_size as f64 * subsample_ratio) as usize).max(1));

This is correct and complete. The .max(1) only engages on the underflow case (floor → 0), so all existing behaviour for sample sizes ≥ 1 is byte-identical. The panic in find_best_split at 0..sorted_idxs.len() - 1 on an empty slice is fully prevented. The rationale for clamping rather than erroring (cross-validation folds shrinking the dataset) is well-argued and consistent with scikit-learn's max(1, int(subsample * n_samples)).

---

Test Coverage

The four tests cover the important boundaries well:

• test_subsample_smaller_than_one_sample_does_not_panic — end-to-end fit/predict with 3 rows at 0.3 (the original bug)
• test_subsample_on_single_row_does_not_panic — single-row edge case
• test_sample_without_replacement_clamps_to_one_row — unit test covering multiple (population, ratio) pairs
• test_sample_without_replacement_keeps_the_ratio_above_one_row — regression guard ensuring .max(1) doesn't inflate normal samples

One gap worth noting: there is no test for population_size == 0. Currently sample_without_replacement(0, any_ratio, rng) will call truncate(1) on an empty Vec, which is a safe no-op in Rust and returns []. This is probably fine since fit validates n_samples > 0 upstream, but a short comment or debug_assert!(population_size > 0) would make the invariant explicit.

---

Minor Suggestions

• with_subsample docstring: the new line says "a minimum of one row" — worth adding a parenthetical like (provided the dataset is non-empty) for precision.
• sample_without_replacement docstring: same clarification would help here.
• CHANGELOG.md: the entry is accurate but the last two sentences read more like a commit message than a changelog bullet — consider trimming to just the user-visible behaviour.

None of these block merging. Great fix overall — clean, targeted, and well-tested.

[Mec-iS]

Second Pass Review

Having looked more carefully at the diff, a few additional observations beyond my first comment.

---

Correctness of the fix in context of find_best_split

The panic originates at 0..sorted_idxs.len() - 1 on an empty slice. It is worth noting this is a usize underflow, not a bounds panic — 0 - 1 wraps to usize::MAX in debug builds and panics explicitly; in release it silently produces an enormous range. The .max(1) guard fully eliminates the empty-slice case, so both paths are covered. ✅

The shuffle-before-truncate ordering

The PR notes that shuffle consumes the same RNG state regardless of the clamp. This is true and important: the fix does not change the RNG sequence for any call where the sample size was already ≥ 1, so existing serialised/reproducible models are unaffected. ✅

Potential issue: subsample_ratio not clamped to [0, 1] inside sample_without_replacement

The function signature accepts any f64. If subsample_ratio > 1.0 somehow reaches it (e.g. via a future refactor that bypasses fit's guard), (population_size as f64 * subsample_ratio) as usize silently exceeds population_size, and truncate on a shorter vec is a no-op — so you get all rows rather than erroring. This is benign today, but a debug_assert!(subsample_ratio > 0.0 && subsample_ratio <= 1.0) at the top of the function would make the contract explicit at zero runtime cost in release builds.

Missing blank line between tests

        assert!(predictions[0].is_finite());
    }
    #[test]   // ← no blank line before this attribute
    fn test_sample_without_replacement_clamps_to_one_row() {

Minor style nit — all other test functions in this module have a blank line separating them. rustfmt won't flag it but it breaks the visual rhythm.

test_subsample_on_single_row_does_not_panic — consider asserting the predicted value

With a single-row dataset and 5 estimators each forced to train on that one row, the model should perfectly memorise the target. Adding assert!((predictions[0] - 5.0_f64).abs() < 1e-6) (or similar tolerance) would make the test document the expected numerical behaviour, not just the absence of a panic.

---

None of these are blockers. The core change is sound and I'd be happy to see this merged as-is. The debug_assert suggestion is the only one I'd consider worth a follow-up issue if not addressed here.