-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Fix/restore secondary axis support for Transform-type functions #27267
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix/restore secondary axis support for Transform-type functions #27267
Conversation
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.
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.
if (isinstance(functions, tuple) and len(functions) == 2 and | ||
callable(functions[0]) and callable(functions[1])): | ||
# make an arbitrary convert from a two-tuple of functions | ||
# forward and inverse. | ||
self._functions = functions | ||
elif isinstance(functions, Transform): | ||
self._functions = ( | ||
functions.transform, functions.inverted().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.
This is dangerous, as Transform.inverted
returns a copy with a snapshot of the transform, and thus won't be in sync if the input transform changes. Instead, you'd probably want (identity - input).
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 agree, but is it preferable to implicitly allow arguments to be mutated after the fact as a side effect at all? I.e., would a better way to ensure consistency be to simply copy the passed transform to begin with? Let me know which way to go (not sure if there's precedent here)
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.
Yes, we tend to be lazily evaluated, e.g., if we had Axes.transData
here, it might change depending on limits (though of course that specific transform won't work here, being 2D, and as you've noted above this argument is 1D.)
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.
Actually, I'm not finding that to be the case (that subtraction avoids inconsistency upon mutation), at least with the Translation
class I defined:
import matplotlib.pyplot as plt
import matplotlib.transforms as mtransforms
class Translation(mtransforms.Transform):
input_dims = 1
output_dims = 1
def __init__(self, dx):
self.dx = dx
def transform(self, values):
return values + self.dx
def inverted(self):
return Translation(-self.dx)
forward = Translation(1)
backward = mtransforms.IdentityTransform() - forward
print(forward.transform(1), backward.transform(1))
forward.dx = 2
print(forward.transform(1), backward.transform(1))
prints
2 0
3 0
Reviewing Transform.__sub__
, I can't actually say I understand how lazy inversion ever occurs... let me know what you think.
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 your test is wrong? The second printout should be 3 -1
, if backward
and forward
are linked (which is what we want here, since the user only supplied the forward 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.
That's my point - forward
and IdentityTransform() - forward
are not linked, and I don't see any mechanism for that operation being lazy in Transform.__sub__
.
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.
Oh, I see what you're saying now. Yes, I misremembered what happened there. What you will have to do is put a lambda
in that calculates the inverse when it's called.
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.
Clever - 2803407
The coverage drop is because it is missing some of the uploads (it is only seeing 5, should be atlesat 9). |
Thank you for you work @zachjweiner and congratulations on your first merged PR to Matplotlib 🎉 We hope to hear from you again. |
Cheers, thanks! (and thanks @QuLogic for iterating on this!) |
PR summary
Closes #25691 and adds a test.
First contribution to matplotlib, so please let me know if I've missed anything.
PR checklist