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 · 11 comments

Comments

Projects
None yet
3 participants
@philippjfr
Member

philippjfr commented Mar 6, 2018

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.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Mar 6, 2018

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

@philippjfr

This comment has been minimized.

Member

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

This comment has been minimized.

Member

jlstevens commented Mar 6, 2018

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

@philippjfr

This comment has been minimized.

Member

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

This comment has been minimized.

Member

jlstevens commented Mar 6, 2018

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 referenced this issue Apr 22, 2018

Merged

Deal with clashing style and plot options #2604

3 of 3 tasks complete

@philippjfr philippjfr modified the milestones: v1.10.x, v1.10.2 Apr 24, 2018

@philippjfr

This comment has been minimized.

Member

philippjfr commented Apr 26, 2018

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

This comment has been minimized.

Member

philippjfr commented Apr 26, 2018

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

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

@jbednar

This comment has been minimized.

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

This comment has been minimized.

Member

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

This comment has been minimized.

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

This comment has been minimized.

Member

philippjfr commented Apr 26, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment