Skip to content

Deprecate apply_theta_transforms=True to PolarTransform #24834

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Jan 3, 2024
Merged

Deprecate apply_theta_transforms=True to PolarTransform #24834

merged 7 commits into from
Jan 3, 2024
+75 −26

Conversation

dstansby
Copy link
Member

@dstansby dstansby commented Dec 28, 2022 •
edited
Loading

PR Summary

This parameter was introduced in #4699 for backwards compatabilty reasons, when shifting theta values was refactored out of PolarTransform.

  • It think it's reasonable to not expect PolarTransform to do a theta shift first
  • We do not currently test the case _apply_theta_transforms = True
  • Removing this will simplify the polar transform code slightly

So I think it's worth deprecating and eventually removing this.

PR Checklist

Documentation and Tests

  • Has pytest style unit tests (and pytest passes)
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • New plotting related features are documented with examples.

Release Notes

  • New features are marked with a .. versionadded:: directive in the docstring and documented in doc/users/next_whats_new/
  • API changes are marked with a .. versionchanged:: directive in the docstring and documented in doc/api/next_api_changes/
  • Release notes conform with instructions in next_whats_new/README.rst or next_api_changes/README.rst

@dstansby dstansby added this to the v3.7.0 milestone Dec 28, 2022
@dstansby dstansby marked this pull request as draft December 28, 2022 23:14
@dstansby dstansby changed the title Deprecate _apply_theta_transforms parameter to PolarTransform Deprecate _apply_theta_transforms=True to PolarTransform Dec 28, 2022
@dstansby dstansby force-pushed the _apply_theta_transforms branch 4 times, most recently from 6d7d585 to 9bed666 Compare January 1, 2023 19:24
@anntzer
Copy link
Contributor

anntzer commented Jan 3, 2023

InvertedPolarTransform also needs this?

@tacaswell tacaswell modified the milestones: v3.7.0, v3.8.0 Jan 5, 2023
@tacaswell
Copy link
Member

Pushing to 3.8 as I do not think this is critical to get in and is still draft. Please push back if I am incorrect @dstansby .

@dstansby dstansby force-pushed the _apply_theta_transforms branch 2 times, most recently from b1c7cf4 to deeb925 Compare January 10, 2023 15:38
@dstansby dstansby force-pushed the _apply_theta_transforms branch from deeb925 to b91198f Compare January 15, 2023 17:52
@dstansby dstansby force-pushed the _apply_theta_transforms branch from b91198f to b19f15e Compare January 26, 2023 17:54
@dstansby dstansby marked this pull request as ready for review January 26, 2023 17:54
@efiring efiring requested a review from QuLogic February 16, 2023 20:37
@tacaswell
Copy link
Member

We talked about this on the call this week and we felt we did not have enough context to understand why we want to do this and concern about exposing an underscored arg name to the users.

There was also surprise about the claim that this was not tested given it is the default!

Do we want to make this public if we are going to document it like this? Should we just have a second class?

@jklymak
Copy link
Member

jklymak commented Feb 16, 2023

I guess the point is that a polar axes sets this to False, which is our internal use. So this would only affect power users who are using PolarTransform directly? OTOH, it seems we should have a public toggle for this, and document it?

@dstansby
Copy link
Member Author

I think the original intention was to keep the 'version' of the class where no theta transform was applied private, but for backwards API compatability keep a version where a theta transform was applied. So I think making the argument public and going through a deprecation as is in this PR might be a reasonable way forward?

The motivation for this was to make PolarTransform internal code less complicated for maintenance reasons. I figured that there probably aren't lots of people out there using this, and there's a clear and simple alternative to _apply_theta_transforms=True (just add a theata value to the outputs), so it was worth doing.

@dstansby dstansby changed the title Deprecate _apply_theta_transforms=True to PolarTransform Deprecate apply_theta_transforms=True to PolarTransform Feb 17, 2023
@dstansby dstansby force-pushed the _apply_theta_transforms branch from bfce982 to 7f0847c Compare February 18, 2023 09:08
jklymak
jklymak approved these changes Feb 20, 2023
View reviewed changes
Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is OK since this is probably only used by developers, if anyone. Maybe not the nicest dance, but improves maintainability.

QuLogic
QuLogic reviewed Jun 24, 2023
View reviewed changes
Copy link
Member

@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a rebase now.

doc/api/next_api_changes/deprecations/24834-DS.rst Outdated
To silence this warning and adopt future behaviour,
set ``apply_theta_transforms=False``. If you need to retain the behaviour
where theta values are transformed, chain the ``PolarTransform`` with
another transform that performs the theta shift and/or sign shift.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add an example of which transform to use?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 added a note that an Affine2D can be used to do this.

@ksunden ksunden modified the milestones: v3.8.0, v3.9.0 Aug 8, 2023
@dstansby dstansby marked this pull request as draft December 28, 2023 17:26
@dstansby dstansby force-pushed the _apply_theta_transforms branch from 7f0847c to c1b80c5 Compare December 28, 2023 17:26
@dstansby dstansby force-pushed the _apply_theta_transforms branch from c77a03c to ee48fb3 Compare December 28, 2023 21:03
@dstansby dstansby marked this pull request as ready for review December 29, 2023 10:31
@QuLogic QuLogic merged commit 8703dc5 into matplotlib:main Jan 3, 2024
@dstansby dstansby deleted the _apply_theta_transforms branch January 3, 2024 10:08
@dstansby dstansby mentioned this pull request May 3, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tacaswell tacaswell tacaswell left review comments

@QuLogic QuLogic QuLogic approved these changes

@jklymak jklymak jklymak approved these changes

Assignees
No one assigned
Labels
topic: polar
Projects
None yet
Milestone
v3.9.0
Development

Successfully merging this pull request may close these issues.
6 participants
@dstansby @anntzer @tacaswell @jklymak @QuLogic @ksunden