Skip to content

Deprecate LocatableAxes from toolkits #10403

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

Merged
merged 6 commits into from
Jul 19, 2018

Conversation

QuLogic
Copy link
Member

@QuLogic QuLogic commented Feb 9, 2018

PR Summary

LocatableAxes in the toolkits provides stuff like get_axes_locator, but these are provided in the base Axes class. So deprecate all of the classes and related functions. Additionally, remove all code from the deprecated axes_grid, and point it to the split apart implementations.

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • [N/A] New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • [N/A] Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@QuLogic QuLogic force-pushed the deprecate-locatable-axes branch 3 times, most recently from 9743d47 to e994a5f Compare February 9, 2018 06:35
@tacaswell tacaswell added this to the v3.0 milestone Feb 9, 2018
@tacaswell
Copy link
Member

I would rather push this to 3.0 (hoping to tag 2.2RC1 this weekend....only a week late)

@efiring
Copy link
Member

efiring commented Feb 10, 2018

I haven't looked in detail, but I will be glad to see this go in whenever it does, and the duplication go away.

@QuLogic QuLogic force-pushed the deprecate-locatable-axes branch from e994a5f to 2805e5c Compare February 12, 2018 05:39
@QuLogic
Copy link
Member Author

QuLogic commented Feb 12, 2018

Updated deprecation messages to say 3.0 instead of 2.2.

@QuLogic QuLogic force-pushed the deprecate-locatable-axes branch from 2805e5c to 6dfa3dd Compare February 22, 2018 21:23
@QuLogic
Copy link
Member Author

QuLogic commented Feb 22, 2018

Rebased due to conflicts. These files are messy, so it would be nice to get this in quicker to reduce conflicts.

Copy link
Member

@efiring efiring left a comment

Choose a reason for hiding this comment

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

This seems low-risk; let's get it in.

@efiring
Copy link
Member

efiring commented Feb 25, 2018

@anntzer or @dopplershift, perhaps one of you could have a look and merge this if it looks OK, so it doesn't have to be rebased again.

@anntzer
Copy link
Contributor

anntzer commented Feb 25, 2018

I'm not even close to knowing enough about axes_locator to decide whether the functionality is indeed duplicated or not.

@QuLogic
Copy link
Member Author

QuLogic commented Feb 25, 2018

Mainly you can look at the LocatableAxesBase class which adds [gs]et_axes_locator and then modifies apply_aspect to use it before drawing. But looking at the regular _AxesBase.draw, it does the same thing.

if 'sharex' in kwargs and 'sharey' in kwargs:
raise ValueError("Twinned Axes may share only one axis.")
ax2 = type(self)(self.figure, self.get_position(True), *kl, **kwargs)
ax2.set_axes_locator(self.get_axes_locator())
Copy link
Member

Choose a reason for hiding this comment

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

The base Axes._make_twin_axes is missing this locator-copying line--maybe that is a bug.

@efiring
Copy link
Member

efiring commented Feb 26, 2018

LocatableAxesBase dates all the way back to 8e3b931, Dec. 9, 2008, when all of the AxesGrid family was started. That original commit (which looks like a combination of SVN commits?) includes the duplication of get_axes_locator, etc., and its use in draw prior to apply_aspect, in Axes and in LocatableAxesBase. It looks to me like originally it was in the latter, and then @leejjoon decided to add it to Axes but didn't remove it from LocatableAxesBase. The _axes_locator is used only in connection with the AxesGrid family, as far as I can see; there are references to it in tight_layout and bbox_inches=tight, but only the AxesGrid family actually sets it.

It looks to me like we need to either support _axes_locator more thoroughly in Axes (which at the moment means taking care to keep it synchronized in the twin case, requiring a modification of the setter as well as of _make_twin_axes), or strip it out of Axes and leave the locatable axes class, factory function, etc. in place, possibly with some changes.

I'm not sure which would be best; it's a little odd to have an attribute in Axes that is used only by a toolkit, isn't it? On the other hand, the cost is fairly low.

@QuLogic
Copy link
Member Author

QuLogic commented Feb 26, 2018

There are times when @leejjoon recommended using axes_locator and not the full toolkit, so I think the intention was to put it into the base library, but I can't say that 100%. I can also find a random use in the wild, but it's hard to search when a bunch of people commit their venvs to git.

@jklymak
Copy link
Member

jklymak commented May 8, 2018

Ooh, this fell through the cracks. Any chance we can resurrect?

@jklymak jklymak modified the milestones: v3.0, v3.1 Jul 9, 2018
@QuLogic
Copy link
Member Author

QuLogic commented Jul 18, 2018

I can rebase; depends on @efiring's thoughts.

@leejjoon
Copy link
Contributor

My opinion is highly biased as I am the one who introduced the axes_locator. But I would like to see it stays. While I understand this is underused, this makes certain things a lot easier (although may not relevant for normal users). Also, #11026 requires axes_locator.

@QuLogic
Copy link
Member Author

QuLogic commented Jul 18, 2018

This doesn't remove it though; it deprecates the one in axes_grid for the one in the main library, which appears to be what #11026 uses.

@leejjoon
Copy link
Contributor

Yes. I was just expressing my opinion to what @efiring said, and I was referring to axes_locator in the base. I am happy with the axes_grid becoming deprecated.

@efiring
Copy link
Member

efiring commented Jul 18, 2018

I'm more than a little confused--I haven't looked at this for a while, it deals with functionality I am not familiar with, and so the spinup in trying to come back to it is considerable. @leejjoon, it's good to hear from you--but could you be more explicit with respect to this PR? Do you favor it as is (after a rebase), or with minor changes, or major changes, or not at all?

@leejjoon
Copy link
Contributor

I am in favor of this PR. In fact, I think we can actually remove axes_grid toolkit from the code base.
On the other hand, I would prefer _axes_locator stays within Axes. Let me know if I am still not clear.

@efiring
Copy link
Member

efiring commented Jul 18, 2018

@leejjoon, thanks very much, that's what I thought. @QuLogic, let's try to get this in now. Once it is rebased and passes the tests, anyone is welcome to merge it.

@QuLogic QuLogic force-pushed the deprecate-locatable-axes branch from 6dfa3dd to 36e3701 Compare July 18, 2018 22:54
QuLogic added 5 commits July 18, 2018 19:02
This is provided for backwards-compatibility, so point directly to the
implementations instead of the compatibility location.
All its functionality is provided by the matplotlib.axes.Axes class now
so it does not need to exist as all alternative Axes classes derive from
the main one.
It's similar to LocatableAxesBase and provided in base Axes classes.
It's now no longer used for anything, since maxes.Axes is locatable
already.
@QuLogic QuLogic force-pushed the deprecate-locatable-axes branch from 36e3701 to 7421b0e Compare July 18, 2018 23:04
@QuLogic
Copy link
Member Author

QuLogic commented Jul 19, 2018

OK, rebase is done.

@efiring efiring merged commit 2eb26ee into matplotlib:master Jul 19, 2018
@QuLogic QuLogic deleted the deprecate-locatable-axes branch July 19, 2018 08:06
@QuLogic QuLogic modified the milestones: v3.1, v3.0 Jul 19, 2018
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.

6 participants