Add corner coordinate helper methods to Ellipse/Rectangle #21977
Conversation
ericpre
left a comment
There was a problem hiding this comment.
It would be good to add a test with rotation.
2251bd9 to
5930b57
Compare
09dd9aa to
f155550
Compare
|
Sorry for the messy PR... I've dropped edge_centers (I'll add them as private API in the widget PR), and should have resolved all the other comments. |
lib/matplotlib/patches.py
Outdated
| def get_corners(self): | ||
| """ | ||
| Return the corners of the ellipse bounding box, moving anti-clockwise | ||
| from the lower left. |
There was a problem hiding this comment.
Is it really lower left? Can't angle be over 90?
There was a problem hiding this comment.
Yes, it's lower left in the de-rotated frame. I'm not sure what the best way to phrase this is - perhaps just "from the lower left before rotation is applied."?
There was a problem hiding this comment.
Would it better and clear enough to use "anchor point" (xy), as this term is used in the Rectangle patch docstring?
There was a problem hiding this comment.
I don't think that works for Ellipse though, since the "anchor point" is the center.
There was a problem hiding this comment.
Indeed, it isn't sensible for the Ellipse patch - sorry I read too quickly!
Is it worth having a get_corners property as public API for the Ellipse patch? Maybe there could a private API _get_corners for both (Rectangle and Ellipse) and Rectangle patch could have the public get_corners as a convenience property?
f155550 to
195b243
Compare
ericpre
left a comment
There was a problem hiding this comment.
#21977 (comment) could be improved but this is already good.
195b243 to
2f334c8
Compare
PR Summary
This is pulled out of #21945 because it's a standalone feature, and I think it's worth the scrutiny of a separate PR.
PR Checklist
Tests and Styling
pytestpasses).flake8-docstringsand runflake8 --docstring-convention=all).Documentation
doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).