[MRG + 1] BUG :#5782 check_classification_targets returns y instead of y_type #5783
Conversation
|
lgtm thanks |
|
umm can you add a test? |
|
Have added test case . Please review. |
| def test_check_classification_targets(): | ||
| # Test that check_classification_target return correct type. #5782 | ||
| y = np.array([0.0, 1.1, 2.0, 3.0]) | ||
| assert_raises(ValueError, check_classification_targets, y) |
There was a problem hiding this comment.
This would pass previously as well. Can you use assert_warns_message` and check that y_type is present in the message?
There was a problem hiding this comment.
Can I use assert_warns_message with ValueError ?
There was a problem hiding this comment.
Sorry, can you catch the error message and check. You could look at the tests in sklearn/tests/test_common.py for inspiration.
There was a problem hiding this comment.
I have modified test case.
Please review .
MechCoder
changed the title
| # Test that check_classification_target return correct type. #5782 | ||
| y = np.array([0.0, 1.1, 2.0, 3.0]) | ||
| msg = type_of_target(y) | ||
| assert_raise_message(ValueError, msg, check_classification_targets, y) |
There was a problem hiding this comment.
Can you do something similar to this? (https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/utils/estimator_checks.py#L351)
There was a problem hiding this comment.
why? This one is much nicer, I think.
There was a problem hiding this comment.
right. I did not read this properly.
amueller
changed the title
|
LGTM |
[MRG + 1] BUG :#5782 check_classification_targets returns y instead of y_type
|
Thanks ! |
Please review.