Deprecate LocatableAxes from toolkits #10403

QuLogic · 2018-02-09T04:12:16Z

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

tacaswell · 2018-02-09T22:33:52Z

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

efiring · 2018-02-10T00:24:04Z

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 · 2018-02-12T05:39:27Z

Updated deprecation messages to say 3.0 instead of 2.2.

QuLogic · 2018-02-22T21:25:06Z

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

efiring

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

efiring · 2018-02-25T21:56:42Z

@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 · 2018-02-25T22:42:21Z

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

QuLogic · 2018-02-25T23:17:11Z

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.

efiring · 2018-02-25T23:33:08Z

lib/mpl_toolkits/axes_grid1/axes_divider.py

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


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

efiring · 2018-02-26T02:34:39Z

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 · 2018-02-26T04:18:35Z

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 · 2018-05-08T04:34:59Z

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

QuLogic · 2018-07-18T03:04:10Z

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

leejjoon · 2018-07-18T05:18:12Z

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 · 2018-07-18T05:23:38Z

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 · 2018-07-18T05:47:37Z

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 · 2018-07-18T06:56:35Z

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 · 2018-07-18T08:16:37Z

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 · 2018-07-18T17:01:34Z

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

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 · 2018-07-19T00:21:54Z

OK, rebase is done.

QuLogic force-pushed the deprecate-locatable-axes branch 3 times, most recently from 9743d47 to e994a5f Compare February 9, 2018 06:35

tacaswell added this to the v3.0 milestone Feb 9, 2018

QuLogic added the topic: mpl_toolkit label Feb 9, 2018

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

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

efiring approved these changes Feb 22, 2018

View reviewed changes

efiring reviewed Feb 25, 2018

View reviewed changes

QuLogic mentioned this pull request Feb 26, 2018

Random image failures with test_curvelinear4 #10602

Closed

jklymak modified the milestones: v3.0, v3.1 Jul 9, 2018

Remove old ChangeLog from axes_grid.

eee9112

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

Remove cross-references to axes_grid.

ff6139c

This is provided for backwards-compatibility, so point directly to the implementations instead of the compatibility location.

Deprecate LocatableAxesBase and make it a no-op.

15d9b8e

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.

Deprecate locatable_axes_factory and make it a no-op.

947ebe9

It's similar to LocatableAxesBase and provided in base Axes classes.

Deprecate any LocatableAxes in toolkits.

e041adc

It's now no longer used for anything, since maxes.Axes is locatable already.

DOC: Add deprecation changelog for LocatableAxes stuff.

7421b0e

QuLogic force-pushed the deprecate-locatable-axes branch from 36e3701 to 7421b0e Compare July 18, 2018 23:04

jklymak approved these changes Jul 19, 2018

View reviewed changes

efiring merged commit 2eb26ee into matplotlib:master Jul 19, 2018

QuLogic deleted the deprecate-locatable-axes branch July 19, 2018 08:06

QuLogic modified the milestones: v3.1, v3.0 Jul 19, 2018

anntzer mentioned this pull request Jul 6, 2019

mpl_toolkits.axes_grid1.axes_divider.LocatableAxes has inconsistent ticks #7378

Closed

Uh oh!

Deprecate LocatableAxes from toolkits #10403

Deprecate LocatableAxes from toolkits #10403

Uh oh!

Conversation

QuLogic commented Feb 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

tacaswell commented Feb 9, 2018

Uh oh!

efiring commented Feb 10, 2018

Uh oh!

QuLogic commented Feb 12, 2018

Uh oh!

QuLogic commented Feb 22, 2018

Uh oh!

efiring left a comment

Choose a reason for hiding this comment

Uh oh!

efiring commented Feb 25, 2018

Uh oh!

anntzer commented Feb 25, 2018

Uh oh!

QuLogic commented Feb 25, 2018

Uh oh!

efiring Feb 25, 2018

Choose a reason for hiding this comment

Uh oh!

efiring commented Feb 26, 2018

Uh oh!

QuLogic commented Feb 26, 2018

Uh oh!

jklymak commented May 8, 2018

Uh oh!

QuLogic commented Jul 18, 2018

Uh oh!

leejjoon commented Jul 18, 2018

Uh oh!

QuLogic commented Jul 18, 2018

Uh oh!

leejjoon commented Jul 18, 2018

Uh oh!

efiring commented Jul 18, 2018

Uh oh!

leejjoon commented Jul 18, 2018

Uh oh!

efiring commented Jul 18, 2018

Uh oh!

QuLogic commented Jul 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

QuLogic commented Feb 9, 2018 •

edited

Loading