Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MRG] Fix sklearn.metrics.classification._check_targets where y_true and y_pred are both binary but the union is multiclass #8377

Merged
merged 2 commits into from Apr 21, 2017

Conversation

@lesteve
Copy link
Member

@lesteve lesteve commented Feb 16, 2017

Reference Issue

Fix part of #8098.

What does this implement/fix? Explain your changes.

In the case where both y_pred and y_true are binary it checks that the union of them only has two values otherwise returns 'multiclass' as type.

Any other comments?

I saw #8094 and thought that it was too many different things as once. This is an attempt of making it do one less thing.

@@ -91,6 +91,10 @@ def _check_targets(y_true, y_pred):
if y_type in ["binary", "multiclass"]:
y_true = column_or_1d(y_true)
y_pred = column_or_1d(y_pred)
if y_type == "binary":

This comment has been minimized.

@lesteve

lesteve Feb 16, 2017
Author Member

This is the less invasive fix I could find. This is not super efficient because np.unique has been already called on y_true and y_pred to figure out their type in type_of_target.

@lesteve lesteve force-pushed the lesteve:fix-check-targets branch from 294e698 to e01f779 Feb 16, 2017
@@ -366,7 +366,8 @@ def test_matthews_corrcoef():
y_true_inv = ["b" if i == "a" else "a" for i in y_true]

assert_almost_equal(matthews_corrcoef(y_true, y_true_inv), -1)
y_true_inv2 = label_binarize(y_true, ["a", "b"]) * -1

This comment has been minimized.

@lesteve

lesteve Feb 16, 2017
Author Member

y_true_inv2 is an array of 0 and 1 whereas y_true is an array of 'a' and 'b'. The test was passing just by chance as noted in #8094 (comment).

@@ -379,8 +380,7 @@ def test_matthews_corrcoef():

# And also for any other vector with 0 variance
mcc = assert_warns_message(RuntimeWarning, 'invalid value encountered',
matthews_corrcoef, y_true,
rng.randint(-100, 100) * np.ones(20, dtype=int))

This comment has been minimized.

@lesteve

lesteve Feb 16, 2017
Author Member

Similar to the previous change, y_true is an array of 'a' and 'b' and we are trying to compare it to an array of ints, so something somewhere should tell us that something is wrong.

@codecov
Copy link

@codecov codecov bot commented Feb 16, 2017

Codecov Report

Merging #8377 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #8377      +/-   ##
==========================================
+ Coverage   94.75%   94.75%   +<.01%     
==========================================
  Files         342      342              
  Lines       60902    60911       +9     
==========================================
+ Hits        57708    57717       +9     
  Misses       3194     3194
Impacted Files Coverage Δ
sklearn/metrics/classification.py 97.8% <100%> (+0.02%)
sklearn/metrics/tests/test_classification.py 99.57% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b75a81...b71c3b4. Read the comment docs.

Copy link
Member

@jnothman jnothman left a comment

LGTM. A bug fix changelog entry is deserved.

lesteve added 2 commits Feb 16, 2017
@lesteve lesteve force-pushed the lesteve:fix-check-targets branch from 37474b1 to b71c3b4 Feb 22, 2017
@lesteve
Copy link
Member Author

@lesteve lesteve commented Feb 22, 2017

Thanks for the review, I added an entry in the changelog.

@MechCoder MechCoder merged commit 24653e8 into scikit-learn:master Apr 21, 2017
5 checks passed
5 checks passed
ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 94.75%)
Details
codecov/project 94.75% (+<.01%) compared to 9b75a81
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MechCoder
Copy link
Member

@MechCoder MechCoder commented Apr 21, 2017

Thanks you.

@lesteve lesteve deleted the lesteve:fix-check-targets branch Apr 25, 2017
@lesteve
Copy link
Member Author

@lesteve lesteve commented Apr 25, 2017

Thanks for the review @MechCoder !

Sundrique added a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017
…and y_pred are both binary but the union is multiclass (scikit-learn#8377)

* Fix _check_targets where y_true and y_pred are both binary

but the union of them is multiclass.

* Add entry in changelog
dmohns added a commit to dmohns/scikit-learn that referenced this pull request Aug 7, 2017
…and y_pred are both binary but the union is multiclass (scikit-learn#8377)

* Fix _check_targets where y_true and y_pred are both binary

but the union of them is multiclass.

* Add entry in changelog
dmohns added a commit to dmohns/scikit-learn that referenced this pull request Aug 7, 2017
…and y_pred are both binary but the union is multiclass (scikit-learn#8377)

* Fix _check_targets where y_true and y_pred are both binary

but the union of them is multiclass.

* Add entry in changelog
NelleV added a commit to NelleV/scikit-learn that referenced this pull request Aug 11, 2017
…and y_pred are both binary but the union is multiclass (scikit-learn#8377)

* Fix _check_targets where y_true and y_pred are both binary

but the union of them is multiclass.

* Add entry in changelog
paulha added a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017
…and y_pred are both binary but the union is multiclass (scikit-learn#8377)

* Fix _check_targets where y_true and y_pred are both binary

but the union of them is multiclass.

* Add entry in changelog
maskani-moh added a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017
…and y_pred are both binary but the union is multiclass (scikit-learn#8377)

* Fix _check_targets where y_true and y_pred are both binary

but the union of them is multiclass.

* Add entry in changelog
jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this pull request Dec 18, 2017
…and y_pred are both binary but the union is multiclass (scikit-learn#8377)

* Fix _check_targets where y_true and y_pred are both binary

but the union of them is multiclass.

* Add entry in changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.