Add RAFT option to make_blobs - #8482
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 (4)
Included review availability: Your plan includes up to 12 reviews per rolling hour; 10 remain after this review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds an optional RAFT-backed ChangesRAFT-backed
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The PR adds an opt-in RAFT path while preserving the existing default behavior; no actionable merge-blocking risk remains after normal checks and review. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
csadorf
left a comment
There was a problem hiding this comment.
Thanks a lot for the contribution! I have a few change requests and questions before we can move forward.
| return_centers=False, | ||
| order="F", | ||
| dtype="float32", | ||
| use_raft=False, |
There was a problem hiding this comment.
We should maintain the CuPy implementation only to overcome any present limitations or if there is a non-trivial regime where CuPy is generally faster (which does not seem to be the case).
| <int64_t>n_centers, | ||
| row_c, | ||
| <const float*>ctr_p, | ||
| <const float*>0, |
There was a problem hiding this comment.
The native API already accepts per-cluster standard deviations through the cluster_std device pointer. Could we pass an array here instead of restricting the RAFT path to a scalar cluster_std? That would preserve the existing Python API and eliminate one fallback case.
|
|
||
| if return_centers and made_ctr: | ||
| raise ValueError( | ||
| "`return_centers=True` with generated centers is not supported " |
There was a problem hiding this comment.
Could we preserve return_centers=True by generating the centers in the Python compatibility layer and passing them explicitly to RAFT? That would allow generated-center calls to use RAFT without narrowing the existing API.
There was a problem hiding this comment.
To be clear, we should only do that for this specific condition.
|
|
||
| # Native labels are integers; Python make_blobs has historically returned | ||
| # them in the requested floating dtype, so keep that behavior here. | ||
| h.sync() |
There was a problem hiding this comment.
make_blobs is decorated with mlfunc, so its CuPy operations run on Stream.ptds, and the default RAFT handle also uses cudaStreamPerThread. Why do we need to sync here?
| # Native labels are integers; Python make_blobs has historically returned | ||
| # them in the requested floating dtype, so keep that behavior here. |
There was a problem hiding this comment.
This in-line comment seems to be at the wrong spot.
Adds an opt-in RAFT path to
cuml.datasets.make_blobs.The existing CuPy path stays the default.
Added focused tests for seeded reproducibility and C/F layouts.
Fixes #7363.