ENH Improves error message when experimental flag is not imported #23194
Conversation
thomasjpfan
changed the title
lesteve
changed the title
There was a problem hiding this comment.
LGTM.
Just wondering, in principle we could put all the logic in __getattr__ (checking if sklearn.experimental.enable_bla is in sys.modules) and remove the code from sklearn.experimental.enable_bla, right?
The advantage would be that the experimental enabling logic in in a single place.
Yes, this is a good idea. I included your suggestion in: |
| setattr(model_selection, "HalvingRandomSearchCV", HalvingRandomSearchCV) | ||
| setattr(model_selection, "HalvingGridSearchCV", HalvingGridSearchCV) | ||
|
|
||
| model_selection.__all__ += ["HalvingRandomSearchCV", "HalvingGridSearchCV"] |
There was a problem hiding this comment.
The __all__ functionality has not been replicated in __getattr__, is that on purpose?
There was a problem hiding this comment.
I have not consider __all__. I do not think there is a way to replicate it in __getattr__. PEP562 allows us to adjust dir, but that does not influence __all__ or the import * mechanism.
With that in mind, I reverted the commit that moves everything into __getattr__.
This reverts commit b952e7a.
…ng_python_experimental_flag
|
Merging, thanks! |
Reference Issues/PRs
Fixes #13964
What does this implement/fix? Explain your changes.
This PR uses PEP 562 to display a better error message when
IterativeImputer,HalvingRandomSearchCV, orHalvingGridSearchCVis imported without the experimental import flag