Repository: scikit-learn/scikit-learn
Pull request: [MRG] FIX fix hinge_loss when labels are provided and not inferred from y_true
<!--
Thanks for contributing a pull request! Please ensure you have taken a look at
the contribution guidelines: https://github.com/scikit-learn/scikit-learn/blob/master/CONTRIBUTING.md#pull-request-checklist
-->

#### Reference Issues/PRs
<!--
Example: Fixes #1234. See also #3456.
Please use keywords (e.g., Fixes) to create link to the issues or pull requests
you resolved, so that they will automatically be closed when your pull request
is merged. See https://github.com/blog/1506-closing-issues-via-pull-requests
-->
Fixes #17630 

#### What does this implement/fix? Explain your changes.
From issue: Multilabel hinge_loss fails if not all labels are present in y_true despite all the labels being explicitly provided using external labels argument (as defined in the function description).

This causes a slightly misleading error message, and stack trace because the function is in the branch assuming that it's handling the binary target case.

This happens because the `labels` parameter is not directly considered in the logic for determining if we're in the multiclass case. This branch is made by checking of the variable `y_true_unique` has more than two values. The fix directly addresses this by including `labels` in the computation of `y_true_unique`.

#### Any other comments?
- [x] Do I need to write a regression test?

<!--
Please be aware that we are a loose team of volunteers so patience is
necessary; assistance handling other issues is very welcome. We value
all user contributions, no matter how minor they are. If we are slow to
review, either the pull request needs some benchmarking, tinkering,
convincing, etc. or more likely the reviewers are simply busy. In either
case, we ask for your understanding during the review process.
For more information, see our FAQ on this topic:
http://scikit-learn.org/dev/faq.html#why-is-my-pull-request-not-getting-any-attention.

Thanks for contributing!
-->


Problem statement:
Multilabel hinge_loss bug
#### Describe the bug

Multilabel hinge_loss fails if not all labels are present in y_true despite all the labels being explicitly provided using external labels argument (as defined in the function description).

#### Steps/Code to Reproduce

Example:
```python

from sklearn.metrics import hinge_loss

# Example dataset
y_true = [0, 1, 2, 0]
y_true_fail = [0, 2, 2, 0]
pred_decision = [[0, 0, 1], [0, 1, 0], [1, 0, 0], [0, 0, 1]]
all_labels = [0, 1, 2]

# Working example
hinge_loss(y_true, pred_decision, labels=all_labels) # returns 1.5

# Fails
hinge_loss(y_true_fail, pred_decision, labels=all_labels) # fails, should return 2.0
```
#### Expected Results
The first example (y_true): function returns 1.5
The second example (y_true_fail): function should return 2.0

#### Actual Results
ValueError: y should be a 1d array, got an array of shape (4, 3) instead.

#### Suggested solution
The function checks whether to use the multilabel or binary class cases using
 
```python
if y_true_unique.size > 2:
    #handles multilabel calculation
    # ...
```

Instead, the suggested solution for the multilabel case is adding the following:
```python
if labels is None:
    n_labels = y_true_unique.size
else:
    n_labels = np.unique(labels).size
    
if n_labels > 2:
    #handling multilabel calculation
```

#### Versions
System:
    python: 3.6.7 (default, Oct 22 2018, 11:32:17)  [GCC 8.2.0]
executable: /home/klemen/virtualenvs/datascience/bin/python3
   machine: Linux-4.15.0-36-generic-x86_64-with-Ubuntu-18.04-bionic

Python dependencies:
          pip: 9.0.1
   setuptools: 39.0.1
      sklearn: 0.23.1
        numpy: 1.18.4
        scipy: 1.4.1
       Cython: None
       pandas: 1.0.3
   matplotlib: 3.2.1
       joblib: 0.15.1
threadpoolctl: 2.0.0

Built with OpenMP: True

Hints from issue discussion:
seems reasonable, would you be kind enough to submit a PR? 
Hi @adrinjalali, I just took a quick look at this, I think the easiest fix is to change the creation of `y_true_unique` to `y_true_unique = np.unique(labels or y_true)`. I can open a PR so we can discuss if this ticks the desired boxes.

Commit message: Fix linting issues

Patch to review:
diff --git a/sklearn/metrics/_classification.py b/sklearn/metrics/_classification.py
index d48f3e242231f..110bb8677db49 100644
--- a/sklearn/metrics/_classification.py
+++ b/sklearn/metrics/_classification.py
@@ -2331,7 +2331,7 @@ def hinge_loss(y_true, pred_decision, *, labels=None, sample_weight=None):
     check_consistent_length(y_true, pred_decision, sample_weight)
     pred_decision = check_array(pred_decision, ensure_2d=False)
     y_true = column_or_1d(y_true)
