-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Align titles #22376 #25591
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
Align titles #22376 #25591
Conversation
…e not keeps the title layout
…moved align_titlelabels from align_labels since align_labels should only align the axis labels, not the title itself.
…rrently it is not showing consistent behaviour
Did some code cleanup in align_titles and fixed small typos in docstring.
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.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a while, please feel free to ping @matplotlib/developers
or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
self.canvas.draw_idle() produces more reliable behaviour when generating the plots. Now the titles are alligned every time.
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.
The other error (with stubtest) is because of the recently added type hints. Since you are adding a function, that also needs to be added to figure.pyi
For this it will look like:
def align_titles(self, axs: list[Axes] | None = ...) -> None: ...
And be placed right above align_xlabels
, as it is here.
I'm in the process of writing up better developer docs for these, but they are new enough that we are still working out some details about how the iteration on that idea goes forth (This is in fact the first new method I think since that got added)
The "nbagg" test failures are understood and unrelated to this PR
|
||
|
||
## TODO add image comparison | ||
@image_comparison(['figure_align_titles_param'], extensions=['png', 'svg'], |
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.
The test failures that are directly related to this PR are due to the baseline images for this image comparison not being added to the repo
If it is passing locally, those two files (png, svg) should be added in the baseline_images
folder
lib/matplotlib/tests/test_figure.py
Outdated
axs[0][1].set_title('Title2', loc="left") | ||
fig.align_titles() | ||
fig.savefig("./result_images/test_figure/figure_align_titles") | ||
compare_images("./lib/matplotlib/tests/baseline_images/figure_align_titles.png", |
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 we need to not use relative paths for this test
The tests use the "classic" style by default. For copying on recent matplotlib, I think |
Is the 'classic' style rendered differently then? Have there been known workarounds or solutions? |
The test images were created before the new default style, so many older tests use the old style. https://matplotlib.org/stable/gallery/style_sheets/style_sheets_reference.html |
Since these are new test images, they should probably set to the new style by passing See this for an example:
|
For new tests it is allowed, even encouraged. (Though if we can avoid image comparisons all together, that is even moreso encouraged... haven't looked too closely here to know how good such tests would be, and not too offended by more image tests, just they are a higher maintenance burden than non-image tests.) It would be a big lift to change old tests, though, and that is why its the default still... There have been talks about explicitly setting old style on all image comparisons where it is not explicit, then transitioning to newer style being default (and thus the case for all new tests) but that is a slow process. https://matplotlib.org/stable/devel/testing.html#writing-an-image-comparison-test Also actually the preferred key is "mpl20" which is just a stable alias for the current "default" style, such that if we change defaults again, the tests using it will retain current behavior. (It is currently just mapped to "default", and the similar alias "mpl15" is mapped to "classic") |
Thanks the info, I really appreciate it! Then we will update the tests asap :) |
FWIW, I think it's probably bad idea to have examples that are clearly broken in some respect - in this case with overlapping labels. Either manually make enough space with subplots_adjusts or some such, or use constrained_layout. In fact this should almost definitely get a test with constrained_layout. |
…ed, however using constrained layout gives some scaling issues now.
@AlextheGreat1509 do you need help addressing the CI failures here? |
Closing in favour of #27952. |
PR Summary
We used the previous pull request: [ENH]: add Figure.align_titles() as inspiration. We fixed the issues associated with it and did some refactoring. This pull request adds support for aligning titles between subplots. All use cases work. We tried to add tests but were unable to make them show the same behaviour as expected even though our manual tests did. We used two different ways to compare and generate the images but still could not get the desired behaviour, even though the images were generated with the same function (savefig).
PR Checklist
Documentation and Tests
pytest
passes)Release Notes
.. versionadded::
directive in the docstring and documented indoc/users/next_whats_new/
.. versionchanged::
directive in the docstring and documented indoc/api/next_api_changes/
next_whats_new/README.rst
ornext_api_changes/README.rst