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

Add options inheritance on OverlayPlot #1060

Merged
merged 6 commits into from Jan 14, 2017

Conversation

Projects
None yet
3 participants
@philippjfr
Member

philippjfr commented Jan 14, 2017

Initial attempt at addressing #638, total WIP to see how tests are affected, but it may really be this simple.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jan 14, 2017

Given the tests pass, I think it might be something to enable for 1.7. It is something that we've wanted for a long time and as long as we give the option to restore the old behavior, this might be an easier win than we anticipated!

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jan 14, 2017

Yes, I'm really pleasantly surprised by this, it behaves exactly like I'd expect and after running through a bunch of notebooks my observations are twofold:

  • All the notebooks I've run through with this change have not exhibited any change in behavior.
  • Almost every notebook I've run through has at least one case where this change would let you simplify the options specification.

However my bandwidth is also limited, so I'd suggest we get as many people playing around with this as possible. As a start, @jlstevens, @jbednar could you both take this PR for a spin on as many notebooks as you can?

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jan 14, 2017

Here's an illustrative example:

%%opts Overlay [xticks=10] Curve [xticks=2 width=600 height=400] Points [show_grid=False bgcolor='WhiteSmoke']
hv.Curve(range(10)) * hv.Curve(np.random.rand(10)) * hv.Points(np.random.rand(100,2)*9)

The effective applied options for this example are: {'xticks': 10, 'width': 600, 'bgcolor': 'WhiteSmoke', 'show_grid': False, 'height': 400}

bokeh_plot 30

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jan 14, 2017

It is a fairly small PR so it should be easy to revert if we suddenly find major issues (which seems doubtful).

For this reason, I think the best way to get it well tested is simply to merge it ASAP. It might still be worth adding a class attribute toggle first though...

@philippjfr philippjfr added this to the v1.7.0 milestone Jan 14, 2017

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jan 14, 2017

The tests have passed so I will merge now. I'm hopeful this will address one of the biggest annoyances without causing much trouble!

@jlstevens jlstevens merged commit 315c870 into master Jan 14, 2017

4 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.002%) to 76.852%
Details
s3-reference-data-cache Test data is cached.
Details

@jbednar jbednar deleted the options_inheritance branch Jan 14, 2017

@jbednar

This comment has been minimized.

Member

jbednar commented Jan 14, 2017

Fabulous! I think that this will go a long way towards making it intuitive to set and use options.

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