-
-
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
Expanded range info shown in HTML repr #821
Conversation
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 are some nice improvements in there!
Looking at more examples, I've found that the range for a List
parameter is a bit confusing, displaying bounds
that for a List
defines a range of number of elements the list can contain, ignoring List.item_type
which is probably more used.
That made me think that:
- in the Range column it'd be nice to display all/most of the constraints that are applicable to the Parameter, except the type as it can be inferred from the Parameter Type. If so, Range could be renamed Constraints?
- especially if we go for 1. above, there should be a mechanism for Parameters to declare what they want to display in that column. This will be useful for Parameters created outside of Param (Panel has
Aspect
/Margin
which now have no information in the Range column.
I considered not showing a range for constant and read-only, since at least after instantiation users cannot set those, but on balance I think showing a range can still help people reason about how to use that parameter (e.g. a read-only parameter whose range is 1 to 10 can be treated differently from one with unbounded ranges.)
I would be in favor of not showing the range/constraints for readonly. Understanding the difference between readonly and constant is pretty difficult, I think this could help a little. Also I find that having readonly/constant in the Range column reads like they affect the range which is misleading, renaming the column name to Constraints or moving readonly/constant to the Type column would help I think.
I don't consider any of these changes to be a blocker for Param 2.0, I expect the HTML repr to change quite a lot when we are going to start using it to build the API/reference docs of Panel/HoloViews.
param/parameterized.py
Outdated
else: | ||
range_ = p.class_.__name__ | ||
elif hasattr(p, 'regex'): | ||
range_ = '.*' if p.regex is None else str(p.regex) |
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.
I don't think it's super frequent to define a regex for a String
Parameter and wouldn't expect all Param users to be knowledgeable about regular expressions. I'd suggest either not including '.*'
or being more explicit with e.g. regex(.*)
.
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 reason to include it is only that when allow_None
is True
and there's no regex
, the range is shown as | None
, which I think is confusing, given that I read |
as "or", so it's "???? or None" (i.e., "what or None??"). Can you think of a better way to convey "any string" than .*
? ''
would be accurate since the empty string is a regex that matches every string, and then it would show '' | None
, if that would look better. Or, sure, regex(.*)
, if you think that's clearer.
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.
Hmm, I agree that most options here aren't very good. Not sure I love it but if there's no regex I'd also be okay with str | None
if allow_None
or str
otherwise.
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.
Actually str | None
for no regex + allow_None and nothing at all for no regex and allow_None=False
is my preference.
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.
(That's the same reason I went with (-Inf,Inf) for numbers; | None
looked odd without it. If anyone has a solution that conveys "any valid input for that type, or None" more clearly, happy to use that instead!)
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.
Hmm; looks like @philippjfr 's notes weren't updated on my screen until after I made the above note. I don't mind "str", but I'd like us to use the same approach for number parameters as for string, since it's precisely the same concept: any allowed string, or any allowed number. Instead of (-Inf, Inf), would a number be "num" in this approach, as in "num | None"?
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.
Actually, None is a different type, so I suppose we really ought to be putting it into the Type column! I.e. the type isn't really Integer
, it's Integer | None
! I think that will eliminate all the awkwardness on the range field from | None
, so I'll change it to do that unless someone strongly objects. Seems like the obvious next step from Philipp's proposal.
Assuming we do that, then there is still a question whether to combine the type and range columns. Combining them makes it clear that the constraints apply to the non-None type, not None: Integer | None
, Integer [0, Inf) | None
, List[Integer] len(0,10) | None
.
I can't quite decide which I prefer; any votes?
Good point. Maybe we should use Python 3 type hint notation and put the type into the "Type" column? I.e. List[float]? Same for Dict and similar container Parameters?
Yes, that's what I was going for, to express as much of the constraints on the allowed values as I could. I was thinking of "range" in terms of mathematical functions, but I guess it's more "domain" in that sense, because it's a set of constraints on an input parameter. Not sure "Domain" will convey much to people, though. "Constraints" sounds pretty generic. "Allowed values"? Would be nice to have a single word. "Allowed"? Or, sure, "Constraints".
Yes, I had thought of that, but didn't want to bite off too much this close to release. I think we should add an extension mechanism like that, but it doesn't have to be right away.
Fine by me. @philippjfr or others, any vote? The questions are:
My votes for showing range info in the cases are 1 (weak Yes), 2 (weak Yes), 3 (strong Yes). I think Maxime is arguing 1 (No), 2 (Yes), 3 (Yes).
If we go with No for 1 above, then "readonly" would be the only item in the range, which seems fine. But that doesn't solve "constant", and I agree it's a bit confusing to put that info into the range, but I don't want a separate column just for that, because it's rare for all parameters but
Agreed. That said, these were what I considered the "easy" or non-controversial improvements (apart from including the docstring, which I am happy to back out), and I wanted to get them locked in before working on #823. #823 is more important, because it strongly affects not just the details of what's in the HTML, but when it appears and how we use it. So this PR is meant to keep these more minor questions separated from #823. It's ok to punt on this one if it's turning out to be time consuming to review. |
I went back and forth reviewing this and trying to reply to your last comments @jbednar. I first wondered whether we should actually display that range in the repr and if we were not conflating the repr with the help too much. Because Parameter attributes are actually writable, and they're of course pretty useful, I figured out having them in the repr is alright.
That works okay for the
Here's how that could look like, the first row being the pattern to follow, followed by a couple of examples:
The work needed to make a Parameter to type hints has been started by Philipp in #677. Because the data displayed can be quite long, it should be truncated above a given length, the full version displayed in a tooltip. I changed my mind about |
I'd like to move this forward; it's time to ship! I agree it would be nice to use Python 3 type info when appropriate, particularly if we can start supporting that in Param itself, which we should. But that seems daunting, and I don't want to hold this up just to get to that. In any case, I agree with @maximlt that I've been conflating the So for the repr, what if we transposed it and just show key:value, to keep it compact? |
I have to say I'm not much enthusiastic about the last proposal, the table being transposed will make it challenging to nicely render Parameterized objects equipped with lots of Parameters. It's also just a slightly fancier version of a dict repr. No really I'm afraid I don't see much benefit of going down that route. I largely prefer the previous version (current on this branch): To which I'd suggest the following changes:
I would like to preserve We've already decided that for now the HTML repr would only be available on Trying not to conflate the repr and help has been tricky indeed. I assume we were driven by the idea of having a pretty HTML display of a Parameterized object we can embed into our docs. Together with @droumis we recently chatted about the API/Component pages of Panel docs and how to improve them. The pages of the Component gallery have been manually written, which even if it's a pain to maintain, has the advantage of being more user-friendly than a pure API reference. Take for instance the gallery page of the
Given our requirements, it is pretty clear to me that the default HTML repr will not be a good fit. The approach we'd take either would be:
|
I strongly vote for the latter, such that our Sphinx setup can provide whatever customization that we need, but that our docs and what is available to the user at the CLI or notebook cell level (presumably through help(), assuming that can be made HTML) are at least roughly the same, so that throughout the docs we pull data directly from the objects, and simultaneously demonstrate to the user how to pull data directly from the objects. I.e. the docs should have the comprehensive info (filtered as we wish) and should consistently show the users how to get that info from whatever object they have. That way when they read our docs, the info is real and complete and up to date, while people walk away from the docs knowing how they can get that same type and level of info when they are working interactively. |
Seems like nullable matches the formatting of readonly and constant better, and is an adjective rather than a verb. A verb makes sense when defining a Parameter, but less so in this context. Anyone have a preference between nullable and allow_None? |
Is it helpful to show |
I have no preference between nullable and allow_None. Indeed the default regex and (-inf, inf) are not useful. |
Ok, last question before merging! In a289244 I switched from mathematical range notation like "[0,2)" to something closer to the Python expression you'd use to test it, like ">=0, <2". The math version is prettier but the code version is probably clearer for programmers, plus for the most common bounds case (non-negative values) it seems highly readable: "nullable >=0" or just ">=0"). I don't have a strong preference, so I'm happy for @maximlt or @philippjfr to merge it as is or to revert a289244 and then merge. Go for it! |
Thanks @jbednar this looks good to me! |
This PR greatly expands the HTML repr's display of the range accepted by each Parameter:
Number
bounds
andSelector
objects
are specifications for the range of that Parameter.class_
under "Range", since again the class_ defines the range of values accepted by this Parameter.regex
that determines which strings that Parameter accepts.[
and]
for inclusive and(
and)
for exclusive as is typical in mathematical interval notation, with an HTML infinity character to indicate no numeric bound. Generally shorter than before, but now shows more information.mode
column information is also now folded into the Range column, because it is generally about the Range. allow_None is shown asNone |
to indicate an alternative, and constant and read-only are shown as modifiers to the range. I considered not showing a range for constant and read-only, since at least after instantiation users cannot set those, but on balance I think showing a range can still help people reason about how to use that parameter (e.g. a read-only parameter whose range is 1 to 10 can be treated differently from one with unbounded ranges.)Here's the new behavior: