MAINT:Fix assert raises in sklearn/preprocessing/tests/#14717
Merged
rth merged 13 commits intoscikit-learn:masterfrom Aug 29, 2019
sameshl:tests_preprocessing
Merged
MAINT:Fix assert raises in sklearn/preprocessing/tests/#14717rth merged 13 commits intoscikit-learn:masterfrom sameshl:tests_preprocessing
sklearn/preprocessing/tests/#14717rth merged 13 commits intoscikit-learn:masterfrom
sameshl:tests_preprocessing
Conversation
Member
|
Same as other PR: you can also change |
glemaitre
reviewed
Aug 23, 2019
| " greater than the number of samples used. Got" | ||
| " 1000 quantiles and 10 samples.", | ||
| QuantileTransformer(subsample=10).fit, X) | ||
| with pytest.raises(ValueError, match="Invalid value for " |
Member
There was a problem hiding this comment.
a nicer way to be able to read it is to move the error message above pytest.raises
err_msg = "Invalid value for 'n_quantiles':0."
with pytest.raises(ValueError, match=err_msg
)You can even make a for loop (you should parametrize but let's keep for another PR)
all_err_msg = ("Invalid value for 'n_quantiles': 0.",
"Invalid value for 'subsample': 0.",
...)
all_params = ({'n_quantiles': 0.}, {'subsample': 0.}, {'subsample': 10})
for params, err_msg in zip(all_params, all_err_msg):
with pytest.raises(ValueError, match=err_msg):
QuantileTransformer(**params).fit(X)
Contributor
Author
There was a problem hiding this comment.
Thanks for the suggestion @glemaitre. Will move the errror msg out of pytest.raises. Will open another PR for parameterization purpose.
glemaitre
reviewed
Aug 23, 2019
| assert_raises_regex(ValueError, "QuantileTransformer only accepts " | ||
| "non-negative sparse matrices.", | ||
| transformer.fit, X_neg) | ||
| with pytest.raises(ValueError, match="QuantileTransformer only accepts " |
Member
There was a problem hiding this comment.
Same comments regarding the error message below. Try to move them out of raises and when possible you can loop.
glemaitre
reviewed
Aug 23, 2019
| ValueError, | ||
| "`drop` should have length equal to the number", | ||
| enc.fit, [['abc', 2, 55], ['def', 1, 55], ['def', 3, 59]]) | ||
| with pytest.raises(ValueError, |
added `match=err_msg.replace('(', '\(').replace(')', '\)'))`
because test case failing due as the regexp parameter of
the match method is matched with the re.search function
Contributor
Author
|
@glemaitre I have made the required changes. Let me know if anything else needs to be done |
rth
reviewed
Aug 28, 2019
Member
rth
left a comment
There was a problem hiding this comment.
Few comments, appart from that LGTM. Thanks
| assert_raise_message(ValueError, | ||
| "n_bins must be a scalar or array of shape " | ||
| "(n_features,).", est.fit_transform, X) | ||
| err_msg = "n_bins must be a scalar or array of shape (n_features,)." |
Member
There was a problem hiding this comment.
Suggested change
| err_msg = "n_bins must be a scalar or array of shape (n_features,)." | |
| err_msg = r"n_bins must be a scalar or array of shape \(n_features,\)." |
then remove the double .replace below.
| "(n_features,).", est.fit_transform, X) | ||
| err_msg = "n_bins must be a scalar or array of shape (n_features,)." | ||
| with pytest.raises(ValueError, | ||
| match=err_msg.replace('(', r'\(').replace(')', r'\)')): |
| "(n_features,).", est.fit_transform, X) | ||
| err_msg = "n_bins must be a scalar or array of shape (n_features,)." | ||
| with pytest.raises(ValueError, | ||
| match=err_msg.replace('(', r'\(').replace(')', r'\)')): |
| "('onehot', 'onehot-dense', 'ordinal'). " | ||
| "Got encode='invalid-encode' instead.") | ||
| with pytest.raises(ValueError, | ||
| match=err_msg.replace('(', r'\(').replace(')', r'\)')): |
| "('uniform', 'quantile', 'kmeans'). " | ||
| "Got strategy='invalid-strategy' instead.") | ||
| with pytest.raises(ValueError, | ||
| match=err_msg.replace('(', r'\(').replace(')', r'\)')): |
Contributor
Author
|
Thanks for the suggestions @rth. I have made them |
Contributor
Author
|
@rth Let me know if anything else needs to be done. |
rth
approved these changes
Aug 29, 2019
43 tasks
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
replaced
assert_raisesandassert_raises_regexwithpytest.raisescontext manager.related to #14216