FIX: restore creating new axes via plt.subplot with different kwargs #19438

tacaswell · 2021-02-02T18:20:30Z

This adds a small amount of additional state to the Axes created via
Figure.add_axes and Figure.add_subplot (which the other Axes creation
methods eventually funnel through) to track the kwargs passed
through.

We then use that state in pyplot.subplot to determine if we should
re-use an Axes found at a given position or create a new one (and
implicitly destroy the existing one).

closes #19432
closes #10700

PR Summary

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

tacaswell · 2021-02-02T18:47:27Z

If people are happy with this (which seems to be the case) I still need to chase through the docs to make sure they are still correct.

dstansby · 2021-02-02T19:05:52Z

lib/matplotlib/tests/test_figure.py

    fig = plt.figure()
    ax = plt.subplot(1, 2, 1)
    ax1 = plt.subplot(1, 2, 1)
    ax2 = plt.subplot(1, 2, 2)
+    # This will delete ax / ax1 as they fully overlap


Isn't the reason for deletion actually that the projection keywords are not the same?

That is what I thought too, but the logic is

matplotlib/lib/matplotlib/pyplot.py

Lines 1236 to 1244 in ea68032

bbox = ax.bbox

axes_to_delete = []

for other_ax in fig.axes:

if other_ax == ax:

continue

if bbox.fully_overlaps(other_ax.bbox):

axes_to_delete.append(other_ax)

for ax_to_del in axes_to_delete:

delaxes(ax_to_del)

which says "delete any axes which the now axes fully covers" .

@tacaswell is correct. I wasn't aware of this behavior, but it's well documented, and it's been around a long time, so this is 👍 for me.

dstansby

Looks good to me 👍

anntzer · 2021-02-02T19:55:46Z

I believe this suffers from the same underlying problem that caused the old implementation to be so roundabout: some keys are not hashable because they are mutable. To be concrete, if you write e.g.

lims = [.1, .1, .9, .9]
ax1 = fig.add_axes(lims)
lims[0] = .2
ax2 = fig.add_axes(lims)
ax3 = fig.add_axes([.1, .1, .9, .9])
ax4 = fig.add_axes([.2, .1, .9, .9])

what are the axes that you think should be the same? (A similar, and probably less artificial, argument applies e.g. for add_subplot(111, projection=some_mutable_thing).

(Holding onto kwargs can be useful, but I think this more or less requires (in this specific case) enforcing that projections be immutable.)

jklymak · 2021-02-02T20:52:59Z

@anntzer, I'm not following - first this PR doesn't do anything for add_axes etc. it only affects plt.subplot, and the only danger of

plt.subplot(projection=myMutableProjection)
myMutableProjection.mutate()
plt.subplot(projection=myMutableProjection)

is that they will lose the original plot because the projection has changed, in which case they should just keep the reference:

ax = plt.subplot(projection=myMutableProjection)
myMutableProjection.mutate()
plt.sca(ax)

That seems pretty esoteric to me. But it is certainly better than breaking the people who expect the axes to clear when doing

plt.subplot(projection=proj1)
...
plt.subplot(projection=proj2)

anntzer · 2021-02-02T21:25:40Z

I wrote the example with add_axes for simplicity, but again something similar can happen with projections if they are not immutable. I don't actually know much about cartopy or WCSAxes, but I don't think it would be incredible to have some projections library that supports e.g. proj.set_central_longitude(whatever) (for example), in which case you run into the same issue (proj = Proj(); subplot(proj); <sometime later>; proj.set_central_longitude(whatever); subplot(proj) which doesn't seem so unlikely to happen).

And if the project is actually immutable, then it should normally be hashable too, so that would be one thing to check.

jklymak · 2021-02-02T21:41:09Z

proj = Proj(); subplot(proj); <sometime later>; proj.set_central_longitude(whatever); subplot(proj)

right, and with this change, the second subplot will overwrite the first. But a) thats hopefully rare, and b) when that happens, presumably its pretty obvious, and we can just tell people to save the reference to the subplot, rather than fragile grabbing from the subplot stack

