Some general cleanups by anntzer · Pull Request #6573 · matplotlib/matplotlib

anntzer · 2016-06-11T06:56:05Z

Remove some very old compatibility layers (sets module, md5 module, etc.)
Use astype(float) instead of astype(np.float) and astype(np.float_)
Use isinstance(..., MaskedArray) instead of isMaskedArray / isMA.

Some other cleanup ideas:

Get rid of if debug prints (mostly present in backend implementations just to indicate that a method has been called): I think that a tool such as https://pypi.python.org/pypi/hunter is typically more appropriate anyways.
Reimplement the verbose class on top of the stdlib logging and read off an environment variable such as MPLVERBOSITY instead of command-line arguments which are likely to be set for other purposes.

- Remove some very old compatibility layers (sets module, md5 module, etc.) - Use `astype(float)` instead of `astype(np.float)` and `astype(np.float_)` - Use `isinstance(..., MaskedArray)` instead of `isMaskedArray` / `isMA`.

efiring · 2016-06-11T07:20:54Z

On 2016/06/10 8:56 PM, Antony Lee wrote:

Use |isinstance(..., MaskedArray)| instead of |isMaskedArray| / |isMA|.

What is the advantage of this change?

jenshnielsen · 2016-06-11T09:33:08Z

lib/matplotlib/transforms.py


 DEBUG = False
-# we need this later, but this is very expensive to set up
-MINFLOAT = np.MachAr(float).xmin


Have you benchmarked this change. It was done like this for performance reasons. See
2d0cc7d

Ok read the docs it seems like finfo is a cached version of MachAr so it is probably fine

It is a slower than just reading off a global but still much faster than MachAr and even than allocating a new array of size 1.

I am also mildly concerned about this, but this is almost certainly not the bottle neck any more.

anntzer · 2016-06-11T15:48:07Z

re: isinstance(..., MaskedArray). I always felt a bit uneasy about isMaskedArray / isMA: when I read a function like this my first instinct is "oh, I guess if there's such a function it must do a bit more than checking the type (does it check exact type equality ignoring subclassing? does it do something with array-likes? etc.)". But in fact it's just implemented (after a page-long docstring...) just as a call to isinstance.
Say Python suddenly decided to add an isfloat() function... I think it would widely be considered as unpythonic. I think the same of isMaskedArray.
On the other hand I hardly feel strongly about this change.

efiring · 2016-06-11T16:47:57Z

On 2016/06/11 5:48 AM, Antony Lee wrote:

re: |isinstance(..., MaskedArray)|. I always felt a bit uneasy about
|isMaskedArray| / |isMA|: when I read a function like this my first
instinct is "oh, I guess if there's such a function it must do a bit
more than checking the type (does it check exact type equality ignoring
subclassing? does it do something with array-likes? etc.)". But in fact
it's just implemented (after a page-long docstring...) just as a call to
|isinstance|.
Say Python suddenly decided to add an |isfloat()| function... I think it
would widely be considered as unpythonic. I think the same of
|isMaskedArray|.
On the other hand I hardly feel strongly about this change.

I have no objection to making the change; your rationale is fine.

tacaswell · 2016-06-12T22:38:04Z

lib/matplotlib/cbook.py

    vertices = _step_validation(x, *args)
    # create the output array
-    steps = ma.zeros((vertices.shape[0], 2 * len(x) - 1), np.float)
+    steps = np.zeros((vertices.shape[0], 2 * len(x) - 1), np.float)


These got missed in the np.float -> float conversion.

It's also not the masked array zeros, if that makes a difference.

There's quite a few np.float left; this one is safe to replace but I'd need to do some more research to figure out how this changes calls to np.issubdtype and others.
WRT to np.zeros vs ma.zeros, see below.

tacaswell · 2016-06-12T22:40:14Z

Moving to use logging is yet another of the big projects that I think everyone agrees should be done, but no one has yet found the time to do.

QuLogic · 2016-06-12T22:56:15Z

lib/matplotlib/__init__.py

                                cycler)

 import numpy
+import numpy.ma


What effect does this have?

Ensure that everyone can directly use np.ma.<xyz> without importing numpy.ma.

Actually, it doesn't do anything at all. It's superfluous.

Indeed, removed.

plus other comments on the PR.

tacaswell · 2016-06-17T16:15:32Z

👍 if @efiring is happy with the way the MA stuff ended up.

General cleanups.

71d9386

- Remove some very old compatibility layers (sets module, md5 module, etc.) - Use `astype(float)` instead of `astype(np.float)` and `astype(np.float_)` - Use `isinstance(..., MaskedArray)` instead of `isMaskedArray` / `isMA`.

mdboom added the status: needs review label Jun 11, 2016

jenshnielsen reviewed Jun 11, 2016
View reviewed changes

tacaswell reviewed Jun 12, 2016
View reviewed changes

tacaswell added this to the 2.1 (next point release) milestone Jun 12, 2016

QuLogic reviewed Jun 12, 2016
View reviewed changes

Finish swapping np.float{,_} -> float; ma -> np.ma

91034ee

plus other comments on the PR.

anntzer force-pushed the cleanups branch from a101a9b to 91034ee Compare June 13, 2016 04:52

Remove a reference to Python 2.6.

850ef93

efiring merged commit 7ae1b06 into matplotlib:master Jun 17, 2016

efiring removed the status: needs review label Jun 17, 2016

anntzer deleted the cleanups branch June 28, 2016 18:55

Uh oh!

Conversation

anntzer commented Jun 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

efiring commented Jun 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anntzer commented Jun 11, 2016

Uh oh!

efiring commented Jun 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anntzer Jun 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tacaswell commented Jun 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tacaswell commented Jun 17, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

anntzer commented Jun 11, 2016 •

edited

Loading

anntzer Jun 13, 2016 •

edited

Loading