Add K-Nearest Neighbors classifier - #7563
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ 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. 🚀 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!
|
|
||
| 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
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()anIllegalArgumentExceptionis 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 anIllegalStateException. Infit()IAE becomes correct. throwsExceptionWhenKIsGreaterThanNumberOfTrainingSampleswill need to expect the throw fromfit()instead:
KNearestNeighbors knn = new KNearestNeighbors(7);
assertThrows(IllegalArgumentException.class, () -> knn.fit(features, labels));
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.
|
|
||
| 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()anIllegalArgumentExceptionis 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 anIllegalStateException. Infit()IAE becomes correct. throwsExceptionWhenKIsGreaterThanNumberOfTrainingSampleswill need to expect the throw fromfit()instead:
KNearestNeighbors knn = new KNearestNeighbors(7);
assertThrows(IllegalArgumentException.class, () -> knn.fit(features, labels));
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