-
-
Notifications
You must be signed in to change notification settings - Fork 192
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
Update VolcanoPlot props #554
Conversation
@HammadTheOne I confirmed this works by setting:
Should we set the defaults to something that looks cleaner? |
Thanks for confirming that it worked! I think having a nice, clean default legend/layout is a good idea. We had this originally, and I think we should have a non-white bgcolor and have the legend outside the plot area:
I'll make an update for those, but if you have any suggestions, I'd love to hear them. We can use the Plotly graphing library reference for inspiration. |
- Additional keys (misc.): Arbitrary arguments can be passed to modify the | ||
Layout and styling of the graph. A full reference of acceptable args is | ||
available [here](https://plotly.com/python-api-reference/generated/plotly.graph_objects | ||
.Layout.html). |
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.
Since we're accepting **kwargs
here and passing them to go.Layout
, do we actually need any of the other args you're adding here? We can list some of the common ones (including the ones you're including explicitly now) in the description here, they'd all work identically as part of kwargs.
x_axis
and y_axis
: in the docstring there's no _
and I'd argue it's cleaner without, then these can just be left to kwargs as well.
In order to keep the defaults you have but allow users to update the layout arbitrarily with kwargs after that, I suspect the easiest would be to generate the default layout first, then do layout.update(**kwargs)
. A little exploring shows that this does a deep merge, so if your kwargs are just xaxis=dict(color="red")
you'll still get the rest of the x axis as set in the default layout.
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.
do we actually need any of the other args you're adding here?
I thought it might be nice to have the most commonly used args available explicitly, especially for users that use IDE's which show parameter info, but simply including them as part of the documentation is also an option. However, retaining them as explicit args would also potentially save users a trip to the Plotly graphing library docs (unless we also update dash-docs with some better examples). I think it's a trade-off between cleaner params (with everything as kwargs) and easier "default" use of the component.
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.
Fair point re: IDE docs & completion. That said for everything except height
and width
you're still going to need to visit the graphing library docs to find out what goes inside the object, and it feels a bit arbitrary to be pulling in just a few layout options - as if the ones in kwargs are less-fully-supported or something. So I still think we'll be better off removing all of them and just documenting a few (the set you've proposed adding here looks good) in the docstring.
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.
you're still going to need to visit the graphing library docs to find out what goes inside the object,
That's a great point - Updated in a5b996a.
@alexcjohnson I think the last couple of commits should address the rest of the points you brought up, and the tests are now passing successfully. |
- xaxis (dict: optional): Dict with compatible properties for the x-axis of | ||
the figure layout. | ||
- yaxis (dict: optional): Dict with compatible properties for the y-axis of | ||
the figure layout. |
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.
Let's move these (and height, width, margin, legend below) into the "Additional keys" section of the docstring, saying something like "Some commonly-used keys are:" and then exactly the text you have currently. These keys don't explicitly show up in the function signature so it'll be confusing to have them mixed in like this.
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.
Updated in e3dfcb4.
xlabel=None, | ||
ylabel='-log10(p)', | ||
xaxis=None, | ||
yaxis=None, |
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 there any difference between explicitly including these to pass to figure.update_xaxes(**xaxis)
, and omitting them so they get picked up by layout.update(**kwargs)
? I suppose if we were to have multiple x or y axes, the former would update all of them, the latter would only update the first pair. But I don't believe we do have multiple axes, and anyway if we did I imagine you'd want the ability to update them separately.
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 was worried that key-worded arguments wouldn't be unpacked down to xaxes
without overwriting the defaults, but after seeing your comment about the deep merge and testing it out, there is no difference. Fixed in 56948c3.
@@ -355,13 +389,18 @@ def figure( | |||
col = 'black' | |||
|
|||
layout = go.Layout( | |||
title=title, | |||
title={'text': 'Volcano Plot', |
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 we be using the title
arg here? As it is we're ignoring it, right? Though passing the title
arg directly to title.text
in the layout means you can't use the dict form of title
to modify its styling. Then the only way to do that would be to use magic underscores in the kwargs (title_font_color="red"
etc).
Alternatively we could drop title
as an arg completely, so it gets picked up by layout.update
. The downside of that approach is that passing in a string for title
would wipe out the default style you have below - at least until plotly/plotly.py#3148 gets fixed, which hopefully will not be too long (🙏 @nicolaskruchten )
So yeah, I guess my inclination is to drop title
as an arg, even though it's a breaking change in the short term, because the current behavior will be restored with much more flexibility after we fix plotly/plotly.py#3148
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.
Dropped in 3e2ce66.
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.
Looks great! The change is pretty simple now, which bodes well :) 💃
About
Description of changes
As a continuation of #551 , this PR adds the
height/width
,margin
andlegend
props to theVolcanoPlot
component. In addition, we addkwargs
to pass arbitrary values in to theplotly.graph_objects.Layout
ofVolcanoPlot
so that any arguments available forLayout
are able to be passed here as well (eg.bgcolor
ortemplate
).