Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork8.1k
Add Figure parameter layout and discourage tight_layout / constrained_layout#19892
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
f42a21b tob92db60Comparejklymak commentedApr 7, 2021
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 imagineother layout managers, and maybe this kwarg could take a class that does the layout? |
Uh oh!
There was an error while loading.Please reload this page.
timhoffm commentedApr 8, 2021
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.
This is quite analogous to the |
jklymak commentedApr 21, 2021
Maybe
This leaves us open in the future to have these be user-constructible, and just have a draw-hook that calls It'd be nice to get 20016 in so it'd be nice to hammer this out |
jklymak left a comment
There was a problem hiding this comment.
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 thelayout 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'tthink that will be much slower, if at all if I'm smart about caching.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
jklymak commentedApr 25, 2021
looks good, I think some of the docs need adjusting now. |
timhoffm commentedApr 25, 2021
You mean examples? |
jklymak commentedApr 25, 2021
No I mean we aren't accepting a dict... |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
e6be062 toc216bd7Comparemwaskom commentedApr 26, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 |
jklymak commentedApr 26, 2021
Not sure there is appetite to change the name of constrained_layout. But fair enough about this not being strictly the layout. Maybe |
mwaskom commentedApr 26, 2021
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 commentedApr 26, 2021
I appreciate the naming considerations. I've chosen the names for maximal recognizability
For 1. we could go with I'm a bit wed to |
jklymak commentedApr 26, 2021
That seems reasonable to me, if it makes sense to@tacaswell and@story645 That still doesn't say if |
tacaswell commentedMay 14, 2021
Does this need to be propagated to the docstrings of the pyplot functions? |
jklymak commentedJul 11, 2021
I agree with your statement of what the feature does. Name: I think default: I guess I almost always use |
mwaskom commentedJul 11, 2021
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 commentedJul 11, 2021
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 managershould be on by default. |
timhoffm commentedJul 11, 2021
I think the |
timhoffm commentedJul 11, 2021
Since this is a significant change, I would like to briefly discuss this in the next dev call.
Depending on these questions, I propse the following parameter name:
|
jklymak commentedJul 11, 2021
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 wehave a drawtime callback. Regardless I like having it be its own kwarg as that is less mysterious. |
timhoffm commentedJul 29, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
We decided in the dev call today to name the parameter |
QuLogic left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Minus the rebase.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
jklymak commentedAug 23, 2021
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 commentedAug 23, 2021
I was going to merge and leave docs for later, but I think we definitely want some sort of test coverage at least. |
jklymak commentedAug 23, 2021
I don't know if we can leave the docs this way unless this waits for 3.6 |
timhoffm commentedAug 23, 2021
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 commentedAug 23, 2021
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 commentedAug 24, 2021
I added tests and changed the exception to use the |
tacaswell commentedAug 24, 2021
anyone can merged on green, happy to merge the tests on just my review of@QuLogic 's work. |
Uh oh!
There was an error while loading.Please reload this page.
PR Summary
Having separate and competing parameters
tight_layoutandconstrained_layoutis not a great API choice and can cause confusion(#17339).
This PR addresses the problem by introducing a new parameter
layoutand suggests to use it instead.Since
tight_layouthas 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_layoutandconstrained_layoutare 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:
layout.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".