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

Implement support for setting custom options on plot components #4463

Merged
merged 15 commits into from
Jul 21, 2023

Conversation

philippjfr
Copy link
Member

@philippjfr philippjfr commented Jun 8, 2020

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:

{'colorbar.title_text_color': 'red'}

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.

  • Add tests
  • Implement for matplotlib and plotly backends
  • Implement some fuzzy matching to give helpful suggestions when the spec is slightly mis-specified.
  • Add documentation.

Copy link
Member

@jbednar jbednar left a 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")?

holoviews/plotting/bokeh/element.py Outdated Show resolved Hide resolved
holoviews/plotting/bokeh/element.py Outdated Show resolved Hide resolved
@philippjfr
Copy link
Member Author

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. colorbar.title_text_color could be set as colorbar_title_text_color but that would make early validation impossible I suppose.

@jbednar
Copy link
Member

jbednar commented Jun 10, 2020

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?

@jonmmease
Copy link
Collaborator

For the Plotly backend, it's not as clear cut what to use as the root component(s) for the dot notation. For the Scatter element for example, many of the style properties live under scatter.marker (like color) in the plotly schema, but some live directly under scatter (like selectedpoints). Other properties (like dragmode above) live under the figure.layout hierarchy.

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 Scatter style property names matched their path relative to the plotly scatter trace, with underscores separating nested levels. This would mean using marker_color instead of color. This wouldn't match bokeh and matplotlib, but it would make it much easier to document and validate. The hv.help output could directly reference where in the plotly documentation to look for descriptions of the options (e.g. https://plotly.com/python/reference/scatter/). And we could automate the lookup of the appropriate plotly.py validator if we wanted to delegate property validation to plotly.py.

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.

@philippjfr
Copy link
Member Author

Note there isn't necessarily any one root element, the dot notation starts by inspecting the .handles on the plot class where we can put anything and establish convenient conventions that way.

@jbednar
Copy link
Member

jbednar commented Sep 30, 2020

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.

@jonmmease
Copy link
Collaborator

jonmmease commented Oct 1, 2020

Note there isn't necessarily any one root element, the dot notation starts by inspecting the .handles on the plot class where we can put anything and establish convenient conventions that way.

Oh, ok. So for the plotly ScatterPlot we could add references to multiple anchor points in the same figure dictionary to handles.

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 figure, layout, trace, marker, xaxis, or yaxis.

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 hv.help making it pretty easy to cross reference the appropriate plotly documentation section.

I'm wondering if we could work toward the following split:

  • All properties with names that we want to be consistent across backends, and all properties that HoloViews does special handling for (e.g. dim expression support, colormap construction, etc.) are moved to plot options.
  • All style options have names that systematically match the corresponding backend (e.g. with underscores separating nested levels as above), and HoloViews passes them directly through to the backend without validation or conversions.

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.

@jbednar
Copy link
Member

jbednar commented Oct 1, 2020

Sounds like a good plan, though I don't know what backwards compatibility issues that would raise.

@jlstevens
Copy link
Contributor

jlstevens commented Oct 2, 2020

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 custom_setters imho which describes the idea more accurately imho.

At first glance, I don't like the idea of something like hv.declare_option(hv.Points, 'colorbar_title_color', 'colorbar.title_text_color') automatically (as this will hugely explode the number of options) but I might support it instead of custom_opts because then these would appear as style options rather than plot options which I think is more appropriate.

Mapping the . to an underscore also makes things ambiguous which isn't great: colorbar_title_color might have been derived from colorbar.title_color, colorbar_title.color, colorbar.title.color or colorbar_title_color. Unless the suggestion is to only allow up to one level of access this way? There would still be ambiguity so ideally there would be a way to know for sure what is due to attribute access (but I think this is impossible to do sensibly given valid Python identifier rules).

You also have the possibility of clashes e.g if foo_bar is a style option and there is also foo.bar.

@philippjfr
Copy link
Member Author

philippjfr commented Oct 2, 2020

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.

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.

At first glance, I don't like the idea of something like hv.declare_option(hv.Points, 'colorbar_title_color', 'colorbar.title_text_color') automatically (as this will hugely explode the number of options) but I might support it instead of custom_opts because then these would appear as style options rather than plot options which I think is more appropriate.

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.

@jlstevens
Copy link
Contributor

jlstevens commented Oct 2, 2020

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.

@jlstevens
Copy link
Contributor

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.

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...

@philippjfr
Copy link
Member Author

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.

@jonmmease
Copy link
Collaborator

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

I guess my core difficulty is that I just don't grok the difference here. kdims and vdims are annotations about the data and, to me, everything else is visual / styling. How to encode data by position (e.g. logx=True, invert_axes=True) isn't fundamentally different from encoding the same data by color or marker size.

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.

@jbednar
Copy link
Member

