Cleanup: sorted, dict iteration, array.{ndim,size}, ... by anntzer · Pull Request #7549 · matplotlib/matplotlib

anntzer · 2016-12-02T03:53:17Z

Use sorted whereever it improves readbility over list.sort.
Iterating over a dict doesn't require calling iterkeys.
Use ndarray.size/.ndim whereever appropriate.
Une N{...} unicode entities.

QuLogic · 2016-12-02T07:06:13Z

Do Unicode entities work in Python 2? I'm not seeing it in the Unicode howto.

anntzer · 2016-12-02T07:11:37Z

Yes, see table at https://docs.python.org/2.7/reference/lexical_analysis.html#string-literals (or try it yourself :-)).

codecov-io · 2016-12-03T00:16:59Z

Current coverage is 61.90% (diff: 60.69%)

Merging #7549 into master will decrease coverage by <.01%

@@             master      #7549   diff @@
==========================================
  Files           173        173          
  Lines         56103      55917   -186   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits          34731      34614   -117   
+ Misses        21372      21303    -69   
  Partials          0          0

Powered by Codecov. Last update 6223155...e809aaf

twmr

I didn't review the whole PR

twmr · 2016-12-04T00:58:30Z

lib/matplotlib/afm.py

-            break
+    for line in fh:
        line = line.rstrip()
        if len(line) == 0:


here you could have also used 'if not line'

twmr · 2016-12-04T00:59:16Z

examples/style_sheets/style_sheets_reference.py

    # the `default` and `classic` ones, which will be forced resp. in
    # first and second position.
-    style_list = list(plt.style.available)  # *new* list: avoids side effects.
+    style_list = sorted(plt.style.available)  # *new* list: avoids side effects.


please remove the comment

both are fixed

QuLogic

It's a behemoth, but I think I managed to get through it all.

QuLogic · 2016-12-04T08:00:36Z

lib/matplotlib/__init__.py

        Return values in order of sorted keys.
        """
-        return [self[k] for k in self.keys()]
+        return [self[k] for k in self]


Does dropping .keys() and iterating over self correctly preserve the sorted order that the keys method from this subclass produces?

No. But that also means that iteration on the dict itself (for k in rcparams) happens in a different order right now...

QuLogic · 2016-12-04T08:02:16Z

lib/matplotlib/afm.py

    pos = fh.tell()
    try:
-        line = fh.readline()
+        line = next(fh)


Not convinced this is clearer.

The problem is that the outer loop may be using for line in fh first before passing the handle to the function at some point, and Py2 (not Py3) doesn't allow (raises an exception on) mixing for line in fh and fh.readline due to the fact that the former uses a readahead buffer (https://docs.python.org/2/library/stdtypes.html#file.next).

But this is called before the iteration, and the next line is a seek, which should flush the readahead buffer anyway.

I think the gain in legibility with .readline() doesn't compensate the brittleness of only being able to pass in a file with an empty readahead buffer.

QuLogic · 2016-12-04T08:05:07Z

lib/matplotlib/afm.py

    """

-    line = fh.readline()
+    line = next(fh)


This one makes sense though; will need to remain next.

QuLogic · 2016-12-04T08:21:00Z

lib/matplotlib/axis.py

-            locs = [ti[1] for ti in tick_tups]
-            locs.sort()
-            locs = np.array(locs)
-            if len(locs):


I understand this condition is probably not necessary, but I guess it could also be anded onto line 955?

In fact it's probably needed, otherwise we may end up trying to get the first or last item of an empty array...

QuLogic · 2016-12-04T08:23:37Z

lib/matplotlib/backends/backend_gdk.py

-IMAGE_FORMAT  = ['eps', 'jpg', 'png', 'ps', 'svg'] + ['bmp'] # , 'raw', 'rgb']
-IMAGE_FORMAT.sort()
-IMAGE_FORMAT_DEFAULT  = 'png'
+IMAGE_FORMAT = sorted(['eps', 'jpg', 'png', 'ps', 'svg'] + ['bmp']) # , 'raw', 'rgb']


Just put 'bmp' at the front?

Sure, but I'll keep the sorted which makes the intent clear and should be cheap.

QuLogic · 2016-12-04T08:26:55Z

lib/matplotlib/backends/backend_ps.py


    epsfile = tmpfile + '.eps'
