Skip to content
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

Clean up imports #2387

Closed
mdboom opened this issue Sep 6, 2013 · 11 comments
Closed

Clean up imports #2387

mdboom opened this issue Sep 6, 2013 · 11 comments
Milestone

Comments

@mdboom
Copy link
Member

mdboom commented Sep 6, 2013

matplotlib's import sections at the top of files are really messy.

We should go through and:

  1. Convert everything to absolute imports where appropriate (to avoid problems like the ambiguity between collections and matplotlib.collections, for example)
  2. Remove unused imports
  3. Per PEP8, sort imports into:
  • standard library imports
  • related third party imports
  • local application/library specific imports
    4) Favor importing modules rather than members of modules -- that leads to a large global namespace in the module with larger potential for namespace clashes.

Additionally, I've never been much of a fan of

from matplotlib import transforms as mtransforms

though I understand the need when the module name is too generic. I'd prefer to reduce this usage by renaming where appropriate.

Perhaps instead we do:

from matplotlib import transforms as mpl_transforms

Before I embark on this, I thought I'd get some feedback. @pelson, @dmcdougall, @NelleV, @WeatherGod, @efiring

@WeatherGod
Copy link
Member

+1 on import cleanups. -1 on changing conventions away from mtransforms. A convention is a convention, and I don't see how mpl_transforms is any better than mtransforms.

@WeatherGod
Copy link
Member

Also, we do need to be careful with removing "unneeded" imports. For example, there is probably some confusion over which module to import Colormaps from (you can import it from both colors.py and cm.py, and guess which module the class is defined?!)

@mdboom
Copy link
Member Author

mdboom commented Sep 6, 2013

Good point about unneeded imports. I was thinking more along the lines of things from stdlib etc. Things from within matplotlib should be taken with much more care -- and maybe things like that we come across should be commented so we know they are being kept around for API reasons.

@mrterry
Copy link
Contributor

mrterry commented Sep 6, 2013

+1 It is my own dumb fault for turning flake8 up to 11, but this sounds great and will reduce the number of not-actually-errors I sift through.

@dmcdougall
Copy link
Member

I think a good first step is to cleanup the 'unneeded' inputs. By this I don't mean simply remove them, instead I suggest reworking the design of the relevant module.

@efiring
Copy link
Member

efiring commented Sep 8, 2013

@dmcdougall Would you elaborate, please? I have no idea what you mean.
@mdboom, I agree with the general cleanup idea, with @WeatherGod's provisos. I don't see any point in mpl_transforms; better to just use mpl.transforms if an alternative to mtransforms is needed.
@WeatherGod, maybe long-term cm and colors should be consolidated or renamed? I have never been able to remember for more than a few minutes what is in each of these, so I simply look in one, and often as not I don't find what I am looking for and then have to go look in the other one.

@dmcdougall
Copy link
Member

@dmcdougall Would you elaborate, please? I have no idea what you mean.

For example, in axes/_axes.py, see this line that registers a date converter. It'd be much cleaner, in my opinion, to import a specific function from matplotlib.dates, say register_date_converter, that does the registering for you. Then the messy import as _ is cleaned up and turned into from matplotlib.dates import register_date_converter, rather than relying on the global namespaces of matplotlib.dates.

If I recall correctly, it is the case that this same line set off some alarms with pyflakes for those attempting to contribute code that adheres to the PEP8 standard.

@dmcdougall
Copy link
Member

@mdboom, I agree with the general cleanup idea, with @WeatherGod's provisos. I don't see any point in mpl_transforms; better to just use mpl.transforms if an alternative to mtransforms is needed.

I'm open here. I don't really mind which one is used within the codebase; the mfoo custom is fairly well hidden from users. If you're wanting to try and change the custom users are exposed to, would this just be a case of modifying the examples to use mpl_foo instead of mfoo?

@WeatherGod, maybe long-term cm and colors should be consolidated or renamed? I have never been able to remember for more than a few minutes what is in each of these, so I simply look in one, and often as not I don't find what I am looking for and then have to go look in the other one.

I agree. Furthermore, the handle cm has never been didactic in my opinion. Consolidating colors and cm offers the benefit of less confusion.

@efiring
Copy link
Member

efiring commented Sep 8, 2013

On 2013/09/08 10:20 AM, Damon McDougall wrote:

For example, in |axes/_axes.py|, see this line
https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/axes/_axes.py#L21
that registers a date converter. It'd be much cleaner, in my opinion, to
import a specific

Agreed! That is one UGLY line!

@tacaswell tacaswell added this to the v1.5.x milestone Aug 17, 2014
@tacaswell tacaswell modified the milestones: v2.0, v1.5.x Nov 26, 2014
@tacaswell tacaswell modified the milestones: v1.5.x, v2.0 Nov 26, 2014
@efiring
Copy link
Member

efiring commented Feb 16, 2015

@mdboom, I think that as part of the PEP-8-ification, much of this has been done--maybe most of the noncontroversial parts. Can this issue be closed?

@pelson
Copy link
Member

pelson commented Feb 20, 2015

Yep. Not all of the suggestions have been done, and there is always room for improvement, but I don't think the existence of this issue will make any difference to any future work.

@pelson pelson closed this as completed Feb 20, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants