-
-
Notifications
You must be signed in to change notification settings - Fork 74
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added HTML repr for Parameterized #425
Conversation
It's still WIP, with various open issues:
|
Actually, should I be focusing on making an HTML repr for |
Yes, I like this, it's also less discoverable but on balance I think this is nice. Alternatively just do both and then if the Parameterized repr is overridden you can still view it via |
Sounds good; always have obj.param's repr available, but also by default have it as the repr for the object itself. For that to work, any idea how to avoid Param's repr hiding the HoloViews repr (which it does in the current implementation)? |
Not sure why but this no longer appears to be an issue. |
A way to see the doc string would be very valuable. For example for interactive docs and cheat sheets |
This looks great, thanks! Any answers for the checklist items above? On a quick scan they nearly all still appear to be valid issues. |
@jbednar @philippjfr do you think this is going to be ready for 2.0? |
I think it's a huge usability improvement so I'd like to push this over the finish line. |
@@ -3538,6 +3642,10 @@ def _state_pop(self): | |||
elif hasattr(g,'state_pop') and isinstance(g,Parameterized): | |||
g.state_pop() | |||
|
|||
@bothmethod |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I would expect and hope for rich output for both classes and instances. Maybe a missing @bothmethod
elsewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation of bothmethod
was turning the method into a functools.partial
object whose type is not MethodType
(not FunctionType
either). IPython checks the type of the repr function (e.g. _repr_html_
) on the object to be displayed:
This was failing with the current implementation, which I have replaced in 618b88e to satisfy IPython's inspection approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost 3 years in the making, great work @jbednar and @philippjfr ! |
@philippjfr , the intention was to require |
Didn't you agree there the rich display should be used in both cases? |
Yep, what @maximlt said. |
Ah! I know what happened with your HoloViews examples @jbednar in #425 (comment). So when you forget to load the extension it will fallback to displaying the rich representation. Same for Panel objects, except that an informative warning is displayed: Presumably HoloViews should also display the same warning? |
Why not? |
That said, I do think if we're displaying the class it should include the module like a regular |
I would expect it to show me the |
Personally don't feel super strongly about it but in the discussion above we did agree to implement the repr for classes, instances and for the .param accessor. |
I would say this is an unexpected side effect of the discussion made above. |
How so? |
I understand that from a technical perspective, it makes sense why it appears. But for me, as a user, I do not see I would expect When calling |
I have mixed feelings about this.
Isn't that conflating a little the help with the repr? If we really want the rich repr to be used as an alternative help, shouldn't we go one step further and make it more look like the current help? E.g. including the class docstring (e.g. Panel objs include a link to the docs which is very useful), making the Parameters' doc first-class citizens. Oh and we all know that the (rich) repr isn't providing complete information about the type of objects you can provide to a constructor, as a class can implement some conversion in its Looking at the examples below, there are a few things that can be confusing:
|
Yes, but I think I was answering a different question than I was being asked. I thought the question was whether classes should provide an HTML repr as well as instances, and my answer is as stated (yes!). But there's a different question about whether it needs to be invoked with For comparison, Xarray does not: I agree with @maximlt that we are conflating the help with the repr. If it's feasible, I'd like the repr to be a representation that is much more compact and brief than the help, but conveying the same information when the user interacts with it by hovering or by expanding sections. Conversely, invoking help would give you a fuller version, already expanded and more easily scannable by eye. In any case, yes, it's odd not to include the docstring. |
Given the doubts above, how about we remove Simon also just found out that VS Code:
We'll have to disentangle that but it's another argument for removing |
Okay, I think apart from the overall merits it's clear that this has too much scope to disrupt things so I vote removing it too. |
Parameterized's repr works well for small numbers of parameters, but as it's all on a single line of text, quickly becomes not very useful for larger classes:
This PR adds an HTML representation that is similar to the parameter listing in the help output:
but as a table listing only the parameters: