Skip to content

ENH: Align titles#27952

Merged
rcomer merged 5 commits into
matplotlib:mainfrom
trananso:align-titles
Apr 4, 2024
Merged

ENH: Align titles#27952
rcomer merged 5 commits into
matplotlib:mainfrom
trananso:align-titles

Conversation

@trananso

@trananso trananso commented Mar 20, 2024

Copy link
Copy Markdown
Contributor

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

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

Copy link
Copy Markdown
Contributor Author

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

@rcomer rcomer left a comment

Copy link
Copy Markdown
Member

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.

Comment thread galleries/examples/subplots_axes_and_figures/align_labels_demo.py Outdated
Comment thread lib/matplotlib/figure.py Outdated
Comment thread lib/matplotlib/tests/test_figure.py Outdated
Comment thread doc/users/next_whats_new/figure_align_titles.rst Outdated

@jklymak jklymak left a comment

Copy link
Copy Markdown
Member

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

Comment thread lib/matplotlib/tests/test_figure.py Outdated
Comment thread lib/matplotlib/tests/test_figure.py Outdated
@rcomer

rcomer commented Mar 31, 2024

Copy link
Copy Markdown
Member

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.

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

trananso commented Apr 3, 2024

Copy link
Copy Markdown
Contributor Author

Not sure why this failed

@rcomer

rcomer commented Apr 3, 2024

Copy link
Copy Markdown
Member

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

@jklymak jklymak left a comment

Copy link
Copy Markdown
Member

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

Comment thread galleries/examples/subplots_axes_and_figures/align_labels_demo.py Outdated
@rcomer

rcomer commented Apr 3, 2024

Copy link
Copy Markdown
Member

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.

@trananso

trananso commented Apr 3, 2024

Copy link
Copy Markdown
Contributor Author

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

@ksunden

ksunden commented Apr 4, 2024

Copy link
Copy Markdown
Member

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

rcomer commented Apr 4, 2024

Copy link
Copy Markdown
Member

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

@trananso

trananso commented Apr 4, 2024

Copy link
Copy Markdown
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
@trananso trananso 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