-
-
Notifications
You must be signed in to change notification settings - Fork 1
Port to plotly.py v4_subplots and renderers #88
Conversation
plotly_express/_core.py
Outdated
showgrid=args["marginal_x"] == 'histogram', | ||
row=row | ||
) | ||
fig.update_xaxes( |
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.
has this been run through black
? :)
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.
Done now
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.
Thanks! I know the plotly.py repo doesn't have that in place but this one does for now ;)
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, I'm interested in using black in plotly.py too. Have you looked into the git integration https://black.readthedocs.io/en/stable/version_control_integration.html?
plotly_express/_core.py
Outdated
|
||
# Find row for trace, handling facet_row and marginal_x | ||
if m.facet == 'row': | ||
if has_marginal_x: |
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.
Is this case for marginals + facets? that's not supported right now in the grouping logic.
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.
Yes. With these changes I think marginals + facets are working. See the screenshot in the description above.
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.
aha I see, that's interesting. Cool that it works!
I think that for combining them, I was envisioning per-row and per-column marginals rather than per-subplot marginals, which isn't quite as straightforward (although not impossible, just requires a second group-by pass). Thoughts?
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 liked per subplot marginals, but I suppose per row/col could useful to. Would it make sense for this to be the default and then later add an option to change the behavior?
can I see an example? Is the spacing too big or too small? |
I see two issues:
|
# Conflicts: # plotly_express/_core.py
If faceting and marginal specified then faceting wins. It's still possible to facet columns and have marginals per columns or to facet rows and have marginals per row.
@nicolaskruchten, I just updated the PR with master and removed the marginals per facet. If faceting and marginal specified then faceting wins. It's still If this looks alright, I'll start pulling it into the v4_integration branch of plotly.py |
) | ||
TraceSpec = namedtuple("TraceSpec", ["constructor", "attrs", "trace_patch"]) | ||
TraceSpec = namedtuple("TraceSpec", ["constructor", "attrs", "trace_patch", "marginal"]) |
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.
This is a good idea, and maybe should be generalized later to model the "role" of the trace in the figure... Is it the main event, or a marginal, or a trendline, or some other future thing like the raw scatter that someone might want to add to a density heatmap?
log_key = "log_" + letter | ||
range_key = "range_" + letter | ||
if log_key in args and args[log_key]: | ||
layout[axis]["type"] = "log" | ||
axis["type"] = "log" |
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.
if axis
is a go
then it would be nicer to have axis.type
here right?
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.
(not necessary, just noting)
@@ -38,25 +45,6 @@ def set_mapbox_access_token(token): | |||
MAPBOX_TOKEN = token | |||
|
|||
|
|||
class ExpressFigure(go.Figure): |
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.
should grep the docstrings for ExpressFigure
and change that out :)
This looks great! Having marginals vs facets work this way on a per-direction basis is really 👍 !! |
Let's pull this into v4 |
Thanks @nicolaskruchten, superseded by plotly/plotly.py#1613 |
Overview
This PR updates plotly_express to use the plotly.py
plotly.subplots.make_subplots
functionality to build faceted figures, rather than the currentlayout.grid
approach. By building onplotly.subplots.make_subplots
, it will be much easier for users to modify faceted figures in plotly.py.It also removes
ExpressFigure
and relies on the plotly.pyplotly.io.renderers
configuration to display figures.Because this PR requires plotly.py to be imported with the
v4_subplots
andrenderer_defaults
future flags. If plotly_express is imported first then this is done automatically, but if plotly.py is import first, without these future flags, then an error will be raised when plotly_express is imported.Examples
Update a y-axis based on row
Update a trace based on row
Marginals and Faceting
Support for combining marginals and faceting
Faceting non-cartesian trace types
This PR doesn't add
facet_row
/facet_col
to any additional plot types, but when these args are added, faceting will "work" automatically. I didn't add it in this PR because the default spacing for other trace types can look pretty bad, so we'll need to work through how these other subplot types shoud look with faceting.cc @nicolaskruchten