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

Turn shared_axes, stale_viewlims into {axis_name: value} dicts. #20407

Merged
merged 1 commit into from Jul 7, 2021

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Jun 10, 2021

This means that various places can now iterate over the dicts
and directly support 3D axes: Axes3D doesn't need to override
_request_autoscale_view or _unstale_viewLim or set_anchor anymore, and
can shed most of set_aspect; various subtle points that were missing
before also get fixed (restoring z-axis sharing upon unpickling;
resetting locators and formatters after deleting an Axes3D sharing a
z-axis with another still present Axes3D); etc.

There's just some slightly annoying interaction with ColorbarAxes
vampirizing its inner_ax, but that can be worked around.

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

Copy link
Member

@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

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

Also needs some small doc fix.

lib/matplotlib/axis.py Outdated Show resolved Hide resolved
lib/matplotlib/axis.py Outdated Show resolved Hide resolved
lib/matplotlib/figure.py Outdated Show resolved Hide resolved
This means that various places can now iterate over the dicts
and directly support 3D axes: Axes3D doesn't need to override
_request_autoscale_view or _unstale_viewLim or set_anchor anymore, and
can shed most of set_aspect; various subtle points that were missing
before also get fixed (restoring z-axis sharing upon unpickling;
resetting locators and formatters after deleting an Axes3D sharing a
z-axis with another still present Axes3D); etc.

There's just some slightly annoying interaction with ColorbarAxes
vampirizing its inner_ax, but that can be worked around.
Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

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

Since we now have a number of getattr(ax, f"{name}axis"), we may take this one step further and also store the Axises in an internal dict and turn the *axis attributes into properties mapping to that dict:

@property
def xaxis(self):
    return self._axises['x']

But this does not have to be part of this PR.

grouper = getattr(self, grouper_name)
state[grouper_name] = (grouper.get_siblings(self)
if self in grouper else None)
state["_shared_axes"] = {
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean we break pickling-compatibility between old and implementation? If so this needs an API-change note.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The pickling representation is explicitly not stable (see __mpl_version__ in Figure.__setstate__), changes fairly regularly without API note (nearly every time we add an attribute to Figure or Axes, we make old pickles unloadable on new versions, as they would be missing that attribute), and I wouldn't want to start pretending that we put any care in making the pickles stable.

@anntzer
Copy link
Contributor Author

anntzer commented Jun 12, 2021

Agreed that making the axis_map the "real" underlying representation may be best, and also agreed that it can wait for another PR :)

Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

looks helpful to me!

@jklymak jklymak merged commit 8c764dc into matplotlib:master Jul 7, 2021
@QuLogic QuLogic added this to the v3.5.0 milestone Jul 7, 2021
@anntzer anntzer deleted the axis_name-shared-stale branch July 7, 2021 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants