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

Plot and style option name clashes #2411

Closed
philippjfr opened this issue Mar 6, 2018 · 12 comments
Closed

Plot and style option name clashes #2411

philippjfr opened this issue Mar 6, 2018 · 12 comments
Labels
tag: component: options type: bug Something isn't correct or isn't working
Milestone

Comments

@philippjfr
Copy link
Member

The new .options method relies on unique names for plot and style options. At least in one case that is currently not true, namely the width option on Bars is both a plot options (controlling the width of the plot) and a style option (setting the width of the bars). We need to validate that these do not clash, and in the case of width rename the width style option to bar_width.

@philippjfr philippjfr added type: bug Something isn't correct or isn't working tag: component: options labels Mar 6, 2018
@jlstevens
Copy link
Contributor

I hope this is the only example of a clash we find.

@philippjfr
Copy link
Member Author

philippjfr commented Mar 6, 2018

There's a few more, but it's not too bad:

from itertools import combinations

def option_intersections(backend):
    options = hv.Store.options(backend)
    for k, opts in options.items():
        if len(k) > 1: continue
        eltype = k[0]
        valid_options = {k: set(o.allowed_keywords)
                         for k, o in opts.groups.items()}
        for g1, g2 in combinations(['plot', 'style', 'norm'], 2):
            intersection = valid_options[g1] & valid_options[g2]
            if intersection:
                print('%s element %s and %s %s backend options intersect on: %s'
                      % (eltype, g1, g2, backend, intersection))
       
for backend in hv.Store.renderers:
    option_intersections(backend)
Bars element plot and style **bokeh** backend options intersect on: {'width'}
BoxWhisker element plot and style **bokeh** backend options intersect on: {'width'}

Arrow element plot and style **matplotlib** backend options intersect on: {'fontsize'}
Text element plot and style matplotlib backend options intersect on: {'fontsize'}

Curve element plot and style **plotly** backend options intersect on: {'width'}
ErrorBars element plot and style **plotly** backend options intersect on: {'width'}

@jlstevens
Copy link
Contributor

Maybe for Arrow it could be called textsize. Not sure about all the others though.

@philippjfr
Copy link
Member Author

philippjfr commented Mar 6, 2018

Bokeh Bars: width -> bar_width
Bokeh BoxWhisker: width -> box_width
Matplotlib Arrow: fontsize -> textsize
Matplotlib Text: fontsize -> textsize
Plotly Curve/ErrorBars: delete width as a style option (or if it refers to line width use line_width)

@jlstevens
Copy link
Contributor

Those suggestions seem reasonable enough. The problem then is retaining the old names (and therefore the clashes) for backwards compatibility...

@philippjfr philippjfr added this to the v1.10 milestone Mar 7, 2018
@philippjfr philippjfr modified the milestones: v1.10, v1.10.x Apr 17, 2018
@philippjfr philippjfr modified the milestones: v1.10.x, v1.10.2 Apr 24, 2018
@philippjfr
Copy link
Member Author

Okay, I've come up with a plan for this, I'm going to make sure that plot options take precedence over style options in the expand_options method, since those are more common. For now we will leave the clashing style options registered, which means that they can still be set using opts or the magic but I will also register the new non-clashing names as aliases. Eventually we can deprecate the clashing options.

@philippjfr
Copy link
Member Author

Matplotlib Arrow: fontsize -> textsize
Matplotlib Text: fontsize -> textsize

These two were actually never used since the element defines that parameter.

@jbednar
Copy link
Member

jbednar commented Apr 26, 2018

I'd consider those options deprecated immediately; deprecated just means "don't use in new code", right? So as soon as we know they shouldn't be in new code, shouldn't they be deprecated?

@philippjfr
Copy link
Member Author

philippjfr commented Apr 26, 2018

So as soon as we know they shouldn't be in new code, shouldn't they be deprecated?

True, I wasn't sure whether to add a warning, but I suppose we should.

@jbednar
Copy link
Member

jbednar commented Apr 26, 2018

Whether to add a warning is a different matter than deprecating them. I'd say the thing to do immediately is to mark them deprecated in the release notes. Then in the following release, there can be a warning.

@philippjfr
Copy link
Member Author

I'd say the thing to do immediately is to mark them deprecated in the release notes. Then in the following release, there can be a warning.

Sounds good.

Copy link

This issue 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 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
tag: component: options type: bug Something isn't correct or isn't working
Projects
None yet
Development

No branches or pull requests

3 participants