| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
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? |
Sorry, something went wrong.
|
@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) |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
Although I could be wrong. Don't think I fully understand the scope of this feature. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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)). |
Sorry, something went wrong.
|
Newest commits:
|
Sorry, something went wrong.
| 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.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
This now raises...
Sorry, something went wrong.
|
jklymak#2 has some additional commits for this (including a fix to the failing test). |
Sorry, something went wrong.
|
@jklymak I merged what I think fixes a stray .dpi , I plan to merge this is CI is green. |
Sorry, something went wrong.
|
🎉 I'm excited to get this in! |
Sorry, something went wrong.
|
Thanks for all your help @anntzer, @QuLogic, @timhoffm and @tacaswell I know this was a big one to get in. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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
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