fix(xgboost): return error instead of panicking on empty training data

[SAY-5]

Fixes #446

Checklist

• My branch is up-to-date with main branch.
• Everything works and tested on latest stable Rust.
• Coverage and Linting have been applied

Current behaviour

XGRegressor::fit panics on a training set with zero rows. find_best_split runs 0..sorted_idxs.len() - 1, which underflows on an empty slice; in debug builds this is attempt to subtract with overflow and in release builds it surfaces as index out of bounds. A zero-row matrix is reachable through the public Array2::take, so this is reachable from safe user code.

New expected behaviour

fit validates that the training data has at least one row and returns Err(Failed::because(FailedError::ParametersError, ...)) for empty data, mirroring the existing subsample validation a few lines above. A model trained on no data is not useful, so this matches the error direction the reporter preferred. Non-empty inputs are unaffected. A regression test covers the empty-data case.

[Mec-iS]

thanks. I am looking into this

[Mec-iS]

Code Review

Thanks for the fix, @SAY-5 — this is a well-scoped, sensible change. Here are some observations and suggestions:

---

✅ What works well

• The guard is placed correctly, immediately after data.shape() is destructured and before any indexing logic, so it short-circuits cleanly.
• FailedError::ParametersError is the right error variant — it mirrors the existing subsample guard a few lines above, keeping the error handling pattern consistent.
• The regression test is realistic: it constructs the empty matrix via Array2::take(&[], 0) — which is the exact public-API path that triggered the underflow — rather than constructing an artificial empty matrix through some private bypass, making the test a true coverage of the reported crash vector.
• The test verifies both that the result is Err and that the error variant is ParametersError, which is the right level of assertion for a typed error API.

---

🟡 Suggestions / Minor Issues

1. Error message wording

"Training data must have at least one row."

This is fine, but it could be slightly more actionable. Consider:

"Training data must contain at least one sample; got 0 rows."

This makes it immediately obvious to a user reading the error message what went wrong without having to trace back the call.

---

2. XGClassifier / other trees — is the same bug present?

The panic originates in find_best_split, which is shared logic between XGRegressor and — if there is an XGClassifier or a gradient boosted classifier in this codebase — likely also used there. A quick audit to check whether fit in those counterparts also lacks the empty-row guard would be worth doing now while the context is fresh, or at least opening a follow-up issue.

---

3. Test: full construction could use a named constant

let full = DenseMatrix::from_2d_vec(&vec![vec![1.0, 1.0], vec![2.0, 1.0]]).unwrap();

The two columns here are arbitrary; a comment like // 2 rows × 2 features, values are irrelevant would make it immediately clear that the specific values don't matter and the test is purely about the empty-slice case.

---

4. y type annotation is redundant with inference

let y: Vec<f64> = vec![];

The type annotation is necessary here because vec![] alone is ambiguous without a downstream use that pins the type. Since XGRegressor::fit requires TY: Number, and there is no other constraint, keeping the explicit annotation is correct — this is fine as-is.

---

🔴 Edge case to consider

Zero columns (n_features == 0)

The guard checks n_samples == 0 but does not check n_features == 0. A 0×k matrix is the natural crash path reported here, but a n×0 matrix would also produce degenerate behaviour downstream (e.g. no splits ever found, infinite loops, or a trivially vacuous model). It is worth either:

• Adding n_features == 0 to the same guard, or
• Opening a companion issue to track it.

For this PR, at minimum documenting this as a known limitation in a follow-up comment would be helpful.

---

Summary

The core fix is correct and the test covers the reported panic path. The main ask before merging is a check of whether XGClassifier (or any other fit implementations sharing find_best_split) has the same vulnerability. The zero-columns edge case is a nice-to-have follow-up rather than a blocker.

[Mec-iS]

Code Review — fix(xgboost): return error instead of panicking on empty training data

Thanks for the clear, focused fix @SAY-5. The change is well-placed and the regression test is solid. A few observations, suggestions, and one open question before this can merge:

---

✅ What's good

• Guard placement is correct. n_samples == 0 is checked immediately after data.shape() is destructured, before any indexing or iteration, so it short-circuits cleanly with no risk of partial state being mutated.
• Error variant is consistent. FailedError::ParametersError mirrors the subsample guard a few lines above — using the same error variant for analogous input-validation failures keeps the API coherent.
• Test uses the real crash path. DenseMatrix::take(&[], 0) is the exact public-API sequence that triggered the underflow, so this is a true regression test rather than an artificial construction. The assertion checks both is_err() and the specific FailedError::ParametersError variant — right level of precision for a typed error API.

---

🟡 Minor suggestions

1. Error message could be more actionable

Current:

"Training data must have at least one row."

Suggested:

"Training data must contain at least one sample; got 0 rows."

Saying "got 0 rows" makes it immediately obvious what the caller passed in, without requiring them to trace back through the call.

2. Test: a brief comment on the full matrix would help

let full = DenseMatrix::from_2d_vec(&vec![vec![1.0, 1.0], vec![2.0, 1.0]]).unwrap();

The specific values 1.0/2.0 are irrelevant here. A comment like:

// 2 rows × 2 features — values are arbitrary; only the empty-row case is under test

makes it immediately clear to future readers that this matrix is just scaffolding for .take(&[], 0).

---

🔴 Edge case: n_features == 0 is not covered

The guard only checks n_samples == 0. A matrix with shape (n, 0) — zero columns — would also produce degenerate or undefined behaviour downstream (no valid split candidates, trivially vacuous model, possible infinite-loop in find_best_split). This is arguably a separate issue but is worth either:

• Extending the guard: if n_samples == 0 || n_features == 0 { ... } with an appropriate message, or
• Opening a follow-up issue to track it explicitly.

At minimum, a comment near the guard noting this as a known unhandled edge case would reduce surprise for anyone who hits it next.

---

🟡 Does XGClassifier share find_best_split?

If there is an XGClassifier (or any gradient-boosted classifier sharing the same find_best_split logic), it almost certainly has the same vulnerability. It would be worth a quick audit of the classifier's fit path to confirm whether an analogous empty-row guard is needed there, or opening a tracking issue so it isn't forgotten.

---

Summary

The core fix is correct and merges cleanly with main. Before merging, the main asks are:

1. Consider extending the guard to cover n_features == 0 or open a follow-up issue.
2. Check whether XGClassifier::fit (if it exists) has the same gap.
3. Optionally tighten the error message and add the test comment.

Happy to approve once (1) and (2) are addressed or acknowledged as follow-ups.

[codecov]

Codecov Report

❌ Patch coverage is 25.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.92%. Comparing base (9eaae9e) to head (c887efe).
⚠️ Report is 182 commits behind head on main.

Files with missing lines	Patch %	Lines
src/xgboost/xgb_regressor.rs	25.00%	3 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #448       +/-   ##
===========================================
+ Coverage   43.97%   63.92%   +19.95%     
===========================================
  Files          85       95       +10     
  Lines        7281     8217      +936     
===========================================
+ Hits         3202     5253     +2051     
+ Misses       4079     2964     -1115     

☔ 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]

Please check the code coverage report: #448 (comment)

[Mec-iS]

thanks.
I will merge this and add some minor changes