Avoid hard import of sklearn in base module. #5663
Conversation
csadorf
added
bug
| @@ -447,9 +451,10 @@ class Base(TagsMixin, | |||
|
|
|||
| def _repr_mimebundle_(self, **kwargs): | |||
There was a problem hiding this comment.
Can you verify if IPython handles this _repr_mimebundle_ function returning None in the same way as it handles this function not existing? In other words, is the notebook output without sklearn worse than the output before #5630?
There was a problem hiding this comment.
I don't know the answer, but I think you could return {"text/plain": repr(self)} in the case of scikit-learn not being installed and that would lead to the same thing being displayed as now. The way the mimebundles work is that you can return several (text, html, png, video, ...) and the UI then chooses the one it things is best. So if we only provide the text based on, it should always choose that?
There was a problem hiding this comment.
I read the documentation on this and it interpreted the phrase "[i]f this returns something, other _repr_*_ methods are ignored" to mean that if we return None on this function we are not returning "something" so other functions should be used. I also ran some tests in a Jupyter notebook which seemed to confirm that. However, could confirm that. I don't think that returning {"text/plain": repr(self)} is a good idea, because it seems that this function would take precedence over other potentially applicable functions. Which could lead to surprising behavior, especially when child classes try to specialize the representation in a meaningful way.
@betatim Was there a specific reason that you chose to use _repr_mimebundle_ as opposed to overriding _repr_html_?
There was a problem hiding this comment.
I'm slightly revising my comment. I think returning {"text/plain": repr(self)} is a good idea as long as we use the _repr_mimebundle_ function, however, I'm wondering whether it wouldn't be be better to use _repr_html_ in the first place.
There was a problem hiding this comment.
I agree that it’s better to use _repr_html_ for supporting only one special repr format.
|
I merged in branch-23.12 so that the clang-tidy job would pass. |
|
/merge |
Fixes #5662.