Skip to content
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

Add String pane to support any representable object #21

Merged
merged 6 commits into from Sep 5, 2018
Merged

Conversation

jbednar
Copy link
Member

@jbednar jbednar commented Sep 4, 2018

@jlstevens suggested we add support for a string-based representation for any Python object as a Pane. About 20 minutes after his suggestion, I already realized that I needed that, so here it is.

Adding the Repr class is the only change; the other diffs are just to reorganize the other classes to be in a more readable order.

Questions:

  • What range are the priority values meant to have? I chose 10 for this one to be higher than HTML's current value of 1 and as an indication that this class is really a last resort, but we could instead keep them all in [0.0,1.0] as is typical for Parameter precedences. Up to @philippjfr.
  • Should we have an (optional?) character limit on the representation, to be useful for objects whose representation might be many pages of text?
  • Should we escape the values in some way, or let HTML be interpreted as HTML if there is any HTML in the representation?

@codecov-io
Copy link

codecov-io commented Sep 4, 2018

Codecov Report

Merging #21 into master will increase coverage by 0.14%.
The diff coverage is 64.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #21      +/-   ##
==========================================
+ Coverage   92.64%   92.78%   +0.14%     
==========================================
  Files          19       19              
  Lines        2230     2441     +211     
==========================================
+ Hits         2066     2265     +199     
- Misses        164      176      +12
Impacted Files Coverage Δ
panel/param.py 90.85% <100%> (ø) ⬆️
panel/plotly.py 100% <100%> (ø) ⬆️
panel/pane.py 83.86% <62.5%> (+1.55%) ⬆️
panel/tests/test_layout.py 100% <0%> (ø) ⬆️
panel/tests/test_panes.py 98.38% <0%> (+0.44%) ⬆️
panel/util.py 78.09% <0%> (+0.95%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e07b100...f5bb6d2. Read the comment docs.

@philippjfr
Copy link
Member

philippjfr commented Sep 4, 2018

What range are the priority values meant to have? I chose 10 for this one to be higher than HTML's current value of 1 and as an indication that this class is really a last resort, but we could instead keep them all in [0.0,1.0] as is typical for Parameter precedences.

I have no opinion at all here, so far I've only used 0 and 1. I also meant to invert precedences such that a higher value actually gives higher precedence but it seems like leaving it might be more intuitive? I'll leave that up to you.

Should we have an (optional?) character limit on the representation, to be useful for objects whose representation might be many pages of text?

As long as you give the bokeh model a fixed width it should wrap.

Should we escape the values in some way, or let HTML be interpreted as HTML if there is any HTML in the representation?

I can't think of a case in which a standard repr would ever contain HTML (unless it's one of the objects parameter values in which case I think it should be escaped).

@jbednar
Copy link
Member Author

jbednar commented Sep 4, 2018

After trying it out with a few things, it seems to me that calling str() is more useful than calling repr(). repr() more often gives a representation that can be evaluated, but here it's for human consumption, and I think the result of str() is more often what we'd want. E.g. the repr() of 'somestring' is 'somestring' (with quote marks), but people probably just want to see somestring. Plus this way people can provide arbitrary HTML, as a string (bypassing what Philipp is discussing for the repr above).

Here's an example of using it twice:

from time import sleep
import param, panel as pp
pp.extension()

defaults = dict(a=0.2, b=0.4, c=0.6, d=0.8)

class C(param.Parameterized):
    a = param.Magnitude(defaults["a"])
    b = param.Magnitude(defaults["b"])
    c = param.Magnitude(defaults["c"])
    d = param.Magnitude(defaults["d"])
    
    reset_ = param.Action(lambda x: x.reset())
    
    @param.depends('a', 'b', 'c', 'd')
    def view(self):
        sleep(1)
        return "<H2>"+str((self.a, self.b, self.c, self.d))+"</H2>"

    def reset(self):
        for k,v in defaults.items():
            setattr(self,k,v)

c=C(name="")
pp.Row(c,pp.Column("<H1>Current&nbsp;values:</H2>", c.view))

image

The return value from the view() method is just a string here, as is the "Current values" heading. I'm not sure why the tuple gets split across lines like that; presumably some container needs to be told it's not zero in width?

@jbednar
Copy link
Member Author

jbednar commented Sep 4, 2018

As long as you give the bokeh model a fixed width it should wrap.

Even if it wraps, it seems like it could go way off the page, but I guess we can worry about that if we encounter it.

@jbednar jbednar changed the title Add Repr pane to support any representable object Add String pane to support any representable object Sep 4, 2018
@jbednar
Copy link
Member Author

jbednar commented Sep 4, 2018

I have no opinion at all here, so far I've only used 0 and 1. I also meant to invert precedences such that a higher value actually gives higher precedence but it seems like leaving it might be more intuitive? I'll leave that up to you.

Given that within Panel itself, the main use for precedence is to ensure that certain classes (HTML and String in this case) are not used when there is some other more specific one available, I think inverting the precedence makes sense -- 0 is a nice special value at the low end, but there's no equivalent at the high end. So I'd invert it and use maybe 0.5 as the default precedence, then 0.1 for String, 0.2 for HTML, and leave the other numbers for user stuff. Just my guess...