anntzer · 2021-02-02T21:57:43Z

the second subplot will overwrite the first

I believe that what will happen is that the second call will return the preexisting axes as is?

presumably its pretty obvious, and we can just tell people to save the reference to the subplot

Just to be clear, it is far from obvious to me what people using the implicit machinery may be expecting, because it's been too long since I've last used it for this kind of manipulations, but heh.

tacaswell · 2021-02-03T01:23:11Z

The key behavior change in #19153 (and what caused the re-write from #18978 is that we moved the re-use or new (and remove) logic into pyplot from the Figure methods. I think everyone is still on board with add_subplot doing what it says on the tin and always adding a new Axes to the Figure. I think it is fair to ask people who are using the explicit (nee OO) version of the API to explicitly keep track of their Axes(es) and the only behavior still up for discussion is what pyplot does.

The argument for keeping the "create or query" logic in pyplot is that if the users are going fully down the implicit route (a number of years ago I had an lengthy discussion with a user who was convinced beyond doubt that the implicit API was easier and clearer to use, 30-45 minutes of effort and several drinks had no effect) then this is the only way for them to come back to an Axes via the internal sca call (which I discovered while writing this up was broken so fixed that too).

If we have a well behaved non-mutable input (or the users just never mutate it ;) ) we have:

obj = SomeClass()
ax1 = plt.subplot(111, keyword=obj)
obj2 = SomeClass()
ax2 = plt.subplots(111, keyword=obj2)

on 3.3 this returns a new Axes and removes the existing one
on master this return the first Axes because we ignore the kwargs when doing the matching (which @astrofrog makes a good case is a major API change, I think we have to back out of one way or the other)
with this patch it return a new Axes and remove the existing one.

However @anntzer is correct that this is opening is up to some new odd behavior when the values are mutable. Python is too flexible to reliably tell if something is mutable (and even checking if it is hashable is not technically enough as you can over-ride __hash__ is questionable ways).

obj = MutableClass()
ax1 = plt.subplot(111, keyword=obj)
obj.mutate()
ax2 = plt.subplots(111, keyword=obj)

On 3.3, this gives you back the same Axes or fail to create the first axes depending on if obj is hashable.
On master we fully ignore the kwarg the second time through and return the existing axes
With this patch we still get back the same Axes because we decide that the kwargs are equal

Reusing the axes may or may not be the right thing because it will end up depending on how the projections work (which is between the objects being based in and the project code we have discovered). If they look at what ever state is in obj and draw time, then getting back the same Axes is correct, if the state is used at Axes creation time then this behavior is wrong.

We could make a shallow copy of all of the values, but that would only push the problem "down a layer" or make things wrong in the opposite case as above. We could make a deep copy which may be expensive and inverts the cases where the behavior is wrong.

There is already going to be an issue the values are numpy arrays ("truth value of an array is ambiguous!"), but they also didn't work in <=3.3 so I am not worried about that

I think on net, the behavior in this patch is the best option (modulo adding some docs). In addition we could:

always warn in pyplot with kwargs, but that would be annoying to polar or 3D users (who are giving us string values).
explicitly allow some known OK keys / values and warn on anything else
try to hash the values as a hokey test

I'm inclined to leave it as-is and not get too deep on the semantics of this behavior with the "fix" for user problems is "please use the explicit API where you have a variable name to keep track of what Axes you want to use rather than a proxy variable name through some shared state".

jklymak · 2021-02-03T01:35:48Z

OK, I misunderstood @anntzer. The problem is that the user might mutate the object and think that they should get a new axes when they call subplot the second time. I guess I am OK with that being squishy and I still stand by my approval of this approach...

tacaswell · 2021-02-03T02:11:42Z

I am now a bit worried about pyplot.axes as well.

