BUG Adjusts html_repr based on configuration by thomasjpfan · Pull Request #17093 · scikit-learn/scikit-learn

thomasjpfan · 2020-04-30T15:11:11Z

_repr_html_ was defined to work to make sphinx-gallery work properly. This PR makes sure the return is just the repr, if the display option is not set.

In a jupyter notebook, the _repr_mimebundle_ will be used so this PR will not have take any effect.

thomasjpfan · 2020-04-30T17:27:51Z

@jnothman @amueller @NicolasHug @ogrisel

This is most likely only used by sphinx gallery. Currently the display configuration option does not do anything in sphinx gallery, because gallery will always should _repr_html_ which is always the graphical HTML representation.

I part of me think this PR is not needed, but I am open to other opinions.

NicolasHug · 2020-04-30T17:36:13Z

Is there an example were a block ends with an estimator? I don't think we have much of these

I'm fine eitherway. We can always merge this and then decide to use setconfig(display=diagram) in conf.py...

rth · 2020-04-30T18:03:52Z

sklearn/base.py

+        """
+        if get_config()["display"] == 'diagram':
+            return estimator_html_repr(self)
+        return f"<code>{html.escape(repr(self))}</code>"


Can't we just raise an AttibuteError here, shouldn't it be handled by sphinx gallery etc?

thomasjpfan · 2020-04-30T18:11:57Z

We have some raw output in the gallery, for example on master.

rth · 2020-04-30T18:18:55Z

~~I see but the <code> </code> is specific to sphinx right?~~ I see it's not.

Can't we detect that we are running inside sphinx via some env variable and otherwise raise AttributeError. I'm just not sure the priority between repr_html and mimetypebundle is correctly implemented in all environements.

rth

Nevermind, I think it should be fine. Thanks @thomasjpfan !

rth · 2020-04-30T18:26:18Z

Actually this changes the default repr in jupyter,

so I think we should raise an attribute error outside of sphinx, instead of trying to reproduce the behaviour without repr_html manually.

…inx_gallery

thomasjpfan · 2020-04-30T21:23:39Z

I wanted to return an AttributeError but this happens:

The only way around this is to return None, but it was considered brittle. But in this case we have _repr_mimebundle_, which gets priority, it should be okay.

thomasjpfan · 2020-04-30T21:30:43Z

Updated PR to return None, this should be okay since the _repr_mimebundle_ takes priority in jupyter contexts.

rth · 2020-04-30T21:45:20Z

I wanted to return an AttributeError but this happens:

Cant we do the same hack as in SVC predict_proba of a property that either returns a function or raises AttributeError, that way hasattr(obj, '_repr_html_') would behave consistently.

thomasjpfan · 2020-04-30T21:55:52Z

Cant we do the same hack as in SVC predict_proba of a property that either returns a function or raises AttributeError, that way hasattr(obj, 'repr_html') would behave consistently.

Yes, we can do it here as well. PR updated with the hack.

rth

LGTM, thanks !

thomasjpfan · 2020-05-02T03:18:45Z

Looks like this is needed even in jupyter lab, because _repr_mimebundle_ is not taking priority.

ogrisel

A few suggestions but other than that +1 on my side as well.

ogrisel · 2020-05-03T15:37:09Z

sklearn/base.py

+        """HTML representation of estimator.
+
+        Used to display the HTML representation in sphinx-gallery.
+        Jupyer kernels will use `_repr_mimebundle_`.


There is a typo:

Suggested change

Jupyer kernels will use `_repr_mimebundle_`.

Jupyter kernels will use `_repr_mimebundle_`.

and apparently this is wrong based on your last comment. I am not sure if _repr_mimebundle_ will ever take the priority. Maybe we can just state:

This is redundant with the logic of _repr_mimebundle_. The latter should be favored in the long term, _repr_html_ is only implemented for consumers who do not interpret _repr_mimebundle_.

ogrisel · 2020-05-03T15:39:46Z

