Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Use left/right top/bottom instead of width/height in Rectangle#9072
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Excellent, I think this is a much better solution. I have two suggestions for improvement:
|
The reason I didn't do it as an image test is because the axis isn't formatted as datetime (an independent bug), so it would only be changed when that bug is fixed. Thanks for the extra test, I'll merge it. |
Right, thisshould be working now... once the tests pass I'll squash down to a couple of commits. |
All good to go! |
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.
Looks reasonable, apart from the question about get_bbox.
lib/matplotlib/patches.py Outdated
| self.stale = True | ||
| def get_bbox(self): | ||
| return transforms.Bbox.from_bounds(self._x, self._y, | ||
| self._width, self._height) | ||
| return transforms.Bbox.from_extents(self._x0, self._y0, |
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.
Doesn't this need to use the unit conversions, as in _update_patch_transform()?
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 guess it probably does, but no-one every added it originally. I'll add it in another commit.
I've also taken the opportunity to clean the docstring, since it needed changing anyway. |
@dstansby this has been reviewed twice, so should be good to go, but there is a conflict. |
Fix up some incorrect bits in RectangleRemember to update x1/y1 when setting x0/y0Add methods to update x1/y1
| self.stale = True | ||
| def get_bbox(self): | ||
| return transforms.Bbox.from_bounds(self._x, self._y, | ||
| self._width, self._height) | ||
| x0, y0, x1, y1 = self._convert_units() |
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.
Why this call toconvert_units? Am I missing a side effect or was this meant to be passed to thefrom_extents call?
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.
The_x0,_y0 etc. coords are now all stored in the class with units attached, so this strips the units because I don't thinkfrom_extents takes units.
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.
but the next line passes inself._x0 etc notx0.
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.
Ah, I see. PR incoming
This changes the transform in
Rectangleto usetransforms.Bbox.from_extentsinstead oftransforms.Bbox.from_bounds. This allows for rectangles whose "width" may be a different type to the actual coordinates (eg.datetimefor coords andtimedeltafor width/height).The api is maintained as before, all that has changed is stuff under the hood.
Fixes#4916, thanks to@pganssle for the idea.