FIX: fix bbox_inches=tight and constrained layout bad interaction #19342

jklymak · 2021-01-22T22:12:01Z

PR Summary

Closes #19339

import matplotlib.pyplot as plt
import numpy as np

fig, ax = plt.subplots(constrained_layout=True)
ax.set_aspect(1.0)
fig.savefig('/tmp/New.png', bbox_inches='tight')

Old

Bad interaction between width and current size of the axes:

New:

Suppress constrained layout after the first draw in the bbox_inches=tight logic

Sorry, this obviously needs a new image test unless someone knows how to mock up bbox_inches='tight' behaviour without actually saving the figure.

PR Checklist

Has pytest style unit tests (and pytest passes).
Is Flake 8 compliant (run flake8 on changed files to check).
New features are documented, with examples if plot related.
Documentation is sphinx and numpydoc compliant (the docs should build without error).
Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

timhoffm

I'm not claiming I've fully dug into the code, but the structure looks a bit odd:

There's a lot of distance between saving cl_state and deactivating the layout.
The save/restore always happens, but deactivating only happens if bbox_inches == "tight"

Can't this be pulled closer together? Ideally, if we had a with figure._constrained_layout_deactivated() context manager could this be used here?

jklymak · 2021-01-22T23:25:24Z

The issue is that bbox_inches=tight makes an extra draw, and then changes the size of the figure. You only want to turn CL off after that extra draw. Otherwise you want to leave it as the user set it because the print_method call calls a draw (which retriggers CL). I don't think there is anything that can be wrapped in a context manager.

jklymak · 2021-01-23T00:50:56Z

I guess the other way of expressing this is that I think the original issue is that the way we do the bbox_inches='tight' is not terribly intuitive. In an ideal world, we would just make the figure normally, and crop it afterwards in a backend-appropriate way. Instead, we make a smaller canvas, and temporarily transform all the artists a dx and dy, and then draw the figure. CL balks at this because it then tries to resize on the smaller trimmed figure using margins etc that were based on the bigger figure. Its a case of two types of semi-fragile magics not getting along.

jklymak · 2021-01-23T01:15:14Z

Though thinking about it some more, I forgot to check if bbox_inches=Bbox([[1, 0], [3, 2]]) and indeed that was incorrect as well for the same reason. So the new commit unconditionally does a draw if either constrained layout is needed or bbox_inches="tight", and then turns CL off when the printed version is rendered. and then turns it back on after.

timhoffm · 2021-01-23T09:28:16Z

lib/matplotlib/backend_bases.py

+                # we have already done CL above, so turn it off:
+                cl_state = self.figure.get_constrained_layout()
+                self.figure.set_constrained_layout(False)


I suspect this should not be part of try. We're not expecting it to fail.

Also, does it make sense to

cl_state = self.figure.get_constrained_layout() if cl_state: self.figure.set_constrained_layout(False) [...] if cl_state: self.figure.set_constrained_layout(cl_state)

to prevent any overhead if constrained is not used?

Note that this can easily be refactored to a context manager now:

with self.figure._constrained_layout_deactivated(): try: [...] finally: [...] return result

Sure, the set can be outside the try.

anntzer · 2021-01-23T16:55:56Z

In an ideal world, we would just make the figure normally, and crop it afterwards in a backend-appropriate way.

Actually, now that you write this, I think that's probably doable (not in the 3.4 time frame of course)... I'll keep that in the back of my mind for the future.

jklymak · 2021-01-23T17:19:56Z

Actually, now that you write this, I think that's probably doable (not in the 3.4 time frame of course)... I'll keep that in the back of my mind for the future.

For raster backends, it is pretty trivial - just crop the canvas before write.

For the vector backends, I'm not sure what the incantation needs to be to do that robustly....

anntzer · 2021-01-23T17:41:43Z

For svg we can probably handle with https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/viewBox.
For ps/pdf I guess something like /PageSize or %BoundingBox or setting some global translation that gets applied to everything.

