You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Everything works and tested on latest stable Rust.
Coverage and Linting have been applied (fmt and clippy)
Current behaviour
KNNClassifier::fit with with_k(1) comes back with k should be > 1, k=[1], so there's no way to get a 1-nearest-neighbour classifier, and a loop over k from 1 upwards falls over on the first one. KNNRegressor has taken k = 1 since 92dad01 relaxed its copy of the check, the classifier just never got the same change.
New expected behaviour
Same check as KNNRegressor, so k = 1 works and k = 0 is still an error. The new test fits with k = 1 and checks taht every training point gets its own label back.
It's two characters of fix really, the test is most of the diff.
Thanks for the fix, @NotAFlightRisk. I reviewed the diff (1 file, +12/−2) against main.
Summary: The change is correct and minimal. parameters.k <= 1 becomes parameters.k < 1, and the error text becomes k should be > 0. This is the same guard KNNRegressor::fit already uses (k < 1, "k should be > 0"), so the two estimators are now consistent. The downstream search code accepts k = 1 too: LinearKNNSearch requires k >= 1 && k <= len(data), and CoverTree::find only rejects k == 0. I don't see a reason for the old <= 1 restriction.
Suggestions (non-blocking):
Test the boundary. The new knn_fit_predict_k1 test covers k = 1, but nothing pins down k = 0 still failing. The description says "k = 0 is still an error", so a short test would guard that: assert!(KNNClassifier::fit(&x, &y, KNNClassifierParameters::default().with_k(0)).is_err());.
Strengthen the k=1 test. Five points with labels [2,3,2,3,2] only check that each training point returns its own label. That is fine, but it would also be worth running it against the non-default algorithms (KNNAlgorithmName::CoverTree and LinearSearch), since the two backends handle k independently. A predict_proba check at k = 1 (one-hot rows) would also be cheap and cover the second public entry point.
CHANGELOG. Other recent fixes (predict method on DecisionTreeRegressor panics when tree is not fit #469/Decision tree panic #470) have entries in CHANGELOG.md. The PR description has a "Change logs" section but the file isn't touched. Please add a Fixed line under Unreleased. Because the error message string changes (> 1 → > 0), a downstream user matching on it would be affected, so it's worth a mention, as was done for MultiClassSVC.
Docs. Please check whether the KNNClassifierParameters::k doc comment or the module docs say "k > 1" anywhere, and update them if so.
Nit. There is a typo in the PR description: "taht" → "that".
CI:lint, MSRV (1.85), the check_features jobs and i686-linux tests have passed. The remaining test jobs (linux, macOS, Windows, wasm) and coverage were still running when I looked. Please confirm they are green, in particular that no existing test asserted the old k <= 1 failure.
With the k = 0 test and the CHANGELOG entry added, this looks good to merge from my side. cc @Mec-iS
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
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
KNNClassifier::fitwithwith_k(1)comes back withk should be > 1, k=[1], so there's no way to get a 1-nearest-neighbour classifier, and a loop overkfrom 1 upwards falls over on the first one.KNNRegressorhas takenk = 1since 92dad01 relaxed its copy of the check, the classifier just never got the same change.New expected behaviour
Same check as
KNNRegressor, sok = 1works andk = 0is still an error. The new test fits withk = 1and checks taht every training point gets its own label back.It's two characters of fix really, the test is most of the diff.
Change logs
Changed
KNNClassifieracceptsk = 1