Additional cleanups by anntzer · Pull Request #7547 · matplotlib/matplotlib

anntzer · 2016-12-01T23:04:49Z

Call min/max directly on a generator or on two arguments rather than via a list.
Remove old __cmp__ method.
Use reshape instead of assigning to shape at least when it looks more legible.
Use np.min instead of amin, np.max (amax), np.abs (fabs, absolute), np.conj (conjugate).
dict/set comprehensions and set literals where appropriate.

QuLogic

It's long, but I don't see too many major issues.

QuLogic · 2016-12-01T23:10:42Z

doc/utils/pylab_names.py

+    border = '=' * maxfunc + ' ' + '=' * maxdoc
    print(border)
-    print(' '.join(['symbol'.ljust(maxfunc), 'description'.ljust(maxdoc)]))
+    print('{:<{}} {:<{}}'.format('symbol', maxfunc, 'description', maxdoc))


print inserts a space, so print('symbol.ljust(maxfunc), 'description'.ljust(maxdoc)) would have worked, right?

Yes but that looks a bit "unintended" to me.

QuLogic · 2016-12-01T23:14:05Z

examples/pylab_examples/figimage_demo.py

-Z = np.arange(10000.0)
-Z.shape = 100, 100
-Z[:, 50:] = 1.
+Z = np.arange(10000).reshape(100, 100)


Missing parentheses in shape.

QuLogic · 2016-12-01T23:18:42Z

examples/pylab_examples/layer_images.py


-Z1 = np.array(([0, 1]*4 + [1, 0]*4)*4)
-Z1.shape = (8, 8)  # chessboard
+Z1 = np.add.outer(range(8), range(8)) % 2  # chessboard


What does np.add.outer do (it works, but I've never seen it used that way)?

It makes the first argument a column array and the second a row array, broadcasts them and applies the ufunc.

QuLogic · 2016-12-01T23:20:40Z

examples/pylab_examples/mri_demo.py

 dfile = cbook.get_sample_data('s1045.ima.gz')
-im = np.fromstring(dfile.read(), np.uint16).astype(float)
-im.shape = (256, 256)
+im = np.fromstring(dfile.read(), np.uint16).reshape((256, 256))


No astype?

Also, can write np.fromfile(dfile, np.uint16) instead of the two step process.

you can imshow an integer array just as well.

Does the one below this still need astype, then?

fixed below too.

Actually, fromfile doesn't work with gzip.open'ed files (it seems to try to construct an array from the compressed representation). Reverting to read().

Ah, unfortunate. Based on np.fromfile seeming to need fileno (since it doesn't work with BytesIO), it looks like it tries to stream from the fd directly, which GzipFile points at the underlying file object. In a way, it's a half-a-bug in both coming together to make one.

Left a comment on numpy/numpy#7713; we'll see where it goes...

QuLogic · 2016-12-01T23:21:05Z

examples/pylab_examples/mri_with_eeg.py

 dfile = cbook.get_sample_data('s1045.ima.gz')
-im = np.fromstring(dfile.read(), np.uint16).astype(float)
-im.shape = (256, 256)
+im = np.fromstring(dfile.read(), np.uint16).astype(float).reshape((256, 256))


Same np.fromfile comment as above.

QuLogic · 2016-12-01T23:28:53Z

lib/matplotlib/backends/backend_pdf.py

+            {val[0]: val[1] for val in six.itervalues(self.alphaStates)})
        self.writeHatches()
        self.writeGouraudTriangles()
        xobjects = dict(x[1:] for x in six.itervalues(self._images))


One more dict comprehension?

QuLogic · 2016-12-01T23:30:26Z

lib/matplotlib/cbook.py

@@ -1408,15 +1408,15 @@ def finddir(o, match, case=False):

 def reverse_dict(d):


Could probably be used in a couple places you've updated...

I'd rather deprecate this function... I think everyone can understand what {v: k for k, v in d.items()} does and this way you don't have to go and check whether the function does something funny with duplicates.

No argument there.

QuLogic · 2016-12-01T23:31:38Z

lib/matplotlib/cbook.py

 # by backends
 # The reverse mapper is for mapping full names to short ones
-ls_mapper_r = dict([(ls[1], ls[0]) for ls in _linestyles])
+ls_mapper_r = reverse_dict(ls_mapper)


Can _linestyles be dropped and ls_mapper written as a literal now?

QuLogic · 2016-12-01T23:40:01Z

lib/matplotlib/quiver.py

-            theta *= (np.pi / 180.0)
-        theta.shape = (theta.shape[0], 1)  # for broadcasting
+            theta = ma.masked_invalid(np.deg2rad(self.angles)).filled(0)
+            theta = theta.reshape((-1, 1))  # for broadcasting


Was the change of indent intended?

QuLogic · 2016-12-01T23:52:31Z

lib/matplotlib/ticker.py

-                exponent -= 1
-            scale = max([self.numticks-1, 1]) ** (-exponent)
+            exponent = round(math.log10(vmax - vmin)
+                             / math.log10(max(self.numticks - 1, 1))) - 1


I tested some completely arbitrary numbers that perhaps will never exist in these lines, but I'm not sure the result is the same. Perhaps I missed something:

>>> vr = np.linspace(1e-8, 1000, 100000) # vmax - vmin >>> numticks = np.arange(30) # I expect 0 is invalid, but no matter... >>> exponent, remainder = divmod(np.log10(vr[:, None]), np.log10(np.maximum(numticks - 1, 1))[None, :]) __main__:1: RuntimeWarning: invalid value encountered in floor_divide __main__:1: RuntimeWarning: invalid value encountered in remainder >>> exponent[remainder < 0.5] -= 1 __main__:1: RuntimeWarning: invalid value encountered in less >>> exponent2 = np.round(np.log10(vr)[:, None] / np.log10(np.maximum(numticks - 1, 1))[None, :]) - 1 >>> exponent array([[ nan, nan, nan, ..., -7., -6., -6.], [ nan, nan, nan, ..., -2., -2., -2.], [ nan, nan, nan, ..., -2., -2., -2.], ..., [ nan, nan, nan, ..., 1., 1., 1.], [ nan, nan, nan, ..., 1., 1., 1.], [ nan, nan, nan, ..., 1., 1., 1.]]) >>> exponent2 array([[-inf, -inf, -inf, ..., -7., -7., -7.], [-inf, -inf, -inf, ..., -2., -2., -2.], [-inf, -inf, -inf, ..., -2., -2., -2.], ..., [ inf, inf, inf, ..., 1., 1., 1.], [ inf, inf, inf, ..., 1., 1., 1.], [ inf, inf, inf, ..., 1., 1., 1.]]) >>> np.sum(exponent == exponent2) 2388308 >>> np.size(exponent) 3000000 # Percentage the same including nan/inf: >>> np.sum(exponent == exponent2)/np.size(exponent) 0.79610266666666663 >>> mask = np.isfinite(exponent) >>> np.all(mask == np.isfinite(exponent2)) True >>> np.sum(exponent[mask] == exponent2[mask]) 2388308 # Percentage of finite values that are the same: >>> np.sum(exponent[mask] == exponent2[mask]) / np.sum(mask) 0.88455851851851852 >>>

You are correct that this is different: the previous implementation compares the remainder with 0.5 whereas I thought it was comparing it with half the divisor. My guess is that the previous version was incorrect.

However I can also just revert to the previous version...

codecov-io · 2016-12-04T00:12:11Z

Current coverage is 61.92% (diff: 60.86%)

Merging #7547 into master will increase coverage by <.01%

@@             master      #7547   diff @@
==========================================
  Files           173        173          
  Lines         56177      56125    -52   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits          34784      34757    -27   
+ Misses        21393      21368    -25   
  Partials          0          0

Powered by Codecov. Last update e66d2f1...7c2bc4c

Kojoley · 2016-12-04T22:06:35Z

lib/matplotlib/mlab.py

    Implemented as a separate function (not a call to :func:`norm` for speed).
    """
-    return np.sqrt(np.sum(np.absolute(a)**2))
+    return np.sqrt(np.sum(np.abs(a) ** 2))


np.abs(a) ** 2 == a ** 2

Not for complex values.

anntzer · 2016-12-04T22:29:28Z

(rebased following conflicts with sticky edges PR)

QuLogic · 2016-12-05T03:18:31Z

AppVeyor failure looks transient.

QuLogic reviewed Dec 2, 2016

View reviewed changes

anntzer force-pushed the additional-cleanups branch 4 times, most recently from 78b4bcc to 7c2bc4c Compare December 3, 2016 22:14

Kojoley reviewed Dec 4, 2016

View reviewed changes

anntzer added 6 commits December 4, 2016 14:17

Call min/max directly on generator or two args.

de302f5

Remove old __cmp__ method.

46b9d47

Use reshape instead of assigning to shape.

1504540

Use shorter function names (min, max, abs, conj).

3d00576

set/dict literals/comprehensions.

69622dd

Minor fixes following comments.

f1b25e6

anntzer force-pushed the additional-cleanups branch from 7c2bc4c to f1b25e6 Compare December 4, 2016 22:25

Kojoley approved these changes Dec 4, 2016

View reviewed changes

QuLogic merged commit 409d8b7 into matplotlib:master Dec 5, 2016

QuLogic added this to the 2.1 (next point release) milestone Dec 5, 2016

anntzer deleted the additional-cleanups branch December 5, 2016 03:19

tacaswell mentioned this pull request Dec 11, 2016

Don't call np.min on generator. #7608

Merged

jklymak mentioned this pull request Dec 14, 2017

Fixed critical typo in mlab.cohere #10004

Merged

6 tasks

		@@ -1408,15 +1408,15 @@ def finddir(o, match, case=False):

		def reverse_dict(d):

Uh oh!

Conversation

anntzer commented Dec 1, 2016

Uh oh!

QuLogic left a comment

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!

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!

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Dec 4, 2016

Current coverage is 61.92% (diff: 60.86%)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anntzer commented Dec 4, 2016

Uh oh!

QuLogic commented Dec 5, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants