Skip to content

Refactoring the axes module (part II) #2213

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

Closed
wants to merge 12 commits into from
Closed

Refactoring the axes module (part II) #2213

wants to merge 12 commits into from

Conversation

NelleV
Copy link
Member

@NelleV NelleV commented Jul 11, 2013

This is part of the refactoring process. This PR is still work in progress.

All plots methods should be moved to their own modules.

In this PR, I've started the refactoring by moving the axhline to its own module, lines, which should contain lines and spans
plotting function.

What is left to do before merging:

  • Study the possibility of using mixins instead of manually updating the Axes class

@pelson
Copy link
Member

pelson commented Jul 11, 2013

This is the kind of direction I had in mind also. My advice for this whole change is to produce many PRs each of which pulls out a group of methods into a sub package. That will keep the whole process moving more smoothly IMHO.

@NelleV
Copy link
Member Author

NelleV commented Jul 11, 2013

That's what I was thinking too. I'd like to take advantage of this refactoring to also move things slowly to MEP10. I need #2209 merged before this one to finish up on the MEP10 of these plotting functions thought.

@tacaswell
Copy link
Member

The naming might be a little confusing (sorry if this was discussed at length and I missed it/can't find it). _lines seems to generic and one might expect to find plot, or anything which generates which generates a Line2D object or LineCollection to be found in it.

@NelleV
Copy link
Member Author

NelleV commented Jul 11, 2013

@tacaswell the splitting and naming of modules haven't been discussed. I'm right now following the namings and splittings which are already done in the axes module. Any suggestion is welcomed.

@WeatherGod
Copy link
Member

Now might not be the time to concern ourselves with the specific namings of
existing variable names. Because there is so much code churn that is
happening right now, I would feel more comfortable if issues like the
_lines name that come up during these reviews are made into a separate
issue tickets that we can address after the main refactor is complete and
settled down.

@NelleV
Copy link
Member Author

NelleV commented Jul 11, 2013

Sounds good to me, specially as these modules are private. The users should never have access to it, which simplified further refactoring/renaming if necessary.

@mdboom
Copy link
Member

mdboom commented Jul 16, 2013

Looks good, and I like the idea of putting the methods into loose categories like this. There may be some gray lines between categories -- I think that's unavoidable -- but something is better than nothing.

I do have one concern though -- we're replacing all of the methods in the main Axes class with stubs that chain onto the ones in the "category-specific" module. I'm worried that over time these two may accidentally get out of sync, and adding new methods will require updating things in two places. We have this kind of thing now in pyplot.py, and its whole boilerplate generator, and it's kind of a mess and picky. I'd like to get away from doing that kind of thing again.

What if, instead, we import the _lines module and then add all of its functions to the Axes class programmatically? I know it's a bit "magical", but it gets around a lot of this duplication. It would also provide a way for third-party packages to add plotting commands to the Axes namespace.

NelleV added 9 commits July 18, 2013 11:52
This is part of the refactoring process. All plots methods should be moved to
their own modules. In this commit, I've started the refactoring by moving the
axhline to its own module, lines, which should contain lines and spans
plotting function.
@NelleV
Copy link
Member Author

NelleV commented Jul 18, 2013

I think this is ready to be merged. I've moved all the docstrings of the _lines submodule to MEP10.

@mdboom I think we can address the issue of manual or automatic updates of axes later on. I think it might even be worth writing a MEP for this, as there seems to be divergent ideas on how to do this.

@mdboom
Copy link
Member

mdboom commented Jul 18, 2013

I don't know if we need a whole MEP for that, and I'd rather we do something about the duplication up front -- we will have a lot more follow-ons like this one for other categories of plotting methods, and I'd like to get it right (or at least close to right) up front. I think @efiring's suggestion (on the mailing list) to have Axes inherit some the categories of methods as mixins is not a bad one -- it will mean Axes inherits from a lot of things, but it doesn't involve writing any code to merge the methods in etc. It doesn't seem like that would be too difficult.

@NelleV
Copy link
Member Author

NelleV commented Jul 18, 2013

I'll check out how to use mixins, but I'm not convince this is going to make things easier to maintani.. Using mixins is the first thing I thought of when starting the axes module, and I gave up on that idea quite quickly.

@mdboom
Copy link
Member

mdboom commented Jul 18, 2013

What were the specific downsides of the mixins? They would remove the need for the stub functions, of course, which is my main concern. What problems do they introduce?

@NelleV
Copy link
Member Author

NelleV commented Jul 18, 2013

Right now, I don't even see how this could work.

@mdboom
Copy link
Member

mdboom commented Jul 18, 2013

What I was thinking (based on @efiring's description), is that _lines.py would have a class like this:

class LinesMethods(object):
    def axhspan(self, ymin, ymax, xmin=0, xmax=1, **kwargs):
         # The full implementation goes here...

and then in _axes.py:

class Axes(_lines.LineMethods, _AxesBase):
    # exactly what we have now, but no stub methods for the methods in _lines.py

Does that work? I admit, I haven't tried this, so I very well might be missing something.

@NelleV
Copy link
Member Author

NelleV commented Jul 19, 2013

I've attempted something similar, and it didn't work. I've never used mixins for this, so I probably should read a bit more about it before attempting anything else.
Up to now, I had only used mixins with classes that have completely different attributes. Here, Axes and LinesMethods both need to inherit from _AxesBase, and hence to have the same attributes.

@efiring
Copy link
Member

efiring commented Jul 19, 2013

@NelleV, I don't think LinesMethods needs to inherit from anything. It is not in any way a standalone class, it is just a way of adding methods to Axes. At runtime, a method from LinesMethods sees exactly the same attributes as if it had been defined in Axes instead. It's "self" is an Axes instance.

Lines and Spans function are set to be in a class.
Axes now inherits from _AxesBase, and LinesAndSpans is a mixin
@NelleV
Copy link
Member Author

NelleV commented Jul 19, 2013

@efiring Indeed, now it works (and now I'm really confused about mixins...). I'll run the tests, and update the PR.

@tacaswell
Copy link
Member

What is the state of this PR?

@tacaswell
Copy link
Member

@NelleV I am closing this PR as it no longer will merge and I think we should think bigger on this sort of thing.

Please reopen if you disagree.

@tacaswell tacaswell closed this Nov 12, 2014
@QuLogic QuLogic modified the milestones: unassigned, proposed next point release (2.1) Oct 11, 2015
@QuLogic QuLogic changed the title MRG Refactoring the axes module (part II) Refactoring the axes module (part II) Dec 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants