TST: Fix assert raises in sklearn/metrics/cluster/tests/
#14707
Conversation
|
Same as other PR: |
| r'Number of labels is %d\. Valid values are 2 ' | ||
| r'to n_samples - 1 \(inclusive\)' % len(np.unique(y)), | ||
| silhouette_score, X, y) | ||
| with pytest.raises(ValueError, |
There was a problem hiding this comment.
You can put the message out of raises
|
@glemaitre I removed all instances of |
@sameshl That looks like a typo; the resulting |
I thought the same, but felt it might me better if I ask someone before proceding. |
rth
left a comment
rth
left a comment
There was a problem hiding this comment.
A few comments, otherwise LGTM. Thanks!
| expected = "labels_true must be 1D: shape is (2" | ||
| assert_raise_message(ValueError, expected, score_func, | ||
| [[0, 1], [1, 0]], [1, 1, 1]) | ||
| with pytest.raises(ValueError, match=re.escape(expected)): |
There was a problem hiding this comment.
Better to manually escape with raw strings the message above. (and then remove the re import above).
There was a problem hiding this comment.
Better to manually escape with raw strings the message above. (and then remove the
reimport above).
@rth
But I thought this way is better for readability. What do you think?
There was a problem hiding this comment.
Well readers won't necessarily know exactly what re.escape does without checking the docs. I think explicitly escaping the string simpler to understand.
| expected = "labels_pred must be 1D: shape is (2" | ||
| assert_raise_message(ValueError, expected, score_func, | ||
| [0, 1, 0], [[1, 1], [0, 0]]) | ||
| with pytest.raises(ValueError, match=re.escape(expected)): |
| silhouette_score, X, y) | ||
| with pytest.raises(ValueError, | ||
| match=r'Number of labels is %d\. Valid values are 2 ' | ||
| r'to n_samples - 1 \(inclusive\)' % len(np.unique(y))): |
There was a problem hiding this comment.
Please put the message above,
msg = (r'..')
with pytest.raises(ValueError, match=msg):
| silhouette_score, X, y) | ||
| with pytest.raises(ValueError, | ||
| match=r'Number of labels is %d\. Valid values are 2 ' | ||
| r'to n_samples - 1 \(inclusive\)' % len(np.unique(y))): |
rth
dismissed
their stale review
I meant to say that my comments above still need addressing before this PR is merged.
|
@rth I have made the changes. |
|
@thomasjpfan Review this as well. Thanks! |
rth
changed the title
sklearn/metrics/cluster/tests/sklearn/metrics/cluster/tests/
4 participants
replaced
assert_raisesandassert_raises_regexwithpytest.raisescontext manager.related to #14216