ENH Raises error in hinge_loss when 'pred_decision' is invalid #19643

PierreAttard · 2021-03-08T13:34:43Z

Reference Issues/PRs

Fixes #19638

What does this implement/fix? Explain your changes.

With multiclass target (more than 2 classes), the pred_decision argument must have a shape of type :
(n_sample, n_classes) like written in the doc.
If it is not the case, an arror is raised with a clear message.

Any other comments?

A new test has been added in test_classification in order to check that situation.

…number of classes with a multiclass target case. New test in order to check this situation in 'test_classification.py'

thomasjpfan

Thank you for the PR @PierreAttard !

thomasjpfan · 2021-03-09T17:49:19Z

sklearn/metrics/_classification.py

+        if (pred_decision.ndim == 1) or \
+                (labels is not None and pred_decision.ndim > 1 and
+                 np.size(y_true_unique) != pred_decision.shape[1]):
+            raise ValueError("The shape of pred_decision is not "
+                             "consistent with the number of classes. "
+                             "pred_decision shape must be "
+                             "(n_samples, n_classes) with "
+                             "multiclass target")


The logically flow in this section is a bit strange. I think the following would be clearer:

labels = np.unique(labels if labels is not None else y_true) if labels.size > 2: if pred_decision.ndim == 1 or labels.size != pred_decision.shape[1]: raise ValueError(...) le = LabelEncoder() le.fit(labels) ...

This would cover both cases for input validation. What do you think?

Thank you for the PR @PierreAttard !

It's a pleasure !

Indeed, I agreed. I made a modification that looks like what you have proposed at first.
But in this case, we will can not cover the situation labels is None :

if (labels is None and pred_decision.ndim > 1 and (np.size(y_true_unique) != pred_decision.shape[1])): raise ValueError("Please include all labels in y_true " "or pass labels as third argument")

If it's ok for you, I can do the modification you proposed and adapt the below unitary test in file test_classification.py to the new situation :

def test_hinge_loss_multiclass_missing_labels_with_labels_none

I am thinking of combing the two if statements into one because they almost do the same check, but looking at this more closely I think we want the two error messages. How about this:

if pred_decision.ndim > 1 and np.size(y_true_unique) != pred_decision.shape[1]): if labels is not None: raise ValueError("The shape of pred_decision is not consistent ...") else: raise ValueError("Please include ..")

while leaving everything else the same.

Seems a good solution to me.

Ah finally, we miss the situation of the intial issues #19638 :

pred_decision with only 1 dimension

Whereas it is a multiclass problem

So, we can do add pred_decision.ndim == 1:

if pred_decision.ndim == 1 or (pred_decision.ndim > 1 and np.size(y_true_unique) != pred_decision.shape[1]): if labels is not None: raise ValueError("The shape of pred_decision is not consistent ...") else: raise ValueError("Please include ..")

But we would get the wrong error message.

Yea that makes sense. I think we can also remove one of the conditions:

if pred_decision.ndim <= 1 or y_true_unique.size != pred_decision.shape[1]: if labels is not None: raise ValueError("The shape of pred_decision is not consistent ...") else: raise ValueError("Please include ..")

This would catch the ndim == 0 case as well. This feels like an exercise in boolean logic :)

This feels like an exercise in boolean logic :)

Indeed !!

I just add labels is not None or pred_decision.ndim <= 1 in order to get the right error message when you put a 1d array for pred_decision whereas it is a multiclass problem.

thomasjpfan · 2021-03-10T22:31:57Z

sklearn/metrics/_classification.py

+            if labels is not None or pred_decision.ndim <= 1:
+                raise ValueError("The shape of pred_decision is not "
+                                 "consistent with the number of classes. "
+                                 "pred_decision shape must be "
+                                 "(n_samples, n_classes) with "
+                                 "multiclass target")
+            else:
+                raise ValueError("Please include all labels in y_true "
+                                 "or pass labels as third argument")


A small nit. With another condition, on labels is None, the logic becomes harder to follow at a glance. At this point, I think creating invalid_decision_shape would make this condition more explicit:

invalid_decision_shape = (pred_decision.ndim > 1 and y_true_unique.size != pred_decision.shape[1])) if labels is None: if invalid_decision_shape: raise ValueError("Please include all the labels...") elif invalid_decision_shape or pred_decision.ndim <= 1: raise ValueError("The shape of pred_decision...")

Ok, indeed, and it's more elegant.