-    with io.open(epsfile, 'wb') as epsh:
+    with io.open(epsfile, 'wb') as epsh, io.open(tmpfile, 'rb') as tmph:


Do we even need io. when it's binary?

What's the difference between io.open and open in Py2?

I don't think there is one in binary mode, is there?

I guess...
On the other hand there seems to be an awful lot use of temporary files in backend_ps, most of which could probably just get rewritten using pipes to communicate via the subprocesses' standard streams.
Another great cleanup project :-)

QuLogic · 2016-12-04T09:00:10Z

lib/matplotlib/table.py

@@ -331,8 +328,7 @@ def _get_grid_bbox(self, renderer):

        Only include those in the range (0,0) to (maxRow, maxCol)"""
        boxes = [self._cells[pos].get_window_extent(renderer)


Since the value is actually used here, it might make sense to use items() below.

QuLogic · 2016-12-04T09:09:03Z

lib/mpl_toolkits/axisartist/angle_helper.py

        if kwargs:
-            raise ValueError("Following keys are not processed: %s" % \
-                             ", ".join([str(k) for k in kwargs.keys()]))
+            raise ValueError("Following keys are not processed: %s"


I'm going to remove this whole thing in #7545.

QuLogic · 2016-12-04T09:15:00Z

lib/mpl_toolkits/mplot3d/axes3d.py

-        p0, p1 = edges[edgei]
+        p0, p1 = min(self.tunit_edges(),
+                     key=lambda edge: proj3d.line2d_seg_dist(
+                         edge[0], edge[1], (xd, yd)))


I prefer to break after edge[1], but that's minor.

QuLogic · 2016-12-04T09:16:06Z

lib/mpl_toolkits/mplot3d/proj3d.py

-    tis = (vecw[0] >= 0) * (vecw[0] <= 1) * (vecw[1] >= 0) * (vecw[1] <= 1)
-    if np.sometrue(tis):
-        tis =  vecw[1] < 1
+    tis = (vecw[0] >= 0) & (vecw[0] <= 1) & (vecw[1] >= 0) & (vecw[1] <= 1)


Since you're editing the line already, can you reorder it so that it looks like an implied and?

Kojoley · 2016-12-04T17:17:02Z

examples/misc/multiprocess.py


        def call_back():
-            while 1:
+            while True:


Cannot this be while self.pipe.poll(): (and remove if not self.pipe.poll(): break below)?

Kojoley · 2016-12-04T17:48:06Z

lib/matplotlib/axes/_base.py

-        while 1:
-
-            if len(remaining) == 0:
+        while True:


while args:?

Kojoley · 2016-12-04T17:52:26Z

lib/matplotlib/axes/_base.py

+        if not self.figure.canvas.is_saving():
+            artists = [a for a in artists
+                       if not a.get_animated() or a in self.images]
+        artists = sorted(artists, key=lambda artist: artist.get_zorder())


Is not attrgetter faster than lambda?

I doubt it matters but sure.

Kojoley · 2016-12-04T18:18:25Z

lib/matplotlib/cm.py

-        spec = datad[cmapname]
-        spec_reversed = _reverse_cmap_spec(spec)
-        datad[cmapname + '_r'] = spec_reversed
+    for cmapname, spec in list(six.iteritems(datad)):


list is redundant

No because we're modifying the dict at the same time; sidestepped the issue using update.

QuLogic · 2016-12-04T23:11:39Z

lib/matplotlib/table.py

            boxes = [self._cells[pos].get_window_extent(renderer)
-                     for pos in six.iterkeys(self._cells)
-                     if pos[0] >= 0 and pos[1] >= 0]
+                     for pos in self._cells if pos[0] >= 0 and pos[1] >= 0]


I knew I should have commented the first time, but here's another one that could use items.

NelleV · 2016-12-19T14:40:45Z

Can you rebase this PR?

NelleV · 2016-12-19T14:41:30Z

examples/style_sheets/style_sheets_reference.py

-    style_list.sort()
-    style_list.insert(0, u'default')
-    style_list.insert(1, u'classic')
+    style_list = ['default', 'classic'] + sorted(


NelleV · 2016-12-19T14:42:19Z

lib/matplotlib/afm.py

-        line = fh.readline()
-        if not line:
-            break
+    for line in fh:


anntzer · 2016-12-20T11:00:48Z

After Christmas break, probably.

anntzer · 2016-12-21T10:57:00Z

Actually it wasn't that bad.

tacaswell added this to the 2.1 (next point release) milestone Dec 2, 2016

anntzer changed the title ~~Cleanup sorted~~ Cleanup: sorted, dict iteration Dec 2, 2016

anntzer changed the title ~~Cleanup: sorted, dict iteration~~ Cleanup: sorted, dict iteration, array.{ndim,size} Dec 2, 2016

anntzer changed the title ~~Cleanup: sorted, dict iteration, array.{ndim,size}~~ Cleanup: sorted, dict iteration, array.{ndim,size}, ... Dec 2, 2016

anntzer force-pushed the cleanup-sorted branch 3 times, most recently from d163a75 to bec79f4 Compare December 2, 2016 07:03

anntzer force-pushed the cleanup-sorted branch from bec79f4 to 179b3ed Compare December 2, 2016 07:07

anntzer force-pushed the cleanup-sorted branch 2 times, most recently from 9d0a612 to 5044c22 Compare December 3, 2016 00:08

anntzer force-pushed the cleanup-sorted branch 3 times, most recently from d5fe5e4 to d17d6b5 Compare December 3, 2016 21:45

twmr reviewed Dec 4, 2016

View reviewed changes

anntzer force-pushed the cleanup-sorted branch from 3883fa0 to b22d353 Compare December 4, 2016 03:26

QuLogic reviewed Dec 4, 2016

View reviewed changes

Kojoley reviewed Dec 4, 2016

View reviewed changes

anntzer force-pushed the cleanup-sorted branch from 3b5197c to e809aaf Compare December 4, 2016 21:18

QuLogic reviewed Dec 5, 2016

View reviewed changes

anntzer force-pushed the cleanup-sorted branch from e809aaf to 32df815 Compare December 5, 2016 02:31

Kojoley approved these changes Dec 5, 2016

View reviewed changes

anntzer force-pushed the cleanup-sorted branch from 32df815 to 84baecd Compare December 7, 2016 06:11

NelleV changed the title ~~Cleanup: sorted, dict iteration, array.{ndim,size}, ...~~ [MRG+1] Cleanup: sorted, dict iteration, array.{ndim,size}, ... Dec 19, 2016

NelleV reviewed Dec 19, 2016

View reviewed changes

Cleanup: use sorted() whereever possible.

70909c4

anntzer added 8 commits December 21, 2016 11:52

Iterating over a dict is easy.

7a83fc4

Use ndim, size whereever appropriate.

5bfd4b7

Use \N{} unicode entities.

1d4192c

dict.keys() is nearly always useless.

39352a8

safezip is (mostly) overrated.

338f6da

alltrue and sometrue are known as all and any.

43f2f7c

Cleanup while 1: ... and file iteration.

04062b6

More cleanups following PR comments.

14b47ba

anntzer force-pushed the cleanup-sorted branch from 84baecd to 14b47ba Compare December 21, 2016 10:56

QuLogic changed the title ~~[MRG+1] Cleanup: sorted, dict iteration, array.{ndim,size}, ...~~ Cleanup: sorted, dict iteration, array.{ndim,size}, ... Dec 22, 2016

QuLogic merged commit 7131876 into matplotlib:master Dec 22, 2016

anntzer deleted the cleanup-sorted branch December 23, 2016 07:06

timhoffm mentioned this pull request Oct 24, 2018

RcParams should not inherit from dict #12577

Closed

		@@ -331,8 +328,7 @@ def _get_grid_bbox(self, renderer):

		Only include those in the range (0,0) to (maxRow, maxCol)"""
		boxes = [self._cells[pos].get_window_extent(renderer)

Uh oh!

Conversation

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

Uh oh!

Uh oh!

QuLogic commented Dec 2, 2016

Uh oh!

anntzer commented Dec 2, 2016

Uh oh!

codecov-io commented Dec 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 61.90% (diff: 60.69%)

Uh oh!

twmr 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!

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!

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

anntzer commented Dec 2, 2016 •

edited

Loading

codecov-io commented Dec 3, 2016 •

edited

Loading

anntzer Dec 4, 2016 •

edited

Loading