MNT Use a pytest-like context manager in estimator_checks.py #18418
Conversation
|
@rth @thomasjpfan I hope you guys will like this |
There was a problem hiding this comment.
This is similar to from sklearn.utils._testing import assert_raises without the match feature:
scikit-learn/sklearn/utils/estimator_checks.py
Lines 1949 to 1951 in 2d0d625
This is an alias to python's assertRaises
Are we able to use this instead?
| return _Raises(expected_exp_type, match, may_pass, err_msg) | ||
|
|
||
|
|
||
| class _Raises(contextlib.AbstractContextManager): |
There was a problem hiding this comment.
Did you explore the contextlib.contextmanager implementation and found this to be cleaner?
There was a problem hiding this comment.
I don't believe @contextlib.contextmanager allows to define the __exit__ function, which is the core of the CM for catching exceptions
There was a problem hiding this comment.
For ref this is the pytest implem: https://github.com/pytest-dev/pytest/blob/master/src/_pytest/python_api.py#L702 (I kept it quite similar)
No, this is different from |
|
Would assertRaisesRegex work for pattern matching? |
|
Only partially. The point of this PR is to unify everything. You'll see in the diff that a lot of the code is now cleaner and simpler. |
There was a problem hiding this comment.
When compared to assertRaisesRegex, I think this adds:
- Support for
excepted_exp_typeas a list. may_passraised_and_matchedattribute
Do we need 1 or 3 in check_estimators.py?
For may_pass, I was thinking of reusing assertRaisesRegex
- If
may_pass=TrueandassertRaisesRegexraises an error than catch it and not raise anything - If
may_pass=False, then propagate the exception.
Overall, I am trying to think of ways to not re-implement features if we can.
Most importantly, this adds:
Yes (please see the diff ;)) |
This CM supports features that are not implemented anywhere. It will also make strict_mode adoption easier. |
| if cm.raised_and_matched: | ||
| # ValueError was raised with proper error message | ||
| return | ||
|
|
||
| assert_array_equal( | ||
| classifier.predict(X_test), y, err_msg=error_string_predict | ||
| ) |
There was a problem hiding this comment.
This is the only place where we use raised_and_matched.
Do we have any classifier that actually raises an error with "Classifier can't predict predict when only one class is present"?
There was a problem hiding this comment.
I think so, because otherwise CodeCov would give us a warning here on the GitHub GUI.
Also note that the error isn't expected in predict but in fit.
|
Thanks @thomasjpfan and @ogrisel for the reviews, I addressed all comments! All green so I'll merge |
This PR introduces a new
raisescontext manager that works likepytest.raisesbut with a few tweaks that makes it suitable for thecheck_estimatorsuite (where pytest isn't allowed).This allows to have a unified way of asserting exceptions in
estimator_checks.py, instead of usingassert_raises,assert_raises_regex, and a bunch of complextry/exceptstatements.It will also make #18415 more extensible and cleaner.