Use left/right top/bottom instead of width/height in Rectangle#9072
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. |
dstansby
force-pushed
the
rect-timedelta
branch
from
b2c5640 to
4cafa2c
Compare
|
Right, this should be working now... once the tests pass I'll squash down to a couple of commits. |
dstansby
force-pushed
the
rect-timedelta
branch
from
4cafa2c to
521754f
Compare
|
All good to go! |
efiring
left a comment
efiring
left a comment
There was a problem hiding this comment.
Looks reasonable, apart from the question about get_bbox.
| 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.
Doesn't this need to use the unit conversions, as in _update_patch_transform()?
There was a problem hiding this comment.
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 Rectangle Remember to update x1/y1 when setting x0/y0 Add methods to update x1/y1
dstansby
force-pushed
the
rect-timedelta
branch
from
229837e to
8ee4da2
Compare
| 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.
Why this call to convert_units? Am I missing a side effect or was this meant to be passed to the from_extents call?
There was a problem hiding this comment.
The _x0, _y0 etc. coords are now all stored in the class with units attached, so this strips the units because I don't think from_extents takes units.
There was a problem hiding this comment.
but the next line passes in self._x0 etc not x0.
8 participants
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.