Added axis limit check for non-finite values #7744

bcongdon · 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 · 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 · Jan 5, 2017

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 · Jan 5, 2017

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 · Jan 7, 2017

tacaswell requested changes Jan 7, 2017

View changes

Docs and code are now out of sync.

tacaswell · Jan 7, 2017

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 · Jan 13, 2017

Please note #7733 (comment).

bcongdon · Jan 20, 2017

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 · 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 · Feb 5, 2017

Rebased. 👍

dstansby · Feb 5, 2017

dstansby reviewed Feb 5, 2017

View changes

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

Not needed on current master.

QuLogic · Feb 5, 2017

QuLogic reviewed Feb 5, 2017

View changes

'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)'.

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

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

QuLogic · Feb 6, 2017

QuLogic reviewed Feb 6, 2017

View changes

Still need to swap top and bottom.

Whoops, my bad.

QuLogic · Feb 6, 2017

$@#{review_dismissed_event.actor.login}$ QuLogic dismissed dstansby’s review Feb 6, 2017

Not needed on master.

QuLogic · Feb 6, 2017

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

QuLogic · Feb 26, 2017

QuLogic approved these changes Feb 26, 2017

View changes

QuLogic · Feb 26, 2017

Ping @tacaswell?

tacaswell · Mar 7, 2017

tacaswell approved these changes Mar 7, 2017

View changes

tacaswell added this to the 2.1 (next point release) milestone Jan 7, 2017

anntzer referenced this pull request Jan 13, 2017
Open
Trying to set_ylim(bottom=0) on a log scaled axis changes plot #7733

QuLogic changed the title from Added axis limit check for non-finite values to [MRG+1] Added axis limit check for non-finite values Feb 26, 2017

This was referenced Mar 7, 2017

Merged

Fixed Issue #7460: Raised an error if argument to xlim is invalid #8022

Closed

Raise errors on invalid axis limits #8216

QuLogic changed the title from [MRG+1] Added axis limit check for non-finite values to Added axis limit check for non-finite values Mar 7, 2017

QuLogic referenced this pull request Apr 13, 2017
Closed
Raise error if argument to xlim is invalid, e.g., nan #7460

@@ -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():

+    +        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))

+    +        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))

@@ -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))

matplotlib/matplotlib

Added axis limit check for non-finite values #7744

bcongdon commented Jan 5, 2017

dstansby commented Jan 5, 2017

bcongdon commented Jan 5, 2017 • edited

codecov-io commented Jan 5, 2017 • edited

tacaswell added this to the 2.1 (next point release) milestone Jan 7, 2017

tacaswell requested changes Jan 7, 2017

View changes

tacaswell commented Jan 7, 2017

anntzer referenced this pull request Jan 13, 2017

Trying to set_ylim(bottom=0) on a log scaled axis changes plot #7733

anntzer commented Jan 13, 2017

bcongdon commented Jan 20, 2017

dstansby commented Feb 5, 2017

bcongdon commented Feb 5, 2017

dstansby reviewed Feb 5, 2017

View changes

QuLogic reviewed Feb 5, 2017

View changes

QuLogic reviewed Feb 6, 2017

View changes

$@#{review_dismissed_event.actor.login}$ QuLogic dismissed dstansby’s review Feb 6, 2017

QuLogic commented Feb 6, 2017

QuLogic approved these changes Feb 26, 2017

View changes

QuLogic changed the title from Added axis limit check for non-finite values to [MRG+1] Added axis limit check for non-finite values Feb 26, 2017

QuLogic commented Feb 26, 2017

tacaswell approved these changes Mar 7, 2017

View changes

tacaswell merged commit `1d45c08` into matplotlib:master Mar 7, 2017
4 checks passed

4 checks passed

This was referenced Mar 7, 2017

Fixed Issue #7460: Raised an error if argument to xlim is invalid #8022

Raise errors on invalid axis limits #8216

QuLogic changed the title from [MRG+1] Added axis limit check for non-finite values to Added axis limit check for non-finite values Mar 7, 2017

QuLogic referenced this pull request Apr 13, 2017

Raise error if argument to xlim is invalid, e.g., nan #7460

matplotlib/matplotlib

Join GitHub today

Added axis limit check for non-finite values #7744

Conversation

bcongdon commented Jan 5, 2017

dstansby commented Jan 5, 2017

bcongdon commented Jan 5, 2017 • edited

codecov-io commented Jan 5, 2017 • edited

Current coverage is 62.12% (diff: 100%)

tacaswell added this to the 2.1 (next point release) milestone Jan 7, 2017

tacaswell requested changes Jan 7, 2017 View changes

tacaswell commented Jan 7, 2017

anntzer referenced this pull request Jan 13, 2017

anntzer commented Jan 13, 2017

bcongdon commented Jan 20, 2017

dstansby commented Feb 5, 2017

All checks have passed

bcongdon commented Feb 5, 2017

dstansby reviewed Feb 5, 2017 View changes

QuLogic reviewed Feb 5, 2017 View changes

QuLogic reviewed Feb 6, 2017 View changes

All checks have passed

QuLogic dismissed dstansby’s review Feb 6, 2017

QuLogic commented Feb 6, 2017

QuLogic approved these changes Feb 26, 2017 View changes

QuLogic changed the title from Added axis limit check for non-finite values to [MRG+1] Added axis limit check for non-finite values Feb 26, 2017

QuLogic commented Feb 26, 2017

tacaswell approved these changes Mar 7, 2017 View changes

Hide details View details tacaswell merged commit 1d45c08 into matplotlib:master Mar 7, 2017 4 checks passed

4 checks passed

This was referenced Mar 7, 2017

QuLogic changed the title from [MRG+1] Added axis limit check for non-finite values to Added axis limit check for non-finite values Mar 7, 2017

QuLogic referenced this pull request Apr 13, 2017

tacaswell requested changes Jan 7, 2017

View changes

dstansby reviewed Feb 5, 2017

View changes

QuLogic reviewed Feb 5, 2017

View changes

QuLogic reviewed Feb 6, 2017

View changes

$@#{review_dismissed_event.actor.login}$ QuLogic dismissed dstansby’s review Feb 6, 2017

QuLogic approved these changes Feb 26, 2017

View changes

tacaswell approved these changes Mar 7, 2017

View changes

tacaswell merged commit `1d45c08` into matplotlib:master Mar 7, 2017
4 checks passed