Apply new validation to metrics.confusion_matrix - #8049
Merged
rapids-bot[bot] merged 5 commits intoMay 6, 2026
Merged
Conversation
Replace legacy input_to_cuml_array / input_to_cupy_array calls in cuml.metrics.confusion_matrix with the new cuml.internals.validation helpers (check_array, check_consistent_length, check_sample_weight), matching the pattern established by the _classification, _ranking, and regression metrics migrations (xref NVIDIA#7998).
csadorf
added
improvement
📝 WalkthroughSummary by CodeRabbit
WalkthroughRefactors confusion_matrix to use standardized validation helpers ( ChangesConfusion Matrix Input Validation & Tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/cuml/cuml/metrics/confusion_matrix.py (1)
110-138:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winIncorrect return type annotation.
The function declares
-> CumlArraybut returns a rawcp.ndarray. Update the annotation tocp.ndarrayor remove it if not strictly required.🤖 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/confusion_matrix.py` around lines 110 - 138, The function's declared return type (-> CumlArray) doesn't match the actual return (a cupy ndarray); update the function signature to return cp.ndarray (or remove the explicit annotation) where the function is defined (confusion_matrix in confusion_matrix.py), and ensure any related type hints/exports referencing CumlArray are adjusted accordingly so the annotation matches the actual return type.
🤖 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.
Outside diff comments:
In `@python/cuml/cuml/metrics/confusion_matrix.py`:
- Around line 110-138: The function's declared return type (-> CumlArray)
doesn't match the actual return (a cupy ndarray); update the function signature
to return cp.ndarray (or remove the explicit annotation) where the function is
defined (confusion_matrix in confusion_matrix.py), and ensure any related type
hints/exports referencing CumlArray are adjusted accordingly so the annotation
matches the actual return type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 95d03e3e-b3f4-4dad-bbd8-fd3d647404cd
📒 Files selected for processing (2)
python/cuml/cuml/metrics/confusion_matrix.pypython/cuml/tests/test_metrics.py
The confusion_matrix function creates a cupy ndarray via .toarray() and operates on it with cp.divide / cp.nan_to_num throughout. Declaring -> CumlArray was therefore incorrect. - Change return annotation from -> CumlArray to -> cp.ndarray - Remove unused CumlArray import
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/cuml/cuml/metrics/confusion_matrix.py (1)
28-54:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign the return docstring with the new concrete return type.
Line 28 now annotates
cp.ndarray, but the Returns section still describes a generic “array-like (device or host)”. Tightening this avoids API ambiguity for callers.📝 Suggested doc update
- C : array-like (device or host) shape = (n_classes, n_classes) - Confusion matrix. + C : cupy.ndarray of shape (n_classes, n_classes) + Confusion matrix on device.🤖 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/confusion_matrix.py` around lines 28 - 54, Update the Returns docstring for the confusion_matrix function to match the concrete return type cp.ndarray: replace "array-like (device or host) shape = (n_classes, n_classes)" with "cp.ndarray shape = (n_classes, n_classes)" and keep the descriptive text "Confusion matrix" (and mention device/host behavior if desired). Edit the Returns section for the confusion_matrix function so the documented type aligns with the annotated return cp.ndarray.
🤖 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.
Outside diff comments:
In `@python/cuml/cuml/metrics/confusion_matrix.py`:
- Around line 28-54: Update the Returns docstring for the confusion_matrix
function to match the concrete return type cp.ndarray: replace "array-like
(device or host) shape = (n_classes, n_classes)" with "cp.ndarray shape =
(n_classes, n_classes)" and keep the descriptive text "Confusion matrix" (and
mention device/host behavior if desired). Edit the Returns section for the
confusion_matrix function so the documented type aligns with the annotated
return cp.ndarray.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 016a8c9f-15d0-41e4-9aa5-da5533b351fe
📒 Files selected for processing (1)
python/cuml/cuml/metrics/confusion_matrix.py
viclafargue
approved these changes
May 6, 2026
viclafargue
left a comment
viclafargue
left a comment
Contributor
There was a problem hiding this comment.
Thanks for working on this! LGTM
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
python/cuml/cuml/metrics/confusion_matrix.py (1)
47-50:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate
convert_dtypedocs to reflect sample-weight conversion path.The docstring currently describes coercion for labels/targets only, but Line 95-97 also applies
convert_dtypetosample_weight.Proposed doc tweak
convert_dtype : bool, optional (default=False) When set to True, the confusion matrix method will automatically - convert the predictions, ground truth, and labels arrays to np.int32. + convert the predictions, ground truth, and labels arrays to np.int32, + and coerce sample_weight to a supported numeric dtype when needed.As per coding guidelines: "Missing docstrings for public methods, undocumented hyperparameters, or missing scikit-learn compatibility notes in documentation must be addressed."
🤖 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/confusion_matrix.py` around lines 47 - 50, Update the confusion_matrix docstring to state that convert_dtype not only coerces predictions, ground truth, and labels to np.int32 but also coerces sample_weight to np.int32 (see the conversion logic at the lines applying convert_dtype around sample_weight). Modify the parameter description for convert_dtype in the confusion_matrix function/class docstring to explicitly mention sample_weight conversion and any related behavior or caveats so the docs match the implementation.python/cuml/cuml/dask/metrics/confusion_matrix.py (1)
63-66:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument that
normalize=Noneis supported.Line 63 currently documents only string values, but Line 82 explicitly accepts
None. Please align the parameter docs with runtime behavior.Proposed doc tweak
- normalize : string in ['true', 'pred', 'all'] + normalize : {'true', 'pred', 'all'} or None, default=NoneAs per coding guidelines: "Missing docstrings for public methods, undocumented hyperparameters, or missing scikit-learn compatibility notes in documentation must be addressed."
🤖 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/dask/metrics/confusion_matrix.py` around lines 63 - 66, Update the docstring for the normalize parameter in the confusion_matrix function to state that normalize may be one of the strings 'true', 'pred', or 'all' or None (default), and describe that None means no normalization will be applied; ensure the normalize description in the parameter block matches the runtime behavior checked in the function where normalize is compared to None.
🤖 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.
Outside diff comments:
In `@python/cuml/cuml/dask/metrics/confusion_matrix.py`:
- Around line 63-66: Update the docstring for the normalize parameter in the
confusion_matrix function to state that normalize may be one of the strings
'true', 'pred', or 'all' or None (default), and describe that None means no
normalization will be applied; ensure the normalize description in the parameter
block matches the runtime behavior checked in the function where normalize is
compared to None.
In `@python/cuml/cuml/metrics/confusion_matrix.py`:
- Around line 47-50: Update the confusion_matrix docstring to state that
convert_dtype not only coerces predictions, ground truth, and labels to np.int32
but also coerces sample_weight to np.int32 (see the conversion logic at the
lines applying convert_dtype around sample_weight). Modify the parameter
description for convert_dtype in the confusion_matrix function/class docstring
to explicitly mention sample_weight conversion and any related behavior or
caveats so the docs match the implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cd97eb4d-aacc-4869-9896-d4fafcb9facb
📒 Files selected for processing (2)
python/cuml/cuml/dask/metrics/confusion_matrix.pypython/cuml/cuml/metrics/confusion_matrix.py
jcrist
approved these changes
May 6, 2026
Member
|
/merge |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Convert metrics.confusion_matrix to new input validation system.
Part of #7998