Apply new validation to metrics.hinge_loss - #8060
Merged
rapids-bot[bot] merged 7 commits intoMay 8, 2026
Merged
Conversation
csadorf
added
improvement
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe ChangesHinge Loss Implementation Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
python/cuml/tests/test_metrics.py (1)
1773-1785: ⚡ Quick winAdd a regression for invalid binary score shapes.
The new binary coverage only exercises valid 1D scores, so the flattening bug in the binary path would still pass CI. A small
pytest.raises(ValueError)case forpred_decision.shape == (n_samples, 2)would lock in the intended validation. As per coding guidelines,**/*.py: "Update unit tests when making code changes".🤖 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 1773 - 1785, Add a negative test to ensure the binary hinge path rejects 2D score arrays: in test_hinge_loss_binary add a pytest.raises(ValueError) case that calls cuml_hinge with y_true (1D labels) and pred_decision shaped (n_samples, 2) and asserts a ValueError is raised; reference the existing test_hinge_loss_binary and cuml_hinge to locate where to add this regression test so the flattening bug is prevented.
🤖 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/hinge_loss.py`:
- Around line 135-137: The code currently flattens any 2D pred_decision in
hinge_loss, which can corrupt binary inputs; update the logic in the hinge_loss
function to only ravel pred_decision when pred_decision.ndim == 2 and
pred_decision.shape[1] == 1 (i.e., a single-column 2D array), and otherwise
raise a clear ValueError rejecting multi-column binary score arrays; keep the
rest of the flow (use of pos_label and classes) unchanged so multi-column inputs
fail early with an explicit error instead of silent corruption or opaque
broadcasts.
---
Nitpick comments:
In `@python/cuml/tests/test_metrics.py`:
- Around line 1773-1785: Add a negative test to ensure the binary hinge path
rejects 2D score arrays: in test_hinge_loss_binary add a
pytest.raises(ValueError) case that calls cuml_hinge with y_true (1D labels) and
pred_decision shaped (n_samples, 2) and asserts a ValueError is raised;
reference the existing test_hinge_loss_binary and cuml_hinge to locate where to
add this regression test so the flattening bug is prevented.
🪄 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: ebd74ba0-8364-41ca-9da4-bb2065ab4235
📒 Files selected for processing (2)
python/cuml/cuml/metrics/hinge_loss.pypython/cuml/tests/test_metrics.py
Comment on lines
+135
to
+137
| if pred_decision.ndim > 1: | ||
| pred_decision = cp.ravel(pred_decision) | ||
| pos_label = classes[-1] |
There was a problem hiding this comment.
Reject multi-column binary score arrays.
Line 135 currently ravels any 2D pred_decision. In the binary path that can silently corrupt the sample axis: a (1, 2) input becomes two losses for one sample, while larger (n, 2) inputs fail later with an opaque broadcast error. Only a single-column 2D input should be flattened here.
Possible fix
- if pred_decision.ndim > 1:
- pred_decision = cp.ravel(pred_decision)
+ if pred_decision.ndim == 2 and pred_decision.shape[1] == 1:
+ pred_decision = cp.ravel(pred_decision)
+ elif pred_decision.ndim != 1:
+ raise ValueError(
+ "pred_decision must be 1D for binary hinge loss, "
+ f"got shape {pred_decision.shape}."
+ )🤖 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/hinge_loss.py` around lines 135 - 137, The code
currently flattens any 2D pred_decision in hinge_loss, which can corrupt binary
inputs; update the logic in the hinge_loss function to only ravel pred_decision
when pred_decision.ndim == 2 and pred_decision.shape[1] == 1 (i.e., a
single-column 2D array), and otherwise raise a clear ValueError rejecting
multi-column binary score arrays; keep the rest of the flow (use of pos_label
and classes) unchanged so multi-column inputs fail early with an explicit error
instead of silent corruption or opaque broadcasts.
csadorf
force-pushed
the
issue-7998-apply-new-validation-to-metrics-hinge-loss
branch
from
May 7, 2026 18:34
f29f08e to
1f7a697
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/cuml/tests/test_metrics.py (1)
1880-1963: 💤 Low valueConsider adding test coverage for binary path shape handling.
Once the multi-column binary validation fix is applied, add tests for:
- A 2D single-column input
(n, 1)that should work correctly- A 2D multi-column input
(n, 2)that should raiseValueError🤖 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 1880 - 1963, Add two tests around the binary hinge-loss path to validate 2D shapes: create one test that passes a 2D single-column prediction array shape (n,1) to cuml_hinge (e.g., use y_true = np.array([-1,1,...]) and pred_decision = np.array([[-2.18],[2.36],...])) and assert it matches scikit-learn's sk_hinge result; add another test that passes a 2D two-column prediction array shape (n,2) and assert cuml_hinge raises ValueError (use pytest.raises(..., match="...") consistent with other tests). Place these tests near test_hinge_loss_binary / test_hinge_loss_binary_labels_* so they exercise the binary code paths for cuml_hinge.
🤖 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 1880-1963: Add two tests around the binary hinge-loss path to
validate 2D shapes: create one test that passes a 2D single-column prediction
array shape (n,1) to cuml_hinge (e.g., use y_true = np.array([-1,1,...]) and
pred_decision = np.array([[-2.18],[2.36],...])) and assert it matches
scikit-learn's sk_hinge result; add another test that passes a 2D two-column
prediction array shape (n,2) and assert cuml_hinge raises ValueError (use
pytest.raises(..., match="...") consistent with other tests). Place these tests
near test_hinge_loss_binary / test_hinge_loss_binary_labels_* so they exercise
the binary code paths for cuml_hinge.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7f420bf6-cf7f-4868-af01-23b88fde6ea4
📒 Files selected for processing (2)
python/cuml/cuml/metrics/hinge_loss.pypython/cuml/tests/test_metrics.py
Member
|
/merge |
This was referenced Jun 3, 2026
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
4 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.
Applies the new input validation system to
cuml.metrics.hinge_loss.LabelEncoder/LabelBinarizerround-trip inthe multiclass and binary paths with
cp.searchsortedandcp.where(~3× speedup on typical inputs).
sample_weightsin favor ofsample_weightto matchsklearn's convention (removal in 26.08).
Part of #7998