-    y_true_unique = np.unique(y_true)
+    y_true_unique = np.unique(labels if labels is not None else y_true)
     if y_true_unique.size > 2:
         if (labels is None and pred_decision.ndim > 1 and
                 (np.size(y_true_unique) != pred_decision.shape[1])):
diff --git a/sklearn/metrics/tests/test_classification.py b/sklearn/metrics/tests/test_classification.py
index 01db8e71dd6fc..77f235ce5bb6c 100644
--- a/sklearn/metrics/tests/test_classification.py
+++ b/sklearn/metrics/tests/test_classification.py
@@ -2065,7 +2065,7 @@ def test_hinge_loss_multiclass():
     np.clip(dummy_losses, 0, None, out=dummy_losses)
     dummy_hinge_loss = np.mean(dummy_losses)
     assert (hinge_loss(y_true, pred_decision) ==
-                 dummy_hinge_loss)
+            dummy_hinge_loss)
 
 
 def test_hinge_loss_multiclass_missing_labels_with_labels_none():
@@ -2102,7 +2102,30 @@ def test_hinge_loss_multiclass_with_missing_labels():
     np.clip(dummy_losses, 0, None, out=dummy_losses)
     dummy_hinge_loss = np.mean(dummy_losses)
     assert (hinge_loss(y_true, pred_decision, labels=labels) ==
-                 dummy_hinge_loss)
+            dummy_hinge_loss)
+
+
+def test_hinge_loss_multiclass_missing_labels_only_two_unq_in_y_true():
+    pred_decision = np.array([
+        [+0.36, -0.17, -0.58],
+        [-0.15, -0.58, -0.48],
+        [-1.45, -0.58, -0.38],
+        [-0.55, -0.78, -0.42],
+        [-1.45, -0.58, -0.38]
+    ])
+    y_true = np.array([0, 2, 2, 0, 2])
+    labels = np.array([0, 1, 2])
+    dummy_losses = np.array([
+        1 - pred_decision[0][0] + pred_decision[0][1],
+        1 - pred_decision[1][2] + pred_decision[1][0],
+        1 - pred_decision[2][2] + pred_decision[2][1],
+        1 - pred_decision[3][0] + pred_decision[3][2],
+        1 - pred_decision[4][2] + pred_decision[4][1]
+    ])
+    np.clip(dummy_losses, 0, None, out=dummy_losses)
+    dummy_hinge_loss = np.mean(dummy_losses)
+    assert (hinge_loss(y_true, pred_decision, labels=labels) ==
+            dummy_hinge_loss)
 
 
 def test_hinge_loss_multiclass_invariance_lists():


Code review comments:
diff_hunk: @@ -2102,7 +2102,30 @@ def test_hinge_loss_multiclass_with_missing_labels():
     np.clip(dummy_losses, 0, None, out=dummy_losses)
     dummy_hinge_loss = np.mean(dummy_losses)
     assert (hinge_loss(y_true, pred_decision, labels=labels) ==
-                 dummy_hinge_loss)
+            dummy_hinge_loss)
+
+
+def test_hinge_loss_multiclass_missing_labels_only_two_unq_in_y_true():
+    pred_decision = np.array([
+        [+0.36, -0.17, -0.58],
+        [-0.15, -0.58, -0.48],
+        [-1.45, -0.58, -0.38],
+        [-0.55, -0.78, -0.42],
+        [-1.45, -0.58, -0.38]
+    ])
+    y_true = np.array([0, 2, 2, 0, 2])
+    labels = np.array([0, 1, 2])
+    dummy_losses = np.array([
+        1 - pred_decision[0][0] + pred_decision[0][1],
+        1 - pred_decision[1][2] + pred_decision[1][0],
+        1 - pred_decision[2][2] + pred_decision[2][1],
+        1 - pred_decision[3][0] + pred_decision[3][2],
+        1 - pred_decision[4][2] + pred_decision[4][1]
+    ])
+    np.clip(dummy_losses, 0, None, out=dummy_losses)
+    dummy_hinge_loss = np.mean(dummy_losses)
+    assert (hinge_loss(y_true, pred_decision, labels=labels) ==
+            dummy_hinge_loss)
path: sklearn/metrics/tests/test_classification.py
text: @user1:
when comparing `float`s, we should almost never compare exact values. You could use `numpy.testing.assert_array_almost_equal` instead.