ENH: add ability to remove layout engine #22452

tacaswell · 2022-02-11T20:21:50Z

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

Has pytest style unit tests (and pytest passes).
Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

New features are documented, with examples if plot related.
[N/A] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
[N/A] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
Documentation is sphinx and numpydoc compliant (the docs should build without error).

timhoffm

Ping @jklymak are there any traps when removing layouts?

lib/matplotlib/tests/test_figure.py

jklymak · 2022-02-13T08:20:01Z

I can't think of any. It just doesn't get run.

tacaswell · 2022-03-25T03:30:03Z

I added a NullLayoutEngine despite @anntzer 's concerns because I was worried about

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!

jklymak · 2022-03-28T11:30:12Z

lib/matplotlib/layout_engine.py

+        self._colorbar_gridspec = colorbar_gridspec
+        super().__init__(**kwargs)
+
+    def execute(self, fig):


I guess I'm not sure about adding a Null engine here. It makes set_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.

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.

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.

jklymak · 2022-05-18T09:29:02Z

Doc build error:

WARNING: error while formatting signature for matplotlib.layout_engine.NullLayoutEnigne: Handler <function mangle_signature at 0x7f20ea43a4c0> for event 'autodoc-process-signature' threw an exception (exception: Unknown section Parameter in the docstring of NullLayoutEnigne in /home/circleci/project/lib/matplotlib/layout_engine.py.)

lib/matplotlib/layout_engine.py

jklymak · 2022-06-02T14:31:49Z

I'll move to draft until the doc error is fixed...

lib/matplotlib/layout_engine.py

timhoffm · 2022-06-06T23:08:42Z

lib/matplotlib/layout_engine.py

@@ -101,6 +101,28 @@ def execute(self, fig):
        raise NotImplementedError


+class NullLayoutEngine(LayoutEngine):


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.

This turns the layout engine off, so I don't think it really freeze the layout manager?

There is a difference between None and NullLayoutEngine, 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 state None.

NullLayoutEngineLockedColorbarImplimentation is a bit long!

BTW, I think this needs to be public because the user can get it as a result from get_layout_engine. Maybe what is needed is a sentence of explanation for the user in the docs....

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.

jklymak · 2022-06-09T08:05:15Z

lib/matplotlib/layout_engine.py

