Return Euclidean distances from KMeans transform - #8539
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 SummarySummary by CodeRabbit
Walkthrough
ChangesKMeans distance correction
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to KMeans.transform now returns Euclidean distances compatible with scikit-learn, with regression coverage for expected results. No merge-blocking risk remains. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 1 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
viclafargue
left a comment
There was a problem hiding this comment.
Thanks for opening a PR!
| # C++/cuVS uses L2Expanded, which is squared Euclidean. | ||
| return cp.sqrt(out) |
There was a problem hiding this comment.
Could we use cuVS’s native square-root metric instead?
Set it after _kmeans_init_params() (params.metric = DistanceType.L2SqrtExpanded). cuVS then applies the square root in the distance epilogue, avoiding the extra CuPy kernel and output-sized allocation from cp.sqrt(out).
There was a problem hiding this comment.
Done. transform now sets params.metric to DistanceType.L2SqrtExpanded after _kmeans_init_params so cuVS applies the square root in the distance epilogue. The extra CuPy sqrt and output sized allocation are gone.
Summary
KMeans.transform currently returns squared distances. sklearn returns Euclidean distances.
This change takes the square root of the transform output so the Python API matches sklearn. score is unchanged.
Fixes #8536
Test plan
Added a regression test against the six point example from the issue.