fix: address review nits from #470 - #471
Merged
Merged
Conversation
- RandomForestClassifier::predict_oob: fold the leftover is_none() guard into a single match on (&self.trees, &self.classes), matching Lasso. - XGRegressor::predict: match on (&self.parameters, &self.regressors) to remove the latent unwrap when the two fields are out of sync. - DecisionTreeClassifier: comment why nodes.is_empty() is the guard. - Add wasm_bindgen_test attributes to the new should_not_panic tests. - CHANGELOG: record the unfitted-predict fix and the MultiClassSVC error-message change.
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
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.
Checklist
Current behaviour
The review of #470 left a few nits:
RandomForestClassifier::predict_oobstill used anis_none()guard before matching onself.classes, the same redundancy that was removed from Lasso.XGRegressor::predictmatched onself.parametersbut still calledself.regressors.as_ref().unwrap(), which can panic if the two fields are out of sync after deserialization.DecisionTreeClassifier::predictandpredict_probauseself.nodes.is_empty(), a third guard style, without a comment.should_not_panictests do not carrycfg_attr(..., wasm_bindgen_test), so they do not run in the wasm CI job.predict_proba, notpredict_oob; several guarded methods were missing).MultiClassSVC::predicterror-text change from Decision tree panic #470 is not recorded inCHANGELOG.md.New expected behaviour
predict_oobfolds both checks into onematch (&self.trees, &self.classes), as Lasso does.XGRegressor::predictmatches on(&self.parameters, &self.regressors); no unwrap remains.nodes.is_empty()guards carry a short comment explaining why they are the right check.should_not_panictests carry the standardcfg_attr(all(target_arch = "wasm32", not(target_os = "wasi")), wasm_bindgen_test::wasm_bindgen_test)attribute.CHANGELOG.mdgains an[Unreleased]section with the unfitted-predict fix and the breaking error-message note forMultiClassSVC.Change logs
Fixed
RandomForestClassifier::predict_oobandXGRegressor::predictno longer rely on redundant or panic-prone checks (predictmethod onDecisionTreeRegressorpanics when tree is not fit #469 follow-up to Decision tree panic #470).Changed
should_not_panictests now run underwasm-bindgen-teston wasm targets.CHANGELOG.md: document the Decision tree panic #470 behaviour change, including the newMultiClassSVCnot-fitted error text.Verification
cargo fmt --all -- --checkcargo clippy --all-features -- -Drust-2018-idioms -Drust-2024-compatibility -Dwarningscargo test --all-features