-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
feat(python): Add nicer default plot configuration, link to Altair Chart Configuration docs #18609
Conversation
71a3cf2
to
e405a39
Compare
Configuration docs
e405a39
to
57106a7
Compare
Makes a lot of sense to me! If user don't want these "extra configs" but to use their own theme, they are already familiar with Altair and can also just use it directy. However, this helps all the other users who do 't have their own Altair theme. I'll go through the settings later today to see if I'd have any other suggestions or see any issues. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #18609 +/- ##
==========================================
- Coverage 79.93% 79.92% -0.02%
==========================================
Files 1505 1505
Lines 202618 202638 +20
Branches 2873 2876 +3
==========================================
- Hits 161962 161951 -11
- Misses 40108 40139 +31
Partials 548 548 ☔ View full report in Codecov by Sentry. |
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 suggested some explanatory comments and would suggest to set titleY
to -25
instead of -20
as in my experience, this works for more charts.
Co-authored-by: Stefan Binder <binder_stefan@outlook.com>
Co-authored-by: Stefan Binder <binder_stefan@outlook.com>
@MarcoGorelli relating to these comments:
What if you re-aliased our I don't think they make sense in # https://github.com/MarcoGorelli/polars/blob/039d3c0b3c6df9f860d8739bc612351ce1731671/py-polars/polars/dataframe/plotting.py#L9
from altair.typing import (
ChannelColor as Color,
ChannelOrder as Order,
ChannelSize as Size,
ChannelTooltip as Tooltip,
ChannelX as X,
ChannelY as Y,
EncodeKwds,
) I can't remember the specific behavior, but IIRC aliasing on the import vs a |
.configure_axis( | ||
labelFontSize=16, | ||
titleFontSize=16, | ||
titleFontWeight="normal", | ||
gridColor="lightGray", | ||
labelAngle=0, | ||
labelFlush=False, | ||
labelPadding=5, | ||
) |
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.
As I understand, the alt.Chart.configure_
calls are being used to avoid registering + enabling a theme - which could override a user's custom theme.
These work fine in isolation, but AFAIK would have issues if a user were to layer/concat/facet the result - since config
is only valid at the top-level.
You might want to add tests to see if these ops would still be possible
- https://altair-viz.github.io/user_guide/customization.html#global-config
- https://altair-viz.github.io/user_guide/configuration.html#top-level-chart-configuration
Using a theme would have the benefit of deferring these config settings until the Chart
is rendered - placing them in the top-level only.
It might be worth seeing if we can come to a good solution to this as part of vega/altair#3519 since we have already discussed issues with the theme route
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 @dangotbanned for spotting this and for taking a look!
indeed, you're right, it does break combining charts with e.g. chart1 + chart2
, it raises an error
OK I think I'll close this for now and open a docs improvement PR, we can revisit the themes later
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.
No worries @MarcoGorelli
I'm quite interested in making this easier soon, hopefully we will have the right tools in place for when you revisit this
Summary:
title
,x_axis_title
,y_axis_title
args to plotting functionsQuick demo
Before:
After:
Personally I think this looks better by default - I've included a link to the Altair Chart Customisation docs anyway so anyone wanting further customise can use that
EDIT: still a bit torn between adding
title
/x_axis_title
/y_axis_title
vs just documenting that you can passalt.X
and set it like that...will sleep on it