Skip to content

Added axis limit check for non-finite values #7744

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 2 commits into from
Mar 7, 2017
Merged

Added axis limit check for non-finite values #7744

merged 2 commits into from
Mar 7, 2017

Conversation

bcongdon
Copy link
Contributor

@bcongdon bcongdon commented Jan 5, 2017

Addresses #7460.

Raises a ValueError if non-finite values like np.nan or np.inf are passed as arguments to set_xlim or set_ylim.

@dstansby
Copy link
Member

dstansby commented Jan 5, 2017

Should this raise a warning instead of an error? Currently invalid limits on a log axis raises a warning (#7367), which I think is best. Either way either a warning or error should be raised consistently.

@bcongdon
Copy link
Contributor Author

bcongdon commented Jan 5, 2017 •
edited
Loading

Yeah, that sounds like the better approach. Passing an invalid value to plt.xlim results in unexpected behavior, but a plot can still be drawn.

@codecov-io
Copy link

codecov-io commented Jan 5, 2017 •
edited
Loading

Current coverage is 62.12% (diff: 100%)

Merging #7744 into master will increase coverage by <.01%

@@             master      #7744   diff @@
==========================================
  Files           174        174          
  Lines         56028      56032     +4   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          34805      34808     +3   
- Misses        21223      21224     +1   
  Partials          0          0

Powered by Codecov. Last update 015d8d5...e5cc8e1

@tacaswell tacaswell added this to the 2.1 (next point release) milestone Jan 7, 2017
tacaswell
tacaswell requested changes Jan 7, 2017
View reviewed changes
Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

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

Docs and code are now out of sync.

@tacaswell
Copy link
Member

The consensus in #7460 seemed to be in favor of errors, not warnings. It seems that warnings either just annoy people (because they are doing the thing for a good reason) or are ignored.

I think this will still re-set the limits, which was the original complaint.

@anntzer anntzer mentioned this pull request Jan 13, 2017
Closed
@anntzer
Copy link
Contributor

anntzer commented Jan 13, 2017

Please note #7733 (comment).

@bcongdon
Copy link
Contributor Author

I've looked over the discussion on #7733 and related issues. Still a bit confused on how to proceed. I think the issues are somewhat unrelated in that the solution for #7735 - letting invalid values pass through - is actually the cause of the issue this PR addresses (#7460).

I've reset back to the commit that raises errors instead of warnings.

@dstansby
Copy link
Member

dstansby commented Feb 5, 2017

I think the consensus is to raise an error, which sounds good to me (see #7460). @bcongdon could you rebase this on to the current master branch? Otherwise it looks good to go.

@bcongdon
Copy link
Contributor Author

bcongdon commented Feb 5, 2017

Rebased. 👍

dstansby
dstansby previously requested changes Feb 5, 2017
View reviewed changes
lib/matplotlib/tests/test_axes.py
@@ -4931,3 +4931,15 @@ def test_bar_single_height():
ax.bar(range(4), 1)
# Check that a horizontal chart with one width works
ax.bar(0, 1, bottom=range(4), width=1, orientation='horizontal')


def test_invalid_axis_limits():
Copy link
Member

Choose a reason for hiding this comment

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

One last thing, could a @cleanup decorator go above this line. Otherwise looks good!

Copy link
Member

Choose a reason for hiding this comment

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

Not needed on current master.

QuLogic
QuLogic reviewed Feb 5, 2017
View reviewed changes
lib/matplotlib/axes/_base.py Outdated
if ((left is not None and not np.isfinite(left)) or
(right is not None and not np.isfinite(right))):
raise ValueError("xlim limits must be finite. "
"instead, found: (%s, %s)" % (left, right))
Copy link
Member

Choose a reason for hiding this comment

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

'xlim limits' is a bit redundant. Also, 'instead' comes after a period so should be capitalized. Something like 'Specified x limits must be finite; instead found: (%s, %s)'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, sounds good. That redundancy bothered me too, but I couldn't think of the right verbiage to describe the warning. Thanks!

lib/matplotlib/axes/_base.py Outdated
if ((top is not None and not np.isfinite(top)) or
(bottom is not None and not np.isfinite(bottom))):
raise ValueError("ylim limits must be finite. "
"instead, found: (%s, %s)" % (top, bottom))
Copy link
Member

Choose a reason for hiding this comment

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

Same comment here, but also, 'bottom' should come before 'top'.

QuLogic
QuLogic reviewed Feb 6, 2017
View reviewed changes
lib/matplotlib/axes/_base.py Outdated
@@ -3176,7 +3176,7 @@ def set_ylim(self, bottom=None, top=None, emit=True, auto=False, **kw):

if ((top is not None and not np.isfinite(top)) or
(bottom is not None and not np.isfinite(bottom))):
raise ValueError("ylim limits must be finite. "
raise ValueError("Specified y limits must be finite; "
"instead, found: (%s, %s)" % (top, bottom))
Copy link
Member

Choose a reason for hiding this comment

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

Still need to swap top and bottom.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, my bad.

@QuLogic QuLogic dismissed dstansby’s stale review February 6, 2017 22:51

Not needed on master.

@QuLogic
Copy link
Member

QuLogic commented Feb 6, 2017

@tacaswell To what documentation are you referring in your review? Can it be dismissed?

@QuLogic QuLogic changed the title Added axis limit check for non-finite values [MRG+1] Added axis limit check for non-finite values Feb 26, 2017
@QuLogic
Copy link
Member

QuLogic commented Feb 26, 2017

Ping @tacaswell?

@tacaswell tacaswell merged commit 1d45c08 into matplotlib:master Mar 7, 2017
This was referenced Mar 7, 2017
Merged
Closed
@QuLogic QuLogic changed the title [MRG+1] Added axis limit check for non-finite values Added axis limit check for non-finite values Mar 7, 2017
@QuLogic QuLogic mentioned this pull request Apr 13, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tacaswell tacaswell tacaswell approved these changes

@QuLogic QuLogic QuLogic approved these changes

@dstansby dstansby dstansby left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v2.1
Development

Successfully merging this pull request may close these issues.
6 participants
@bcongdon @dstansby @codecov-io @tacaswell @anntzer @QuLogic