Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
ENH: add ability to remove layout engine#22452
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
b0c1c49 tocb0410aCompareThere 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.
Ping@jklymak are there any traps when removing layouts?
Uh oh!
There was an error while loading.Please reload this page.
I can't think of any. It just doesn't get run. |
aa2a746 todd7c2e0CompareI added a fig.set_layout_engine('tight')fig.set_layout_engine('none')fig.set_layout_engine('constrained') as away of "going through 0" and doing something that we put a bunch of effort into preventing! |
dd7c2e0 to8640cebCompare| self._colorbar_gridspec = colorbar_gridspec | ||
| super().__init__(**kwargs) | ||
| def execute(self, fig): |
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 guess I'm not sure about adding a Null engine here. It makesset_layout_engine('none') go through a (slightly) different codepath than never setting the layout engine at all. If we do this, we should probably initialize the figure with this NullLayoutEngine? But I'm not sure why we want to have this at all.
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 can not stick a formatter on at init time without making the bools tri-states ({unset, True, False}). This acts as a no-op place holder that remember the colorbar related settings without inventing another side-band way to store that.
Unfortunately I think that "never been set" vs "was set and then removed" are in fact different.
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.
Ah I see - we don't want to switch colorbar behaviour after it has started to be used... So we are turning off the algorithm, but keeping its side effect.
At some point we need to figure out how to ditch the two ways of placing colorbars without breaking everybody.
Doc build error: |
Uh oh!
There was an error while loading.Please reload this page.
I'll move to draft until the doc error is fixed... |
Uh oh!
There was an error while loading.Please reload this page.
lib/matplotlib/layout_engine.py Outdated
| @@ -101,6 +101,28 @@ def execute(self, fig): | |||
| raise NotImplementedError | |||
| class NullLayoutEngine(LayoutEngine): | |||
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.
Do we need to have this public?
Can we think of a better name? Something that catches "mirror the behavior of whatever layout engine it is replacing"? Maybe FrozenLayoutEngine or LayoutEngineSnapshot? Also this behavior is the key reason for the existence of the class. It should be prominently in the docstring, not only in the parameter description.
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 turns the layout engine off, so I don't think it really freeze the layout manager?
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.
There is a difference betweenNone andNullLayoutEngine, and we have to capture that at least in the docstring but if possible already in the class name.NullLayoutEngine sounds as if it does nothing at all and intuitively I'd have expected that that's equvalent with the initial stateNone.
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.
NullLayoutEngineLockedColorbarImplimentation is a bit long!
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.
BTW, I think this needs to be public because the user can get it as a result fromget_layout_engine. Maybe what is needed is a sentence of explanation for the user in the docs....
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.
On the other hand,NullLayoutEngineLockedColorbarImplimentation is not something we expect a user to ever directly create and it very clear about what is it and communicates to the user something funny is going on.
If we document this publicly then it will also be a very unique search term to land users at the right place in the docs.
lib/matplotlib/layout_engine.py Outdated
| """ | ||
| A no-op LayoutEngine. | ||
| This is primarily to act as a place holder if we remove a 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.
| Thisisprimarilytoactasaplaceholderifweremovealayoutengine. | |
| Thisisactsasaplaceholderifweremovealayoutengine.However,differentlayoutengines | |
| requiredifferentcolorbarimplementationsandsomearenotcompatiblewith``subplots_adjust``, | |
| sothisNullengineretainstherequirementsofthelast-usedlayoutengine. |
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 is an improvement 👍 . Can you make it even more explicit: "different layout engines require different ..." / "some are not compatible". To my knowledge we have exactly two layout engines we are talking about.
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.
True, but the goal of this hook was to make it easy to add more / let external libraries provide their own so I think that leaving this general is the optimistic position.
... moved to draft pending rebase and rename (which I mean, fine, I don't care what the name is) |
dcc9693 to1ff8d49CompareThis also adds a "place holder" layout engine to ensure that users can not "gothrough zero" and change to an incompatible layout engine.Co-authored-by: Jody Klymak <jklymak@gmail.com>
1ff8d49 tof7f3bb6CompareIs this still draft@tacaswell ? |
@jklymak Are you happy with the documentation in this PR? |
Still looks fine to me! Thanks, |
| - 'tight' uses `~.TightLayoutEngine` | ||
| - 'none' removes layout engine. | ||
| If `None`, the behavior is controlled by :rc:`figure.autolayout` |
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.
Were we doing*None*?
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 seem to have about the same number of both
$ ack '\*None\*' -l | wc 56 56 1865$ ack '`None`' -l | wc 61 61 2223[edit updated to exclude docs build product]
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
PR Summary
This may be too simplistic as it just sets it to None which gives you ability
to "go through zero" and change to an incompatible layout engine.
PR Checklist
Tests and Styling
pytestpasses).flake8-docstringsand runflake8 --docstring-convention=all).Documentation
doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).