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
Merged

Refactored plotly backend #3256

merged 33 commits into from Dec 11, 2018

Conversation

@philippjfr
Copy link
Member

@philippjfr 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
Copy link
Member Author

@philippjfr 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
Copy link
Member

@jonmmease 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
Copy link
Member Author

@philippjfr 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
Copy link
Member Author

@philippjfr philippjfr commented Dec 9, 2018

Ready to review and merge.

@jonmmease
Copy link
Member

@jonmmease 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 Member

Don't see any.

@jlstevens
Copy link
Contributor

@jlstevens 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
@jlstevens
Copy link
Contributor

@jlstevens 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
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
@philippjfr
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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants