Skip to content
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

ENH: 3D Transforms #28098

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Conversation

AnsonTran
Copy link
Contributor

@AnsonTran AnsonTran commented Apr 17, 2024

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.

  • Adds dims=2 keyword argument to Affine2DBase. Renamed to AffineImmutable
  • 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 to BlendedAffine.
  • CompositeAffine2D can compose transforms of any dimension, as long as the dimensions of the transforms match. Renamed to CompositeAffine.

PR checklist

@oscargus
Copy link
Contributor

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.

@oscargus oscargus requested a review from timhoffm April 19, 2024 09:32
@AnsonTran
Copy link
Contributor Author

AnsonTran commented Apr 19, 2024

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 dims keyword isn't provided. I imagine subclasses will continue to inherit from AffineBase2D instead of AffineImmutable until the deprecation period is over.

Where things get a bit messy, is with the transform factory functions blended_transform_factory and composite_transform_factory. Which instances should these create?

@AnsonTran
Copy link
Contributor Author

AnsonTran commented Apr 19, 2024

On second thought, should it be safe to rename BlendedAffine2D and CompositeAffine2D, since users are expected to use the factory instead of creating these instances themselves?

@oscargus
Copy link
Contributor

oscargus commented Apr 19, 2024

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.)

@AnsonTran AnsonTran force-pushed the bugfix-logscale3d branch 3 times, most recently from 86ce4ec to 74dbb4f Compare April 20, 2024 22:30
@timhoffm
Copy link
Member

I ping @timhoffm as the API lead to see if there is some input.

I'm not enough of a transform expert to have a definite opinion. I can see that AnsonTran has put some thought into it and at least navigated 2-3 potential pitfalls 👍. Still, I'm very defensive to touch the existing transforms until (1) we have a complete picture for 3D and (2) we know that generalizing AffineBase2D, BlendedAffine2D and CompositeAffine2D is safe - there may be implicit assumptions that they are 2D in some contexts.

If I was going to do this, I'd either

  • build the 3D transforms in Isolation to see what's really needed (even if that leads temporarily to code duplication). - We can afterwards (possibly before release) merge both transform codes; or
  • build further on top of this PR - either in this PR with additional commits (I suggest to then reasonably squash commits to get multiple logical parts, which can be reviewed independently) or create another PR on top of this one.

Other core devs with more transforms insight may override my opinion if they are sure, we're on the right path here.

@AnsonTran AnsonTran force-pushed the bugfix-logscale3d branch 2 times, most recently from 983b318 to 97c7da7 Compare April 21, 2024 22:33
@github-actions github-actions bot added the Documentation: tutorials files in galleries/tutorials label Apr 21, 2024
@AnsonTran AnsonTran force-pushed the bugfix-logscale3d branch 4 times, most recently from c74fa89 to ae05fe1 Compare April 23, 2024 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants