GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Raise an ValueError when Axes.pie accepts negative values #15923 by Inception95 · Pull Request #15935 · matplotlib/matplotlib

Conversation

@Inception95

@Inception95 Inception95 commented Dec 14, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

PR Summary

Issue #15923
Axes.pie accepts negative values which outputs the wrong pie chart. This issue is caused by not checking the positive and negative of all input wedge sizes of "x" in Axes.pie.

Now it will raise a ValueError, which indicates wedge sizes should be non-negative when feeding negative values into Axes.pie. It is done by adding an if condition right before calculating the portion of all sizes in the "x" `array.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@Inception95
Raise an ValueError when Axes.pie accepts negative values
1e23003
jklymak
jklymak reviewed Dec 14, 2019
View reviewed changes
Comment thread lib/matplotlib/axes/_axes.py Outdated
"and will be removed %(removal)s; pass a 1D array instead.")
x = np.atleast_1d(x.squeeze())

for part in x:

@jklymak jklymak Dec 14, 2019

Copy link
Copy Markdown
Member

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 think you can use np.any?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@Inception95 Inception95 Dec 14, 2019

Copy link
Copy Markdown
Contributor Author

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, np.any(x<0) is neater!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@jklymak

jklymak commented Dec 14, 2019

Copy link
Copy Markdown
Member

Thanks for the PR. This should probably get a test as well...
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Inception95

Inception95 commented Dec 14, 2019

Copy link
Copy Markdown
Contributor Author

@jklymak Sure, I will add the test.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Inception95
Add test code and change negative number detect
d0d64ff
jklymak
jklymak approved these changes Dec 14, 2019
View reviewed changes
@jklymak jklymak added this to the v3.3.0 milestone Dec 14, 2019
@jklymak jklymak added the API: argument checking Validation of acceptable argument values label Dec 14, 2019
anntzer
anntzer approved these changes Dec 14, 2019
View reviewed changes
@anntzer

anntzer commented Dec 14, 2019

Copy link
Copy Markdown
Contributor

Thanks for you contribution, hoping to see you around again :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@anntzer
anntzer merged commit b12c761 into matplotlib:master Dec 14, 2019
@Inception95

Inception95 commented Dec 14, 2019

Copy link
Copy Markdown
Contributor Author

@anntzer Happy to help! Thanks for this great opportunity.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Inception95
Inception95 deleted the pie_takes_negative_values branch December 14, 2019 17:13
@timhoffm timhoffm mentioned this pull request Dec 14, 2019
pie takes negative values #15923
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anntzer anntzer anntzer approved these changes
@jklymak jklymak jklymak approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

API: argument checking Validation of acceptable argument values

Projects

None yet

Milestone

v3.3.0

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@Inception95 @jklymak @anntzer
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.