sklearn/base.py


        return out

+    @property


Maybe add a comment to explain that @property is necessary to make hasattr(estimator, "_repr_html_") return True or False depending on get_config()["display"] .

ogrisel · 2020-05-03T15:41:54Z

sklearn/base.py

+            raise AttributeError("_repr_html_ is only defined when the "
+                                 "'display' configuration option is set to "
+                                 "'diagram'")
+        return self.__repr_html_


Is the use of leading dunder intentional? I find the behavior of private name mangling potentially confusing. Maybe self._repr_html_inner would be more explicit?

…inx_gallery

adrinjalali

Thanks @thomasjpfan

adrinjalali · 2020-05-05T07:57:59Z

Seems like all other comments are addressed as well, merging.

* ENH Adjusts html_repr based on configuration * CLN Returns None instead * CLN Uses property hack * CLN Address comments

ENH Adjusts html_repr based on configuration

3bd7dac

rth reviewed Apr 30, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into use_code_html_sph…

0aaa429

…inx_gallery

CLN Returns None instead

33901e5

CLN Uses property hack

e4a7bba

rth approved these changes May 1, 2020

View reviewed changes

thomasjpfan added this to the 0.23 milestone May 2, 2020

thomasjpfan added the Blocker label May 2, 2020

ogrisel approved these changes May 3, 2020

View reviewed changes

thomasjpfan added 2 commits May 4, 2020 11:41

Merge remote-tracking branch 'upstream/master' into use_code_html_sph…

4300f1f

…inx_gallery

CLN Address comments

5dd3dd5

thomasjpfan mentioned this pull request May 4, 2020

Add more PRs to 0.23.X #17120

Merged

adrinjalali approved these changes May 5, 2020

View reviewed changes

adrinjalali merged commit f23b940 into scikit-learn:master May 5, 2020

adrinjalali pushed a commit that referenced this pull request May 5, 2020

FIX Adjusts html_repr based on configuration (#17093)

9c0e62d

* ENH Adjusts html_repr based on configuration * CLN Returns None instead * CLN Uses property hack * CLN Address comments

gio8tisu pushed a commit to gio8tisu/scikit-learn that referenced this pull request May 15, 2020

FIX Adjusts html_repr based on configuration (scikit-learn#17093)

3200d0d

* ENH Adjusts html_repr based on configuration * CLN Returns None instead * CLN Uses property hack * CLN Address comments

	Jupyer kernels will use `_repr_mimebundle_`.
	Jupyter kernels will use `_repr_mimebundle_`.

Uh oh!

Conversation

thomasjpfan commented Apr 30, 2020

Uh oh!

thomasjpfan commented Apr 30, 2020

Uh oh!

NicolasHug commented Apr 30, 2020

Uh oh!

rth Apr 30, 2020

Choose a reason for hiding this comment

Uh oh!

thomasjpfan commented Apr 30, 2020

Uh oh!

rth commented Apr 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rth left a comment

Choose a reason for hiding this comment

Uh oh!

rth commented Apr 30, 2020

Uh oh!

thomasjpfan commented Apr 30, 2020

Uh oh!

thomasjpfan commented Apr 30, 2020

Uh oh!

rth commented Apr 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thomasjpfan commented Apr 30, 2020

Uh oh!

rth left a comment

Choose a reason for hiding this comment

Uh oh!

thomasjpfan commented May 2, 2020

Uh oh!

ogrisel left a comment

Choose a reason for hiding this comment

Uh oh!

ogrisel May 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogrisel May 3, 2020

Choose a reason for hiding this comment

Uh oh!

ogrisel May 3, 2020

Choose a reason for hiding this comment

Uh oh!

adrinjalali left a comment

Choose a reason for hiding this comment

Uh oh!

adrinjalali commented May 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rth commented Apr 30, 2020 •

edited

Loading

rth commented Apr 30, 2020 •

edited

Loading

ogrisel May 3, 2020 •

edited

Loading