-
Notifications
You must be signed in to change notification settings - Fork 120
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
Enable colors
as list in plot.pie()
#563
Conversation
Codecov Report
@@ Coverage Diff @@
## main #563 +/- ##
=====================================
Coverage 93.6% 93.7%
=====================================
Files 50 50
Lines 5313 5322 +9
=====================================
+ Hits 4978 4987 +9
Misses 335 335
Continue to review full report at Codecov.
|
622527b
to
a5a40be
Compare
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.
Great new feature! Some small adjustments requested. Is it possible to add a test of some kind for the 'future proofing' via groupby
? Not required given all tests currently pass, but would be nice for future us's to not change..
pyam/plotting.py
Outdated
@@ -274,7 +274,7 @@ def pie( | |||
value="value", | |||
category="variable", | |||
legend=False, | |||
title=True, | |||
title=False, |
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.
Here I would suggest a default value of None
. In other plotting funcs, title can be True
if there is logic available to find a reasonable default. If not, then it will be set by the kwarg value. Would suggest also updating docstring to string, optional
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 - also change the docstring for the title
keyword argument to follow Axes.set_title()
tests/test_plotting.py
Outdated
( | ||
plot_df.filter( | ||
variable="Primary Energy", model="test_model", year=2010 | ||
).plot.pie(ax=ax, category="scenario", colors=["red", "blue"], title="foo") |
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 know this is pedantic, but can you use values other than red and blue here so this test is easily visually distinguishable from the default values (e.g. in test_pie_plot_labels.png
).
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
Thanks for the suggestions, all implemented! Not quite sure what you mean by adding a test for resolving a future-warning? If you go back to a previous test run, section "Test with pytest", line 270, there is a warning that this particular code will stop working. If you compare this to the current test runs, this warning is gone. (There are plenty of others, though, tackling them one at a time). |
Specifically the line
I was curious it is was possible to add any test for this. If not, that's fine. |
Ahh ok, I understand now. Thanks! |
Please confirm that this PR has done the following:
Name of contributors Added to AUTHORS.rstDescription of PR
This PR enables passing an explicit
colors
arg as list to to the pie plot, see https://matplotlib.org/stable/api/_as_gen/matplotlib.pyplot.pie.htmlIt also implements the
title
keyword argument, which was previously ignored in the implementation, and future-proofs pandas (usingmin(level=0)
will be disabled).