-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Raise exception if negative height or width is passed to axes() #7783
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.
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
Conversation
@@ -483,6 +483,9 @@ def __init__(self, fig, rect, | ||
self._position = rect | ||
else: | ||
self._position = mtransforms.Bbox.from_bounds(*rect) | ||
x0, y0, x1, y1 = self._position.extents | ||
if ((x1 - x0) < 0 or (y1 - y0) < 0): |
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.
You can use self._position.width < 0 or self._position.height < 0
.
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 for the tip!
@@ -483,6 +483,8 @@ def __init__(self, fig, rect, | ||
self._position = rect | ||
else: | ||
self._position = mtransforms.Bbox.from_bounds(*rect) | ||
if self._position.width < 0 or self._position.height < 0: | ||
raise ValueError('Height and width specified must be non-negative') |
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.
Very minor point: probably "Width and height must be non-negative" is better (as the order is x, y). I wouldn't add "specified" but I'll let a native speaker chime in :p
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'm a native speaker ;p
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.
That's fine, the point about order still remains valid :p
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 would suggest squashing the commits too.)
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.
Got it!
Commits squashed! |
Current coverage is 62.11% (diff: 50.00%)@@ master #7783 diff @@
==========================================
Files 174 174
Lines 56028 56030 +2
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
- Hits 34805 34804 -1
- Misses 21223 21226 +3
Partials 0 0
|
LGTM. |
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.
LGTM. Thanks for the contribution! 🎉
Fixes #7487