Skip to content

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

Closed
wants to merge 14 commits into from

Conversation

AlextheGreat1509
Copy link

@AlextheGreat1509 AlextheGreat1509 commented Mar 31, 2023

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

  • [WIP] Has pytest style unit tests (and pytest passes)
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • New plotting related features are documented with examples.

Release Notes

  • [N/A] New features are marked with a .. versionadded:: directive in the docstring and documented in doc/users/next_whats_new/
  • [N/A] API changes are marked with a .. versionchanged:: directive in the docstring and documented in doc/api/next_api_changes/
  • [N/A] Release notes conform with instructions in next_whats_new/README.rst or next_api_changes/README.rst

@AlextheGreat1509 AlextheGreat1509 marked this pull request as draft March 31, 2023 13:08
Copy link

@github-actions github-actions bot left a 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.

@AlextheGreat1509 AlextheGreat1509 marked this pull request as ready for review March 31, 2023 13:19
@AlextheGreat1509 AlextheGreat1509 marked this pull request as draft April 3, 2023 13:54
self.canvas.draw_idle() produces more reliable behaviour when generating the plots. Now the titles are alligned every time.
Copy link
Member

@ksunden ksunden left a 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'],
Copy link
Member

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

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",
Copy link
Member

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

@SirSkillful
Copy link

First of all, thank you for looking into this! We really appreciate it!
Secondly, I wanted to show an example of the error we are having with the tests:
If you run the following code in Jupyter notebook or just a normal python script, the image generated by fig.savefig() and plt.savefig() will look like this:
Code:
fig, axs = plt.subplots(2, 2,
subplot_kw={"xlabel": "x", "ylabel": "",
"title": "Title"})
axs[0][0].imshow(plt.np.zeros((5, 3)))
axs[0][1].imshow(plt.np.zeros((3, 5)))
axs[1][0].imshow(plt.np.zeros((2, 1)))
axs[1][1].imshow(plt.np.zeros((1, 2)))

axs[0][1].set_title('Title2', loc="left")
fig.draw_without_rendering()
fig.align_titles()
fig.draw_without_rendering()
fig.savefig("fig_align_titles.png")

Generated image:
fig_align_titles

The problem we are facing now is, that the image generated by pytest when fig.savefig() is used looks like this:
Image generated by pytest:
test_image

When align_labels() is used though, the labels in images generated by pytest do show aligned labels. We are not sure what else we should do.

@jklymak
Copy link
Member

jklymak commented Apr 4, 2023

The tests use the "classic" style by default. For copying on recent matplotlib, I think plt.style.use('classic') should look the same.

@SirSkillful
Copy link

Is the 'classic' style rendered differently then? Have there been known workarounds or solutions?

@jklymak
Copy link
Member

jklymak commented Apr 4, 2023

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
We don't change the style so that we don't have to regenerate the images, which makes our repository larger and more unwieldily.

@ksunden
Copy link
Member

ksunden commented Apr 4, 2023

Since these are new test images, they should probably set to the new style by passing style="default" to the image_comparison decorator:

See this for an example:

@image_comparison(['polar_axes'], style='default', tol=0.012)

@SirSkillful
Copy link

SirSkillful commented Apr 4, 2023

@jklymak That makes sense. Thanks for clarifying.
@ksunden Thanks for pointing that out! I didn't know you could do that.
Is this an allowed approach? If that is the case, we would change the tests to use the 'default' style.

@ksunden
Copy link
Member

ksunden commented Apr 4, 2023

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

@SirSkillful
Copy link

Thanks the info, I really appreciate it! Then we will update the tests asap :)

@jklymak
Copy link
Member

jklymak commented Apr 4, 2023

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.

@melissawm
Copy link
Member

@AlextheGreat1509 do you need help addressing the CI failures here?

@trananso trananso mentioned this pull request Mar 20, 2024
5 tasks
@rcomer
Copy link
Member

rcomer commented Mar 31, 2024

Closing in favour of #27952.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Waiting for author
Development

Successfully merging this pull request may close these issues.

7 participants