Repository navigation
Switch transOffset to offset_transform. - #21965
Conversation
dacd3fe to
125b87c
Compare
| if self._offset_trf is None: | ||
| self._offset_trf = transforms.IdentityTransform() | ||
| elif (not isinstance(self._offset_trf, transforms.Transform) | ||
| and hasattr(self._offset_trf, '_as_mpl_transform')): | ||
| self._offset_trf = self._offset_trf._as_mpl_transform(self.axes) |
There was a problem hiding this comment.
I wonder why this sort of thing didn't happen in the setter? Is it some kind of optimization to avoid calling _as_mpl_transform?
If it were in the setter, you wouldn't need offset_trf because they'd all fit in a line even with the full name.
There was a problem hiding this comment.
I can't remember a strong reason for it (though there might have been one). Perhaps there is some delay in getting an axes on a collection, and you can only guarantee it exists when you get_offset_transform?
Certainly worth exploring though.
There was a problem hiding this comment.
Let's defer this to another issue, perhaps?
There was a problem hiding this comment.
OK, I was mostly trying to avoid the offset_trf abbreviation entirely, and not go into performance details just now. Mainly, I don't mind the name within a function, but as the private member, I'd prefer it matched the name of the property/argument.
There was a problem hiding this comment.
Fair point, I renamed to _offset_transform.
There was a problem hiding this comment.
I'd go as far as preferring offset_transform also for local variables (unless you have really bad wrapping issues). trf does not speak to me at all.
There was a problem hiding this comment.
But we already use trf for local variables if quite a few other places, and I'd rather try to establish it as a standard abbreviation (transform is quite a mouthful). Or we can try to standardize on trans, if you prefer?
There was a problem hiding this comment.
I find trans slightly better than trf. I agree that a standard abbreviation is desireable.
Note that most APIs *previously* already accepted *offset_transform* as kwarg, due to the presence of the `set_offset_transform` setter. Prefer that name (shortening it to `offset_trf` for local variables). Backcompat for the old `transOffset` name is kept in most places by introducing a property alias.
QuLogic
left a comment
QuLogic
left a comment
There was a problem hiding this comment.
So long as you work out the name.
|
Let's get this in as is and poll the devs in #22156 :) |
While `offset_transform` is introduced since `matplotlib>=3.6.0`, (matplotlib/matplotlib#21965), `transOffset` can still be used. Also, instead of `_internal_update` (matplotlib/matplotlib#22451), `update` can still be used.
Note that most APIs previously already accepted offset_transform as
kwarg, due to the presence of the
set_offset_transformsetter. Preferthat name (shortening it to
offset_trffor local variables).Backcompat for the old
transOffsetname is kept in most places byintroducing a property alias.
PR Summary
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).