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

Refactored plotly backend #3256

Merged
merged 33 commits into from Dec 11, 2018

Conversation

Projects
None yet
3 participants
@philippjfr
Copy link
Contributor

philippjfr commented Dec 4, 2018

Cleanup and refactoring of the plotly backend.

  • Adds Path3D element #2453
  • Consistently handles invert_axes and invert_x/y/zaxis options
  • Adds support for xticks/yticks/zticks
  • Ports basic dim transforms to plotly backend
  • Updates TablePlot to trace type
  • Adds LabelPlot, AreaPlot, SpreadPlot, ViolinPlot
  • Adds unit tests for element plot classes
  • Add documentation for plotly containers
  • Added/updated element reference notebooks

@philippjfr philippjfr added the WIP label Dec 4, 2018

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Dec 5, 2018

@jonmmease Still working on this a bit but I wanted to check with you before getting much further into this. The PR refactors the element plotting classes a bit. The main changes is that instead of declaring the trace_type each class now declares some trace_kwargs which can include a bit more info than the type and then it refactors the get_data and graph_options methods to allow returning multiple sets of data.

@jonmmease

This comment has been minimized.

Copy link
Collaborator

jonmmease commented Dec 5, 2018

@philippjfr, I did look over it briefly last night and this looks/sounds like a nice generalization that will be useful to support future elements. Thanks!

@philippjfr philippjfr force-pushed the plotly_updates branch from e91ac7c to 40feac8 Dec 9, 2018

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Dec 9, 2018

Okay, I think I'm mostly done here. I think it is indeed a nice generalization which makes writing new plotting element plotting classes a lot easier. One thing that should be generalized further is the way styling is applied, i.e. certain style options are set on the marker while others are set on the data itself and that is not nicely handled yet. However for the time being this is already much more flexible than it was. I've also finally added unit tests for all the element plots.

@philippjfr philippjfr added plotting enhancement and removed WIP labels Dec 9, 2018

@philippjfr philippjfr referenced this pull request Dec 9, 2018

Open

Master list of potential Plotly backend enhancements #3196

6 of 21 tasks complete
@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Dec 9, 2018

Ready to review and merge.

@jonmmease

This comment has been minimized.

Copy link
Collaborator

jonmmease commented Dec 9, 2018

wow @philippjfr, this is awesome! The plotly backend is really coming along now! Everything looks great from my end, and I'm excited to play with it soon 🙂

]
}
],
"metadata": {

This comment has been minimized.

@jlstevens

jlstevens Dec 10, 2018

Contributor

Notebook metadata needs to be stripped out.

This comment has been minimized.

@philippjfr

philippjfr Dec 11, 2018

Author Contributor

Don't see any.

@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Dec 10, 2018

I've not yet reviewed the code but I do see this PR is introducing a bunch of new notebooks.

I'll be making a PR today updating all the bokeh reference gallery notebooks to use the new options style and I don't want to merge any new notebooks unless they conform to that style as well.

Edit: The PR showing what has been updated for Bokeh is #3269

@philippjfr philippjfr force-pushed the plotly_updates branch from 71a64f2 to 1295c38 Dec 11, 2018

@philippjfr philippjfr force-pushed the plotly_updates branch from 1295c38 to 83bfe17 Dec 11, 2018

Philipp Rudiger Philipp Rudiger
@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Dec 11, 2018

@philippjfr told me this is now ready to merge.

Thanks for updating the notebooks as requested. Merging.

@jlstevens jlstevens merged commit d1b71c8 into master Dec 11, 2018

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.2%) to 90.115%
Details
s3-reference-data-cache Test data is cached.
Details

@philippjfr philippjfr deleted the plotly_updates branch Dec 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.