feat: add dunder method for math operations on Axes Size divider #27938
Conversation
There was a problem hiding this comment.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
| def __mul__(self, other): | ||
| return Fraction(self, other) |
There was a problem hiding this comment.
Looking at the implementation of Fraction this does not work. I'm sorry, this is not well documented, but the Fraction signature is def __init__(self, fraction : numbers.Real, ref_size : _Base), meaning a fraction of the given size. So 0.3 * mysize is 30% of mysize. If anything, you'd have to return Fraction(other, self) as well. But since this is basically a scaling factor, it may be better to only accept the factor on the left.
| def __div__(self, other): | ||
| return Fraction(self, 1/other) |
There was a problem hiding this comment.
Similar to above mysize / 2 is half of mysize. Here, it clearly does not make sense to divide something by a size. So this should not be added.
|
Thanks for the early review! Following your comments, I added some type checking for scaling operations. I do think that allowing multiplication on the right (or div) is nice to have (especially division): Typically to split an axis equally in n part: divider.set_horizontal([Size.AxesX(ax) / n for _ in range(n)] |
|
Rather than |
| return Add(self, Fixed(other)) | ||
|
|
||
| def __sub__(self, other): | ||
| if isinstance(other, _Base): |
There was a problem hiding this comment.
I think __sub__ can just be defined (in all cases, including the exception-raising one) as self + (-other)?
|
The pre-commit failure is real (typo; "subtraction", not "substraction"); also, can you squash the commits? |
d17dbbb to
9468f6d
Compare
|
Thanks for the PR! |
PR summary
Closes #27930. This adds dunder methods (mul, and divide notably) to make interacting with
axes_grid1.Sizeobject better.The original code for Size object interaction is not tested, so I don't know how to proceed to add some test for these changes. Overall, the direct usage of
Sizeobjects is quite niche, so I seek guidance as well for the documentation (maybe a new example ?)PR checklist
Thanks in advance!