anntzer · 2021-02-03T09:23:29Z

Per the above I don't really know what's best for plt.subplot() users, so as long as the implications with mutable args have been thought out I'm fine with that (I agree with @tacaswell fig.add_subplot should just always add a new axes).

lib/matplotlib/figure.py

lpsinger · 2021-02-09T19:58:23Z

I have confirmed that, if merged, this will fix the issues in Astropy. See astropy/astropy#11315.

doc/users/next_whats_new/axes_kwargs_collision.rst

astrofrog · 2021-02-10T11:15:12Z

This looks good to me, thanks @tacaswell!

PR changed....

anntzer · 2021-02-11T20:46:21Z

See also #10700.

tacaswell · 2021-02-12T05:42:45Z

OK, this time I think I got enough of the corner cases. It is a fair bit of code, but I think we have gotten all of the logic layed out in one place now :) Most of the line count is additional tests.

doc/api/next_api_changes/behavior/19438-TAC.rst

anntzer · 2021-02-17T21:22:35Z

lib/matplotlib/pyplot.py

    """
+    proj_placeholder = object()


perhaps call this unset, I guess the conditions below read better?

lpsinger · 2021-02-17T21:12:08Z

doc/api/next_api_changes/behavior/19438-TAC.rst

+should be returned, but if the parameters are different the old Axes is removed
+from the Figure and a new Axes is created and returned.
+
+Due to an implementation detail, it was previously the case that for all


Is this implementation detail what was introduced by my changes in #19153? If so, we need not say that it was "previously the case" because those changes have never been in a release yet.

no, this is a ~~bug~~ behavior that goes back to 2.2.

atleast 2.2

but this text is missing a crucial half-sentence ....

Got it. Thanks!

lib/matplotlib/pyplot.py

lpsinger · 2021-02-17T21:21:43Z

lib/matplotlib/pyplot.py

    """
+    proj_placeholder = object()


Why is the placeholder object necessary? Why not None?

Because the user can palusibly pass in None, the goal here is to detect if the user passed us anyting . @anntzer 's suggest of calling this unset is better.

anntzer · 2021-02-17T21:27:18Z

lib/matplotlib/pyplot.py

@@ -1228,11 +1249,29 @@ def subplot(*args, **kwargs):
        (ax for ax in fig.axes
         if hasattr(ax, 'get_subplotspec') and ax.get_subplotspec() == key),
        None)
+    # if we found an axes at the right position sort out if we


the ax = ax below are a bit weird even though I get the logic. Perhaps rewrite starting by removing the call to next() and then

for ax in fix.axes: if hasattr(ax, "...") and ax.get_subplotspec() == key: p_class, pkw = ... if <reuse>: break # found it else: # didn't find an ax we can reuse, so make a new one ax = fig.add_subplot(...)

Yup, I like that better. I opted to do ax=ax rather than pass in the bodies of the if statements.

anntzer · 2021-02-17T21:28:34Z

lib/matplotlib/pyplot.py

+        p_class, pkw = fig._process_projection_requirements(*args, **kwargs)
+        # if no projection or kwargs were passed, return the axes we
+        # found.
+        if projection is proj_placeholder and pkw == {}:


shouldn't this case be handled by the more general case below with _projection_init == (Axes, {})?

No, because we did not used to include the projection class in the key, only the kwargs, for the matching, and then did isintance checking to see if the Axes was a sub-class of the Axes we found. The first condition is for the "just get me that Axes" logic, the second is for if the user passed anything that we need to resolve.

lib/matplotlib/pyplot.py

jklymak · 2021-02-17T22:10:36Z

lib/matplotlib/tests/test_pyplot.py

+    ax2 = plt.subplot(111, polar=True)
+    ax3 = plt.subplot(111, polar=True, projection='polar')
+    assert ax1 is ax2
+    assert ax1 is ax3


