-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Update the HKMeansTest to Account for Allowing -1 SOAR Assignments #135544
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…se to a centroid and we shouldn't assign them
|
Pinging @elastic/es-search-relevance (Team:Search Relevance) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, with a minor comment
| // verify no duplicates exist | ||
| for (int i = 0; i < assignments.length; i++) { | ||
| assertTrue(soarAssignments[i] >= 0 && soarAssignments[i] < centroids.length); | ||
| assertTrue(soarAssignments[i] >= -1 && soarAssignments[i] < centroids.length); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: I wonder if it's useful to move that special value into a constant, so that we can reuse the same "too close" value both in code and tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can do that real quick: 👍
…earch into update_hkmeanstest
I double checked that we validate that -1 is allowed and accounted for. It indicates when a vector that's being considered for spilling is too close to a centroid to be given a SOAR assignment. So only needed to update the test.
fixes: #135538