MAINT run common tests on IterativeImputer #24930
Conversation
There was a problem hiding this comment.
With this change, calling sklearn.utils.discovery.all_estimators has the side effect of enabling IterativeImputer.
Maybe this is not something we really want, and trying to do something similar in a test file would be more appropriate (even if there is still some side-effect)?
We internally use The contract of |
|
A bit convoluted and maybe not important enough to care, but this is what I meant. With the current changes in this PR, the following snippet runs without error: from sklearn.utils.discovery import all_estimators
estimators = all_estimators()
# now I can import IterativeImputer without doing "from sklearn.experimental import enable_iterative_imputer
from sklearn.impute import IterativeImputer |
|
Can we make a context manager that enables a particular/all experimental estimators? It is a bit weird to have this side effect, and I think it makes it impossible to not have the experimental estimator enabled when calling I had a quick look in the pytest documentation for a way to import something only within a particular test but couldn't find anything. If there was something like this it would be another nice solution. |
|
A (maybe?) simpler option would be to add |
|
That does sound like a simpler way, especially if there is precedent for doing this. For completeness and so I don't lose it. This is the kinda context manager I was thinking of: @contextmanager
def experimental():
from sklearn.impute._iterative import IterativeImputer
from sklearn import impute
setattr(impute, "IterativeImputer", IterativeImputer)
impute.__all__ += ["IterativeImputer"]
yield
delattr(impute, "IterativeImputer")
impute.__all__.remove("IterativeImputer")
with experimental():
from sklearn.imputer import IterativeImputer
print(IterativeImputer)
from sklearn.impute import IterativeImputer # raises ImportError |
I might be the precedent :) Fine with the solution of @lesteve |
| @@ -45,14 +45,17 @@ | |||
|
|
|||
| import sklearn | |||
|
|
|||
| # make it possible to discover experimental estimators when calling `all_estimators` | |||
| from sklearn.experimental import enable_iterative_imputer # noqa | |||
| from sklearn.experimental import enable_halving_search_cv # noqa | |||
There was a problem hiding this comment.
| from sklearn.experimental import enable_halving_search_cv # noqa | |
| from sklearn.experimental import enable_halving_search_cv # noqa | |
| from sklearn.experimental import enable_hist_gradient_boosting # noqa |
While we are at it, also add hist gradient boosting?
There was a problem hiding this comment.
HistGradientBoosting* does not need the experimental import anymore it is there just for backward-compatibility (and warns that it is not needed anymore) ...
There was a problem hiding this comment.
Ah ok. I should have run the code :D
Related to #24923
When debugging #24923, I found out that we don't run the common tests on
IterativeImputerbecause we don't explicitly enable it.