Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Draft
trananso wants to merge9 commits intomatplotlib:main
base:main
Choose a base branch
Loading
fromtrananso:bugfix-logscale3d

Conversation

trananso
Copy link
Contributor

@tranansotrananso commentedApr 17, 2024
edited
Loading

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.

  • Addsdims=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

scottshambaugh and anntzer reacted with hooray emoji
@oscargus
Copy link
Member

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:

classAffineBase2D(AffineImmutable):__init__(...):# emit deprecation warningsuper().__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.

@oscargusoscargus requested a review fromtimhoffmApril 19, 2024 09:32
@trananso
Copy link
ContributorAuthor

trananso commentedApr 19, 2024
edited
Loading

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 thedims keyword isn't provided.I imagine subclasses will continue to inherit fromAffineBase2D instead ofAffineImmutable until the deprecation period is over.

Where things get a bit messy, is with the transform factory functionsblended_transform_factory andcomposite_transform_factory. Which instances should these create?

@trananso
Copy link
ContributorAuthor

trananso commentedApr 19, 2024
edited
Loading

On second thought, should it be safe to renameBlendedAffine2D andCompositeAffine2D, since users are expected to use the factory instead of creating these instances themselves?

@oscargus
Copy link
Member

oscargus commentedApr 19, 2024
edited
Loading

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

@tranansotranansoforce-pushed thebugfix-logscale3d branch 3 times, most recently from86ce4ec to74dbb4fCompareApril 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 thatAnsonTran 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 generalizingAffineBase2D,BlendedAffine2D andCompositeAffine2D 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.

trananso reacted with thumbs up emoji

@tranansotranansoforce-pushed thebugfix-logscale3d branch 2 times, most recently from983b318 to97c7da7CompareApril 21, 2024 22:33
@github-actionsgithub-actionsbot added the Documentation: tutorialsfiles in galleries/tutorials labelApr 21, 2024
@tranansotranansoforce-pushed thebugfix-logscale3d branch 4 times, most recently fromc74fa89 toae05fe1CompareApril 23, 2024 15:46
@trananso
Copy link
ContributorAuthor

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 convertedmplot3d to use them, through theax.M,ax.invM variables. However, it's still sidestepping the transform system used by the rest of the library:ax.transData,ax.transAxes, etc.

I think that the current issue is thatArtist andPath both expect 2D transforms and so require a conversion from 3D to 2D points at some point in the drawing process, and then converting back to 3D. Further integrating 3D transforms may require that matplotlibArtist andPath be refactored to accept them. Would appreciate a second opinion on this!

@scottshambaugh
Copy link
Contributor

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 thisenables 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 sameax.transData andax.transAxes flow as the 2D paths is going to be a big lift, but doesn't have to be solved before this PR I think.

Could you mark the different functions inproj3d as deprecated as you go so we can bookkeep what's remaining? I expect that entire file will be deprecated by the end.

Further integrating 3D transforms may require that matplotlib Artist and Path be refactored to accept them

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.

@trananso
Copy link
ContributorAuthor

trananso commentedMay 14, 2024
edited
Loading

From my understanding, log scales in the 2D case works with theBlendedGenericTransform, by using a 1DLogTransform for each axis. The change that enables this for the 3D case is91d3329, which refactors to allow for more than 2 transforms, with each transform adding another dimension.

I'll work on adding the deprecations forproj3d as I go along

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@timhoffmtimhoffmAwaiting requested review from timhoffm

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

5 participants
@trananso@oscargus@timhoffm@scottshambaugh@rcomer

[8]ページ先頭

©2009-2025 Movatter.jp