My opinion is still that a lot of the logic and copious comments in this PR are to satisfy this case. While I freely admit this worked before, I still do not feel preserving this is worth all this extra logic. The point here is to give the user a handle back to their original object. Already ax4=plt.subplot(111) will do that for them, as well as calling it with the original kwargs. I'd be shocked if we broke anyone by making the asserts fail, or if we did that they would not have a trivial fix to make it work again.

lib/matplotlib/tests/test_pyplot.py

tacaswell · 2021-02-18T22:42:20Z

Simplified the implementation and line-wrapped the comments to 79 characters.

This adds a small amount of additional state to the Axes created via Figure.add_axes and Figure.add_subplot (which the other Axes creation methods eventually funnel through) to track the projection class and (processed) kwargs. We then use that state in `pyplot.subplot` to determine if we should re-use an Axes found at a given position or create a new one (and implicitly destroy the existing one). This also changes the behavior of `plt.subplot` when no kwargs are passed to return the Axes at the location without doing any matching of the kwargs. As part of this work we also fixed two additional bugs: - you can now force Axes "back to" rectilinear Axes by passing projection='rectilinear' - Axes3D instances can now be re-selected at all closes matplotlib#19432 closes matplotlib#10700 Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>

lpsinger

Good news, astropy tests are still passing. Just spotted one typo, and suggested one very minor change that is a matter of style choice.

lib/matplotlib/pyplot.py

lpsinger · 2021-02-19T01:03:53Z

lib/matplotlib/pyplot.py

+        # if we found an axes at the position sort out if we can re-use it
+        if hasattr(ax, 'get_subplotspec') and ax.get_subplotspec() == key:
+            # if the user passed no kwargs, re-use
+            if kwargs == {}:


Suggested change

if kwargs == {}:

if not kwargs:

but I know it's just a matter of style.

I can see arguments either way here. I want to say "If kwargs is equal to the empty dictionary" which is equivalent to "kwargs is falsy", but I think the == version communicates to the reader better the condition they should be thinking when the read it.

MAINT: fix typo from #19438

tacaswell added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Feb 2, 2021

tacaswell added this to the v3.4.0 milestone Feb 2, 2021

tacaswell requested review from astrofrog and dstansby February 2, 2021 18:20

tacaswell mentioned this pull request Feb 2, 2021

Unexpected change in behavior in plt.subplot #19432

Closed

jklymak previously approved these changes Feb 2, 2021

View reviewed changes

tacaswell force-pushed the tweak_subplots branch from aae74d7 to 907311c Compare February 2, 2021 18:46

dstansby reviewed Feb 2, 2021

View reviewed changes

tacaswell force-pushed the tweak_subplots branch from 907311c to 2960490 Compare February 2, 2021 23:00

tacaswell force-pushed the tweak_subplots branch from 1ad9e60 to 8972645 Compare February 3, 2021 06:39

dstansby mentioned this pull request Feb 4, 2021

Add warning when subplot kwargs are ignored #19436

Closed

jklymak mentioned this pull request Feb 7, 2021

FIX: allow add option for Axes3D(fig) #19413

Closed

7 tasks

lpsinger reviewed Feb 9, 2021

View reviewed changes

lib/matplotlib/figure.py Show resolved Hide resolved

QuLogic reviewed Feb 10, 2021

View reviewed changes

doc/users/next_whats_new/axes_kwargs_collision.rst Outdated Show resolved Hide resolved

tacaswell force-pushed the tweak_subplots branch from 84a06d1 to 5d32ae0 Compare February 11, 2021 04:54

jklymak removed the request for review from astrofrog February 11, 2021 15:10

tacaswell force-pushed the tweak_subplots branch from 3ef9989 to 4c16c0d Compare February 12, 2021 05:40

tacaswell marked this pull request as ready for review February 12, 2021 05:41

QuLogic reviewed Feb 12, 2021

View reviewed changes

