-
-
Notifications
You must be signed in to change notification settings - Fork 402
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
Implement support for setting custom options on plot components #4463
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.
This looks great! I'm wondering how this can/should interact with the options system. Would there be a useful way for someone to "register" such an option without necessarily setting it yet, so that the option can be set using the usual options system? This might provide an easy way for options to be added over time. E.g. hv.declare_option(hv.Points, 'colorbar_title_color', 'colorbar.title_text_color')
, then hv.Points(...).opts(colorbar_title_color="red")
?
I like that idea a lot. I did consider whether if you fully qualify it could just support it by default, i.e. |
True. Well, we could extract the valid options at least from BokehJS, right? Perhaps at build time? So in that case we could still support early validation? |
For the Plotly backend, it's not as clear cut what to use as the root component(s) for the dot notation. For the These are things I'd like to clean up eventually, but it runs directly into the larger tension of whether to try to unify style option names across backends. For someone used to the plotly API, it would be really nice if all of the If we went this direction and were willing to change the plotly style names in a HoloViews 2.0 to be more consistent with the plotly schema, then I think this proposal could work for the Plotly backend as well. The alternative is the keep style options as-is and work towards unifying them across the plotting backends. This is the case where I'd especially want #4638 as a fallback. |
Note there isn't necessarily any one root element, the dot notation starts by inspecting the |
Right; it sounds like this could be just what I was envisioning in #4638 , i.e., a concise way of specifying options on some hierarchy of objects, for whatever hierarchy is available on a given plotting backend, which is particularly valuable if it also has a way to map those hierarchical specifications into flat option names. |
Oh, ok. So for the plotly self.handles["figure"] = fig
self.handles["layout"] = fig["layout"]
self.handles["trace"] = fig["data"][0]
self.handles["marker"] = fig["data"][0]["marker"]
self.handles["xaxis"] = fig["layout"]["xaxis"]
self.handles["yaxis"] = fig["layout"]["yaxis"] So this way, the custom property string could start with scatter.opts(trace_visible=True, marker_color="green", layout_dragmode="pan", xaxis_tickfont_family="courier") I'd be happy with this from the plotly side. These anchor points could be documented in I'm wondering if we could work toward the following split:
To me, this would be a lot less confusing than having the style options consist of a mix of options that are passed through with matching names in the backend, and options that are have special handling by HoloViews and/or names that are designed to be consistent across backends. |
Sounds like a good plan, though I don't know what backwards compatibility issues that would raise. |
I'm not sure how I feel about this as a plot option given that there is no obvious way to generalize this idea to plotly and matplotlib. The name should also be something like At first glance, I don't like the idea of something like Mapping the You also have the possibility of clashes e.g if |
That's exactly what we're discussing above. There absolutely are ways of generalizing this to both plotly and matplotlib but you're right it's not totally straightforward.
I'm also wary about this, doing this automatically and exploding the option space in this way will definitely make the tab-completions less useful for instance. |
I can't remember exactly how the tab completion works but I think we could allow these keywords without suggesting them. This makes the proposal for automatic expansion more palatable to me (modulo the possibility of namespace clashes). This would need some way of declaring a list of style options that shouldn't be tab completed. |
That was always the idea of plot options versus style options but unfortunately the boundary isn't always as hard as we would like. In some cases plot options simply can't be easily made consistent across backends and sometimes style options need some special handling before they are passed through to the plotting library. What you suggest has always been our goal but being totally strict about it for the ambiguous cases would be more confusing than helpful imho... |
In practical terms style options have always been things that control the visual styling of a particular glyph/artist/trace, while plot options have applied to everything else axes/colorbars/legends/titles etc. However it is also true that for plot option we've generally tried to maintain compatibility between backends (as much as possible), so this definitely kind of straddles the line of the two since most of these options would be about axes/colorbars/legends etc. but they are also definitely backend specific. |
I guess my core difficulty is that I just don't grok the difference here. I'd argue that the marker color/size encoding is more related to the core nature of the plot than current plot options like the padding around the colorbar. The distinction of whether options are uniform across backends is a very practical distinction. |
I haven't previously been pushing for automatically registering these extra options, just to make it easy for a user to register some. But thinking about it in terms of root elements for each backend, I wonder if there is a good middle ground: register a finite set of root elements for each backend, and then accept options automatically and without validation, but only for those root elements. I.e., validate up to the first underscore, and leave the rest to the backend. That way we wouldn't lose validation in general, while also supporting the full set of options available in each backend. Of course, if we could query the backend for the actual available options, as is possible for Bokeh and may also be possible for other backends, then we could provide validation and tab completion even in that case as well. |
Discussing this with @jbednar , @philippjfr and @jonmmease I think I'm fine with the PR as is, if it can be generalized to matplotlib and plotly as well with I suggest Having the option for users to add such options to the allowed keywords seems fragile to me as that code would be having a side effect potentially far from where the keyword is used (e.g someone copies and pastes a cell at the bottom of a notebook using a keyword enabled at the top of the notebook). Doing such expansion automatically seems problematic due to namespace clashes and the reduction in utility of tab-completion. |
I would suggest that the option name include But other than that I'm happy with it as well and I could work on the Plotly backend portion. For the plotly backend, I think I could actually implement early validation if that was desirable. Wouldn't need to be this PR necessarily, but the remaining question for me is discoverability. Could we add a backend-specific section to
One last thing. Some plotly elements include lists, so it would be nice to support list indexing in the strings as well. e.g. {'layout.images[0].opacity': 0.5} |
Hmm. "backend_" makes it wordy without contributing much, especially since people always get confused about what constitutes a backend (given that most people think of Bokeh and Plotly as frontends, in that they are what the users interact with directly). What about the actual backend name as a prefix instead? I.e.
I guess attribute access covers dictionary-like access already, for the only reasonable case (string keys), and thus we don't need to consider supporting dictionaries too?
|
The names |
Also happy with |
Yes, that was the idea, I'd definitely like to avoid having strings in strings and the hassles that causes. |
Works for me! |
@jonmmease @jbednar @jlstevens should we aim to get this into the release? |
I wasn't picturing we would for this release, given release date goal. |
Up to you, @philippjfr ; if it's feasible to implement something you're happy with, I'd find a way to review it in time. |
|
||
# the logic handles resolving something like: | ||
# legend.get_texts()[0].set_fontsize | ||
if '[' in acc and acc.endswith(']'): |
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.
What if there are multiple getitems? Seems rare but possible in theory.
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.
Added support for "legend.get_texts()[3,4].fontsize": 388,
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.
How does it handle something like "legend.get_texts()[3][4].fontsize"
?
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.
Before adding even more complexity, can you think of a valid use case for that?
I don't think this is valid:
legend.get_texts()[1][2].fontsize
legend.get_texts() => ["a label", "b label"]
[1] => "a label"
[2] => "l"
l.set_fontsize() => raise AttributeError (because l is str
)
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.
It was definitely not a valid example. I was curious if a pattern like this will run successfully or if it will it error out (because of the multiple getitem
and not because it is not a valid example).
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.
It will likely fail
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.
Left a few comments. The parser does have a few limitations which are okay but maybe it's worth validating against them.
Co-authored-by: Philipp Rudiger <prudiger@anaconda.com>
Thanks for the review! I've added support for multiple indices, slices, and kwargs, but I couldn't immediately think of a practical example for kwargs (there probably is some use case somewhere). |
@ahuang11, Python 3.8 tests on Ubuntu tests are failing, can you take a look at it? So we can get this PR merged. |
Thinking about how to address this since I don't have a ubuntu machine. Wondering what version of matplotlib is being used.
|
Ir is available in the capture environment section in holoviz-dev/holoviz_tasks/install@v0.1a15 |
Thanks; seems like the matplotlib is a bit old...
|
Okay, it's fixed now! |
I think people are going to love this feature. Thank you, @ahuang11, for finishing this PR! |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
A long time ago we introduced hooks to allow users to apply customizations for options we haven't exposed on the plot classes themselves. However writing custom hooks is cumbersome and quite verbose. This PR implements a suggestions I've been mulling for a long time (I haven't been able to find the issue where I laid out this proposal). Basically it allows declaring a dictionary with accessor specification for setting custom options on the plot components, e.g. something like this:
Looks up the colorbar model on the plot and then sets the `title_text_color' to 'red'. This approach should work for arbitrarily nested objects. The user just needs some knowledge of the attributes of the underlying plotting library (which is also needed for hooks) but here we can at least in theory apply some fuzzy-matching to give good suggestions.