Skip to content
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

Merged
merged 5 commits into from
Apr 4, 2024
Merged

ENH: Align titles #27952

merged 5 commits into from
Apr 4, 2024

Conversation

AnsonTran
Copy link
Contributor

@AnsonTran AnsonTran commented Mar 20, 2024

PR summary

Adds a function Figure.align_titles, that aligns titles of axes on the same row.

Closes #22376. Continuation of PRs #25591, #22793

Figure_3

Updated gallery example for align labels

PR checklist

@AnsonTran AnsonTran marked this pull request as ready for review March 20, 2024 19:49
@AnsonTran AnsonTran force-pushed the align-titles branch 2 times, most recently from 3712c0f to 8af2b25 Compare March 20, 2024 21:28
@AnsonTran AnsonTran changed the title [ENH] Align titles ENH: Align titles Mar 22, 2024
@AnsonTran
Copy link
Contributor Author

Hello, would appreciate having someone look over this PR whenever you have the chance. Thanks!

Copy link
Member

@rcomer rcomer left a 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.

lib/matplotlib/figure.py Outdated Show resolved Hide resolved
lib/matplotlib/tests/test_figure.py Outdated Show resolved Hide resolved
doc/users/next_whats_new/figure_align_titles.rst Outdated Show resolved Hide resolved
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 it's a valid question if align_labels should also align the titles

lib/matplotlib/tests/test_figure.py Outdated Show resolved Hide resolved
lib/matplotlib/tests/test_figure.py Outdated Show resolved Hide resolved
@rcomer
Copy link
Member

rcomer commented Mar 31, 2024

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.

@AnsonTran AnsonTran force-pushed the align-titles branch 5 times, most recently from 5e09660 to 6c8e5d5 Compare April 1, 2024 18:03
@AnsonTran
Copy link
Contributor Author

Not sure why this failed

@rcomer
Copy link
Member

rcomer commented Apr 3, 2024

That test is currently failing on all the PRs. #28007 is trying to fix it.

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.

Looks good except for one nit

@rcomer
Copy link
Member

rcomer commented Apr 3, 2024

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:
figure_align_titles_tight-failed-diff
I'm very confused how this is a failure.

@AnsonTran
Copy link
Contributor Author

I see... Is it just on the new test? or other image comparison tests as well?

@ksunden
Copy link
Member

ksunden commented Apr 4, 2024

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:

diff_levelshifted

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:

https://github.com/QuLogic/matplotlib/blob/48bc62d6088c262b1b5507981f2948d117e63601/lib/matplotlib/tests/test_contour.py#L167-168

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).

@rcomer
Copy link
Member

rcomer commented Apr 4, 2024

Hi @AnsonTran we prefer rebase rather than merge to get the branch up-to-date with main. Are you happy to do that?

Edit: in case I'm not clear

git fetch upstream
git rebase upstream/main

@AnsonTran
Copy link
Contributor Author

Yes sorry about that, just wanted to figure out what caused the codecov to fail

@rcomer rcomer added this to the v3.9.0 milestone Apr 4, 2024
@rcomer rcomer merged commit 23ea909 into matplotlib:main Apr 4, 2024
44 checks passed
@AnsonTran AnsonTran deleted the align-titles branch April 4, 2024 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ENH]: align_titles
4 participants