Skip to content

Navigation Menu

Sign in

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
Appearance settings

Fix center of rotation with rotation_mode='anchor' #29199

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
Open
wants to merge 1 commit into
base: main
Choose a base branch
Loading
from
Open

Fix center of rotation with rotation_mode='anchor' #29199

wants to merge 1 commit into from

Conversation

WPurre
Copy link

@WPurre WPurre commented Nov 27, 2024

PR summary

This will close #13044.

I see that there is a pull request already #20057, however that has not been active for a long time and has since been closed.
I believe that they correctly identified the cause of the bug in that pull request but that their implementations wasn't quite right.
As such this pull request picks up where they left off.

I will be using the following code from the issue as a way of showing the result of the code:

import matplotlib.pyplot as plt

fig, axes = plt.subplots(2, 2)
for idx, ax in enumerate(axes.flatten()):
      ax.axvline(.5, linewidth=.5, color='.5')
      ax.axhline(.5, linewidth=.5, color='.5')
      ax.text(
          .5, .5, 'pP', size=50,
          bbox=dict(facecolor='None', edgecolor='red', pad=0),
          rotation=idx*90,
          va='center_baseline',
          rotation_mode='anchor'
      )

This code produces the following plot:
image

After the changes the plot changes to:
result

Showing that the text now properly rotates around the point (0.5, 0.5) instead of the leftmost side of the text.
We can also see that the text more properly aligns with the bounding box.

As was diagnosed in #20057 the issue is caused by the offset of the text not being rotated with the rest of the text, causing the text to rotate around itself rather than the point it should be rotating around.

This understandably causes some tests to fail since rotated text will look slightly different.
Here is an example of what the difference looks like for a failed test, along with the produced image:
difference_constrained_layout2
image

As far as I can tell all of the failed tests are of this nature, rotated text being moved slightly due to the new logic.
The only test that differs is tight_layout1 where the images look like this:

difference_tight_layout1
image

I believe that the difference in the line is caused by the available space being changed by moving the text but would love feedback on this.

PR checklist

github-actions[bot]
github-actions bot reviewed Nov 27, 2024
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

@ksunden
Copy link
Member

ksunden commented Dec 4, 2024

Empirically, I think I agree that the changed behavior is better (at least with the main example provided here, and don't have any reason to suspect it is special).

However, the main hangup with the previous PR appears to be a request for an explanation of why the offset math works out this way in the comments (phrased as a request for a "brief proof"), which I do not think this PR provides at this time.

If I read the code correctly the primary idea is that you are adjusting the anchor point by the applied rotation. The comment that exists "Standard 2D rotation transformation" does not in my opinion adequately explain it's connection to the anchor point. The change in sign in the rounding line is also mildly confusing to me (not saying it is wrong, just not sure why it changed)

Finally, yes, before this can be merged, the tests will have to be updated so they are passing. I don't love the quantity (or magnitude) of changes, as we do try to have stable outputs. However, correctness is certainly a good reason to change them.

I'm not sure why the one test changes the line at all... if it was positioning/spacing of the axes I'd have expected the axis frame to have differences in the diff output. Further confused why it has a gradient of magnitude of the change.

@WPurre WPurre force-pushed the fixing-rotation-bug branch 2 times, most recently from f26ed0e to 5739086 Compare December 8, 2024 14:51
@WPurre
Copy link
Author

WPurre commented Dec 8, 2024

Thank you for the comment.

I've updated the code to hopefully adequately explain the logic behind the changes.
Regarding the failing tests, should I include the updated images in this pull request or is there some other procedure for how to update them (assuming that this is the correct solution and that they should be changed)?

What's the best thing to do about the tight_layout test? I don't see why the image is changed but at the same time I see no issues with other plots that I think would be present if my changes did something weird with the plotting logic.

@QuLogic
Copy link
Member

QuLogic commented Dec 12, 2024

I'm not sure why the one test changes the line at all... if it was positioning/spacing of the axes I'd have expected the axis frame to have differences in the diff output. Further confused why it has a gradient of magnitude of the change.

That's a tight layout test; when the text moves a little bit, so does everything else. It's fine so long as the text movement is expected.

Regarding the failing tests, should I include the updated images in this pull request or is there some other procedure for how to update them (assuming that this is the correct solution and that they should be changed)?

Yes, you should update the test images in the same PR. You can use tools/triage_tests.py to check them.

@WPurre WPurre force-pushed the fixing-rotation-bug branch from 5739086 to 14dc50d Compare December 12, 2024 13:01
@WPurre WPurre force-pushed the fixing-rotation-bug branch from 14dc50d to 8269aa0 Compare December 12, 2024 13:51
@WPurre WPurre force-pushed the fixing-rotation-bug branch from 8269aa0 to 6009467 Compare December 12, 2024 16:21
@WPurre
Copy link
Author

WPurre commented Dec 12, 2024

Yes, you should update the test images in the same PR. You can use tools/triage_tests.py to check them.

I've now updated the images to the new ones. Thank you for the information.

scottshambaugh
scottshambaugh approved these changes Jan 30, 2025
View reviewed changes
Copy link
Contributor

@scottshambaugh scottshambaugh left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think this looks great, thank you for the fix!

  • The math looks good, and the comments explain what's happening
  • Your plots to show the bounding box change make sense and show the problem & solution well
  • I think the difference is most noticeable in the test contour labels plot, where the new labels looks noticeably better centered and spaced.

I would recommend this be released with v3.11.0 rather than v3.10.2, since it's a minor visual bug that may impact a large number of downstream image tests.

@scottshambaugh scottshambaugh added this to the v3.11.0 milestone Jan 30, 2025
@tacaswell
Copy link
Member

Can you add a new image test which is the reproducer from this issue? Although any future changes to this will clearly cause a bunch of failures, having one very clear test case is still very valuable.

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

@github-actions github-actions[bot] github-actions[bot] left review comments

@scottshambaugh scottshambaugh scottshambaugh approved these changes

Assignees
No one assigned
Labels
backend: agg status: needs rebase topic: mplot3d
Projects
First Time Contributors
Status: Needs review
Milestone
v3.11.0
Development

Successfully merging this pull request may close these issues.

Center of rotation for text with rotation_mode='anchor'
5 participants
@WPurre @ksunden @QuLogic @tacaswell @scottshambaugh
Morty Proxy This is a proxified and sanitized view of the page, visit original site.