8000 [SPRINT] Refactored the axes module to split it in smaller chunks. by NelleV · Pull Request #1931 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

[SPRINT] Refactored the axes module to split it in smaller chunks. #1931

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Jun 29, 2013
Merged

[SPRINT] Refactored the axes module to split it in smaller chunks. #1931

merged 8 commits into from
Jun 29, 2013

Conversation

NelleV
Copy link
Member
@NelleV NelleV commented Apr 22, 2013

This PR is work in progress.

I've started refactoring the axes module to split it into several files. In this PR, I've split the axes 9kloc module, into 3 submodules:

  • _axes, which is "only" 6kloc, containing all the code linked to plots
  • _base (3kloc), which contains mostly a base class for Axes to inherit; It contains everything linked to drawing, interactive manipulation, data limits & ticks formatting, adding & tracking artists
  • _subplots, which contains the classes and functions about subplots.

This may not be the best way to split the axes module: feedback is welcome.

These changes are completely backward compatible, except for private methods, that are not accessible from the axes module anymore.

@efiring efiring mentioned this pull request May 21, 2013
Closed
@dmcdougall
Copy link
Member

Woah, I thought that splitting a class wasn't possible in Python. I suppose that's because I had never really utilised relative imports to their full potential. Thanks for this work, @NelleV.

dmcdougall
dmcdougall reviewed Jun 16, 2013
View reviewed changes
lib/matplotlib/axes/_subplots.py
def update_params(self):
"""update the subplot position from fig.subplotpars"""

self.figbox, self.rowNum, self.colNum, self.numRows, self.numCols = \
Copy link
Member

Choose a reason for hiding this comment

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

I presume you did a copy and paste from axes.py. There's a lingering trailing backslash here. I'm not sure if this PR is the right place to deal with PEP8 issues that linger from the old setup. What do you think?

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'm not sure. Maybe once the restructuring is completely done. I think there's still a lot of work to do on this PR.

@NelleV
Copy link
Member Author
NelleV commented Jun 28, 2013

I'm not able to reproduce the failing tests on py3k.

@dmcdougall
Copy link
Member

Great work Nelle. I'm going to do some playing locally with the OO interface just to make sure I can create different types of Axes objects.

I'm going to trust you that all the subplotting stuff was ported over correctly.

@dmcdougall
Copy link
Member

I looked at globals().keys() (sorted) before and after these changes and this is the diff:

--- before.rst  2013-06-29 11:01:12.000000000 -0500
+++ after.rst   2013-06-29 11:03:11.000000000 -0500
@@ -25,24 +25,20 @@
 _iii
 _oh
 _sh
-allow_rasterization
 cbook
 division
 docstring
 exit
-font_manager
 get_ipython
 help
 is_sequence_of_strings
 is_string_like
-itemgetter
 iterable
 itertools
 ma
 martist
 math
 matplotlib
-maxis
 mcoll
 mcolors
 mcontour
@@ -54,8 +50,6 @@
 mpatches
 mpath
 mquiver
-mscale
-mspines
 mstack
 mstream
 mtable

@NelleV
Copy link
Member Author
NelleV commented Jun 29, 2013

Most of these are other matplotlib's module, and some ipython specific stuff. Two functions are "missing" (they are both from the cbook module):

  • is_sequence_of_strings

  • is_string_like

    I think that the code is cleaner this way (in the sense that only methods that should be in the namespace are). I can add in the documentation where those modules and functions are imported from, in order for people to fix their code quickly if by mistake they used them from the axes module.

@dmcdougall
Copy link
Member

I've also constructed an Axes object programmatically using the OO interface and a basic plot command works as expected.

import matplotlib
matplotlib.use('pdf')
from matplotlib.figure import Figure
from matplotlib.backends.backend_pdf import FigureCanvasPdf as figcan

fig = Figure()
can = figcan(fig)
ax = fig.add_axes([0.1, 0.1, 0.8, 0.8])
ax.plot([1, 2, 3])
fig.savefig('asdf.pdf')

3D axes also works using the usual from mpl_tookits.mplot3d import Axes3D and setting the projection on the ax object.

Notice the repr (or str, I can never remember which one) has changed to <matplotlib.axes._axes.Axes at 0x10a0b91d0>. That said, it's reasonable for us to assume that people don't depend on that output.

@NelleV
Copy link
Member Author
NelleV commented Jun 29, 2013

I've documented the changes in the namespace of the axes module. I think this is ready to be merged.

@dmcdougall
Copy link
Member

Thanks @NelleV. I think anybody relying on, for example, martist from the axes namespace has sufficient information to update their scripts in api_changes.rst. Though this may break for some people, it wasn't well documented initially.

I'm going to merge this now. We should be aware of possible issues, but I think we've done the right thing here.

dmcdougall added a commit that referenced this pull request Jun 29, 2013
@dmcdougall
Merge pull request #1931 from NelleV/axes_module
2930635 
[SPRINT] Refactored the axes module to split it in smaller chunks.
@dmcdougall dmcdougall merged commit 2930635 into matplotlib:master Jun 29, 2013
@dmcdougall
Copy link
Member

Merged. With much trepidation.

@ajdawson ajdawson mentioned this pull request Jul 10, 2013
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@NelleV @dmcdougall
0