NotificationsYou must be signed in to change notification settings
Fork7.9k
Star21.3k

Text rotation patch#19114

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

Jump to bottom

Closed

Alex-Carter01 wants to merge3 commits intomatplotlib:mainfromAlex-Carter01:text-rotation-patch

Closed

Text rotation patch#19114

Alex-Carter01 wants to merge3 commits intomatplotlib:mainfromAlex-Carter01:text-rotation-patch

Conversation

Copy link

Alex-Carter01 commentedDec 14, 2020

PR Summary

Resolves issue#13044. Fixing calculation error of Y-offset for specific rotation mode of text. This pull also includes a proposed minimal test (based off the original bug report).

PR Checklist

Has pytest style unit tests (andpytest passes).
IsFlake 8 compliant (runflake8 on changed files to check).
[N/A] New features are documented, with examples if plot related.
[N/A] Documentation is sphinx and numpydoc compliant (the docs shouldbuild without error).
Conforms to Matplotlib style conventions (installflake8-docstrings and runflake8 --docstring-convention=all).
[N/A] New features have an entry indoc/users/next_whats_new/ (follow instructions in README.rst there).
[N/A] API changes documented indoc/api/next_api_changes/ (follow instructions in README.rst there).

Alex-Carter01 added3 commits

December 14, 2020 17:28

add comparison image for rotation test in test_text

8a6dc24

patch to rotation error in anchor, center_baseline mode

7c939d9

fix to error in calculation of Y offset in text rotation.todo: test and test baseline

add corresponding test for proposed patch to text rotation

762c32e

see baseline_images for corresponding baseline

Copy link

Member

jklymak commentedDec 15, 2020

Thanks a lot for the pull request! Unfortunately this broke some existing tests intest_text.py, so this isn't compatible with the existing behaviour. So you will either have to change the old tests and argue that they were wrong, or modify this....

jklymak added status: needs rebase status: needs revision labels

Apr 6, 2021

Copy link

Member

story645 commentedApr 6, 2021

hey@Alex-Carter01, do you have any bandwidth to finish up this PR?

timhoffm mentioned this pull request

Apr 7, 2021

Center of rotation for text with rotation_mode='anchor'#13044

Open

jklymak added the status: orphaned PR label

Apr 23, 2021

jklymak marked this pull request as draft

April 23, 2021 16:58

Copy link

Member

WeatherGod commentedApr 24, 2021

Oh, and we should double-check with the Cairo renderer, although I suspect it might have the same bug if this is happening at a layer above AGG/Cairo?

safdarzareef mentioned this pull request

Apr 24, 2021

I changed the offset code for draw_text for RendererAgg fixing issue #13044#20057

Closed

7 tasks

Copy link

Member

dstansby commentedDec 18, 2021

Thanks for opening this PR. Unfortunately I don't think this is the correct fix.ymax1 - baseline / 2.0 seems to be the correct logic, putting the offset halfway between the baseline and ymax, so I think subtracting ymin1 is incorrect. As such I'm going to close this PR - if I've missed something feel free to comment and I will open it up again, and thanks again for the contribution.