fix: cosine kmeans - #1723
Merged
Merged
Conversation
eddyxu
force-pushed
the
lei/cosine_kmeans
branch
from
December 15, 2023 17:19
7495c8e to
cf9ef38
Compare
eddyxu
marked this pull request as ready for review
December 15, 2023 18:39
wjones127
approved these changes
Dec 15, 2023
Comment on lines
+177
to
+178
| cnts[i] = cnts[largest_idx] / 2; | ||
| cnts[largest_idx] /= 2; |
Contributor
There was a problem hiding this comment.
How does this handle odd counts? Is there any requirement for these counts to be exact?
Member
Author
There was a problem hiding this comment.
At this moment, it does not matter. The new centroids ( sum / cnt ) has already been computed. And here, the cnts are just useful to find the empty / largest clusters.
Comment on lines
+475
to
+481
| MetricType::Cosine => { | ||
| let normalized = values | ||
| .chunks(dimension) | ||
| .flat_map(normalize) | ||
| .collect::<Vec<_>>(); | ||
| return compute_partitions_l2(centroids_array, &normalized, dimension) | ||
| .collect(); |
Contributor
There was a problem hiding this comment.
So, IIUC, the idea is that once you normalize, vectors that are close in L2 distance will also be close in cosine distance?
Member
Author
There was a problem hiding this comment.
This was referenced Sep 7, 2026
LuciferYang
added a commit
to LuciferYang/lance
that referenced
this pull request
Sep 7, 2026
…cking The training and assignment dispatches accepted any distance type for a float column, and the membership pass then panicked with "not supported" unless the centroid HNSW happened to be built, which bypasses that match. Narrow both dispatches to L2 and Dot so the existing descriptive error arms handle the rest. lance.util.KMeans documents cosine, so the binding now normalizes its input and clusters with l2, the way the index build path does. That makes cosine behave the same at every k instead of panicking below the HNSW threshold. The module and new_with_params doc comments claimed cosine inputs are normalized each iteration. That was implemented in lance-format#1723 and removed in lance-format#2015 while the comment was carried forward.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Use L2 over normalized vector to run cosine for kmeans.