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

Filter style keywords according to backend #802

Merged
merged 6 commits into from Jul 27, 2016

Conversation

Projects
None yet
3 participants
@jlstevens
Member

jlstevens commented Jul 27, 2016

I think this functionality is best expressed with a screenshot:

screen shot 2016-07-27 at 2 54 03 pm

Note that linewidth applies to matplotlib and line_width applies to Bokeh.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jul 27, 2016

In the last commit, I skip filtering if style_opts is an empty list.

The assumption is that things like OverlayPlot don't support styles (although they may support plot options). Applying filtering on OverlayPlot was part of why the tests failed.

Is this assumption reasonable?

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jul 27, 2016

In the last commit, I skip filtering if style_opts is an empty list.

Sounds reasonable, yes. Not sure why not doing that would make it fail though.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jul 27, 2016

Without it, I saw odd things in the Elements notebook...for instance the first overlay with errorbars ended up having red errorbars instead of the usual black ones.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jul 27, 2016

Thanks for the last fix! Ready to merge once the tests pass.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jul 27, 2016

Okay, tests passing at last. Merging.

@philippjfr philippjfr merged commit fe40185 into master Jul 27, 2016

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 increased (+0.06%) to 46.691%
Details
s3-reference-data-cache Test data is cached.
Details

@philippjfr philippjfr removed the in progress label Jul 27, 2016

@jbednar

This comment has been minimized.

Member

jbednar commented Jul 27, 2016

In the future, in cases where multiple backends use the same keyword to mean different things, should we support adding an identifier to specify the backend? E.g. 'bokeh:linewidth=5" or "mpl:linewidth=10"? This could also be used if we wanted to customize the two plots separately, e.g. to choose one color that looks good against mpl's default gray background, and a different color for Bokeh...

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jul 27, 2016

E.g. 'bokeh:linewidth=5" or "mpl:linewidth=10"?

Seems like a good suggestion - please file an issue for this as a feature request.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jul 27, 2016

That does sounds like a good suggestion, although I'd go for something like mpl:(linewidth=5 facecolor='red') bokeh:(line_width=5 fill_color='red') to separate them more cleanly.

@philippjfr philippjfr added this to the v1.6.1 milestone Jul 27, 2016

@philippjfr philippjfr deleted the filter_style_backend branch Sep 2, 2016

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