Skip to content

Add Figure parameter layout and discourage tight_layout / constrained_layout#19892

Merged
tacaswell merged 2 commits into
matplotlib:masterfrom
timhoffm:fig-layout
Aug 24, 2021
Merged

Add Figure parameter layout and discourage tight_layout / constrained_layout#19892
tacaswell merged 2 commits into
matplotlib:masterfrom
timhoffm:fig-layout

Conversation

@timhoffm

@timhoffm timhoffm commented Apr 7, 2021

Copy link
Copy Markdown
Member

PR Summary

Having separate and competing parameters tight_layout and constrained_layout is not a great API choice and can cause confusion
(#17339).

This PR addresses the problem by introducing a new parameter layout and suggests to use it instead.

Since tight_layout has been around for ages and is used probably a lot, I dare not right away deprecate the parameters and force all users to adapt. Therefore, tight_layout and constrained_layout are only marked as "discouraged" for now. (We might deprecate eventually some versions down the road). This is somewhat cumbersome, but IMHO the best way forward.

I put the "discouraged" concept in as a proposal. This adds a bit of overhead in the docs and code-wise, but gently paves the way for a better API. Feedback is welcome.

If this gets accepted, there will be followup actions:

  • Update all examples to use layout.
  • Consider adding a new rcParams['figure.layout'] value to unify defaults as well. This needs a bit fiddling to get proper fallbacks to the existing defaults, but we should be able to make it work "as one would expect".

@timhoffm timhoffm added the API: changes Changes to the public API, typically requiring deprecation. label Apr 7, 2021
@timhoffm timhoffm added this to the v3.5.0 milestone Apr 7, 2021
@timhoffm timhoffm force-pushed the fig-layout branch 2 times, most recently from f42a21b to b92db60 Compare April 7, 2021 21:17
@jklymak

jklymak commented Apr 7, 2021

Copy link
Copy Markdown
Member

In general this seems good.

OTOH I have been considering whether constrained_layout (or tight_layout) even needs to be part of the figure creation anymore. It definitely needs to be part of figure drawing...

The point being that we could imagine other layout managers, and maybe this kwarg could take a class that does the layout?

Comment thread lib/matplotlib/figure.py Outdated
@timhoffm

timhoffm commented Apr 8, 2021

Copy link
Copy Markdown
Member Author

OTOH I have been considering whether constrained_layout (or tight_layout) even needs to be part of the figure creation anymore. It definitely needs to be part of figure drawing...

It's reasonable to have the layout mechanism as state on the Figure. And for practicality, configuring that state during creation is reasonable. It may well be that we don't have to actually do anything with that information until draw time.

The point being that we could imagine other layout managers, and maybe this kwarg could take a class that does the layout?

This is quite analogous to the projection case where accept some strings, but also classes. For that generalization we'd need a layout manager interface description though. But from the user-facing API, the layout parameter is compatible with such an extension.

@jklymak

jklymak commented Apr 21, 2021

Copy link
Copy Markdown
Member

Maybe

  1. Some string shortcuts (layout='tight', layout='constrained') to the existing kw args.
  2. A new layout class that has tight_layout and constrained_layout as subclasses that can be instantiated and hold options for those classes. i.e. fig = plt.figure(layout=mpl.layouts.constrained_layout(wspace=0.04, pad=0.02))
  3. keep set_constrained and set_constrained_pad and whatever is done for tight_layout for back compat, but have them call methods in the layout classes.

This leaves us open in the future to have these be user-constructible, and just have a draw-hook that calls layout.execute() at draw time.

It'd be nice to get 20016 in so it'd be nice to hammer this out

@jklymak jklymak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks good. Suggest we get the string-only version working first. Then one of us can follow up with the layout super-class as a possible argument. I'm 99.9% sure this can be done for CL without a problem, and actually will fix some issues with it not updating to changes in things like the widths kwarg to gridspec. i.e. a new layout algorithm will just build itself at draw time. I don't think that will be much slower, if at all if I'm smart about caching.

Comment thread lib/matplotlib/figure.py Outdated
Comment thread lib/matplotlib/figure.py Outdated
@jklymak

jklymak commented Apr 25, 2021

Copy link
Copy Markdown
Member

looks good, I think some of the docs need adjusting now.

@timhoffm

Copy link
Copy Markdown
Member Author

You mean examples?

@jklymak

jklymak commented Apr 25, 2021

Copy link
Copy Markdown
Member

No I mean we aren't accepting a dict...

Comment thread lib/matplotlib/figure.py Outdated
Comment thread lib/matplotlib/figure.py Outdated
Comment thread doc/api/next_api_changes/deprecations/19892-TH.rst Outdated
@timhoffm timhoffm force-pushed the fig-layout branch 2 times, most recently from e6be062 to c216bd7 Compare April 25, 2021 21:11
@mwaskom

mwaskom commented Apr 26, 2021

Copy link
Copy Markdown

I think this is a welcome API improvement. Just two thoughts about naming...

I would again (cf. a gitter conversation from a little while back) make a plug for naming the "constrained" operation something that describes the result, not something that describes the implementation.

Second, in terms of the parameter name, "layout" suggests something about the general position of the subplots, not the method for "cleaning up" their fine positioning and extents. In fact, I just checked, and "layout" is the name of the parameter for the arrangement of the subplots in subplot_mosaic. Feels like that's bound to cause confusion...

@jklymak

jklymak commented Apr 26, 2021

Copy link
Copy Markdown
Member

Not sure there is appetite to change the name of constrained_layout.

But fair enough about this not being strictly the layout. Maybe layout_engine, layout_algorithm, layout_method? None are as concise as layout, but leaving layout off would make it too enigmatic.

@mwaskom

mwaskom commented Apr 26, 2021

Copy link
Copy Markdown

Appreciate the reluctance to change the name; I mention it because this API change would be the moment to do it, if it's going to happen. My expectation would be that keeping "constrained" will lead to relative disuse of the feature. Given the choice between "tight" and "constrained", "tight" sounds better to me.

All of the bigrams are better names, but require so much typing :(

If it were up to me, I would search for a parameter name that is more concise and corresponds more closely to exactly what the feature does, which is automatically arrange the subplots for optimal use of space. So, some synonym of "arrange", "marshal", "cleanup" might make sense...

@timhoffm

Copy link
Copy Markdown
Member Author

I appreciate the naming considerations.

I've chosen the names for maximal recognizability [kind]_layout=True --> layout='[kind]'.

subplot_mosaic(layout, ..., **fig_kw) is indeed a show stopper. We should not have a figure parameter with the same name, because it cannot be passed. Two options (both viable in principle):

  1. Rename the first subplot_mosaic parameter. This should be managable. subplot_mosaic is still provisional, and I doubt many people are using layout as a keyword there.
  2. Find another parameter name for the layout engine.

For 1. we could go with subplot_mosaic(arrangement, ...) or similar.

I'm a bit wed to layout because it's already used for these operations in the code. Also it reminds me of the Qt layout system. Neither of "arrange", "marshal", "cleanup" talk to me in this context. Some other possibilities: figure(... positioning='tight'), figure(..., adjustment='tight'), figure(..., spacing='tight').
As for the values: I think 'tight' should stay as tight_layout is a long standing name many people know. Also it doesn't sound too positive, which is good 😄. I agree that 'constrained' is quite technical. Since 'constrained_layout` is still experimental, renaming is an option. "what the feature does exactly" is in principle better than naming the technical solution. However that's hard to define. Generally, "The layout mechanism adjusts axes sizes and positions all artists so that there is reasonable spacing around all artists". That's true for both algorithms and doesn't help me with naming 😞. Instead of 'constrained' we might characterize the look of the result as "clean", "spaced", "distributed", "optimal", "relaxed", ...

@jklymak

jklymak commented Apr 26, 2021

Copy link
Copy Markdown
Member

For 1. we could go with subplot_mosaic(arrangement, ...) or similar.

That seems reasonable to me, if it makes sense to @tacaswell and @story645

That still doesn't say if layout is the right choice here. I don't know that typing something longer is the end of the world. After all, we have been typing constrained_layout=True the whole time, so layout_optimizer='constrained' isn't that bad.

@tacaswell

Copy link
Copy Markdown
Member

Does this need to be propagated to the docstrings of the pyplot functions?

@jklymak

jklymak commented Jul 11, 2021

Copy link
Copy Markdown
Member

I agree with your statement of what the feature does.

Name: I think spacing (or optimize_spacing) is good.

default: I guess I almost always use constrained_layout=True. However, I am somewhat leery about a complicated algorithm as the default. For instance it can go wrong sometimes, and lead to quite strange layouts (mostly when an end-user has specified positions in physical units so that there is no way to satisfy the constraints). For prior art of magic causing problems, the inline backend in jupyter does bbox_inches="tight" by default, which causes a lot of confusion for users (I would guess there are 2-3 stackoverflow questions about this a week).

@mwaskom

mwaskom commented Jul 11, 2021

Copy link
Copy Markdown

For prior art of magic causing problems, the inline backend in jupyter does bbox_inches="tight" by default, which causes a lot of confusion for users (I would guess there are 2-3 stackoverflow questions about this a week).

How many questions a week do you think there would be along the lines of "why are my axis labels cut off in matplotlib" if it weren't the default, though?

@jklymak

jklymak commented Jul 11, 2021

Copy link
Copy Markdown
Member

"why are my axis labels cut off in matplotlib" if it weren't the default, though?

I don't think any of the other backends do this by default, so while I'm sure there are some questions along those lines, they are usually answered by "use tight_layout". But the point is well-taken that if 95% of plots are made with tight_layout turned on (and I bet that is not an exaggeration), maybe a spacing manager should be on by default.

@timhoffm

Copy link
Copy Markdown
Member Author

I think the inline backends bbox_inches="tight" is not quite comparable. Automatic cutting is surprising (you specify a figsize, but the figure ends up smaller). I don't expect that a resonable automatic, spacing is surprising. Concerning the corner cases: We should be able to detect cases where constraints cannot be fulfilled and fall back to now layout with a warning.

@timhoffm

Copy link
Copy Markdown
Member Author

Since this is a significant change, I would like to briefly discuss this in the next dev call.
In particular the questions:

  • Can we be specific or do we want to stay more open to extensions?
  • Do we want to activate a default optimizer in the foreseeable future?

Depending on these questions, I propse the following parameter name:

more specific open for extensions
go for a default optimizer optimize_spacing optimize_layout
no default optimizer spacing layout

@jklymak

jklymak commented Jul 11, 2021

Copy link
Copy Markdown
Member

I can't make this week's call unfortunately. But we could do the following week.

I think we should allow "extensions". When thinking about this, this is all really just a draw-time hook, and we have a drawtime callback. Regardless I like having it be its own kwarg as that is less mysterious.

@timhoffm

timhoffm commented Jul 29, 2021

Copy link
Copy Markdown
Member Author

We decided in the dev call today to name the parameter layout because that is the most intuitive transition (tight_layout=True -> layout='tight'). The name may not be perfect but it's good enough. Also that pandas is using layout for the subplot arrangement is not a great worry. It's in a distant enough context that it should not be confusing. And we plan to propose to them to switch from subplots=True, layout=(2, 2) to subplots=(2, 2).

@timhoffm timhoffm marked this pull request as ready for review July 29, 2021 20:18

@QuLogic QuLogic left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Minus the rebase.

Comment thread doc/api/next_api_changes/deprecations/19892-TH.rst Outdated
Comment thread doc/api/next_api_changes/deprecations/19892-TH.rst Outdated
Comment thread doc/api/next_api_changes/deprecations/19892-TH.rst Outdated
Comment thread lib/matplotlib/figure.py Outdated
@jklymak

jklymak commented Aug 23, 2021

Copy link
Copy Markdown
Member

Whats the plan here @timhoffm - here must be very many places where this needs to be changed in the docs? The test coverage is also null at the moment?

@QuLogic

QuLogic commented Aug 23, 2021

Copy link
Copy Markdown
Member

I was going to merge and leave docs for later, but I think we definitely want some sort of test coverage at least.

@jklymak

jklymak commented Aug 23, 2021

Copy link
Copy Markdown
Member

I don't know if we can leave the docs this way unless this waits for 3.6

@timhoffm

Copy link
Copy Markdown
Member Author

I think it's ok to change the docs later for API changes that are only discouraging existing behavior. The transition on the user side is gradual anyway and there is no deadline to catch. So it's not the end of the world if somebody sees the old way in an example and still uses that. Of course, we want to encourage the new API and the docs should reflect that. They should follow soonish, whether for 3.5.0 or 3.5.1 does not matter too much.

Having the new API is more important than advertizing it. If we delay this to 3.6, we force all users / downstream libraries that want to use it to a min. dependency of 3.6, which will delay adoption.

@timhoffm

Copy link
Copy Markdown
Member Author

I'm happy if somebody could add a test or two. It should be straight forward to adapt an existing layout test / make an image comparision, but I have little bandwidth right now.

@QuLogic

QuLogic commented Aug 24, 2021

Copy link
Copy Markdown
Member

I added tests and changed the exception to use the _api wrapper.

@tacaswell

Copy link
Copy Markdown
Member

anyone can merged on green, happy to merge the tests on just my review of @QuLogic 's work.

@tacaswell tacaswell merged commit 2ae73c6 into matplotlib:master Aug 24, 2021
@timhoffm timhoffm deleted the fig-layout branch August 24, 2021 06:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

API: changes Changes to the public API, typically requiring deprecation. topic: geometry manager LayoutEngine, Constrained layout, Tight layout

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants