[MRG+1] Update class_weight.py #8319
Merged
Conversation
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
Changed ValueError message to reflect that a class label may be non-integer
Codecov Report
@@ Coverage Diff @@
## master #8319 +/- ##
==========================================
+ Coverage 94.73% 94.75% +0.02%
==========================================
Files 342 342
Lines 60674 60893 +219
==========================================
+ Hits 57482 57702 +220
+ Misses 3192 3191 -1
Continue to review full report at Codecov.
|
|
thanks. please add a test
…On 9 Feb 2017 6:02 am, "Tom Dupré la Tour" ***@***.***> wrote:
***@***.**** approved this pull request.
LGTM
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#8319 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEz6-QKumDztvjUt5cstLw96e1aLAYdks5rahFcgaJpZM4L7BOb>
.
|
|
Joel,
This is my first time with open source, GitHub, etc. Can you point me to documentation on how others add a test, do a merge, etc?
|
|
Does looking at sklearn/utils/tests/test_class_weight.py give you some idea?
…On 9 February 2017 at 07:30, MMeketon ***@***.***> wrote:
Joel,
This is my first time with open source, GitHub, etc. Can you point me to
documentation on how others add a test, do a merge, etc?
From: Joel Nothman ***@***.***
Sent: Wednesday, February 08, 2017 3:27 PM
To: scikit-learn/scikit-learn
Cc: Meketon, Marc; Author
Subject: Re: [scikit-learn/scikit-learn] [MRG+1] Update class_weight.py
(#8319)
thanks. please add a test
On 9 Feb 2017 6:02 am, "Tom Dupré la Tour" ***@***.***<
***@***.***>> wrote:
> ***@***.**** approved this pull request.
>
> LGTM
>
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub
> <#8319#
pullrequestreview-20840217>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAEz6-
QKumDztvjUt5cstLw96e1aLAYdks5rahFcgaJpZM4L7BOb>
> .
>
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub<https://github.com/
scikit-learn/scikit-learn#8319#issuecomment-278451141>, or mute the
thread<https://github.com/notifications/unsubscribe-auth/AMpYD0MipENn94E-
vExg1DAcIyeNQ4izks5raiUTgaJpZM4L7BOb>.
________________________________
This e-mail and any attachments may be confidential or legally privileged.
If you received this message in error or are not the intended recipient,
you should destroy the e-mail message and any attachments or copies, and
you are prohibited from retaining, distributing, disclosing or using any
information contained herein. Please inform us of the erroneous delivery by
return e-mail. Thank you for your cooperation.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#8319 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEz66yLZIutvyhxF9Vj-sN3ZpVs9m7kks5raiXsgaJpZM4L7BOb>
.
|
|
Yes. Very helpful. Will create the test tonight. Appreciate this pointer.
|
|
@MMeketon have you made progress on the test? It seems like a useful fix. |
|
Sorry, I got caught up on other work. Will post the test later today.
From: Loïc Estève [mailto:notifications@github.com]
Sent: Wednesday, February 15, 2017 3:42 AM
To: scikit-learn/scikit-learn
Cc: Meketon, Marc; Mention
Subject: Re: [scikit-learn/scikit-learn] [MRG+1] Update class_weight.py (#8319)
@MMeketon<https://github.com/MMeketon> have you made progress on the test? It seems like a useful fix.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#8319 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AMpYD_70Ids4-B150tZE6kr4PLDLskC5ks5rcrpzgaJpZM4L7BOb>.
…________________________________
This e-mail and any attachments may be confidential or legally privileged. If you received this message in error or are not the intended recipient, you should destroy the e-mail message and any attachments or copies, and you are prohibited from retaining, distributing, disclosing or using any information contained herein. Please inform us of the erroneous delivery by return e-mail. Thank you for your cooperation.
|
|
I just submitted the test.
Being a newbie to this, it took awhile to establish the right test environment. The test itself was pretty small.
-Marc
From: Joel Nothman [mailto:notifications@github.com]
Sent: Wednesday, February 08, 2017 7:01 PM
To: scikit-learn/scikit-learn
Cc: Meketon, Marc; Author
Subject: Re: [scikit-learn/scikit-learn] [MRG+1] Update class_weight.py (#8319)
Does looking at sklearn/utils/tests/test_class_weight.py give you some idea?
On 9 February 2017 at 07:30, MMeketon ***@***.******@***.***>> wrote:
Joel,
This is my first time with open source, GitHub, etc. Can you point me to
documentation on how others add a test, do a merge, etc?
From: Joel Nothman ***@***.***
Sent: Wednesday, February 08, 2017 3:27 PM
To: scikit-learn/scikit-learn
Cc: Meketon, Marc; Author
Subject: Re: [scikit-learn/scikit-learn] [MRG+1] Update class_weight.py
(#8319)
thanks. please add a test
On 9 Feb 2017 6:02 am, "Tom Dupré la Tour" ***@***.***<
<mailto:notifications@github.com%3c%0b>> mailto:notifications@github.com>> wrote:
> ***@***.**** approved this pull request.
>
> LGTM
>
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub
> <#8319#
pullrequestreview-20840217>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAEz6-
<https://github.com/notifications/unsubscribe-auth/AAEz6-%0b>> QKumDztvjUt5cstLw96e1aLAYdks5rahFcgaJpZM4L7BOb>
> .
>
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub<https://github.com/
<https://github.com/%0b>> scikit-learn/scikit-learn#8319#issuecomment-278451141>, or mute the
<https://github.com/notifications/unsubscribe-auth/AMpYD0MipENn94E-%0b>> vExg1DAcIyeNQ4izks5raiUTgaJpZM4L7BOb>.
________________________________
This e-mail and any attachments may be confidential or legally privileged.
If you received this message in error or are not the intended recipient,
you should destroy the e-mail message and any attachments or copies, and
you are prohibited from retaining, distributing, disclosing or using any
information contained herein. Please inform us of the erroneous delivery by
return e-mail. Thank you for your cooperation.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#8319 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEz66yLZIutvyhxF9Vj-sN3ZpVs9m7kks5raiXsgaJpZM4L7BOb>
.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub<#8319 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AMpYDwmbdyCChtM4iclASnSm0izoZUN_ks5ralcpgaJpZM4L7BOb>.
…________________________________
This e-mail and any attachments may be confidential or legally privileged. If you received this message in error or are not the intended recipient, you should destroy the e-mail message and any attachments or copies, and you are prohibited from retaining, distributing, disclosing or using any information contained herein. Please inform us of the erroneous delivery by return e-mail. Thank you for your cooperation.
|
|
I just added the test.
From: Loïc Estève [mailto:notifications@github.com]
Sent: Wednesday, February 15, 2017 3:42 AM
To: scikit-learn/scikit-learn
Cc: Meketon, Marc; Mention
Subject: Re: [scikit-learn/scikit-learn] [MRG+1] Update class_weight.py (#8319)
@MMeketon<https://github.com/MMeketon> have you made progress on the test? It seems like a useful fix.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#8319 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AMpYD_70Ids4-B150tZE6kr4PLDLskC5ks5rcrpzgaJpZM4L7BOb>.
…________________________________
This e-mail and any attachments may be confidential or legally privileged. If you received this message in error or are not the intended recipient, you should destroy the e-mail message and any attachments or copies, and you are prohibited from retaining, distributing, disclosing or using any information contained herein. Please inform us of the erroneous delivery by return e-mail. Thank you for your cooperation.
|
It is not part of the diff. You probably haven't pushed your commit to your branch then. |
|
I think I figured it out. Let me know if the “diff” is now there.
From: Loïc Estève [mailto:notifications@github.com]
Sent: Thursday, February 16, 2017 4:45 PM
To: scikit-learn/scikit-learn
Cc: Meketon, Marc; Mention
Subject: Re: [scikit-learn/scikit-learn] [MRG+1] Update class_weight.py (#8319)
I just added the test.
It is not part of the diff. You probably haven't pushed your commit to your branch then.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#8319 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AMpYD-bldrtR-KfXUXPOZQHdZrfj-nzZks5rdMNGgaJpZM4L7BOb>.
…________________________________
This e-mail and any attachments may be confidential or legally privileged. If you received this message in error or are not the intended recipient, you should destroy the e-mail message and any attachments or copies, and you are prohibited from retaining, distributing, disclosing or using any information contained herein. Please inform us of the erroneous delivery by return e-mail. Thank you for your cooperation.
|
|
No, nothing's changed. |
|
You can look at the diff yourself clicking on the "Files changed" tab, which you can access through this URL: |
…und in the class_weights dictionary Previously when class_weights were used, a check was made to see if there were elements in the class_weights dictionary that were not in the classes array. If so, a ValueError was raised. But there was a bug in the formatting of the ValueError that assumed the classes were integer and the ValueErorr was not raised when the classes were not numbers. This test is to verify that the ValueError is properly raised.
lesteve
reviewed
Feb 20, 2017
| classes = np.unique(y) | ||
| class_weights = {c: 1.0 for c in classes} | ||
| class_weights['D'] = 1.0 # This should get a proper ValueError | ||
| cw = assert_raises(ValueError, compute_class_weight, class_weights, |
There was a problem hiding this comment.
Well done for going through your git/github initiation! There are a few ways you can simplify this test. One is to
reuse test_compute_class_weight_not_present and add your test after this line rather than creating a new function .
I think that just adding this should be enough:
# Fix exception in error message formatting when missing label is a string
# https://github.com/scikit-learn/scikit-learn/issues/8312
assert_raise_message(ValueError,
'Class label label_not_present not present',
compute_class_weight,
{'label_not_present': 1.}, classes, y)There was a problem hiding this comment.
I used test_compute_class_weight_not_present as a template for the test I introduced, but did not want to change it. I'm personally OK with multiple asserts in a single test case, but others (e.g., Robert Martin, "Clean Code", Ch 9) are against it, so I didn't want to upset others with multiple asserts.
I did incorporate your suggestion of using assert_raise_message which is an excellent idea - thank you.
…layed A big thank you to Loïc Estève for pointing out that an assert_raises_message is a better test than assert_raises.
MMeketon
commented
Feb 20, 2017
|
I pushed the change I mentioned in #8319 (comment). I'll wait for the CIs to be green and merge this one. @MMeketon please ping me if I forget to do it. |
|
Merged, thanks a lot @MMeketon! |
sergeyf
pushed a commit
to sergeyf/scikit-learn
that referenced
this pull request
Feb 28, 2017
when the missing class label is a string.
Closed
Sundrique
pushed a commit
to Sundrique/scikit-learn
that referenced
this pull request
Jun 14, 2017
when the missing class label is a string.
NelleV
pushed a commit
to NelleV/scikit-learn
that referenced
this pull request
Aug 11, 2017
when the missing class label is a string.
paulha
pushed a commit
to paulha/scikit-learn
that referenced
this pull request
Aug 19, 2017
when the missing class label is a string.
maskani-moh
pushed a commit
to maskani-moh/scikit-learn
that referenced
this pull request
Nov 15, 2017
when the missing class label is a string.
lemonlaug
pushed a commit
to lemonlaug/scikit-learn
that referenced
this pull request
Jan 6, 2021
when the missing class label is a string.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
closes #8312
Changed ValueError message to reflect that a class label may be non-integer
Reference Issue #8312
What does this implement/fix? Explain your changes.
The original line
ValueError("Class label %d not present." %c)implicitly assumed the class labelcwas an integer. The fixValueError("Class label {} not present.".format(c))allows the class label to be a more general type, such as a stringAny other comments?
This is my first ever attempt to fix anything in "open source" using github. Let me know if I goofed in the process.