-
-
Notifications
You must be signed in to change notification settings - Fork 402
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
New subplot foundation for Plotly backend #3255
Conversation
This builds a figure out of a 2D grid of sub figures. It overcomes the limitations of the make_subplots method because it handles all trace types and it works recursively
This gives us more freedom in the low-level figure manipulation utilities (those added in `holoviews/plotting/plotly/util.py`) and it is somewhat faster as well
…ring This way we still get full property validation, but it only happens once at the very end.
This will trigger property validation errors during testing if any of the backend operations produce invalid plotly figure dicts
currently needed.
Wow, cool! |
@jonmmease You continue to amaze me! Is this in a good place to start review? We were about to declare a feature freeze, but it would be really great to get these improvements into 1.11.0. |
Haha, thanks @philippjfr 🙂 Yeah, this is good to go from my end (pending CI tests passing) |
@@ -123,3 +193,126 @@ def test_layout_instantiate_subplots_transposed(self): | |||
plot = plotly_renderer.get_plot(layout(plot=dict(transpose=True))) | |||
positions = [(0, 0), (0, 1), (1, 0), (1, 1), (2, 0), (2, 1), (3, 0), (3, 1)] | |||
self.assertEqual(sorted(plot.subplots.keys()), positions) | |||
|
|||
|
|||
@attr(optional=1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have plans to replace these since they're kind of useless (in fact we plan to migrate to pytest at some point). For now I'd recommend following a pattern where you create a baseclass like this for all plotly tests (you can look in holoviews/tests/plotting/bokeh/testplot.py
to see an example):
class TestPlotlyPlot(ComparisonTestCase):
def setUp(self):
self.previous_backend = Store.current_backend
self.comm_manager = bokeh_renderer.comm_manager
bokeh_renderer.comm_manager = comms.CommManager
if not bokeh_renderer:
raise SkipTest("Plotly required to test plot instantiation")
Store.current_backend = 'plotly'
def tearDown(self):
Store.current_backend = self.previous_backend
bokeh_renderer.comm_manager = self.comm_manager
Callback._callbacks = {}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looked to me like the TestPlotlyPlotInstantiation
class above follows this pattern, and this is where I put the new tests that use the plotly backend. I didn't follow the pattern for TestPlotlyFigureGrid
because it's not actually testing the backend directly, just the new figure_grid
util function. Does that make sense?
Would it make sense to just remove the @attr
for TestPlotlyFigureGrid
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes, totally missed that thanks. I think it's fine, I'll replace all the @attr decorators in one go anyway.
I'll point out a few things but they're all optional and I'd be happy to fix them up subsequently. |
|
||
def _get_subplot_number(subplot_val): | ||
""" | ||
Extract the subplot number from a subplot value string. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've started converting everything to Google style docstrings. For now it's extremely inconsistent and we're still very much transitioning (e.g. I haven't even started on the plotting modules). So this is more of an FIY (which should really be recorded in some as of yet non-existent developer guide).
I'd be happy to merge this PR as is, thanks for the detailed docstrings! I'll probably have to play around a bit to get a good feel for the way the figure merge, subplot merging and figure grid code works but this seems like a very solid foundation to build on. One thing I'd be happy to do at this point is to remove the big warning about the experimental nature of the backend. I may also work on porting the new so called |
First of all, thank you very much @jonmmease for your work here!
Here is the current warning:
I would like to remove this warning but I don't think we would need to debate it at all if @jonmmease were willing to volunteer as the maintainer for the plotly support. It probably wouldn't have to involve all the responsibilities listed above (e.g docs and responding to all user issues) but having someone willing to own the code would definitely help a lot. Alternatively, we could keep the warning but edit it so it if softer (e.g still asking for help but not saying it is an experimental and unsupported feature). |
We can decide on the warning separately as I'll be opening a PR with some cleanup and porting some recent features, but please do let us know if you'd be happy to have at least some ownership over the codebase. For now I'll merge. |
The reason for the warning is that we aren't using the Plotly backend for our own projects at this time, and so its maintenance can't be folded into anything we're working on. I'd be happy to remove the warning as long as someone were looking out for it even a little bit... |
Hi @philippjfr @jlstevens @jbednar , |
Overview
This PR introduces a new foundation for combining plotly elements into a figure.
Background
Currently, the plotly backend makes use of the
plotly.tools.make_subplots
function to support laying out multiple elements in a single plotly figure. This function is pretty flexible when working with Cartesian trace types, but it has limited support for 3D traces and no support for other trace types. Additionally, it cannot be used recursively so it's not possible to usemake_subplots
to build aGridSpace
figure and thenmake_subplots
again to layout multipleGridSpace
views in a single figure.PR Notes
There is a pretty detailed commit log, but here are the two top-level goals:
Replace
make_subplots
withfigure_grid
This PR replaces the use of
make_subplots
with a newfigure_grid
function. This function inputs a 2D list of figuredict
instances, along with optional spacing arguments, and returns a new figure that is the combination of all of the input figures. It handles all plotly trace types along with annotations, shapes, and images that are specified in Cartesian axis coordinates. It also works fine recursively, so it is now possible to, for example, layout aGridSpace
next to aHoloMap
.I may eventually try to roll this functionality into plotly.py directly, but it would need to be a lot more general, so I'd rather start in Holoviews where we have control over how figures are constructed.
Replace
graph_objs
withdicts
at theElement
levelThis PR also changes the elements and layouts to work with
dict
instances rather thangraph_objs
instances. Thegraph_objs
in version 3+ perform a lot more validation than in version 2, which is great when a user is directly building their own figure but it does have a performance cost. For a system like Holoviews that is performing lots of iterative construction, I think it's better to build up the figure using rawdict
andlist
instances and then convert it to agraph_objs.Figure
object for validation just before rendering.I'm not sure if this was the best place for it, but I added the step to convert the figure
dict
to aFigure
instance to the_figure_data
method ofPlotlyRenderer
.Usage highlights
Here are some examples of layouts that did not work previously
3D plots in a layout
Tables in a layout
Note: I still want to replace this
figure_factory
table with the plotly table trace, but using the figure factory implementation here shows how annotations are merged and maintained successfully.GridSpace inside a layout
Note that the
GridSpace
s shared x-axes and y-axes are maintained when placed inside a larger layout.Performance
The plotly backend had felt a bit sluggish to me compared to bokeh and matplotlib, but with these changes it's much faster that it was, and faster than the other backends in some cases. I'm not sure how to time this exactly, but here's a GIF of the display time for a layout of two 4x4
GridSpace
s using each backendThanks for taking a look, and please let me know if you have any questions!