Apply new validation to metrics.pairwise_distances - #8065
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR migrates pairwise distance metrics ( ChangesPairwise Metrics Input Validation Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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.
🧹 Nitpick comments (1)
python/cuml/tests/test_metrics.py (1)
1525-1539: ⚡ Quick winAdd single-feature degenerate layout coverage.
This regression test currently validates only the
X.shape == (1, n_features)degenerate path. The paired logic change also targets degenerate feature-dimension inputs (X.shape == (n_samples, 1)), so that branch should be covered too.Proposed extension
`@pytest.mark.parametrize`( "x_order,y_order", [("C", "C"), ("C", "F"), ("F", "C"), ("F", "F")], ) -def test_pairwise_distances_degenerate_x_layout(x_order, y_order): +@pytest.mark.parametrize("x_shape", [(1, 4), (10, 1)]) +def test_pairwise_distances_degenerate_x_layout(x_order, y_order, x_shape): # When X has a degenerate shape (1 sample), it is both C- and # F-contiguous, so the implementation lets Y choose the layout. # Verify all four input layout combinations match sklearn. rng = np.random.RandomState(0) - X = np.asarray(rng.random_sample((1, 4)), order=x_order, dtype=np.float64) - Y = np.asarray(rng.random_sample((10, 4)), order=y_order, dtype=np.float64) + X = np.asarray(rng.random_sample(x_shape), order=x_order, dtype=np.float64) + Y = np.asarray( + rng.random_sample((10, x_shape[1])), + order=y_order, + dtype=np.float64, + ) S = cp.asnumpy(pairwise_distances(X, Y, metric="euclidean")) S_ref = sklearn_pairwise_distances(X, Y, metric="euclidean") np.testing.assert_array_almost_equal(S, S_ref, decimal=12)🤖 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 1525 - 1539, Extend the existing test_pairwise_distances_degenerate_x_layout to also cover the degenerate-feature case where X has shape (n_samples, 1) (i.e., single feature) so the branch that handles degenerate feature-dimension inputs is exercised; duplicate the parametrized x_order,y_order combinations and create a second subtest that constructs X with shape (10, 1) (or similar multi-sample single-feature) and Y with compatible shape, call pairwise_distances(X, Y, metric="euclidean") and compare its result to sklearn_pairwise_distances using np.testing.assert_array_almost_equal (keep the same dtype/order handling and decimal tolerance as the original test).
🤖 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 1525-1539: Extend the existing
test_pairwise_distances_degenerate_x_layout to also cover the degenerate-feature
case where X has shape (n_samples, 1) (i.e., single feature) so the branch that
handles degenerate feature-dimension inputs is exercised; duplicate the
parametrized x_order,y_order combinations and create a second subtest that
constructs X with shape (10, 1) (or similar multi-sample single-feature) and Y
with compatible shape, call pairwise_distances(X, Y, metric="euclidean") and
compare its result to sklearn_pairwise_distances using
np.testing.assert_array_almost_equal (keep the same dtype/order handling and
decimal tolerance as the original test).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4dc1e803-bb73-4339-ac96-5cf5854b54e4
📒 Files selected for processing (2)
python/cuml/cuml/metrics/pairwise_distances.pyxpython/cuml/tests/test_metrics.py
🚧 Files skipped from review as they are similar to previous changes (1)
- python/cuml/cuml/metrics/pairwise_distances.pyx
|
/merge |
Convert the dense paths in
cuml.metrics.pairwise_distancesandnan_euclidean_distancesto the newcheck_arrayvalidation flow, including layout handling and coverage for non-finite inputs andnan_euclideanbehavior.Part of #7998.