Skip to content
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

Restore _AxesStack to track a Figure's Axes order. #19625

Merged
merged 2 commits into from
Mar 5, 2021

Conversation

QuLogic
Copy link
Member

@QuLogic QuLogic commented Mar 3, 2021

PR Summary

This is a simplified version of the AxesStack removed in #19153, but removing the key entry, and just tracking the index with the Axes. We no longer need to track keys.

Fixes #19598.

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • [n/a] New features are documented, with examples if plot related.
  • [n/a] 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).
  • [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).

@QuLogic QuLogic added this to the v3.4.0 milestone Mar 3, 2021
_api.check_isinstance(Axes, a=a)

if a in self:
return None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably should test this since it seems someone could change this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll check further, but this may not ever happen, as the Figure.add_* might handle that already now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are 3 effects of this early return:

  1. A different return value, but _AxesStack.add is only called from _add_axes_internal, and that doesn't care about the return value.
  2. The Axes doesn't get added to the stack multiple times, but that's what we want anyway.
  3. The Axes would not be made current (i.e., moved to the end), but there's a sca in _add_axes_internal, so we don't need to worry about that.

I thought some tests checked add_axes/add_subplot with an existing Axes, but it appears not if this isn't getting coverage. I will add some more tests and remove the return value, which no-one uses.

lib/matplotlib/figure.py Outdated Show resolved Hide resolved
@tacaswell tacaswell added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Mar 3, 2021
@tacaswell
Copy link
Member

To summarize?

  • we used the stack to track three things: what Axes are in the Figure, the order they were added in, and which one is the "current" figure. When we removed the key-tracking we lost the ability to track the order they were added in inadvertently.
  • Long term it may be simpler to switch to a list and pointer, but this is the lightest touch fix to get back the old behavior.

@@ -622,9 +622,6 @@ def __len__(self):
def __getitem__(self, ind):
return self._elements[ind]

def as_list(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this method removed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was moved from _AxesStack to the base class; now that _AxesStack is back, it's not needed there. Better not to add more public API if not necessary.

@jklymak
Copy link
Member

jklymak commented Mar 4, 2021

I'll wait until today's meeting to merge in case there are any other issues but I think this is good.

@QuLogic
Copy link
Member Author

QuLogic commented Mar 5, 2021

So we can merge now?

@jklymak jklymak merged commit aed5a09 into matplotlib:master Mar 5, 2021
@jklymak
Copy link
Member

jklymak commented Mar 5, 2021

yes 😄

@QuLogic QuLogic deleted the fix-figure-order branch March 5, 2021 22:47
@QuLogic
Copy link
Member Author

QuLogic commented Mar 5, 2021

I don't know why meeseeks is randomly not backporting.

@meeseeksdev backport to v3.4.x

meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Mar 5, 2021
QuLogic added a commit that referenced this pull request Mar 6, 2021
…625-on-v3.4.x

Backport PR #19625 on branch v3.4.x (Restore _AxesStack to track a Figure's Axes order.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Axes order changed in 3.4.0rc1
5 participants