[MRG] Remove class support check estimator and parametrize_with_checks #17134
Conversation
…move_class_support_check_estimator
|
Still WIP? |
|
ready now! |
|
@rth @thomasjpfan this should slightly ease #16963 and the strict mode PRs |
There was a problem hiding this comment.
Thank you @NicolasHug !
This is a change I wanted to see in a while.
The "Checking scikit-learn compatibility of an estimator" section of examples/release_highlights/plot_release_highlights_0_22_0.py may need to be updated.
| # TODO: remove class support in 0.24 and update docstrings | ||
| msg = ("Passing a class is deprecated since version 0.23 " | ||
| "and won't be supported in 0.24." | ||
| msg = ("Passing a class was deprecated in version 0.23 " |
There was a problem hiding this comment.
I do not think we normally keep the deprecation warning version in the error message. Can this be:
Starting from 0.24, only instances are supported.
There was a problem hiding this comment.
OK, I'll remove if others feel the same way. No strong opinion on my side, I just thought it might be useful since we only went for a 1 version deprecation cycle on this
There was a problem hiding this comment.
thoughts on this @rth?
There was a problem hiding this comment.
Starting from 0.24, only instances are supported for the "estimators" parameter.
Yes +1 for this error message, mostly because it tells the user what to do when they get this error. It could be a TypeError instead of a ValueError maybe as well?
| # got a class | ||
| msg = ("Passing a class is deprecated since version 0.23 " | ||
| "and won't be supported in 0.24." | ||
| msg = ("Passing a class was deprecated in version 0.23 " |
…move_class_support_check_estimator
This reverts commit 3e23792.
| @@ -2591,14 +2555,10 @@ def check_estimators_data_not_an_array(name, estimator_orig, X, y, obj_type): | |||
|
|
|||
|
|
|||
| def check_parameters_default_constructible(name, Estimator): | |||
There was a problem hiding this comment.
Can we rename it to estimator or do you think it would be considered an API breakage?
There was a problem hiding this comment.
yeah it's not ideal but I'm concerned about backward compat :/
| # only accept instances, not classes | ||
| Estimator = Estimator.__class__ | ||
|
|
||
| Estimator = Estimator.__class__ |
There was a problem hiding this comment.
This would actually fail in mypy. The reason it doesn't currently is that conda activate fails, and the the CI step silently is marked as successful. So the linting CI job is broken. Fix in #17177
There was a problem hiding this comment.
Are you sure?
mypy --ignore-missing-import sklearn passes one this branch (v 0.770)
I've had mypy and linting issues on PRs recently so it seemed to "work" fine
There was a problem hiding this comment.
and the mypy CI seems to be happy and it was run according to the output "Success: no issues found in 472 source files"
There was a problem hiding this comment.
Ahh, yes you are right, it just installs everything in the main env. Well I would have though that it would error but nevermind.
There was a problem hiding this comment.
We still need to fix the activation issues in #17177 though.
I though it was broken because I keep getting a few new mypy errors with code on master (that I haven't touched) occasionally.
…move_class_support_check_estimator
…move_class_support_check_estimator
|
Does this have your blessing @thomasjpfan? |
No description provided.