-
-
Notifications
You must be signed in to change notification settings - Fork 8k
Fix the vertical alignment of overunder symbols. #23209
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
Conversation
When an overunder symbol has a subscript, it goes below the symbol, and the baseline of the Vlist thus created is actually the baseline of the lowest item in the Vlist. Hence, it must be moved only if there is a subscript, and not otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice with some tests (primarily so that this isn't broken later), but I cannot really figure out a way to do that without adding yet another set of images (which should be avoided to reduce the git load). I think I'll open an issue an try to collect things to put in new tests.
I don't think we should be avoiding image tests where they are needed, like this PR. |
I was quite sure that I recently saw a PR going in which some tests were removed, so I was about to give you some input on what else to add in the test, but I cannot seem to find it now... I must admit that I do not really understand when test images should be added and not. |
There are lots of things that can be tested with "figures_equal" or with hard coded value tests. Other things, however, really require a render to test. In this case I don't think there is anyway to test that the rendering is correct without an image test. |
All right, I'll add a test here with Should the test go in |
Not an expert here, but I think there should be no differences between fonts, so I hope this helps with writing the image comparison test: |
I added the test. Is there anything else to be done? |
You are correct in that it should not be any difference, but I think there is (at least based on other examples). However, I still think this is enough as things like spacing is not really what it could be. The PR solves the problem it claims to do, so I think it is good to go. |
Hi-five ✋ on merging your first pull request to Matplotlib, @tfpf ! We hope you stick around and invite you to continue to take an active part in Matplotlib! Your choices aren’t limited to programming 😉 – you can review pull requests, help us stay on top of new and old issues, develop educational material, refresh our documentation, work on our website, create marketing materials, translate website content, write grant proposals, and help with other fundraising initiatives. For more info, check out: https://matplotlib.org/stable/devel/index f you haven’t yet, do join us on gitter and discourse 🗣. The former is a chat platform, which is great when you have any questions on process or how to fix something, the latter is a forum which is useful for longer questions and discussions. Last but not least, we have a monthly meeting 👥 for new contributors and a weekly meeting for the maintainers, everyone is welcome to join both! You can find out more about our regular project meetings in this calendar page. |
…209-on-v3.5.x Backport PR #23209 on branch v3.5.x (Fix the vertical alignment of overunder symbols.)
Thanks! Yep, will most certainly hang around! |
This fixes #18085.
When an overunder symbol has a subscript, it goes below the symbol, and the baseline of the
Vlist
thus created is actually the baseline of the lowest item in theVlist
. Hence, it must be moved only if there is a subscript, and not otherwise.PR Summary
MWE derived from the above-mentioned issue.
Before:

After:

(Admittedly, the spacing could be much better. It is just right if there is no subscript and superscript, but not otherwise.)
PR Checklist
Tests and Styling
pytest
passes).flake8-docstrings
and runflake8 --docstring-convention=all
).Documentation
doc/users/next_whats_new/
(follow instructions in README.rst there).doc/api/next_api_changes/
(follow instructions in README.rst there).