+    """
+    A no-op LayoutEngine.
+
+    This is primarily to act as a place holder if we remove a layout engine.


Suggested change

This is primarily to act as a place holder if we remove a layout engine.

This is acts as a place holder if we remove a layout engine. However, different layout engines

require different colorbar implementations and some are not compatible with ``subplots_adjust``,

so this Null engine retains the requirements of the last-used layout engine.

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.

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.

jklymak · 2022-06-23T08:02:59Z

... moved to draft pending rebase and rename (which I mean, fine, I don't care what the name is)

This also adds a "place holder" layout engine to ensure that users can not "go through zero" and change to an incompatible layout engine. Co-authored-by: Jody Klymak <jklymak@gmail.com>

jklymak · 2022-06-30T08:46:51Z

Is this still draft @tacaswell ?

tacaswell · 2022-08-02T16:34:12Z

@jklymak Are you happy with the documentation in this PR?

jklymak · 2022-08-02T18:13:15Z

Still looks fine to me! Thanks,

QuLogic · 2022-08-03T21:55:24Z

lib/matplotlib/figure.py

+            - 'tight' uses `~.TightLayoutEngine`
+            - 'none' removes layout engine.
+
+            If `None`, the behavior is controlled by :rc:`figure.autolayout`


Were we doing *None*?

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]

lib/matplotlib/figure.py

lib/matplotlib/tests/test_figure.py

Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>

tacaswell added this to the v3.6.0 milestone Feb 11, 2022

tacaswell force-pushed the enh_null_layout_engine branch from b0c1c49 to cb0410a Compare February 11, 2022 20:22

timhoffm reviewed Feb 12, 2022

View reviewed changes

lib/matplotlib/tests/test_figure.py Outdated Show resolved Hide resolved

tacaswell force-pushed the enh_null_layout_engine branch 3 times, most recently from aa2a746 to dd7c2e0 Compare March 25, 2022 03:30

tacaswell force-pushed the enh_null_layout_engine branch from dd7c2e0 to 8640ceb Compare March 25, 2022 03:32

jklymak reviewed Mar 28, 2022

View reviewed changes

greglucas mentioned this pull request May 1, 2022

fix and test deprecated layout method toggling #22849

Closed

6 tasks

jklymak approved these changes May 18, 2022

View reviewed changes

jklymak reviewed May 18, 2022

View reviewed changes

lib/matplotlib/layout_engine.py Outdated Show resolved Hide resolved

jklymak added topic: geometry manager LayoutEngine, Constrained layout, Tight layout status: needs review labels May 24, 2022

jklymak added the status: needs revision label Jun 2, 2022

jklymak marked this pull request as draft June 2, 2022 14:31

oscargus reviewed Jun 2, 2022

View reviewed changes

lib/matplotlib/layout_engine.py Outdated Show resolved Hide resolved

tacaswell marked this pull request as ready for review June 2, 2022 17:02

jklymak removed the status: needs revision label Jun 2, 2022

timhoffm reviewed Jun 6, 2022

View reviewed changes

jklymak reviewed Jun 9, 2022

View reviewed changes

jklymak added status: needs rebase status: needs documentation and removed status: needs review labels Jun 23, 2022

jklymak marked this pull request as draft June 23, 2022 08:02

tacaswell force-pushed the enh_null_layout_engine branch from dcc9693 to 1ff8d49 Compare June 26, 2022 03:28

ENH: add ability to remove layout engine

f7f3bb6

This also adds a "place holder" layout engine to ensure that users can not "go through zero" and change to an incompatible layout engine. Co-authored-by: Jody Klymak <jklymak@gmail.com>

tacaswell force-pushed the enh_null_layout_engine branch from 1ff8d49 to f7f3bb6 Compare June 26, 2022 03:30

jklymak approved these changes Jun 30, 2022

View reviewed changes

tacaswell removed the status: needs rebase label Aug 2, 2022

tacaswell marked this pull request as ready for review August 2, 2022 16:33

tacaswell removed the status: needs documentation label Aug 2, 2022

QuLogic reviewed Aug 3, 2022

View reviewed changes

MNT: changes from review

4725daa

Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>

QuLogic approved these changes Aug 9, 2022

View reviewed changes

QuLogic merged commit 357bac5 into matplotlib:main Aug 9, 2022

tacaswell deleted the enh_null_layout_engine branch August 9, 2022 13:27

QuLogic mentioned this pull request Sep 9, 2022

Consolidate release notes for 3.6 #23814

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: add ability to remove layout engine #22452

ENH: add ability to remove layout engine #22452

tacaswell commented Feb 11, 2022

timhoffm left a comment

jklymak commented Feb 13, 2022

tacaswell commented Mar 25, 2022

jklymak Mar 28, 2022

tacaswell Mar 29, 2022

jklymak Mar 30, 2022

jklymak commented May 18, 2022

jklymak commented Jun 2, 2022

timhoffm Jun 6, 2022

jklymak Jun 7, 2022

timhoffm Jun 8, 2022

jklymak Jun 8, 2022

jklymak Jun 9, 2022

tacaswell Jun 9, 2022

jklymak Jun 9, 2022

timhoffm Jun 9, 2022

tacaswell Jun 9, 2022

jklymak commented Jun 23, 2022

jklymak commented Jun 30, 2022

tacaswell commented Aug 2, 2022

jklymak commented Aug 2, 2022

QuLogic Aug 3, 2022

tacaswell Aug 4, 2022 •

edited

Loading

		@@ -101,6 +101,28 @@ def execute(self, fig):
		raise NotImplementedError


		class NullLayoutEngine(LayoutEngine):

-    This is primarily to act as a place holder if we remove a layout engine.
+    This is acts as a place holder if we remove a layout engine.  However, different layout engines
+    require different colorbar implementations and some are not compatible with ``subplots_adjust``,
+    so this Null engine retains the requirements of the last-used layout engine.

ENH: add ability to remove layout engine #22452

ENH: add ability to remove layout engine #22452

Conversation

tacaswell commented Feb 11, 2022

PR Summary

PR Checklist

timhoffm left a comment

Choose a reason for hiding this comment

jklymak commented Feb 13, 2022

tacaswell commented Mar 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jklymak commented May 18, 2022

jklymak commented Jun 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jklymak commented Jun 23, 2022

jklymak commented Jun 30, 2022

tacaswell commented Aug 2, 2022

jklymak commented Aug 2, 2022

Choose a reason for hiding this comment

tacaswell Aug 4, 2022 • edited Loading

Choose a reason for hiding this comment

tacaswell Aug 4, 2022 •

edited

Loading