-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
ENH: 3D Transforms #28098
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
base: main
Are you sure you want to change the base?
ENH: 3D Transforms #28098
Conversation
492d16c
to
4ea7cce
Compare
Thanks for starting working on this. We cannot really rename classes in this way as that will break existing user code, and, hence, there should be a transition path to the new name over a number of releases. It is not really clear to me exactly how this should be done, if it is enough to just keep the old classes, basically subclassed from the new one but with an added deprecation warning, something like: class AffineBase2D(AffineImmutable):
__init__(...):
# emit deprecation warning
super().__init__(...) Or if something more elaborate is required. Do you expect the following to work from a practical perspective? That is, can AffineBase2D be directly replaced with AffineImmutable in exisiting code? I ping @timhoffm as the API lead to see if there is some input. |
Thanks for taking a look at this! I don't see any obstacles to having the old classes subclassing the new ones, since these classes keep their 2D behavior if the Where things get a bit messy, is with the transform factory functions |
|
0223035
to
25e479a
Compare
I do not really know how the factory classes work, but I see your point that they should probably be used. However, the standard policy is that anything that is documented should not be suddenly removed. A bit of information is available here: https://matplotlib.org/devdocs/devel/api_changes.html (Edit: once 3.9 is released, I expect more experienced developers to chip in.) |
86ce4ec
to
74dbb4f
Compare
I'm not enough of a transform expert to have a definite opinion. I can see that If I was going to do this, I'd either
Other core devs with more transforms insight may override my opinion if they are sure, we're on the right path here. |
983b318
to
97c7da7
Compare
c74fa89
to
ae05fe1
Compare
ae05fe1
to
04bda58
Compare
ef0c3e5
to
43c8913
Compare
ddcfbac
to
af5946b
Compare
711ce8a
to
c99765f
Compare
c99765f
to
fcfe8b1
Compare
7d5014a
to
1c1eb22
Compare
I chose to build on top of this PR, and I've been slowly working on it in the past few weeks. So far, I've implemented 3D transforms and converted I think that the current issue is that |
Speaking with familiarity on the 3D transforms but not as much on the 2D ones: This is really excellent foundational work, I think it much improves the 3D transformations. It's not totally clear to me yet how this enables the non-affine log scales, but I think the switch to commonality with the 2D transform framework is worth it on its own. Agreed that I think getting the 3D transforms in the same Could you mark the different functions in
I'll keep thinking about this, though I'm not sure I have enough of the full picture of how everything works to have a good opinion. |
From my understanding, log scales in the 2D case works with the I'll work on adding the deprecations for |
5daa70b
to
6b55a0f
Compare
PR summary
Currently, mplot3d tacks on a 3rd dimension to the 2D transformation system used in matplotlib. This is because the Transform system does not allow for more than 2 dimensions.
This pull request aims to be a first step at addressing issue #209, by generalizing Transform classes to allow for three-dimensional transforms.
dims=2
keyword argument toAffine2DBase
. Renamed toAffineImmutable
BlendedAffine2D
accepts more than two transforms, with each transform handling an additional axis. The number of transform arguments provided indicates the dimension of the transform. Renamed toBlendedAffine
.CompositeAffine2D
can compose transforms of any dimension, as long as the dimensions of the transforms match. Renamed toCompositeAffine
.PR checklist