-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
ENH: Layout engine #20426
Conversation
5a69602
to
9049e10
Compare
b4bd9d7
to
544d829
Compare
b219589
to
d48dc8c
Compare
0db5624
to
77aca09
Compare
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? |
@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) |
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. |
Although I could be wrong. Don't think I fully understand the scope of this feature. |
880ba75
to
afbe5bb
Compare
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. |
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 Even if we never relax this, users can work around it by adding one more layer of indirection. A 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 |
e74a28d
to
e0bb291
Compare
Newest commits:
|
8736b47
to
b9c6596
Compare
else: | ||
raise ValueError(f"Invalid value for 'layout': {layout!r}") | ||
|
||
if self._check_layout_engines_compat(self._layout_engine, |
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.
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.
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.
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.
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.
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.
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 now raises...
jklymak#2 has some additional commits for this (including a fix to the failing test). |
8633095
to
ef4fad5
Compare
b6eccf1
to
7063e3f
Compare
7063e3f
to
ec4dfbc
Compare
@jklymak I merged what I think fixes a stray |
🎉 I'm excited to get this in! |
Thanks for all your help @anntzer, @QuLogic, @timhoffm and @tacaswell I know this was a big one to get in. |
PR Summary
This is based on #20229 (constrained_layout is only called at draw time) and #19892 (encourage
layout='tight'
andlayout='constrained'
.This is a backwards compatible re-architecting of how we do layout; in particular
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 providedconstrained_layout_engine
andtight_layout_engine
that are back-compatible so far as I can tell.PR Checklist
pytest
passes).flake8
on changed files to check).flake8-docstrings
and runflake8 --docstring-convention=all
).doc/users/next_whats_new/
(follow instructions in README.rst there).doc/api/next_api_changes/
(follow instructions in README.rst there).