-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
ENH: Align titles #27952
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
ENH: Align titles #27952
Conversation
3712c0f
to
8af2b25
Compare
Hello, would appreciate having someone look over this PR whenever you have the chance. Thanks! |
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.
Thanks for your work on this @AnsonTran, I think it's looking great! I just have some minor comments.
galleries/examples/subplots_axes_and_figures/align_labels_demo.py
Outdated
Show resolved
Hide resolved
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.
I think it's a valid question if align_labels should also align the titles
I don't have an opinion on whether it should but, if it did, I guess it would need to be optional at least for a deprecation period. I don't think we necessarily need to decide within this PR as it could be done as a follow-up. |
5e09660
to
6c8e5d5
Compare
Not sure why this failed |
That test is currently failing on all the PRs. #28007 is trying to fix it. |
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.
Looks good except for one nit
galleries/examples/subplots_axes_and_figures/align_labels_demo.py
Outdated
Show resolved
Hide resolved
The image test is unfortunately failing for a platform that was added since the previous commit #27723. I downloaded the test images and the diff looks like this: |
I see... Is it just on the new test? or other image comparison tests as well? |
If I shift the levels to increase the contrast, we see that there are very slight differences in the right hand plot surrounding the drawn line: These differences are not meaningful, and are likely due to things like rounding modes at the hardware level (it is failing on macos ARM machines). We had to add tolerances to ~50 image tests in #27723 for similar reasons. This is not that unexpected, just some (usually floating point) calculations are not quite exact and so lead to such things. 0.02 is a pretty low RMS. Here is an example of adding such tolerance: The number needs to be at least as big as the number stated in the failure, so with a small buffer I might use something like 0.022 for this test (which says 0.020). |
Hi @AnsonTran we prefer rebase rather than merge to get the branch up-to-date with Edit: in case I'm not clear
|
Yes sorry about that, just wanted to figure out what caused the codecov to fail |
PR summary
Adds a function
Figure.align_titles
, that aligns titles of axes on the same row.Closes #22376. Continuation of PRs #25591, #22793
PR checklist