Apply new validation to metrics.pairwise_kernels - #8050
Conversation
Replace the legacy input_to_cupy_array calls in cuml.metrics.pairwise_kernels with check_array from cuml.internals.validation, matching the pattern established by the regression, _classification, _ranking, confusion_matrix, and hinge_loss migrations (xref NVIDIA#7998). Matches sklearn behavior: non-finite inputs (NaN, inf) now raise ValueError, consistent with sklearn.metrics.pairwise.pairwise_kernels defaults.
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe ChangesInput Validation Refactoring
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes 🚥 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)
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_kernels.py`:
- Around line 278-283: The validation path drops the previous dtype coercion so
Y is no longer converted to X.dtype and the convert_dtype behavior is ignored;
when validating Y inside pairwise_kernels, call check_array with the dtype
matching X (e.g., check_array(Y, input_name="Y", dtype=X.dtype)) or pass the
existing convert_dtype flag into check_array so Y is coerced to X.dtype,
ensuring X and Y keep the same dtype and preserving the original mixed-dtype
behavior.
🪄 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: dd6e5f51-3ddc-4556-88d9-e9c28681600d
📒 Files selected for processing (1)
python/cuml/cuml/metrics/pairwise_kernels.py
| X = check_array(X, input_name="X") | ||
| if Y is None: | ||
| Y = X | ||
| else: | ||
| Y = input_to_cupy_array(Y).array | ||
| Y = check_array(Y, input_name="Y") | ||
| if X.shape[1] != Y.shape[1]: |
There was a problem hiding this comment.
convert_dtype behavior was dropped in the new validation path.
At Line 282, Y is validated but never coerced to X.dtype, so convert_dtype (Line 187) is effectively ignored. That changes prior mixed-dtype behavior and breaks the function’s documented contract.
Proposed fix
X = check_array(X, input_name="X")
if Y is None:
Y = X
else:
Y = check_array(Y, input_name="Y")
+ if convert_dtype and Y.dtype != X.dtype:
+ Y = Y.astype(X.dtype, copy=False)As per coding guidelines, “API breaking changes to Python estimator interfaces … or breaking backward compatibility require at least one release cycle for deprecations.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| X = check_array(X, input_name="X") | |
| if Y is None: | |
| Y = X | |
| else: | |
| Y = input_to_cupy_array(Y).array | |
| Y = check_array(Y, input_name="Y") | |
| if X.shape[1] != Y.shape[1]: | |
| X = check_array(X, input_name="X") | |
| if Y is None: | |
| Y = X | |
| else: | |
| Y = check_array(Y, input_name="Y") | |
| if convert_dtype and Y.dtype != X.dtype: | |
| Y = Y.astype(X.dtype, copy=False) | |
| if X.shape[1] != Y.shape[1]: |
🤖 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/cuml/metrics/pairwise_kernels.py` around lines 278 - 283, The
validation path drops the previous dtype coercion so Y is no longer converted to
X.dtype and the convert_dtype behavior is ignored; when validating Y inside
pairwise_kernels, call check_array with the dtype matching X (e.g.,
check_array(Y, input_name="Y", dtype=X.dtype)) or pass the existing
convert_dtype flag into check_array so Y is coerced to X.dtype, ensuring X and Y
keep the same dtype and preserving the original mixed-dtype behavior.
|
/merge |
Apply new input validation to
cuml.metrics.pairwise_kernels.Part of #7998