Repository navigation
[MRG] FIX fix hinge_loss when labels are provided and not inferred from y_true - #17935
Merged
glemaitre merged 9 commits intoAug 24, 2020
Merged
Conversation
Ultramann
changed the title
Ultramann
marked this pull request as draft
July 16, 2020 14:46
If labels is a ndarray then the evaluating it with or doesn't work.
Member
|
This also needs a test |
Contributor
Author
|
I agree, I'll write one up tonight. |
Ultramann
changed the title
Ultramann
marked this pull request as ready for review
July 17, 2020 14:43
Contributor
Author
|
@adrinjalali could you take a look at this with the added test? Many thanks. |
adrinjalali
reviewed
Aug 21, 2020
Comment on lines
+2127
to
+2128
| assert (hinge_loss(y_true, pred_decision, labels=labels) == | ||
| dummy_hinge_loss) |
Member
There was a problem hiding this comment.
when comparing floats, we should almost never compare exact values. You could use numpy.testing.assert_array_almost_equal instead.
glemaitre
approved these changes
Aug 21, 2020
glemaitre
left a comment
glemaitre
left a comment
Member
There was a problem hiding this comment.
Please add an entry to the change log at doc/whats_new/v*.rst. Like the other entries there, please reference this pull request with :pr: and credit yourself (and other contributors if applicable) with :user:.
glemaitre
changed the title
Member
|
Merging then. THanks @Ultramann |
jayzed82
pushed a commit
to jayzed82/scikit-learn
that referenced
this pull request
Oct 22, 2020
…rue (scikit-learn#17935) Co-authored-by: Ultramann <[email protected]> Co-authored-by: Guillaume Lemaitre <[email protected]>
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
3 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.
Reference Issues/PRs
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
labelsparameter is not directly considered in the logic for determining if we're in the multiclass case. This branch is made by checking of the variabley_true_uniquehas more than two values. The fix directly addresses this by includinglabelsin the computation ofy_true_unique.Any other comments?