8000 TightLayoutEngine.execute runs get_tight_layout_figure twice by leejjoon · Pull Request #26203 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

TightLayoutEngine.execute runs get_tight_layout_figure twice #26203

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

TightLayoutEngine.execute runs get_tight_layout_figure twice #26203

wants to merge 1 commit into from

Conversation

leejjoon
Copy link
Contributor

PR summary

If AutoLocator is used, changing the location of axes may change the ticklabels.
Thus the result of tight_layout may not be optimal.

    fig = plt.figure(layout="tight", figsize=(3, 3))
    fig.subplots(3, 3)
    fig.set_facecolor("0.8")

tightlayout_single

The PR simply runs the algorithm twice.

tightlayout_double

  • ConstrainedLayoutEngine also runs its algorithm twice (_constrained_layout.do_constrained_layout)
  • There are failing tests, because the result of running the algorithm twice can be different for running it only once. We need to update the baseline images if this PR to be accepted.
  • I can add a simple test if requested.
  • My intention is to have this PR merged before I further work on tight_layout support for subfigure #26108.

PR checklist

  • new and changed code is tested

@QuLogic QuLogic added the topic: geometry manager LayoutEngine, Constrained layout, Tight layout label Jun 27, 2023
jklymak
jklymak reviewed Nov 30, 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.

This seems like a fine thing to do. Not sure if we want to change that many images.

@rcomer rcomer mentioned this pull request Feb 6, 2025
Merged
@timhoffm
Copy link
Member
timhoffm commented Feb 6, 2025

Given that most people should be using constrained_layout nowadys, and that tight_layout has other shortcomings as well, I think that changing the behavior - even if it means improving - is not worth it. Those who are using tight_layout are likely ok with the existing behavior. Those who want a better layout engine should switch to constrained_layout.

I therefore propose to close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jklymak jklymak jklymak left review comments

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

Assignees
No one assigned
Labels
topic: geometry manager LayoutEngine, Constrained layout, Tight layout
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@leejjoon @timhoffm @jklymak @QuLogic
0