Adds normalize kwarg to pie function #16985

raphacosta27 · Mar 31, 2020

PR Summary

pie() used to draw a partial pie if the sum of the values was < 1 and already normalized the values if the sum was > 1. I added a kwarg normalize into pie function and deprecated the previous behaviour. Now, the function will always normalize the values to a full pie by default. If one wants to draw a partial pie, one should pass normalize=False explicitly.

PR Checklist

Has Pytest style unit tests
Code is Flake 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

lib/matplotlib/axes/_axes.py

lib/matplotlib/tests/test_axes.py

doc/api/next_api_changes/deprecations.rst

QuLogic · Mar 31, 2020

WRT context, please give your PRs and commit messages more context. One should not have to look up #16905 to understand what a commit does.

lib/matplotlib/axes/_axes.py

doc/api/next_api_changes/deprecations.rst

lib/matplotlib/axes/_axes.py

tacaswell · Apr 1, 2020

Because you are changing the signature of an Axes method, you will also need to re-run python tools/boilerplate.py to re-generate pyplot.py.

tacaswell · Apr 1, 2020

Thanks for working on this. It looks like it is headed in the right direction!

lib/matplotlib/axes/_axes.py

timhoffm · Apr 2, 2020

There are some style issues, see the flake8 run on travis:

./lib/matplotlib/axes/_axes.py:2906:80: E501 line too long (81 > 79 characters)
./lib/matplotlib/axes/_axes.py:2906:82: W291 trailing whitespace
./lib/matplotlib/axes/_axes.py:2907:80: W291 trailing whitespace
./lib/matplotlib/axes/_axes.py:2908:80: W291 trailing whitespace
./lib/matplotlib/axes/_axes.py:2910:80: E501 line too long (89 > 79 characters)
./lib/matplotlib/axes/_axes.py:2912:80: E501 line too long (80 > 79 characters)
./lib/matplotlib/axes/_axes.py:2912:81: W291 trailing whitespace
./lib/matplotlib/axes/_axes.py:2992:18: W291 trailing whitespace
./lib/matplotlib/axes/_axes.py:2994:1: W293 blank line contains whitespace
./lib/matplotlib/axes/_axes.py:2997:1: W293 blank line contains whitespace
./lib/matplotlib/tests/test_axes.py:6424:1: E302 expected 2 blank lines, found 1
./lib/matplotlib/tests/test_axes.py:6429:1: E302 expected 2 blank lines, found 1

Also, please run python tools/boilerplate.py to update the auto-generated function pyplot.pie.

raphacosta27 · Apr 3, 2020

Also, please run python tools/boilerplate.py to update the auto-generated function pyplot.pie.

I've fixed the flake8 issues and also ran boilerplate.py in the previous commit. Please tell if there's any problem regarding the boilerplate.

lib/matplotlib/pyplot.py

lib/matplotlib/axes/_axes.py

tacaswell · Apr 4, 2020