jbednar commented Oct 2, 2020

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.

@jlstevens
Copy link
Contributor

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 custom_opts renamed to something more suitable.

I suggest config as this style is very much like the matplotlib configs/rcfiles. This plot option would take precedence over other options (e.g opacity here would override alpha for plotly). It would then be a very direct way for users to make use of the API corresponding to the selected plotting backend.

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.

@jonmmease
Copy link
Collaborator

I would suggest that the option name include backend to make it more clear that these are options are specific to a particular backend. Maybe backend_opts or backend_config?

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 hv.help that documents what root elements are available for the and directs people to some external resource where they can find the available options? For example, hv.help on a Scatter element with the plotly backend would include something like:

root elements
 - figure (https://plotly.com/python/reference/)
 - trace (https://plotly.com/python/reference/scatter/)
 - marker (https://plotly.com/python/reference/scatter/#scatter-marker)
 - layout (https://plotly.com/python/reference/layout/)
 - xaxis (https://plotly.com/python/reference/layout/xaxis/)
 - yaxis (https://plotly.com/python/reference/layout/yaxis/)

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}

@jbednar
Copy link
Member

jbednar commented Oct 2, 2020

I would suggest that the option name include backend to make it more clear that these are options are specific to a particular backend. Maybe backend_opts or backend_config?

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. bokeh_config={'colorbar.title.color':'red'}, to directly state that these are bokeh configuration options (and similarly for "plotly_config" and "matplotlib_config" or "mpl_config")? bokeh_opts would be shorter but is confusing with the hv options system, so I'd avoid that. bokeh_settings seems clearer than config to me, but it's longer, so maybe not that. Maybe bokeh_cfg; not sure if those three characters would be worth it.

One last thing. Some plotly elements include lists, so it would be nice to support list indexing in the strings as well. e.g.

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?

{'layout["toolbar"].num_colors': 2}

@jlstevens
Copy link
Contributor

The names plotly_config, matplotlib_config and mpl_config are all fine by me if we don't want it to just be config.

@philippjfr
Copy link
Member Author

Also happy with bokeh_config, mpl_config and plotly_config.

@philippjfr
Copy link
Member Author

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?

Yes, that was the idea, I'd definitely like to avoid having strings in strings and the hassles that causes.

@jonmmease
Copy link
Collaborator

Also happy with bokeh_config, mpl_config and plotly_config

Works for me!

@philippjfr
Copy link
Member Author

@jonmmease @jbednar @jlstevens should we aim to get this into the release?

@jonmmease
Copy link
Collaborator

@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.

@jbednar
Copy link
Member

jbednar commented Nov 24, 2020

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(']'):
Copy link
Member Author

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.

Copy link
Collaborator

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,

Copy link
Member

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"?

Copy link
Collaborator

@ahuang11 ahuang11 Jul 13, 2023

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)

Copy link
Member

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).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will likely fail

Copy link
Member Author

@philippjfr philippjfr left a 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.

ahuang11 and others added 2 commits July 12, 2023 12:46
Co-authored-by: Philipp Rudiger <prudiger@anaconda.com>
@ahuang11
Copy link
Collaborator

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).

@hoxbro
Copy link
Member

hoxbro commented Jul 20, 2023

@ahuang11, Python 3.8 tests on Ubuntu tests are failing, can you take a look at it? So we can get this PR merged.

@ahuang11
Copy link
Collaborator

Thinking about how to address this since I don't have a ubuntu machine.

Wondering what version of matplotlib is being used.

FAILED holoviews/tests/plotting/matplotlib/test_elementplot.py::TestElementPlot::test_element_backend_opts - AssertionError: ['', ''] != ['A', 'B']
FAILED holoviews/tests/plotting/matplotlib/test_elementplot.py::TestElementPlot::test_element_backend_opts_alias - AssertionError: ['', ''] != ['A', 'B']

@hoxbro
Copy link
Member

hoxbro commented Jul 20, 2023

Ir is available in the capture environment section in holoviz-dev/holoviz_tasks/install@v0.1a15

@ahuang11
Copy link
Collaborator

Thanks; seems like the matplotlib is a bit old...
Will see if I can reproduce this on my Mac

  matplotlib                3.3.2                         0    conda-forge
  matplotlib-base           3.3.2            py38h5c7f4ab_1    conda-forge
  matplotlib-inline         0.1.6              pyhd8ed1ab_0    conda-forge

@ahuang11
Copy link
Collaborator

Okay, it's fixed now!

@hoxbro
Copy link
Member

hoxbro commented Jul 21, 2023

I think people are going to love this feature.

Thank you, @ahuang11, for finishing this PR!

@hoxbro hoxbro merged commit a9a91cd into main Jul 21, 2023
14 checks passed
@hoxbro hoxbro deleted the custom_opts branch July 21, 2023 06:53
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants