Skip to content

Small cleanups/simplifications/fixes to pie(). #17194

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 20, 2020
Merged

Small cleanups/simplifications/fixes to pie(). #17194

merged 1 commit into from
Apr 20, 2020

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Apr 19, 2020

  • Change the default values of startangle and radius from "None" to
    the more explicit 0 and 1.
  • Apply wedgeprops and textprops separately after the corresponding
    artist has already been instantiated. This avoids conflicts in case
    e.g. textprops contains an "ha" entry (the dict merge won't notice
    that this needs to override "horizontalalignment", but applying the
    user-supplied properties in a second step will "just work".
  • Misc. small cleanups.

PR Summary

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

@anntzer anntzer added this to the v3.3.0 milestone Apr 19, 2020
timhoffm
timhoffm reviewed Apr 19, 2020
View reviewed changes
lib/matplotlib/axes/_axes.py
shad = mpatches.Shadow(w, -0.02, -0.02)
shad.set_zorder(0.9 * w.get_zorder())
shad.set_label('_nolegend_')
shad.set(zorder=0.9 * w.get_zorder(), label='_nolegend_')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason these parameters are not passed directly to Shadow?

I'm also wondering if we need to specify the label, because by default an Artist should not have any label.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't work, likely because of bad interaction with #16098.
Also by default Shadow inherits the parent patch (the Wedge)'s props, including the label. Again perhaps this should be changed on Shadow, but heh.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, was here before. I'll close my eyes and move on.

@timhoffm
Copy link
Member

timhoffm commented Apr 19, 2020 •
edited
Loading

Not sure why tests fail with

=================================== FAILURES ===================================
________________________ test_subplot_theta_range_raise ________________________
[gw0] linux -- Python 3.6.10 /opt/hostedtoolcache/Python/3.6.10/x64/bin/python

    def test_subplot_theta_range_raise():
        with pytest.raises(ValueError, match='The angle range must be <= 2 pi'):
            ax = plt.subplot(111, projection='polar')
>           ax.set_thetalim(0, 3 * numpy.pi)
E           NameError: name 'numpy' is not defined

lib/matplotlib/tests/test_subplots.py:193: NameError
__________________ test_subplot_theta_range_normal_non_raise ___________________
[gw0] linux -- Python 3.6.10 /opt/hostedtoolcache/Python/3.6.10/x64/bin/python

    def test_subplot_theta_range_normal_non_raise():
        ax = plt.subplot(111, projection='polar')
>       ax.set_thetalim(0, 2 * numpy.pi)
E       NameError: name 'numpy' is not defined

lib/matplotlib/tests/test_subplots.py:198: NameError

@anntzer
Copy link
Contributor Author

anntzer commented Apr 19, 2020

looks like crossed PRs, as usual.

@anntzer
Small cleanups/simplifications/fixes to pie().
5ba9cf1 
- Change the default values of `startangle` and `radius` from "None" to
  the more explicit 0 and 1.
- Apply wedgeprops and textprops separately after the corresponding
  artist has already been instantiated.  This avoids conflicts in case
  e.g. `textprops` contains an "ha" entry (the dict merge won't notice
  that this needs to override "horizontalalignment", but applying the
  user-supplied properties in a second step will "just work".
- Misc. small cleanups.
@anntzer
Copy link
Contributor Author

anntzer commented Apr 19, 2020

retriggered tests.

@QuLogic QuLogic merged commit 3232f58 into matplotlib:master Apr 20, 2020
@anntzer anntzer deleted the pie branch April 20, 2020 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QuLogic QuLogic QuLogic approved these changes

@timhoffm timhoffm timhoffm approved these changes

Assignees
No one assigned
Labels
Maintenance
Projects
None yet
Milestone
v3.3.0
Development

Successfully merging this pull request may close these issues.
3 participants
@anntzer @timhoffm @QuLogic