Some improvements to BoundingBox #92
Conversation
There was a problem hiding this comment.
Left one minor inline comment.
Otherwise looks good to me.
@astrofrog - Do you have time to review or should we just merge?
| @@ -153,14 +164,31 @@ def extent(self): | |||
|
|
|||
| def as_patch(self, **kwargs): | |||
| """ | |||
| Return a Matplotlib patch that represents the bounding box. | |||
| Return a `matplotlib.patches.Patch` that represents the bounding | |||
| box. | |||
There was a problem hiding this comment.
Link to the more explicit matplotlib.patches.Rectangle here instead of matplotlib.patches.Patch?
Put this info in a Returns section?
There was a problem hiding this comment.
@cdeil Thanks! Yes and yes. Done.
| >>> bbox.extent # matplotlib order: (x, y) | ||
| (0.5, 9.5, 1.5, 19.5) | ||
| >>> print(bbox.as_patch()) | ||
| Rectangle(0.5,1.5;9x18) | ||
| """ | ||
|
|
||
| def __init__(self, ixmin, ixmax, iymin, iymax): | ||
| if ixmin > ixmax: |
There was a problem hiding this comment.
Should ixmin be strictly less than ixmax since the upper limit is exclusive? (that is, the smallest possible box is if ixmax == ixmin + 1.
There was a problem hiding this comment.
I think you are essentially asking if we should allow ixmin == ixmax. I left that possibility, but of course it is a BoundingBox of size 0 (along such axis). If we don't want that, then I need to change this line to if ixmin >= ixmax (and the corresponding test for y).
There was a problem hiding this comment.
I don't see any pros / cons at the moment to allowing the size 0 case of ixmin == ixmax.
So I'm merging this now.
@astrofrog and all - feel free to make follow-up PRs proposing changes to this check any time.
|
Thanks, @larrybradley ! |
These are mostly API doc changes. I also added an example for
as_patch().