Make default facecolor for subfigures be transparent ("none"). Fix for issue #24910 #25255

robrighter · 2023-02-19T02:33:25Z

PR Summary

This issue was written because the solid facecolor on subfigures was hiding the parent figure suptitle. A few solutions were identified and discussed in the issue thread #24910 . It was decided that the best solution is to change the default facecolor value to "none" (transparent). This PR makes that change.

PR Checklist

Documentation and Tests

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

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

github-actions

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.

jklymak · 2023-02-19T04:29:20Z

This seems good. Perhaps add a change note, and also please include some description in the PR... It's possible your image comparison fails because the wrong version of freetype? https://matplotlib.org/stable/devel/dependencies.html#c-libraries

robrighter · 2023-02-19T19:28:42Z

@jklymak Thanks for the feedback. I think I have everything ready for review now.

There is a failing linting check for eslint. I was unsure how to resolve it because I do not see a way to reproduce it locally.

edit: The eslint failure is because of this known issue with the tool: #25251

rcomer · 2023-02-20T12:05:49Z

lib/matplotlib/figure.py

-        facecolor : default: :rc:`figure.facecolor`
-            The figure patch face color.
+        facecolor : default: ``"none"``
+            Transparent face color.


Suggested change

Transparent face color.

The figure patch face color; transparent by default.

or similar. The point is that any color could be passed.

I think you committed this suggestion, but then it got clobbered in the git confusion. Easily done.

You are right. Sorry about that. I amended the commit to add it back in.

No worries - I've done similar!

rcomer · 2023-02-20T12:20:25Z

lib/matplotlib/tests/test_figure.py

@@ -287,6 +287,16 @@ def test_suptitle_fontproperties():
    assert txt.get_weight() == fps.get_weight()


+@image_comparison(['figure_suptitle_subfigures'],
+                  extensions=['png'])


Suggested change

extensions=['png'])

extensions=['png'], style='mpl20')

Not compulsory, but the newer style is preferred. If you decide to change this, the image will also need replacing. Adding and replacing an image in separate commits in the same PR will cause a CI failure, so you would also need to squash the commits down to one. So whether you take this suggestion should depend on how confident you feel with git!

Thanks @rcomer, I made the changes you suggested. I also squashed down the commits to avoid the image change issue in the CI pipeline.

Well, looks like the added-modified check on that image failed. I'll investigate and get it sorted.

jklymak

This looks good, but not sure we need a visual test...

jklymak · 2023-02-20T22:19:58Z

lib/matplotlib/tests/test_figure.py

@@ -287,6 +287,16 @@ def test_suptitle_fontproperties():
    assert txt.get_weight() == fps.get_weight()


+@image_comparison(['figure_suptitle_subfigures'],
+                  extensions=['png'], style='mpl20')


I'm not sure we really need a visual test here... It should be enough to check that facecolor="none" for the subfigure by default, no?

I can see either way. I guess removing the visual test would keep the build time minimal. However, having the visual test provides some protection if another subtle change impacts the way the subfigures can hide the suptitle. If you think that risk is minimal because the behavior is well understood, I'm happy to remove it.

The suptitle is under the subfigure (if subfigure is called first). There isn't much to be done about that without using a layout manager. So I'd argue either the subfigure has a clear face or it doesn't is all we can test here. If something else ends up in the subfigure that is not clear, then the suptitle will disappear anyway.

Ok I will update the PR to:
(1) Remove the visual test
(2) Add a new test that creates the figure and checks for the default clear face on the subfigures. (And maybe a non-default value test too.)

jklymak · 2023-02-21T01:24:46Z

Note to pass the Cleanliness check you will need to squash the commits. If in doubt about how to do that, suggest you back up to a backup branch.

…ixes issue matplotlib#24910

rcomer · 2023-02-21T14:00:05Z

Thanks @robrighter, I think this is ready to merge, but...

@jklymak I'm not sure what milestone this should go in: from the issue's point of view it's a bugfix, which would normally go in v3.7.1. However, since this changes a default, it's likely to break someone. I understand that's OK to do without warning since subfigures are provisional, but maybe not in a patch release?

jklymak · 2023-02-21T16:22:18Z

Thanks @robrighter I squashed your commits before merging. Congratulations on your first PR to Matplotlib!

@rcomer, I think you are right that this is too big a change for a bug fix release. But I don't feel strongly about it if someone wants to argue for a backport to 3.7.1

github-actions bot reviewed Feb 19, 2023

View reviewed changes

robrighter mentioned this pull request Feb 19, 2023

[Bug]: Suptitle not visible with subfigures #24910

Closed

robrighter changed the title ~~added test for issue 24910~~ Fix for issue #24910 Feb 19, 2023

robrighter marked this pull request as ready for review February 19, 2023 15:22

robrighter changed the title ~~Fix for issue #24910~~ Make default facecolor for subfigures be transparent ("none"). Fix for issue #24910 Feb 19, 2023

rcomer reviewed Feb 20, 2023

View reviewed changes

rcomer linked an issue Feb 20, 2023 that may be closed by this pull request

[Bug]: Suptitle not visible with subfigures #24910

Closed

robrighter force-pushed the bugfix-for-issue-24910 branch from 93009ae to 775ad94 Compare February 20, 2023 20:54

jklymak reviewed Feb 20, 2023

View reviewed changes

jklymak approved these changes Feb 21, 2023

View reviewed changes

RR - Set default facecolor for subfigures to be none (Transparent). F…

058f1d1

…ixes issue matplotlib#24910

robrighter force-pushed the bugfix-for-issue-24910 branch from a20c16b to b82dc38 Compare February 21, 2023 01:55

RR - Resolve merge conflict

9d827a8

robrighter force-pushed the bugfix-for-issue-24910 branch from b82dc38 to 9d827a8 Compare February 21, 2023 11:59

rcomer approved these changes Feb 21, 2023

View reviewed changes

jklymak added this to the v3.8.0 milestone Feb 21, 2023

jklymak merged commit c13683a into matplotlib:main Feb 21, 2023

rcomer mentioned this pull request May 22, 2023

[Doc]: Subfigures still marked as provisional #25947

Closed

tplobo mentioned this pull request Jan 30, 2024

Bump matplotlib to 3.8.0 to tackle invisible suptitle Fusion-Power-Plant-Framework/bluemira#2976

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make default facecolor for subfigures be transparent ("none"). Fix for issue #24910 #25255

Make default facecolor for subfigures be transparent ("none"). Fix for issue #24910 #25255

robrighter commented Feb 19, 2023 •

edited

Loading

github-actions bot left a comment

jklymak commented Feb 19, 2023

robrighter commented Feb 19, 2023 •

edited

Loading

rcomer Feb 20, 2023 •

edited

Loading

rcomer Feb 21, 2023

robrighter Feb 21, 2023

rcomer Feb 21, 2023

rcomer Feb 20, 2023

robrighter Feb 20, 2023

robrighter Feb 20, 2023

jklymak left a comment

jklymak Feb 20, 2023

robrighter Feb 20, 2023

jklymak Feb 20, 2023

robrighter Feb 20, 2023

jklymak commented Feb 21, 2023

rcomer commented Feb 21, 2023

jklymak commented Feb 21, 2023

	Transparent face color.
	The figure patch face color; transparent by default.

Make default facecolor for subfigures be transparent ("none"). Fix for issue #24910 #25255

Make default facecolor for subfigures be transparent ("none"). Fix for issue #24910 #25255

Conversation

robrighter commented Feb 19, 2023 • edited Loading

PR Summary

PR Checklist

github-actions bot left a comment

Choose a reason for hiding this comment

jklymak commented Feb 19, 2023

robrighter commented Feb 19, 2023 • edited Loading

rcomer Feb 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jklymak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jklymak commented Feb 21, 2023

rcomer commented Feb 21, 2023

jklymak commented Feb 21, 2023

robrighter commented Feb 19, 2023 •

edited

Loading

robrighter commented Feb 19, 2023 •

edited

Loading

rcomer Feb 20, 2023 •

edited

Loading