Skip to content

ENH: Layout engine #20426

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jan 14, 2022
Merged

ENH: Layout engine #20426

merged 2 commits into from
Jan 14, 2022

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Jun 13, 2021

PR Summary

This is based on #20229 (constrained_layout is only called at draw time) and #19892 (encourage layout='tight' and layout='constrained'.

This is a backwards compatible re-architecting of how we do layout; in particular

from my_layout_engine import my_favourite_layout

fig = plt.figure(layout=my_favourite_layout(option1=0.4, option2=0.7))

and then my_favourite_layout.execute() is called when the figure is drawn, and presumably moves axes around, or resizes figures, etc before the draw. Two new layout engines are provided constrained_layout_engine and tight_layout_engine that are back-compatible so far as I can tell.

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).

@jklymak jklymak changed the title Layout engine ENH: Layout engine Jun 13, 2021
@jklymak jklymak added topic: geometry manager LayoutEngine, Constrained layout, Tight layout status: waiting for other PR labels Jun 13, 2021
@jklymak jklymak force-pushed the layout-engine branch 2 times, most recently from b4bd9d7 to 544d829 Compare September 14, 2021 12:10
@jklymak jklymak added this to the v3.6.0 milestone Sep 14, 2021
@jklymak jklymak force-pushed the layout-engine branch 4 times, most recently from b219589 to d48dc8c Compare September 15, 2021 13:44
@jklymak jklymak marked this pull request as ready for review September 20, 2021 11:06
@jklymak jklymak force-pushed the layout-engine branch 2 times, most recently from 0db5624 to 77aca09 Compare September 20, 2021 11:08
@anntzer
Copy link
Contributor

anntzer commented Sep 30, 2021

Will probably take some time to fully review, but I very much like the idea. Do you want to get the proplot author involved as well?

@jklymak
Copy link
Member Author

jklymak commented Oct 2, 2021

@lukelbd would this have obviated some of the special classes in proplot? Instead of a special class, you could just have had a layout engine that adjusted your figure to have a larger size in proportion to the fixed-width subplots. (Of course other features of proplot would still be implemented separately)

@lukelbd
Copy link
Contributor

lukelbd commented Oct 2, 2021

Very cool — this is definitely a big improvement. However I don’t think proplot can use it unfortunately.

Proplot’s GridSpec subclass also makes the major backwards-incompatible change of specifying spacing parameters in physical units rather than figure-relative units and subplot-relative units (with em-widths as the default and other units accepted as strings through the physical units engine). For example, right=1 sets the right margin to 1 em width.

It also permits variable spacing between rows and columns (which addresses some use cases for GridSpecFromSubplotSpec), implements a “panel” system for subplot panels and colorbars (where some grid spec slots are hidden and constrained to have fixed widths), and forces users to use a single GridSpec per figure (if they are working manually with GridSpecs).

Maybe some of those later points could be implemented with a layout engine but overall the changes and restrictions are probably too drastic for that to work.

@lukelbd
Copy link
Contributor

lukelbd commented Oct 2, 2021

Although I could be wrong. Don't think I fully understand the scope of this feature.

@jklymak jklymak force-pushed the layout-engine branch 2 times, most recently from 880ba75 to afbe5bb Compare October 8, 2021 11:00
@jklymak
Copy link
Member Author

jklymak commented Oct 8, 2021

Thanks @lukelbd - the goal here is to give downstream libraries and users a draw-time hook that is specifically to be used to change layout. That draw time hook can do what you want with it, though it definitely does not hang extra meta info off gridspecs or axes.

Out of scope for this PR, but if you have suggestions for easy, back compatible, extra meta info that would be useful to add to gridspec and/or axes (min margin sizes etc) that input would be welcome. You could certainly imagine variable minimum column separator widths in gridspec that then are respected by whatever the layout manager is.

@tacaswell
Copy link
Member

I'm between option A (no change) and B (only change if we know it is safe). I am inclined to go with A for now as it is API safe to relax that restriction in the future (so we are not painting our selves into a corner) and is is simpler (and we are signing up to support something of unknown (but probably high) complexity). To some degree we can think of this as a way to get at FigureTightLayout and FigureConstrainedLayout via composition rather than inheritance.

Even if we never relax this, users can work around it by adding one more layer of indirection. A LayoutEnigneDispatcher that exposes some knob to switch between compatible layout engines should be possible to make in an API compatible way ("All problems in computer science can be solved by adding a layer of inderection. All performance problems can be solved by removing a layer of indirection" 😈 ).

The direction I am more concerned that it is possible to share an engine instance across figures (which it looks like you can do now as get_figure is never exercised (I put in suggestions to remove all of the code around that)).

@jklymak jklymak force-pushed the layout-engine branch 2 times, most recently from e74a28d to e0bb291 Compare December 24, 2021 09:59
@jklymak
Copy link
Member Author

jklymak commented Dec 24, 2021

Newest commits:

  1. implement C above. The check is if any axes have a _colorbar attribute, which gets added by a colorbar call.
  2. make colorbar_gridspec and adjust_compatible properties of the classes (ahem, I think I did it correctly)

@jklymak jklymak force-pushed the layout-engine branch 2 times, most recently from 8736b47 to b9c6596 Compare December 24, 2021 15:26
else:
raise ValueError(f"Invalid value for 'layout': {layout!r}")

if self._check_layout_engines_compat(self._layout_engine,
Copy link
Member

Choose a reason for hiding this comment

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

We should either always change it and warn or not change it and raise.

Not changing and warning makes sense if someone is using us directly and interactively, but if we end up behind any layers of indirection will make it both confusing for users and hard for people wrapping us to program against.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we should change if there is a colorbar; the layout will just break.

Im not sure I follow how warning and not doing anything is bad. I mean I guess if someone wraps us and tries to magically switch layout engines behind the users back, that would be a problem, but they shouldn't do that, or if they do, they can add their own more stringent checking. I think its a better user experience from our end just to warn and not do anything.

Copy link
Member

Choose a reason for hiding this comment

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

I still lean towards raising. If we raise something like a RuntimeError, then someone who wants to try and carry on if it fails can do try: ... except, however if we only warn someone who wants to know if it has failed has no good way to do so (yes, you can use various tricks to catch warnings, but that involves some global state (under the hood)).

I think this is a case where our dual application vs library identity is causing problems. In application-mode warning is 100% the right thing to do, in library-mode raising is 100% the right thing to do.

Not going to block over this.

Copy link
Member Author

Choose a reason for hiding this comment

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

This now raises...

@tacaswell
Copy link
Member

jklymak#2 has some additional commits for this (including a fix to the failing test).

@jklymak jklymak force-pushed the layout-engine branch 2 times, most recently from 8633095 to ef4fad5 Compare January 7, 2022 13:00
@jklymak jklymak force-pushed the layout-engine branch 2 times, most recently from b6eccf1 to 7063e3f Compare January 10, 2022 10:29
@tacaswell
Copy link
Member

@jklymak I merged what I think fixes a stray .dpi , I plan to merge this is CI is green.

@tacaswell tacaswell merged commit 8837185 into matplotlib:main Jan 14, 2022
@tacaswell
Copy link
Member

🎉 I'm excited to get this in!

@jklymak
Copy link
Member Author

jklymak commented Jan 14, 2022

Thanks for all your help @anntzer, @QuLogic, @timhoffm and @tacaswell I know this was a big one to get in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: geometry manager LayoutEngine, Constrained layout, Tight layout
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants