X Tutup
Skip to content

Added axis limit check for non-finite values#7744

Merged
tacaswell merged 2 commits intomatplotlib:masterfrom
bcongdon:invalid-xlim-error
Mar 7, 2017
Merged

Added axis limit check for non-finite values#7744
tacaswell merged 2 commits intomatplotlib:masterfrom
bcongdon:invalid-xlim-error

Conversation

@bcongdon
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
Member

tacaswell commented 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 anntzer mentioned this pull request Jan 13, 2017
Closed
@anntzer
Copy link
Copy Markdown
Contributor

anntzer commented Jan 13, 2017

Please note #7733 (comment).

@bcongdon
Copy link
Copy Markdown
Contributor Author

bcongdon commented 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
Copy link
Copy Markdown
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
Copy Markdown
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
ax.bar(0, 1, bottom=range(4), width=1, orientation='horizontal')


def test_invalid_axis_limits():
Copy link
Copy Markdown
Member

@dstansby dstansby Feb 5, 2017

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
Copy Markdown
Member

@QuLogic QuLogic Feb 5, 2017

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
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
Copy Markdown
Member

@QuLogic QuLogic Feb 5, 2017

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
Copy Markdown
Contributor Author

@bcongdon bcongdon Feb 6, 2017

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
Copy Markdown
Member

@QuLogic QuLogic Feb 5, 2017

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
(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
Copy Markdown
Member

@QuLogic QuLogic Feb 6, 2017

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
Copy Markdown
Contributor Author

@bcongdon bcongdon Feb 6, 2017

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
Copy Markdown
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
Copy Markdown
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
X Tutup