Applying zoom to an axes with constrained_layout caused problems because the bounding box for a line on the axes was not correctly calculated when constrained_layout needed it to be. That could lead to a collapsed axes, and singular matrices for other transformations.

This PR doesn't fix the underlying problem with the zoom not setting the transform quick enough, but since layout probably doesn't need to change much under zoom, that's likely OK. However, it does stop constrained layout from being applied if any of the axes have zero width or height.

Note this was not a problem before#10682, because nowax.get_tightbbox checks all the artists in the axes.

The only drawback I can see is if someone deliberately had a zero-width/height axes on their figure. But I don't think constrained_layout would work well in that case anyways.

UPDATE: Per@efiring suggestion, I've also turned constrained_layout off if 'PAN' or 'ZOOM' are active. I think this is much safer.

PR Checklist

Has Pytest style unit tests
Code is PEP 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

jklymak added this to thev3.0 milestone

Jul 11, 2018

jklymak added topic: geometry manager

LayoutEngine, Constrained layout, Tight layout

Release criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. labels

Jul 11, 2018

Copy link

MemberAuthor

jklymak commentedJul 11, 2018

This is minor, and a safety catch, so marking RC for 3.0....

Copy link

Member

efiring commentedJul 12, 2018

Would it make sense, and be reasonably easy, to disable the bbox and constraint calculations when zoom-to-rect or zoom/pan are on?

Copy link

MemberAuthor

jklymak commentedJul 12, 2018•
edited
Loading

Yeah if there is a method to detect that’s what the redraw is for, I think that’d be the safest thing most of the time. Is it easy to know that the redraw was triggered for that reason?

EDIT: done - easier than I though...

jklymak mentioned this pull request

Jul 13, 2018

Should constrained_layout be applied every draw?#11630

Closed

timhoffm approved these changes

Jul 14, 2018

View reviewed changes

lib/matplotlib/_constrained_layout.py Outdated

		@@ -199,15 +207,27 @@ def do_constrained_layout(fig, renderer, h_pad, w_pad,

		fig._layoutbox.constrained_layout_called += 1
		fig._layoutbox.update_variables()
		# Now set the position of the axes...
		# check if layout worked. If not, don't change positions:

Copy link

Member

timhoffmJul 14, 2018

Choose a reason for hiding this comment

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

You could move this whole check to a local functionlayout_is_valid(fig) and then just:

if layout_is_valid(fig):    # Now set the position of the axes...    ...

or even with a second functionapply_layout(fig)

if layout_is_valid(fig):    apply_layout(fig):else:    warnings.warn(...)

I think this would help keeping an overview what's going on.

Copy link

MemberAuthor

jklymakJul 14, 2018

Choose a reason for hiding this comment

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

I guess this is a taste thing. 6 lines is kind of under my "move to its own function" threshold. When debugging I find it pretty annoying to have to chase down chains of three-line rabbit holes that do something trivial.

Copy link

Member

timhoffmJul 14, 2018

Choose a reason for hiding this comment

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

You are height that this is a matter of taste

My rule of thumb is: A couple of lines that have a distinguished purpose usually make up a good function. Even more so, if I need a comment to explain what they are doing. The function name can often replace the comment.