chore(xgboost): apply #445 review follow-ups to the subsample clamp - #447
Merged
Conversation
- Document that the one-row minimum holds for a non-empty dataset - Add debug_asserts for the population_size and subsample_ratio bounds - Assert the single-row fit moves the prediction towards the target - Separate adjacent tests with a blank line - Trim the 0.6.12 changelog bullet to user-visible behaviour
Predict and fit propagated internal unwraps to callers through Result-returning methods: - PredictorBorrow::predict delegates to the inherent predict, which already returns Failed errors. - MultiClassSVC::fit propagates multiclass_fit failures with ?. - predict returns PredictFailed when called before fit, e.g. on a deserialized model with no classifiers. - predict returns PredictFailed instead of panicking when fit saw fewer than two classes.
with_gamma, with_degree, and with_coef0 matched non-applicable kernels with a catch-all arm. A future kernel variant holding these fields would silently ignore the setters. Each setter now lists every variant explicitly. Also marks Kernels #[must_use].
clippy::must_use_candidate flagged 149 callables whose results are meaningless to discard: parameter structs and their with_* builders, dataset loaders, metric constructors, error constructors, and linalg factory methods. The 49 *Parameters/*Params config types gain a type-level #[must_use], which covers every builder method returning Self. The remaining library functions gain fn-level #[must_use]. Kernels was already covered in the previous commit.
Collaborator
Author
|
Widened this PR with three more commits applying the idiomatic-Rust hygiene findings ( |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #445 — review notes from #445 plus an idiomatic-Rust hygiene pass.
Checklist
Current behaviour
PR #445 merged with minor review notes still open. A follow-up review against the idiomatic-rust resource list surfaced additional hygiene findings: panics escaping
ResultAPIs, wildcard match arms, and zero#[must_use]coverage.New expected behaviour
Commit 1 — #445 review follow-ups (
0395b41)with_subsampledoc states the one-row minimum holds only for a non-empty datasetsample_without_replacementgainsdebug_assert!guards for its internal invariantsbase_score = 0.5towards the targetCommit 2 — svm panic fixes (
33db5f4)MultiClassSVC::fitpropagates binary-fit failures instead of unwrappingMultiClassSVC::predictreturnsPredictFailedwhen called beforefit, and whenfitsaw fewer than two classes, instead of panicking through aResultAPIPredictorBorrow::predictimpls delegate directly to the inherent falliblepredictCommit 3 — Kernels setter arms (
5590534)with_gamma/with_degree/with_coef0enumerate every variant explicitly; a future kernel variant holding these fields can no longer silently ignore the settersCommit 4 —
#[must_use]coverage (f6a3f2e)*Parameters/*Paramsconfig types gain type-level#[must_use], covering everywith_*builder returningSelfclippy::must_use_candidate(dataset loaders, metric/error constructors, linalg factories) gain fn-level#[must_use]-Wclippy::must_use_candidateDeliberately not changed
clippy::indexing_slicing(~970 sites): deliberate trade in hot numeric kernels; dimension invariants are checked atfit()/view construction. Suggested as a per-module decision for new code only...Default::default()sites in tests: intentional partial configuration in hyperparameter-grid tests, not production construction.Change logs
Changed
MultiClassSVC::predictnow returnsErr(Failed)in two states where it previously panicked (beforefit; single-class training data).Verification
cargo fmt --all -- --check,cargo clippy --all-features -- -Drust-2018-idioms -Drust-2024-compatibility -Dwarnings, andcargo test --all-features(619 passed) all green. Pre-commit hook re-verifies fmt + clippy on every commit.