Conversation
| */ | ||
| template <typename T> | ||
| void kde_score_samples(raft::resources const& handle, | ||
| const T* query, |
There was a problem hiding this comment.
Can we accept mdspans here instead of raw pointers? That would also simplify the function signatures for the public APIs. Can still expose pointers at the cuml layer for now (for the python layer) if that's easier.
| * @param[in] metric_arg Metric parameter (e.g. p for Minkowski; ignored otherwise) | ||
| */ | ||
| template <typename T> | ||
| void kde_score_samples(raft::resources const& handle, |
There was a problem hiding this comment.
Would it make sense just call this "kde" or is there signficant enough post processing being done after calling this?
| * These are the smoothing kernels used in KDE — distinct from the dot-product | ||
| * kernels (RBF, Polynomial, etc.) in cuvs::distance::kernels used by SVMs. | ||
| */ | ||
| enum class DensityKernelType : int { |
There was a problem hiding this comment.
Can we at least consolidate this struct w/ the existing KernelType struct in distance.hpp?
aamijar
added
non-breaking
|
/ok to test 4690f8d |
|
@Intron7, thanks for the PR! |
|
/ok to test 66d923b |
|
/ok to test 8c852d5 |
viclafargue
left a comment
viclafargue
left a comment
There was a problem hiding this comment.
Thanks @Intron7! LGTM for the most part, but we need testing.
It's hard to have confidence in the correctness without tests. We have to validate against scikit-learn's KernelDensity either through cuML or by adding Python bindings directly in cuVS. At minimum, this PR should include a C++ gtest covering:
- Each metric with the Gaussian kernel (isolates distance correctness)
- Each kernel with Euclidean metric (isolates kernel evaluation + normalization)
- Single-pass vs. multi-pass (force multi-pass with small n_query, verify results match)
- Weighted vs. unweighted inputs
| std::optional<raft::device_vector_view<const T, std::int64_t>> weights, | ||
| raft::device_vector_view<T, std::int64_t> output, | ||
| T bandwidth, | ||
| T sum_weights, |
There was a problem hiding this comment.
Is there a reason sum_weights is caller-provided rather than computed internally from weights? A mismatch silently produces incorrectly scaled output, and a device reduction would eliminate that risk.
|
Tests look good, thanks so much @Intron7. It looks like CI just complaining on the branch being too old. If you can update the branch with upstream main, we can get this merged. |
|
/ok to test d0e8e5c |
viclafargue
left a comment
viclafargue
left a comment
There was a problem hiding this comment.
Thanks for adding the tests. We're almost there, but the tiling system isn't being sufficiently exercised. All tests use d=3, which means the feature tiling loop only ever runs a single iteration. The core cross-tile accumulation path is never tested. We need at least one test with d > 64.
|
/ok to test 6832169 |
|
/ok to test dd28037 |
|
/merge |
github-project-automation
bot
moved this from In Progress
to Done
in Unstructured Data Processing
@cjnolet moved the kernel for KDE from cuml to cuvs. This PR is needed for rapidsai/cuml#7833 Authors: - Severin Dicks (https://github.com/Intron7) - Anupam (https://github.com/aamijar) Approvers: - Victor Lafargue (https://github.com/viclafargue) - Corey J. Nolet (https://github.com/cjnolet) URL: rapidsai#1915
4 participants
@cjnolet moved the kernel for KDE from cuml to cuvs. This PR is needed for rapidsai/cuml#7833