Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Axesbase #1032

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants
Member

WeatherGod commented Jul 21, 2012

This is a rather involved and invasive PR, and isn't ready for merging. But I wanted to give others a chance to try it out and comment on what I have done.

Essentially, I want to segregate the plotting functions from the maintenance methods in the Axes class. There are several reasons for this, some of which is too involved to go into right now. A practical upshot of this is that the API page for axes.py is much smaller! Further, as I move methods over to AxesBase, I have been making them axis-agnostic. I want to eventually eliminate ax.xaxis and ax.yaxis in favor of ax.axis['x'] dictionaries. The idea is that most methods actually do not care about which axis objects to act on be made into a method that operates over an ordered dictionary of axis objects. This will allow for arbitrary names of axis and an arbitrary number of axis objects.

I have put in backwards-compatibility shims where needed. It is still intended that everyone should subclass the Axes class for their uses until this refactor is completed. And even after the refactor, everyone's existing code should still work if they are subclassing Axes instead of AxesBase.

There is only one caveat I could find. The compatibility shim I used for ax.xaxis and ax.yaxis is to define some class properties to intercept the gets and sets to the internal dictionary. This means that xaxis and yaxis exist from the onset of the Axes construction, whereas before, the xaxis and yaxis didn't exist until sometime during the Axes constructor. So, any code that checked for the existance of xaxis and yaxis before acting on them should be changed to check for None.

So,
hasattr(ax, 'xaxis')
should be changed to
ax.xaxis is not None

WeatherGod added some commits Jul 19, 2012

@WeatherGod WeatherGod Initial effort to split out from the Axes class an AxesBase class 9a1e938
@WeatherGod WeatherGod More Axes-to-AxesBase refactor
 * Moved nearly all axis-agnostic methods to AxesBase
 * Added compatibility shims for self.xaxis and self.yaxis
 * Fixed PEP8 issues
 * Fixed outdated python-isms such as '<>'.
 * All hasattr(ax, 'xaxis') calls must now be replaced with
   ax.xaxis is not None and the same for yaxis.
cc91426

@pelson pelson commented on the diff Jul 21, 2012

lib/matplotlib/axesbase.py
@@ -0,0 +1,1039 @@
+
+# We need this future import because we unfortunately have
+# a module name collision with the standard module called "collections".
+from __future__ import absolute_import
+from collections import defaultdict, OrderedDict
@pelson

pelson Jul 21, 2012

Member

can you not just do import matplotlib.collections?

@WeatherGod

WeatherGod Jul 21, 2012

Member

You are getting mixed up. I want defaultdict and OrderedDict from the standard library's "collections" module, not matplotlib's. But because axesbase.py is in the same directory as matplotlib's collections.py, pre-py3k import rules will accidentially grab matplotlib's collections module. It is for this exact reason that implicit relative imports is being deprecated.

@pelson

pelson Jul 22, 2012

Member

Ah yes. Ignore me, I misread.

@pelson pelson commented on the diff Jul 21, 2012

lib/matplotlib/axesbase.py
+ :class:`~matplotlib.axes.Axes` class into their own base class.
+ These methods are largely generalized maintenance operations and
+ other book-keeping actions. Ultimately, the objective is for the
+ Axes class will contain only pure plotting methods.
+
+ It is still intended for developers and extension writers to continue
+ subclassing the Axes class, and no existing code will be broken by
+ this migration effort.
+
+ """
+
+ _shared_axes = defaultdict(cbook.Grouper)
+ _axis_classes = {}
+ _axis_locs = {}
+
+ def __init__(self, fig, rect, axis_names,
@pelson

pelson Jul 21, 2012

Member

Would need a docstring.

@WeatherGod

WeatherGod Jul 21, 2012

Member

Yes, I plan on moving over a significant portion of Axes's docstring over to here, and have Axes's docstring be built partly from it.

Member

pelson commented Jul 21, 2012

Do the tests still pass?

This is a rather involved and invasive PR

Yes, do you have you sights set on subclassing AxesBase for mplot3d? Otherwise, I cannot see that any 2d Axes subclass would want to subclass anything below Axes.

I want to eventually eliminate ax.xaxis and ax.yaxis in favor of ax.axis['x'] dictionaries

I think that is a worthwhile ambition, but I believe it could be done without such an invasive change (as could splitting the Axes class documentation).

Despite my comments, I actually don't have a problem with this change, although it would be helpful/interesting to know some of your other motivations.

Member

WeatherGod commented Jul 21, 2012

All but one test pass, but that one seems to be a minor rendering error with stix fonts. My motivations with mplot3d is that I eventually want to be able to create 3d axes objects by taking any of the 2D axes, and sticking a 3rd dimension on them. Imagine being able to take a polar plot or one of the geo axes and have a z-axis added on to it. Such an axes would then implicitly inherit all of that axes's plotting functions. Furthermore, in mplot3d, I find myself having to copy-n-paste many axes methods just for the sake of the third axis, and then that code gets out of date or a transcription error happened, rendering it as broken code. Furthermore, there are several axes methods that performs an action for explicitly the x and y axis objects. mplot3d then has to over-ride that function, call it, and then add one more instance of that code just for the zaxis. The same issues apply here with transcription errors and out-of-date code. We should have to maintain this stuff in two places.

There is also discussion that the plotting functions will eventually be turned into something more like mathmatica's and chacao's plotting elements, so by working to separate out the plotting and maintenance portions of Axes would be a first step towards this goal. And yes, I do eventually intend for AxesBase to be directly subclassed by others eventually.

Owner

efiring commented Jul 21, 2012

I like the idea of separating the basic Axes functionality from all the plotting machinery, and would prefer to see more of the plotting machinery broken out into related chunks, as with quiver and contour, for example. So I think there is justification for what you are doing here even without mplot3d.

As for mplot3d, I worry about continuing to pour resources (your time, mostly) into an approach that is fundamentally a hack to simulate 3d, and that therefore has major inherent limitations. Granted, it has quite a few users, judging from the email traffic.

Member

WeatherGod commented Aug 17, 2012

I am going to close this PR of mine, and turn it into a MEP instead. These concepts need to be fleshed out better in planning.

@WeatherGod WeatherGod closed this Aug 17, 2012

Owner

mdboom commented Aug 17, 2012

I think this a great idea for a MEP. What's here is already quite good.

I'm sorry I haven't commented on this until now -- I had meant to and it slipped by me. This is something I've thought should be done for a long time, so it's great to see the effort on it.

One thing to consider as you draft the MEP: I've long thought that it would be helpful to group the plotting methods into categories, at least as to where their implementations are, to prevent this sort of "enormous file with no logical ordering" property we have now. We already do this -- sort of -- with the contour and triangulation methods. I'd like to see more of that, for example a module for bar plots, one for pcolor/pcolormesh and friends, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment