-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
Allow disassembled use of check_estimator #11622
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.
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
Comments
We could for instance add a parameter from sklearn.utils.estimator_checks import check_estimator
@pytest.mark.parametrize('check, name, estimator',
check_estimator(SomeEstimatorClass, evaluate=False)
def test_skearn_compatible_estimator(check, name, estimator):
check(name, estimator) the limitation of this approach is that,
itertools.chain_fromiterable(check_estimator(Estimator, evaluate=False)
for Estimator in [Estimator1, Estimator2, etc])
Will make a PR to illustrate this better. |
|
Another way to potentially handle this is to provide, as an alternative to
check_estimator, a pytest fixture that generates the various checks, or
something similar... I've not thought this out in detail.
|
Interesting idea. The problem is that checks are generated based on the estimator, so the fixture would need to know which third party estimators to test which might be difficult. |
what about separating this decision (does check apply to this estimator) from enumerating all checks? simplest way would be to move it into the check, but maybe there's an even better idea? |
Generally some of this was discussed in #6715 but it would require some significant refactoring of the way estimator checks works. Estimator tags (#8022) could also be part of the solution in the long term. We can't just move the checks inside of the corresponding |
I thought I had commented on this. I'm not sure I'm convinced it's good to enable running individual tests because that will encourage not running all tests. If your motivation is debugging, then really we should provide a better error message. |
Thanks for the feedback. Still suppose one has a a single check that doesn't pass in It would give developers in scikit-learn contrib projects more flexibility if needed, without waiting for our 6month-12month release cycle. Flexibility is one of the things that was mentioned as limiting factor in #6715. Of course in the ideal world Also honestly it's annoying to have one tests that runs several dozens checks. That's why we are parametrizing them in scikit-learn (before with yields now with pytest). Giving contrib projects a possibility to do the same, and trusting developers not to misuse it, would be nice IMO. |
Also, some projects will never manage to be fully compliant with the scikit-learn API due to other constraints, e.g. hybrid recommendation, but at the same time, the API is somewhat inspired from scikit-learn, in which case one could imagine testing only the relevant subset of checks in |
@rth that could be done with data formats and possibly tasks / estimator types. |
@rth commented on 24 Jul 2018
I think |
Some possibly useful pointers: I think this might be a good entry point: Here's how to use hooks: I think you want to work with the collection hooks? |
Never mind, I was overthinking it, let's go with #11622 (comment) |
Description
For my downstream project, I'm testing my classifier with check_estimator, and would like to see which of its individual tests failed.
Doing so under nose works by using
for check in _yield_all_checks(name, estimator): yield check, name, my_estimator
(and a separate call to the class-level checks), but py.test won't support yield-tests, and using_yield_all_checks
to parameterize a test doesn't work because the function requires the estimator instance.Therefore the request is to provide all checks from
check_estimator
as a (non-private) iterable, so they can be used separately.I brought this up in #10728 first, errorneously.
Versions
Linux-4.17.5-1-ARCH-x86_64-with-arch-Arch-Linux
Python 3.6.6 (default, Jun 27 2018, 13:11:40)
[GCC 8.1.1 20180531]
NumPy 1.14.5
SciPy 1.1.0
Scikit-Learn 0.19.1
The text was updated successfully, but these errors were encountered: