10000 tight_layout support for subfigure by leejjoon · Pull Request #26108 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

tight_layout support for subfigure #26108

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

tight_layout support for subfigure #26108

wants to merge 1 commit into from

Conversation

leejjoon
Copy link
Contributor

PR summary

TightLayoutEngine from matplotlib.layout_engine fails when called with a subfigure. The PR is an attempt to make it to work with subfigures.

Note that, unlike Figure, Subfigure class does not have a method of tight_layout. So, we need to manually create an instance of TightLayoutEngine and call it with a subfigure. Here is a simple example.

import matplotlib.pyplot as plt

fig = plt.figure()
fig.subplotpars.bottom = 0.3
subfigs = fig.subfigures(3, 3)
sfig = subfigs[0, 0]

sfig.patch.set_fc("y")
ax = sfig.subplots(1, 1)

from matplotlib.layout_engine import TightLayoutEngine
engine = TightLayoutEngine()
engine.execute(sfig)

plt.show()

test_subfigure_tight_layout

PR checklist

@jklymak
Copy link
Member
jklymak commented Jun 11, 2023

Strongly suggest using layout='constrained' for similar figure layouts.

@leejjoon
Copy link
Contributor Author

Strongly suggest using layout='constrained' for similar figure layouts.

Yes, I know the constrained layout works fine with subfigures. But for my "real" use case (which requires different subfigures have some of their subplotpars synced so that their axes align), I found it was more straight forward to to call tight_layout and then to tweak subplotpars. If this is something we can already do with the constrained layout, I will be happy to learn how.

@rcomer
Copy link
Member
rcomer commented Jun 12, 2023 •
edited
Loading

You can trigger a draw and then turn constrained layout off if that helps?
https://matplotlib.org/devdocs/users/explain/axes/constrainedlayout_guide.html#manually-turning-off-constrained-layout

@leejjoon
Copy link
Contributor Author

And is there a way to align axes in different subfigures after that? With tight_layout, I run tight_layout for each of subfigures (they are set to have independent subplotpar attributes), take the maximum value of subplotpar.left of all subfigures, for example, and then apply that value to all subfigures.

By the way, independent of this can be done with the constrained layout, I think it is worthwhile to make TightLayoutEngine to support subfigures, given the fixes are rather straightforward.

@jklymak
Copy link
Member
jklymak commented Jun 12, 2023

It would be easier to see what you are asking with an example.

There is definitely no harm in making tight_layout work with subfigures if it doesn't require a lot of new knobs.

tacaswell
tacaswell reviewed Jun 12, 2023
View reviewed changes
lib/matplotlib/figure.py
@@ -2219,6 +2219,12 @@ def __init__(self, parent, subplotspec, *,
self._set_artist_props(self.patch)
self.patch.set_antialiased(False)

def get_size_inches(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be added to the figure.pyi file.

How odd do we think that it is for SubFigure to have get_size_inches but not set_size_inches? I'm leaning towards that it is OK, with a small thought of "we should implement set_size_inches that raises and says you have to go talk to the (grand-)parent figure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually think that this should be private (and have a similar private method for the top level Figure that the public get_size_inches calls, and then uses where we need the subfigure size can call the private method instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I will introduce an private method (so no SubFigure.get_size_inches). @tacaswell : Do you think we still need to consider some sort of set_size_inches method (likely private) that shows a reasonable error message?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the method is private, I do not have concerns about the missing setter.

@tacaswell tacaswell added this to the v3.8.0 milestone Jun 12, 2023
jklymak
jklymak reviewed Jun 13, 2023
View reviewed changes
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 this also needs a test. I'm still not entirely clear what you are proposing the public API for this is? just attaching an engine to a single subfigure and running tight_layout on the subfigure?

lib/matplotlib/figure.py
@@ -2219,6 +2219,12 @@ def __init__(self, parent, subplotspec, *,
self._set_artist_props(self.patch)
self.patch.set_antialiased(False)

def get_size_inches(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually think that this should be private (and have a similar private method for the top level Figure that the public get_size_inches calls, and then uses where we need the subfigure size can call the private method instead.

@leejjoon
Copy link
Contributor Author

I think this also needs a test. I'm still not entirely clear what you are proposing the public API for this is? just attaching an engine to a single subfigure and running tight_layout on the subfigure?

For the public API, I am open to any suggestions. I personally think it is not necessary to introduce new public api. I don't expect TightLayoutEngine to be as robust as Constrained LayoutEnging for subfigures. On the other hand, it would be good if the TightLayoutEngine handles a figure with subfigures better, at least for simple cases.

I will push some more changes, including tests.

@leejjoon leejjoon mentioned this pull request Jun 27, 2023
Open
1 task
@ksunden ksunden modified the milestones: v3.8.0, v3.9.0 Aug 8, 2023
@jklymak
Copy link
Member
jklymak commented Sep 27, 2023

@leejjoon did you want to push forward with this?

For your use case, as I understand it, I would personally juts keep the axes in the same layout versus having them in subfigures. The idea of subfigures is that the layouts between the subfigures are independent. Of course if you are after a very specific layout - say with a larger column separation between columns of axes, then maybe subfigures+subplotpars is a reasonable way to go. Though even for that use case, I'd probably use width_ratios and blank subplots:

import numpy as np
import matplotlib.pyplot
8000
 as plt

fig, axs = plt.subplot_mosaic([['a', 'b', '.', 'c', 'd'],
                         ['e', 'f', '.', 'g', 'h']
                         ], width_ratios=[1, 1, 0.25, 1, 1],
                         layout='constrained')
for ax in axs:
    axs[ax].set_xticks([])
    axs[ax].set_yticks([])
plt.show()

Figure_1

However, I appreciate that there are many other reasons you may want to use subfigures.

@QuLogic QuLogic modified the milestones: v3.9.0, v3.10.0 Mar 27, 2024
@ksunden ksunden modified the milestones: v3.10.0, v3.11.0 Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tacaswell tacaswell tacaswell left review comments

@jklymak jklymak jklymak left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
status: waiting for other PR
Projects
None yet
Milestone
v3.11.0
Development

Successfully merging this pull request may close these issues.
6 participants
@leejjoon @jklymak @rcomer @tacaswell @QuLogic @ksunden
0