But we still have the same issue.
If the user do like #19638, it means inge_loss(y_true=[2,1,0,1,0,1,1], pred_decision=[0,1,2,1,0,2,1]),
the user will get the message "Please include all the labels..." whereas the true error should be "The shape of pred_decision...". If the user, after the error message "Please include all the labels...", pass the labels parameter, he will still have an error message.

The logical difficulty comes from the fact that there is a "specific case" for the situation multiclass problem AND pred_decision 1d given. It means that the user did not understand the meaning of this parameter (like I didn't).

thomasjpfan · 2021-03-10T22:33:16Z

sklearn/metrics/tests/test_classification.py

+    assert_raise_message(ValueError, error_message, hinge_loss,
+                         y_true=y_true, pred_decision=pred_decision)


We are moving toward pytest.raises for testing error messages.

thomasjpfan · 2021-03-10T22:33:32Z

sklearn/metrics/tests/test_classification.py

+    pred_decision = [[0, 1], [0, 1], [0, 1], [0, 1],
+                     [2, 0], [0, 1], [1, 0]]
+    labels = [0, 1, 2]
+    assert_raise_message(ValueError, error_message, hinge_loss,


Same here regarding pytest.raises.

thomasjpfan · 2021-03-10T22:34:10Z

doc/whats_new/v1.0.rst

+- |Enhancement| A fix to raise an error in :func:`metrics.hinge_loss` when ``pred_decision`` is 1d
+  whereas it is a multiclass classification or when ``pred_decision``
+  parameter is not consistent with ``labels`` parameter.
+  :pr:`19643` by :user:`Pierre Attard <PierreAttard>`.


We also try to keep this < 80 characters

…raise_message removed

E127 and E501

PierreAttard · 2021-03-11T08:45:49Z

So, I proposed to put all consistency checks inside a function, I think it is clearer.

thomasjpfan · 2021-03-11T15:45:31Z

sklearn/metrics/_classification.py

@@ -2285,6 +2285,48 @@ def log_loss(y_true, y_pred, *, eps=1e-15, normalize=True, sample_weight=None,
    return _weighted_sum(loss, sample_weight, normalize)


+def _check_valid_multiclass_decision_shape(y_true_unique, pred_decision,


I would be okay with creating another function if we can use this check in other metrics. I can be missing one, but I do not see a metric that does this validation.

thomasjpfan · 2021-03-11T15:50:22Z

sklearn/metrics/_classification.py

+    if labels is None:
+        if invalid_decision_shape:
+            raise ValueError("Please include all labels in y_true "
+                             "or pass labels as third argument")
+    elif invalid_decision_shape:
+        raise ValueError("The shape of pred_decision is not "


With the pred_decision.ndim <= 1 handled before hand. I think the following is simple enough to inlined into hinge_loss.

if pred_decision.ndim <= 1: raise ValueError("The shape of pred_decision can not be 1d...") # pred_decision.ndim > 1 is true if y_true_unique.size != pred_decision.shape[1]: if labels is None: raise ValueError("Please include all...") else: raise ValueError("The shape of pred_decision...")

Thank you for your patience on this PR @PierreAttard .

No, it's ok !

…terget.

thomasjpfan · 2021-03-11T16:32:17Z

sklearn/metrics/tests/test_classification.py

@@ -135,7 +134,7 @@ def test_classification_report_dictionary_output():
        target_names=iris.target_names, output_dict=True)

    # assert the 2 dicts are equal.
-    assert(report.keys() == expected_report.keys())
+    assert (report.keys() == expected_report.keys())


There are some unrelated changes in this PR, most likely caused by an editor. May these changes be reverted?

I check it.

Indeed, I didn't notice those changes in a commit.

ogrisel

Some more suggestions but otherwise, LGTM.

doc/whats_new/v1.0.rst

sklearn/metrics/_classification.py

sklearn/metrics/tests/test_classification.py

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

ogrisel

LGTM with the following details:

sklearn/metrics/tests/test_classification.py

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

…nto hinge_loss_y_true_consistency # Conflicts: # sklearn/metrics/tests/test_classification.py

ogrisel · 2021-03-12T23:17:41Z

Thank very much @PierreAttard for making the error message much more user-friendly.

ogrisel · 2021-03-12T23:18:05Z

Final review @thomasjpfan?

thomasjpfan

LGTM

…t-learn#19643) Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

raise an error when the 'pred_decision' shape is not consistent with …

3d02369

…number of classes with a multiclass target case. New test in order to check this situation in 'test_classification.py'

github-actions bot added the module:metrics label Mar 8, 2021

add the asset_raise_message import

86197f3

cmarmo added the Bug label Mar 8, 2021

PierreAttard changed the title ~~raise an error when the 'pred_decision' shape is not consistent with number of classes with a multiclass target~~ raise an error when the 'pred_decision' shape is not consistent with number of classes with a multiclass target label:"No Changelog Needed" Mar 8, 2021

PierreAttard changed the title ~~raise an error when the 'pred_decision' shape is not consistent with number of classes with a multiclass target label:"No Changelog Needed"~~ raise an error when the 'pred_decision' shape is not consistent with number of classes with a multiclass target Mar 8, 2021

p.attard and others added 2 commits March 8, 2021 15:22

add changelog

feee8f5

Merge branch 'main' into hinge_loss_y_true_consistency

4d6aa3c

thomasjpfan reviewed Mar 9, 2021

View reviewed changes

cleaner test in order to raise the right error

cc6462f

thomasjpfan reviewed Mar 10, 2021

View reviewed changes

PierreAttard and others added 6 commits March 11, 2021 09:13

all tests for valid multiclass decision inside a function.

50b2878

assert_raise_message replaced by pytest.raises. import of assert_…

f855400

…raise_message removed

line with less than 80 characters.

02ad7f7

Code standard modification

c2937c2

E127 and E501

resolve W605 invalid escape sequence

f2d5a7b

resolve E501 line too long

e30624e

thomasjpfan reviewed Mar 11, 2021

View reviewed changes

remove function use only one time. 1d pred_decision for a multiclass …

a01947c

…terget.

thomasjpfan reviewed Mar 11, 2021

View reviewed changes

revert accidental cosmetic changes caused by an editor.

0d3b31e

ogrisel reviewed Mar 11, 2021

View reviewed changes

doc/whats_new/v1.0.rst Outdated Show resolved Hide resolved

sklearn/metrics/_classification.py Outdated Show resolved Hide resolved

sklearn/metrics/_classification.py Outdated Show resolved Hide resolved

sklearn/metrics/tests/test_classification.py Outdated Show resolved Hide resolved

PierreAttard and others added 2 commits March 11, 2021 19:44

Update doc/whats_new/v1.0.rst

4b8839e

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

Add precision to error messages.

9bd35c2

ogrisel approved these changes Mar 12, 2021

View reviewed changes

sklearn/metrics/tests/test_classification.py Outdated Show resolved Hide resolved

sklearn/metrics/tests/test_classification.py Outdated Show resolved Hide resolved

PierreAttard and others added 3 commits March 12, 2021 18:37

Update sklearn/metrics/tests/test_classification.py

fd2f658

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

hard code expected and observed values in unitest

6cb0dad

Merge remote-tracking branch 'origin/hinge_loss_y_true_consistency' i…

6fd0a21

…nto hinge_loss_y_true_consistency # Conflicts: # sklearn/metrics/tests/test_classification.py

thomasjpfan approved these changes Mar 12, 2021

View reviewed changes

thomasjpfan changed the title ~~raise an error when the 'pred_decision' shape is not consistent with number of classes with a multiclass target~~ ENH Raises error in hinge_loss when 'pred_decision' is invalid Mar 12, 2021

thomasjpfan merged commit f4e692c into scikit-learn:main Mar 12, 2021

marrodion pushed a commit to marrodion/scikit-learn that referenced this pull request Mar 17, 2021

ENH Raises error in hinge_loss when 'pred_decision' is invalid (sciki…

6f05cd3

…t-learn#19643) Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

glemaitre mentioned this pull request Apr 22, 2021

Release 0.24.2 #19954

Merged

12 tasks

		assert_raise_message(ValueError, error_message, hinge_loss,
		y_true=y_true, pred_decision=pred_decision)

		@@ -2285,6 +2285,48 @@ def log_loss(y_true, y_pred, *, eps=1e-15, normalize=True, sample_weight=None,
		return _weighted_sum(loss, sample_weight, normalize)


		def _check_valid_multiclass_decision_shape(y_true_unique, pred_decision,

Uh oh!

ENH Raises error in hinge_loss when 'pred_decision' is invalid #19643

ENH Raises error in hinge_loss when 'pred_decision' is invalid #19643

Uh oh!

Conversation

PierreAttard commented Mar 8, 2021

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

Uh oh!

thomasjpfan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PierreAttard Mar 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PierreAttard Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PierreAttard commented Mar 11, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PierreAttard Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogrisel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ogrisel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ogrisel commented Mar 12, 2021

PierreAttard Mar 10, 2021 •

edited

Loading

PierreAttard Mar 11, 2021 •

edited

Loading

PierreAttard Mar 11, 2021 •

edited

Loading