@philippjfr
Copy link
Member

Now I'm confused, you said you were okay with inverted precedence but then gave String a lower precedence value (i.e. a higher precedence) than the default?

@jbednar
Copy link
Member Author

jbednar commented Sep 4, 2018

We're probably all confused, then! I haven't messed with the code for comparing precedences, which is however you had it. But what I think it should be made to do is for String to be the least preferred (as it always applies), HTML to be the second-least preferred (as it often applies) , and the rest to be the default level of preference.

@philippjfr
Copy link
Member

philippjfr commented Sep 4, 2018

Currently lower precedence values are preferred, so the values right now are:

0 - Bokeh, Matplotlib, HoloViews, Plotly, GGPlot etc.
1 - Param
10 - String

So let's take a holoviews object, three panes could apply: HoloViews, Param and String panes. The HoloViews pane wins out because it has the lowest precedence value and therefore the highest precedence. That is confusing and I'm happy to invert precedences such that the higher value wins, but I'm happy to use whatever scheme and values you suggest.

@jbednar
Copy link
Member Author

jbednar commented Sep 4, 2018

Ah, Param's precedence is set outside of pane.py, so I didn't notice that one, and HTML currently also has a precedence of 1, which will surely cause problems for any Parameterized object that has a _repr_html_. So we've got to do something about that! I don't know whether putting up widgets or showing the HTML representation should be preferred; hard to reason about that!

@philippjfr
Copy link
Member

I don't know whether putting up widgets or showing the HTML representation should be preferred; hard to reason about that!

I strongly feel showing widgets should be preferred, a String representation of an object should be absolutely the last fallback in a dashboard context.

@philippjfr
Copy link
Member

I should read more thoroughly, you were talking about a parameterized object with a _repr_html in which case I'm also not sure. That said you can always be explicit about it yourself, and I'm not aware of any parameterized object with a _repr_html unless you have one in datashader.

@jbednar
Copy link
Member Author

jbednar commented Sep 4, 2018

I do not currently have any Parameterized object in Datashader that has a _repr_html_, but there easily could have been. E.g. a Datashader Image has such a repr, but it's not Parameterized because it inherits from an xarray DataArray. A different implementation could easily have resulted in it being Parameterized.

In general I can imagine many objects that one might want to add representations for, where the parameters are for configuration rather than for representing them. Given the choice between representing the object and configuring the object, I think we should choose to represent the object itself wherever a meaningful representation has been defined (including _repr_png_ or _repr_html_), and only fall back to showing the widgets if there is no such representation. That means that people should probably do panel.Param(obj) if they always want widgets, to be explicit about it. So I vote for making panel.Param be lower precedence than the various graphical representations, though higher than String.

@jlstevens
Copy link
Contributor

I think a limit on the maximum size of the string is a good idea even if bokeh can wrap long strings. I also agree that __str__ make more sense to use here than __repr__.

@jlstevens
Copy link
Contributor

jlstevens commented Sep 4, 2018

I'm not quite sure of this in String:

HTML markup will be interpreted, so this type of pane is also useful for arbitrary HTML code provided as a Python string.

I would prefer to use HTML for HTML strings (or objects with _repr_html_) and String just to call __str__ or use str (and not assume it is HTML).

@jbednar
Copy link
Member Author

jbednar commented Sep 4, 2018

I also agree that __str__ make more sense to use here than __repr__.

Note that it's not using __str__ directly, just str(), because numbers don't have a __str__ method but do work with str().

@jbednar
Copy link
Member Author

jbednar commented Sep 4, 2018

I would prefer to use HTML for HTML strings (or objects with _repr_html_) and String just to call __str__ (and not assume it is HTML).

String just puts whatever you give it into a Bokeh Div, so it inherently supports HTML. We could maybe somehow wrap it as raw text to avoid having the HTML markup interpreted (wrap in "pre" tags, maybe?), but I'm not sure if that buys us anything. I guess it would let us show <HTML> in a String pane.

Meanwhile, pane.HTML currently has a guard that makes it be applied only for objects with a _repr_html_ method, which a string of HTML code does not. That's why I added that note to the String docstring -- if someone does Panel(s) where s='<b>Some text</b>, it will be caught by pane.String, not pane.HTML. In general I don't think we can distinguish between text that is HTML and text that is not. So it's up to us -- do we want a string value to be interpreted as HTML by default, or raw strings of characters by default? It's up to us, I think, but we have to choose one.

@jlstevens
Copy link
Contributor

jlstevens commented Sep 4, 2018

Meanwhile, HTML currently has a guard that makes it be applied only for objects with a repr_html method, which a string of HTML does not.

I think this should be relaxed to allow HTML strings and String should be for strings that we should not assume are HTML.

@jbednar
Copy link
Member Author

jbednar commented Sep 4, 2018

I think this should be relaxed to allow HTML strings.

How would we detect that a string was an HTML string?

@jlstevens
Copy link
Contributor

