Add K-Nearest Neighbors classifier - #7563
Conversation
Sorry, something went wrong.
Codecov Report❌ Patch coverage is 95.58824% with 3 lines in your changes missing coverage. Please review.
@@ Coverage Diff @@
## master #7563 +/- ##
============================================
+ Coverage 80.42% 80.48% +0.05%
- Complexity 7457 7491 +34
============================================
Files 815 816 +1
Lines 24055 24124 +69
Branches 4732 4751 +19
============================================
+ Hits 19346 19415 +69
- Misses 3945 3946 +1
+ Partials 764 763 -1 ☔ View full report in Codecov by Harness.
|
Sorry, something went wrong.
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!
Sorry, something went wrong.
|
|
||
| Map<Integer, Integer> votes = new HashMap<>(); | ||
|
|
||
| if (k > trainingFeatures.length) { |
There was a problem hiding this comment.
The check if (k > trainingFeatures.length) currently happens inside predict(). Consider moving this validation check into fit() so that invalid $k$ values are caught early during dataset initialization rather than failing later during inference.
Sorry, something went wrong.
There was a problem hiding this comment.
+1 to moving this into fit(). A few additional points in favor:
- As written, the check fires only after the full distance pass and the O(n log n) sort, so all that work is wasted before the exception is thrown.
- From predict() an IllegalArgumentException is semantically off — the test point itself is already validated at that point; the real problem is the classifier's configuration vs. the fitted data, which would be an IllegalStateException. In fit() IAE becomes correct.
- throwsExceptionWhenKIsGreaterThanNumberOfTrainingSamples will need to expect the throw from fit() instead:
KNearestNeighbors knn = new KNearestNeighbors(7);
assertThrows(IllegalArgumentException.class, () -> knn.fit(features, labels));
Sorry, something went wrong.
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
- Neighbor could be a record
Nice tests coverage.
Sorry, something went wrong.
|
|
||
| Map<Integer, Integer> votes = new HashMap<>(); | ||
|
|
||
| if (k > trainingFeatures.length) { |
There was a problem hiding this comment.
+1 to moving this into fit(). A few additional points in favor:
- As written, the check fires only after the full distance pass and the O(n log n) sort, so all that work is wasted before the exception is thrown.
- From predict() an IllegalArgumentException is semantically off — the test point itself is already validated at that point; the real problem is the classifier's configuration vs. the fitted data, which would be an IllegalStateException. In fit() IAE becomes correct.
- throwsExceptionWhenKIsGreaterThanNumberOfTrainingSamples will need to expect the throw from fit() instead:
KNearestNeighbors knn = new KNearestNeighbors(7);
assertThrows(IllegalArgumentException.class, () -> knn.fit(features, labels));
Sorry, something went wrong.
Reviewers
Requested changes must be addressed to merge this pull request.
Description
This PR adds a K-Nearest Neighbors (KNN) classifier implementation to the machinelearning package.
Features
Closes #7562