-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Revamp tls.get_subplots #170
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
Conversation
plotly/tools.py
Outdated
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.
I'm thinking about supporting fig = tls.get_subplots(fig, **kwargs)
e.g. if someone wants to augment a already-defined figure object.
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.
i maybe it would make sense as as a method of a figure?
I'd love to get some feedback before I get started. Thanks in advance.
@etpinard , my major two concerns are:
- misplaced functionality, let's make sure we don't extend the use of the get_subplots function beyond it's intuited limits. (related to adding some functionality to a method of Figure OR making a new function to handle a more abstracted figure design process)
- rather than take a complicated structure of subplots and infer locations, it might be better to allow the user to define a grid and then assign subplots to locations on the grid (like matplotlib's gridspec)
@theengineear Thanks for the insight as always.
- Something along the lines of mpl's gridspecs is the way to go I think. I'll try to work something out today.
- [Re making
figan argument fortls.get_subplots] You're right, it does seem a little weird. I wouldn't be opposed to makingget_subplotsa method ofFigure, but let's leave that for a future PR.
- breaks backwards compatibility - horz = 0.2 / columns (same as old if rows==2) - vert = 0.3 / rows (same as old if columns==2)
- subplot domain computation are made using specs whether or not it is a supplied argument
- use tracers (x and y) to keep track of position from subplot to subplot - cases where colspan > 1 or rowspan > 1 require more attention - use _get_shared() to get the shared axis (if any) - use _add_domain() to paste result into fig object - use _fill_grid() to generate the str repr of the subplot grid
- because of new horizontal_spacing and vertical_spacing defaults
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.
@theengineear I had to modify the computed domains. The algorithm in this branch yield slightly (slightly) different values.
I'll make an NB tomorrow showing tls.get_subplots new muscles.
FYI I found a few bugs yesterday while making the NB.
Modifs are coming.
@theengineear good call. _grid_ref and _grid_str make more sense as attributes.
@theengineear @chriddyp version bump and merge?
🍔 from me, but I think we should @chriddyp a little time to respond as well!
plotly/graph_objs/graph_objs.py
Outdated
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.
docstring!
Helper function to add a data traces to your figure that is bound to axes at the row, col index.
The row, col index is generated from figures created withplotly.tools.make_subplotsand
can be viewed withFigure.print_grid.
I'm gonna give this a quick spin 🚗
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.
Maybe changed to:
stack two subplots vertically
fig = tools.get_subplots(rows=2)
fig.append_trace(Scatter(x=[1,2,3], y=[2,1,2]), row=1, col=1)alternatively: fig['data'] += [Scatter(x=[1,2,3], y=[2,1,2], xaxis='x1', yaxis='y1')]
fig.append_trace(Scatter(x=[1,2,3], y=[2,1,2]), row=2, col=1)
alternatively: fig['data'] += [Scatter(x=[1,2,3], y=[2,1,2], xaxis='x2', yaxis='y2')]
Not sure about the "alternatively" text - thoughts? @theengineear , @etpinard . I sort of feel like docstring should be as concise and informative and extendable as possible, so we shouldn't have the extra comment and instead do a more thorough job on our hosted docs
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.
yeah, let's give one example and link to documentation, we can use that thing to link to the appropriate domain too: https://github.com/plotly/python-api/blob/80fcc59309d59c227a91075070882904e29fb006/plotly/utils.py#L286-292
with the @utils.template_doc wrapper, like in https://github.com/plotly/python-api/blob/a0dad3cc0b70a60e195d4a6ea7eaca06937d1bf3/plotly/plotly/plotly.py#L391
Conflicts: plotly/version.py
besides this comment (#170 (comment)), 💃 from me
should we make a note about subtle changes (don't have to list them, but just let the user know) here:
https://github.com/plotly/python-api/blob/revamp-get_subplots/plotly/tools.py#L430-433
@theengineear @chriddyp
tls.get_subplotsis in need of some revamping.More info in this Asana task.
This NB shows off the new features.
Features wishlist:
API issue to resolve:
rowspan> 1 andis_emptyRevamp tls.get_subplots #170 (comment)Solution: Enforce specs to have dimensions (rows, columns). Use
Noneto fill in cells over row / column spans.specskeyword argument Revamp tls.get_subplots #170 (comment)