tacaswell force-pushed the tweak_subplots branch from 8e77575 to d63a18a Compare February 16, 2021 01:28

QuLogic approved these changes Feb 17, 2021

View reviewed changes

anntzer reviewed Feb 17, 2021

View reviewed changes

lpsinger approved these changes Feb 17, 2021

View reviewed changes

lpsinger mentioned this pull request Feb 17, 2021

DOC: fix an overlooked docs change from #19153 #19433

Closed

7 tasks

anntzer reviewed Feb 17, 2021

View reviewed changes

lib/matplotlib/pyplot.py Show resolved Hide resolved

jklymak reviewed Feb 17, 2021

View reviewed changes

tacaswell force-pushed the tweak_subplots branch from 136bf4f to 4f54d37 Compare February 18, 2021 04:38

anntzer reviewed Feb 18, 2021

View reviewed changes

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

FIX: set sca when re-using as Axes with pyplot.subplot

e47efd1

tacaswell force-pushed the tweak_subplots branch from 4f54d37 to 1ced8ca Compare February 18, 2021 22:41

tacaswell and others added 2 commits February 18, 2021 17:45

MNT: use fstring in error message

c189677

tacaswell force-pushed the tweak_subplots branch from 1ced8ca to c189677 Compare February 18, 2021 22:45

anntzer approved these changes Feb 18, 2021

View reviewed changes

QuLogic merged commit 7ae79cc into matplotlib:master Feb 19, 2021

lpsinger suggested changes Feb 19, 2021

View reviewed changes

lpsinger mentioned this pull request Feb 19, 2021

TST: lift xfail from tests fixed upstream in matplotlib astropy/astropy#11315

Merged

lpsinger added a commit to lpsinger/matplotlib that referenced this pull request Feb 19, 2021

MAINT: fix typo from matplotlib#19438

3041612

QuLogic added a commit that referenced this pull request Feb 19, 2021

Merge pull request #19541 from lpsinger/typo-in-pr-19438

cdcb729

MAINT: fix typo from #19438

tacaswell deleted the tweak_subplots branch February 19, 2021 01:40

MihaiAnton pushed a commit to MihaiAnton/matplotlib that referenced this pull request Mar 8, 2021

MAINT: fix typo from matplotlib#19438

36b4895

	bbox = ax.bbox
	axes_to_delete = []
	for other_ax in fig.axes:
	if other_ax == ax:
	continue
	if bbox.fully_overlaps(other_ax.bbox):
	axes_to_delete.append(other_ax)
	for ax_to_del in axes_to_delete:
	delaxes(ax_to_del)

FIX: restore creating new axes via plt.subplot with different kwargs #19438

FIX: restore creating new axes via plt.subplot with different kwargs #19438

Conversation

tacaswell commented Feb 2, 2021 • edited Loading

PR Summary

PR Checklist

tacaswell commented Feb 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dstansby left a comment

Choose a reason for hiding this comment

anntzer commented Feb 2, 2021 • edited Loading

jklymak commented Feb 2, 2021

anntzer commented Feb 2, 2021

jklymak commented Feb 2, 2021

anntzer commented Feb 2, 2021

tacaswell commented Feb 3, 2021

jklymak commented Feb 3, 2021

tacaswell commented Feb 3, 2021

anntzer commented Feb 3, 2021

lpsinger commented Feb 9, 2021

astrofrog commented Feb 10, 2021

anntzer commented Feb 11, 2021

tacaswell commented Feb 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacaswell Feb 17, 2021 • edited Loading

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

Choose a reason for hiding this comment

jklymak Feb 17, 2021 • edited Loading

Choose a reason for hiding this comment

tacaswell commented Feb 18, 2021

lpsinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacaswell commented Feb 2, 2021 •

edited

Loading

anntzer commented Feb 2, 2021 •

edited

Loading

tacaswell Feb 17, 2021 •

edited

Loading

jklymak Feb 17, 2021 •

edited

Loading