Add K-Nearest Neighbors classifier - #7563
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7563 +/- ##
============================================
+ Coverage 80.61% 80.66% +0.04%
- Complexity 7497 7526 +29
============================================
Files 816 817 +1
Lines 24097 24162 +65
Branches 4741 4759 +18
============================================
+ Hits 19427 19491 +64
Misses 3907 3907
- Partials 763 764 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
AKASH02-byte
left a comment
There was a problem hiding this comment.
Left an inline comment regarding exception checking in KNearestNeighbors.java. Overall, the implementation and tests look clean!
alxkm
left a comment
There was a problem hiding this comment.
Solid PR - clean implementation, good tests.
One change requested: move the k > trainingFeatures.length check into fit(), per the thread on line 168.
Improvements (non-blocking):
- fields are declared mid-class, move them to the top next to
k fit()stores the caller's arrays directly — a defensive copy would be safer- the two "not fitted" null checks in
predict()can be collapsed into one Neighborcould be arecord
Nice tests coverage.
|
Hi @alxkm, I’ve addressed the requested changes, including moving the k > trainingFeatures.length validation to fit() and implementing the additional improvements discussed. All 7 CI checks are now passing. Could you please take another look and re-review the PR when you get a chance? Thank you for the feedback! |
alxkm
left a comment
There was a problem hiding this comment.
Looks good. Thank you for the contribution.
Description
This PR adds a K-Nearest Neighbors (KNN) classifier implementation to the
machinelearningpackage.Features
fit().Closes #7562
clang-format -i --style=file path/to/your/file.java