jklymak · 2021-01-23T21:45:16Z

Yeah postscript (and I assume friends) has a translate command. Something like this would be so much more straightforward than the song and dance we do now. But as you say, not for 3.4.

jklymak · 2021-01-23T22:50:11Z

I thought about the context manager, but I am not sure if its worth creating a context manager in figure.py just for this. If we were doing this pattern more, I'd agree, but...

I am happy to include the if-statements if you like, but I think set_constrained_layout is pretty lightweight compared to anything else that is happening during a figure save.

timhoffm · 2021-01-24T00:10:42Z

I'm slightly in favor of a context manager because it makes the code more readable, even if the context manager is only used once.

But I'm also ok without. That's why I've already approved.

jklymak · 2021-01-25T17:50:23Z

@anntzer can you confirm if this fixes the issue for you?

anntzer

Works for me. Thanks for the quick fix :)

anntzer · 2021-01-25T18:24:11Z

I think the context manager, if any, should be setprop_cm (similar to setattr_cm). But that would be another PR.

jklymak · 2021-01-25T18:53:23Z

Thanks @timhoffm and @anntzer

FIX: fix bbox_inches=tight and constrained layout bad interaction

1f60cfb

jklymak added Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. topic: geometry manager LayoutEngine, Constrained layout, Tight layout labels Jan 22, 2021

jklymak added this to the v3.4.0 milestone Jan 22, 2021

timhoffm reviewed Jan 22, 2021

View reviewed changes

jklymak force-pushed the fix-CL-tight branch from d973d9c to 40de439 Compare January 23, 2021 01:15

timhoffm reviewed Jan 23, 2021

View reviewed changes

FIX: make sure that actual bboxes also work

eaf7dc8

jklymak force-pushed the fix-CL-tight branch from 40de439 to eaf7dc8 Compare January 23, 2021 16:12

timhoffm approved these changes Jan 23, 2021

View reviewed changes

jklymak mentioned this pull request Jan 23, 2021

bbox_inches='tight' implementation.... #19349

Open

jklymak requested a review from anntzer January 25, 2021 17:50

anntzer approved these changes Jan 25, 2021

View reviewed changes

anntzer merged commit b6294ff into matplotlib:master Jan 25, 2021

jklymak deleted the fix-CL-tight branch January 25, 2021 18:52

anntzer mentioned this pull request Jan 26, 2021

Add Artist._cm_set for temporarily setting an Artist property. #19369

Merged

7 tasks

Uh oh!

FIX: fix bbox_inches=tight and constrained layout bad interaction #19342

FIX: fix bbox_inches=tight and constrained layout bad interaction #19342

Uh oh!

Conversation

jklymak commented Jan 22, 2021

PR Summary

Old

New:

PR Checklist

Uh oh!

timhoffm left a comment

Choose a reason for hiding this comment

Uh oh!

jklymak commented Jan 22, 2021

Uh oh!

jklymak commented Jan 23, 2021

Uh oh!

jklymak commented Jan 23, 2021

Uh oh!

timhoffm Jan 23, 2021

Choose a reason for hiding this comment

Uh oh!

jklymak Jan 23, 2021

Choose a reason for hiding this comment

Uh oh!

anntzer commented Jan 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jklymak commented Jan 23, 2021

Uh oh!

anntzer commented Jan 23, 2021

Uh oh!

jklymak commented Jan 23, 2021

Uh oh!

jklymak commented Jan 23, 2021

Uh oh!

timhoffm commented Jan 24, 2021

Uh oh!

jklymak commented Jan 25, 2021

Uh oh!

anntzer left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anntzer commented Jan 25, 2021

Uh oh!

jklymak commented Jan 25, 2021

Uh oh!

Uh oh!

anntzer commented Jan 23, 2021 •

edited

Loading

anntzer left a comment •

edited

Loading