Apply new validation to metrics.pairwise_distances - #8061
Conversation
Migrate the dense pairwise_distances and nan_euclidean_distances implementations from cuml.common.input_to_cuml_array to check_array from cuml.internals.validation, matching the pattern established by the prior cuml.metrics migrations (xref NVIDIA#7998). The legacy code derived row/column-major-ness from CumlArray.order ('K' propagated from the input). check_array returns a cupy array, so the orientation is now read directly from the array's contiguity flags. For degenerate X (1 sample or 1 feature) the layout is ambiguous in X alone, so Y is allowed to choose the layout (order='A') and we propagate its choice back into the is_row_major flag. The dtype-mismatch error path now raises ValueError (sklearn-aligned) instead of TypeError; test_pairwise_distances_exceptions updated accordingly. The sparse pairwise_distances path is unchanged: it never used input_to_cuml_array.
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
📝 WalkthroughWalkthroughThis PR migrates ChangesInput Validation & Kernel Integration Migration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/cuml/tests/test_metrics.py (1)
1320-1323: ⚡ Quick winAdd a regression for the
n_features == 1layout path.This PR added explicit degenerate-layout handling, but the current order regression at Lines 1230-1295 still repeats the single-sample
(1, 4)case twice. Please add a(n, 1)variant here as well so the newy_order="A"/ contiguity propagation branch is actually locked down against scikit-learn.As per coding guidelines, "Test files must validate numerical correctness by comparing with scikit-learn, include edge case coverage (empty datasets, single sample, high-dimensional data)".
🤖 Prompt for 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. In `@python/cuml/tests/test_metrics.py` around lines 1320 - 1323, Add a regression that covers the degenerate n_features==1 layout by adding a test variant using X arrays shaped (n, 1) (e.g., 4x1) alongside the existing (1, 4) case so the new y_order="A" / contiguity propagation branch is exercised; construct X_double (float64) and X_float (float32) with shape (n,1) and compare pairwise_distances(...) results against sklearn.metrics.pairwise_distances (using the same metric and convert_dtype settings) to assert numerical equality/consistency as the other order tests do.
🤖 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.
Nitpick comments:
In `@python/cuml/tests/test_metrics.py`:
- Around line 1320-1323: Add a regression that covers the degenerate
n_features==1 layout by adding a test variant using X arrays shaped (n, 1)
(e.g., 4x1) alongside the existing (1, 4) case so the new y_order="A" /
contiguity propagation branch is exercised; construct X_double (float64) and
X_float (float32) with shape (n,1) and compare pairwise_distances(...) results
against sklearn.metrics.pairwise_distances (using the same metric and
convert_dtype settings) to assert numerical equality/consistency as the other
order tests do.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c1bf9ce4-247f-4f6e-8a54-4c6bcbcbacbc
📒 Files selected for processing (2)
python/cuml/cuml/metrics/pairwise_distances.pyxpython/cuml/tests/test_metrics.py
Convert the dense paths in
cuml.metrics.pairwise_distancesandnan_euclidean_distancesto the new input-validation infrastructure.Part of #7998.