@raphacosta27 Could you please squash your commits? If you do so you will need to force-push to github (and ignore it's suggestion to run git merge).

raphacosta27 · Apr 5, 2020

@raphacosta27 Could you please squash your commits? If you do so you will need to force-push to github (and ignore it's suggestion to run git merge).

Hi there, I've fixed the previous suggested comments but I am with problem squashing the commits, but will solve it tomorrow probably.

tacaswell · Apr 5, 2020

If you need help with git drop into the gitter channel (https://gitter.im/matplotlib/matplotlib)

raphacosta27 · Apr 8, 2020

If you need help with git drop into the gitter channel (https://gitter.im/matplotlib/matplotlib)

I think the squash is done by now. Git said that I had some conflicting with the behaviour.rst file but I've already fixed it. Thanks.

timhoffm · Apr 12, 2020

@raphacosta27 is it ok, if I force-push to your branch to clean this up?

raphacosta27 · Apr 13, 2020

@raphacosta27 is it ok, if I force-push to your branch to clean this up?

Sure, no problem! Go ahead.

QuLogic · Apr 17, 2020

lib/matplotlib/axes/_axes.py

@@ -2942,6 +2942,17 @@ def pie(self, x, explode=None, labels=None, colors=None,
        shadow : bool, default: False
            Draw a shadow beneath the pie.

+        normalize: None or bool, default: None
+            ``pie()`` used to draw a partial pie if ``sum(x) < 1``. This


Can you change this to state what normalize does first, and then the deprecated information in a second paragraph?

QuLogic · Apr 17, 2020

lib/matplotlib/axes/_axes.py

+        if normalize is None:
+            if sx < 1:
+                cbook.warn_deprecated(
+                    "3.1", message="normalize=None does not normalize if "


3.3, not 3.1

QuLogic · Apr 25, 2020

lib/matplotlib/axes/_axes.py

+                    "3.1", message="normalize=None does not normalize if "
+                    "the sum is less than 1 "
+                    "but this behavior is deprecated "
+                    "since %(since)s. After the deprecation period "


Use %(removal)s to indicate when the deprecation period is over.

tacaswell

👍 to changes, but it needs a rebase. I will try to take care of that later today.

brunobeltran

I'm overall thumbs up on this change, but think that the behavior in the case where normalize = False and the total adds up to more than a full pie too easily hides bugs where the user didn't mean to have sx > 1.

However, since the user is specifically setting normalize = False, I would be content with adding a comment to the docstring that warns the user that we don't guarantee the plot will "make sense" if they pass normalize = False and sx > 1.

brunobeltran · Apr 27, 2020

lib/matplotlib/axes/_axes.py

+        normalize: None or bool, default: None
+            When *True*, always make a full pie by normalizing x so that
+            ``sum(x) == 1``. When *False*, make a partial pie.
+            When *None*, gives the current behavior and warns if


Instead of "current behavior", this should describe the behavior explicitly (since that will now be the "previous" behavior).

brunobeltran · Apr 27, 2020

lib/matplotlib/axes/_axes.py

+            else:
+                normalize = True
+        if normalize:
+            x = x / sx


What would happen if normalize = False and sx > 1? Unlike normalize = False and sx < 1 (which will look the same as the old behavior), we don't have any precedent for what the output should look like. To me, it doesn't make sense to make a pie plot that that more than 100%, since some of the pies will overlap each other (making it look like a normal pie plot, but where some of the slices look the incorrect size).

I think it would be less surprising to the user if we fail when asked to make pie plots with normalize = False and sz > 1.

normalize = False and sx < 1, a partial pie, this is existing behavior.

normalize = False and sx > 1, more than a full pie....it's not clear to me what this should look like once drawn....and it's new behavior

normalize = True and sx >= 1, the old behavior is unchanged....

normalize = True and sz < 1, the new behavior, I definitely like this being built-in to pie.

We recently made some changes to pie to not accept negative values (which also resulted in a greater-than-full pie),

TIL. I guess it's part of the pie now.

tacaswell · Apr 27, 2020

@raphacosta27 I took the liberty of rebasing, attempting to address the documentation concerns, and force-pushed to your branch.

raphacosta27 · Apr 27, 2020

@raphacosta27 I took the liberty of rebasing, attempting to address the documentation concerns, and force-pushed to your branch.

Thank you!

brunobeltran

Copy edits for the description of the None case and highly optional suggestion for how to shorten and reduce redundancy of the language in the docstring's deprecation warning.

lib/matplotlib/axes/_axes.py

jklymak · Apr 29, 2020

This could really use an example somewhere.

jklymak

In addition to @brunobeltran suggestions, please add an example of the kwargs use, perhaps to a previous pie example.

Co-authored-by: Bruno Beltran <brunobeltran0@gmail.com>

timhoffm · May 1, 2020

Since I'd like to get this into 3.3, I've taken the liberty to push the changes requested by @brunobeltran.

@jklymak While examples are always nice, I wouldn't block on that. We haven't had an example for sum(x) < 1 so far.

jklymak · May 1, 2020

We haven't had an example for sum(x) < 1 so far.

Yeah, but the new behaviour is complicated enough it would greatly benefit from one that could be linked in the API docs.

timhoffm · May 1, 2020

I don't think the behavior is complicated: If normalize scale the values; if not, make a partial pie for sum(x) < 1 and error out otherwise ("overfull pie").

It may look more complicated because of the deprecation.

jklymak · May 1, 2020

I know what "partial pie" means from reading this PR, but not from reading the docstring.

Main point taken care of

timhoffm · May 2, 2020

@jklymak If you have suggestions how to improve the docs, we can always update.

timhoffm reviewed Mar 31, 2020

View reviewed changes

raphacosta27 changed the title ~~Fixes issue #16905~~ Adds normalize kwarg to pie function Apr 1, 2020

tacaswell reviewed Apr 1, 2020

View reviewed changes