Cleanup pairwise distances - #8268
Conversation
- Remove usage of legacy input validation and `CumlArray`/`SparseCumlArray`. - Simplify code flow, making `sparse_pairwise_distances` duplicative. - Release GIL in `libcuml` calls. - Fix bug in input reflection for `nan_euclidean_distances` - Add missing `copy` parameter to `nan_euclidean_distances` - Standardize error messages to better match sklearn - Improve test coverage for error handling and warnings - Improve test coverage for `nan_euclidean_distances`
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughRefactors ChangesPairwise distances API refactor: dense/sparse unification and nan_euclidean rewrite
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/cuml/cuml/metrics/pairwise_distances.pyx`:
- Around line 329-338: The variable `p` declared as `cdef double p` is left
uninitialized and only conditionally assigned when `metric_arg` is in kwds or
when metric equals "minkowski". For all other metrics, the uninitialized
variable is passed to C++ functions (at lines 448, 466, 480, and 493), resulting
in undefined behavior. Initialize `p` with a default value (such as 2.0, the
standard Minkowski default) at the point of declaration to ensure it always has
a defined value regardless of which code path is taken.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ede423a8-c02f-4c52-ad21-815c17cebd60
📒 Files selected for processing (3)
python/cuml/cuml/metrics/pairwise_distances.pyxpython/cuml/cuml/neighbors/kernel_density.pyxpython/cuml/tests/test_metrics.py
This standardizes the signature to `pairwise_distances` to be compatible with sklearn. Users wanting to configure the norm used in minkowski should use `p` instead of `metric_arg`.
ea934d9 to
4ea63d0
Compare
csadorf
left a comment
There was a problem hiding this comment.
Approved with one change recommendation.
| **kwds : optional keyword parameters | ||
| Any additional metric-specific parameters. For example, with | ||
| ``metric="minkowski"``, passing ``p`` sets the norm used. |
There was a problem hiding this comment.
I think we should try to exhaustively document all additional metric-specific parameters as part of the metric doc-string or alternatively redirect to the right documentation. Otherwise it feels like we are just mentioning an example here, but are actually not fully documenting these additional parameters.
There was a problem hiding this comment.
sklearn doesn't document these fully either (you have to read the code to see what would actually work). My intent here was mainly to make our implementation compatible with sklearn, not to fully document all options. I'll open this as a followup issue if anyone wants to take it on, but want to get this in as is for now.
There was a problem hiding this comment.
Perfectly fine to handle this as a potential follow-up. I’m happy to follow sklearn’s lead on API behavior where it makes sense and avoid arbitrary deviations, but I do think it’s important to view cuML’s API documentation independently. We’re serving our users, so “sklearn does this too” shouldn’t by itself be the bar for whether the docs are clear enough.
|
/merge |
Fixes #8281 This PR exhaustively documents all additional keyword parameters accepted by `pairwise_distances` for each metric that supports them: - **minkowski**: `p` (float, default=2.0) — the p-norm to apply - **nan_euclidean**: `squared` (bool), `missing_values`, `copy` (bool) — forwarded to `nan_euclidean_distances` Also notes that all other metrics do not accept additional parameters and that unknown parameters raise `TypeError`. This addresses the review comment in #8268 by @csadorf to exhaustively document metric-specific `**kwds` options. Authors: - mmustafasenoglu (https://github.com/mmustafasenoglu) Approvers: - Jim Crist-Harif (https://github.com/jcrist) URL: #8459
CumlArray/SparseCumlArray. Part of Transition to new array input validation system #7428.sparse_pairwise_distancesduplicative and unneeded.sparse_pairwise_distancesfunction. It's unclear why this was ever made public -pairwise_distancescan do everything it can, and sklearn lacks asparse_pairwise_distancesfunction.libcumlcalls.nan_euclidean_distances. Previously this function failed to reflect at all, always returningcupyoutputs.copyparameter tonan_euclidean_distances, improving sklearn compatibility.nan_euclidean_distances.metric_argtopairwise_distances, in favor of the existingkwdsoption. For minkowski, users should pass inpinstead ofmetric_arg. Fixes [BUG] cuml pairwise_distances API differs from sklearn #4605.kwds. This caught a bug in the tests, where a non-existentoutput_typekeyword argument was silently being ignored.