-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
Add non-strict mode to check_estimator #13969
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
It's true that having a way to differentiate common tests used internally for scikit-learn and those intented to run on 3rd-party packages would be very useful. Related to #6715 . Maybe using something like, if estimator.__module__.startswith('sklearn.'):
# run some scikit-learn specific check in common tests would be better? For the examples you provide they should never apply outside of scikit-learn and this would avoid passing this parameter everywhere.. |
I would rather give the user the control rather than being magic about it. It would add the parameter in a lot of places. |
I'm happy with strict=False param...
|
I think not having the exact error message is good. Now...are we okay with not even checking the types when |
Or strict could be |
Since this seems to be the place, do we want to have a deprecation cycle for certain API changes in |
I'm working on this. NYC WiMLDS |
@hirenmayani I would recommend you start with something smaller if you haven't worked on sklearn before, you can also pick up something more tricky later. |
I think I can fix it. Will ask you if I need any help. |
@hirenmayani ok go for it then. |
Re-opening since we still need to make another PR to use the mechanism added in #17361 in more common tests. Also add a what's new entry. |
Not validating error messages is done in #18415. What else do we want to relax when strict mode is off? |
From slack conversation, it would be nice for scikit-learn/sklearn/utils/estimator_checks.py Line 1735 in 2218ec4
To just: assert y_pred.dtype.kind == "f" # any precision floating-point |
Similar to #16241, moving to 1.1 |
very unlikely to be part of 1.1, moving to 1.2 |
I think now that we have #29699 merged and released. |
Right now the common tests have two different use-cases: checking internal sklearn estimators, and checking 3rd party estimators via
check_estimator
.The common tests are very strict to a degree that's not really reasonable for 3rd party estimators, in particular in checking for particular error messages.
Furthermore, the required error-messages are not documented anywhere (see also #10082).
I think it would be good to add a
strict=False
argument to check_estimator to not require exact messages, and possibly not even the exact error types. That would make it much easier for people to usecheck_estimator
.The text was updated successfully, but these errors were encountered: