Skip to content

[MRG] Better error message for _check_clf_target #2147

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Jul 24, 2013
Merged

[MRG] Better error message for _check_clf_target #2147

merged 6 commits into from
Jul 24, 2013

Conversation

arjoly
Copy link
Member

@arjoly arjoly commented Jul 10, 2013

Should fix #2137

@arjoly arjoly mentioned this pull request Jul 10, 2013
Closed
GaelVaroquaux
GaelVaroquaux reviewed Jul 11, 2013
View reviewed changes
sklearn/metrics/metrics.py

# No metrics support "multiclass-multioutput" format
if (y_type not in set(["binary", "multiclass",
"multilabel-indicator", "multilabel-sequences"])):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that you need the 'set', here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 947bc29

@jaquesgrobler
Copy link
Member

looks good to me.. +1

@arjoly
Copy link
Member Author

arjoly commented Jul 22, 2013

Rebase on top of master

@glouppe
Copy link
Contributor

glouppe commented Jul 22, 2013

It seems ok to me, except the line y_type = y_type.pop() which would benefit from a small one-liner comment to make it clear.

@arjoly
Copy link
Member Author

arjoly commented Jul 22, 2013

I have added a comment.

amueller
amueller reviewed Jul 22, 2013
View reviewed changes
sklearn/metrics/tests/test_metrics.py
expected = EXPECTED[type2, type1]
if expected is None:
assert_raises(ValueError, _check_clf_targets, y1, y2)
for (type1, y1), (type2, y2) in product(EXAMPLES, EXAMPLES):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alternative: product(EXAMPLES, repeat=2)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks ! I didn't know this one.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in 05ba27b

@arjoly
Copy link
Member Author

arjoly commented Jul 24, 2013

I would like to merge this. Is it ok for you @amueller ?

@amueller
Copy link
Member

👍

@arjoly
Copy link
Member Author

arjoly commented Jul 24, 2013

Great thanks !

@arjoly arjoly merged commit bff544e into scikit-learn:master Jul 24, 2013
@arjoly arjoly deleted the better-error-msg branch July 24, 2013 11:47
@arjoly
Copy link
Member Author

arjoly commented Jul 24, 2013

Merged thanks for the reviews !!!

@arjoly arjoly mentioned this pull request Jul 29, 2013
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.14-rc
Development

Successfully merging this pull request may close these issues.

Confusion error message with _check_clf_target in the metrics module
5 participants
@arjoly @jaquesgrobler @glouppe @amueller @GaelVaroquaux