jlstevens commented Sep 4, 2018

How would we detect that a string was an HTML string?

If you explicitly use HTML with a string, you are declaring that the string is HTML. If you just supply a literal string to panel and let it decide, then I think it is reasonable for it to assume it is HTML. What I'm objecting to is the class called String taking arbitrary strings and interpreting them as HTML: in my opinion, those strings should be escaped.

@jbednar
Copy link
Member Author

jbednar commented Sep 4, 2018

If you explicitly use HTML with a string, you are declaring that the string is HTML. If you just supply a string to panel, I think it is reasonable for it to assume it is HTML. What I'm objecting to is the class called String taking arbitrary strings and interpreting them as HTML: in my opinion, those strings should be escaped.

Ok, it sounds like you are proposing that the current pane.HTML class should be changed so that it accepts both strings and things that have _repr_html_, and then either calls that method (if it exists) or just uses the given string as HTML.

It would then be confusing that pane.String would never be chosen automatically for a string, but perhaps it could be renamed pane.Str, and what it would mean would be that it calls str() on the object, as a last resort. Strings would then never actually reach this object unless it is instantiated with them explicitly, because raw strings will be assumed to be HTML and will be caught by pane.HTML. That seems reasonable to me, if that's what you mean.

@jlstevens
Copy link
Contributor

jlstevens commented Sep 4, 2018

Yes, that is what I meant although I have no opinion about renaming to pane.Str or not (though it does make some sense)...

@jbednar
Copy link
Member Author

jbednar commented Sep 5, 2018

Ok, I think I've implemented what was discussed above:

  1. Flipped precedences so that higher numerical values are preferred.
  2. Renamed pane.String to pane.Str.
  3. Made pane.Str escape its object to avoid interpreting HTML markup.
  4. Made pane.HTML accept strings.
  5. Made pane.HTML have higher precedence than pane.Str so that bare strings will be assumed to be HTML

Here's an example of the results:

import param, panel as pp, numpy as np
from panel import pane
pp.extension()

pp.Row("<i>string</i>",
          pane.Pane("<b>Pane</b>"), 
          pane.HTML("<u>HTML</u>"), 
          pane.Str( "<i>Str</i>"),
          pane.Str(np.zeros((5,3)), height=80))

image

As you can see, I decided that it was best to wrap the Str output in <pre>...</pre> tags. The str() representation of Python objects often assumes that it is preformatted, and so if you don't, you get output like:
image
With <pre> tags arrays are formatted reasonably:

image

This all works fine, but there are a couple of issues:

  • I think it's appropriate for a bare string to be interpreted as HTML, but in practice it doesn't seem to work all that well, because it doesn't respect the size of what's rendered. E.g.:
    So in practice one has to explicitly wrap it as pane.HTML("string", height=50) to allow height or width specification, which is very annoying. Isn't there any way for the pane to adapt to the size of what it contains?
  • Even if the pane gets sized to the objects, as you can see with stringPane above we'll want some way to provide control over spacing between panes. Is there already a good way to do that?
  • I'm not sure how to write tests for these classes based on the existing tests for Panes; maybe @philippjfr can add some tests for pane.HTML and pane.Str?

@jlstevens
Copy link
Contributor

Code changes look good to me and I agree using the <pre> tags makes sense. The issue about the vertical height seems to me to be a general one that is not specific to HTML versus raw strings (or even anything to do with strings).

The issue about the horizontal spacing between the panes also seems quite general but maybe a string specific fix might be appropriate? You could pad the supplied string with spaces but maybe that is too hackish...

@jbednar
Copy link
Member Author

jbednar commented Sep 5, 2018

Right; the vertical height issue applies to both HTML and Str, and presumably any Bokeh Div containing just text. I can't tell if there would need to be one or two fixes for the horizontal and vertical issues, but I do know that adding spaces automatically seems hackish...

@philippjfr
Copy link
Member

Isn't there any way for the pane to adapt to the size of what it contains?

This is a problem with bokeh Divs, which I haven't been able to solve. Without providing an explicit size they won't adapt automatically.

@jbednar
Copy link
Member Author

jbednar commented Sep 5, 2018

I organized the list in #2 by "richness", and to make what it says there true (that richer plots are preferred), I've now made interactive plots have a higher precedence than the rest. That way if e.g. HoloViews or any of the other interactive libraries eventually added a _repr_png_ method, the interactive version would still be preferred over converting it to a PNG. @philippjfr, the altair pane would need to have "precedence = 0.8" added to it for this to work properly. I think this PR is mergeable either immediately or after @philippjfr can add any suitable tests.

@philippjfr
Copy link
Member

Even if the pane gets sized to the objects, as you can see with stringPane above we'll want some way to provide control over spacing between panes. Is there already a good way to do that?

You can add spacers, but we could also consider adding some padding to Str/HTML panes.

@philippjfr
Copy link
Member

I've added tests and will merge.

@philippjfr philippjfr merged commit 655b902 into master Sep 5, 2018
@philippjfr philippjfr deleted the repr branch October 17, 2018 02:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants