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.

Sign up for GitHub

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

Jump to bottom

Don't check iterable() before len(). #7767

Merged
merged 1 commit into from
Jan 29, 2017
Merged

Don't check iterable() before len(). #7767

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions lib/matplotlib/axes/_axes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import six
from six.moves import reduce, xrange, zip, zip_longest

from collections import Sized
import itertools
import math
import warnings
Expand Down Expand Up @@ -2940,8 +2941,11 @@ def extract_err(err, data):
data : iterable
x or y from errorbar
'''
if (iterable(err) and len(err) == 2):
try:
a, b = err
except (TypeError, ValueError):
pass
else:
if iterable(a) and iterable(b):
# using list comps rather than arrays to preserve units
low = [thisx - thiserr for (thisx, thiserr)
Expand All @@ -2955,8 +2959,8 @@ def extract_err(err, data):
# special case for empty lists
if len(err) > 1:
fe = safe_first_element(err)
if not ((len(err) == len(data) and not (iterable(fe) and
len(fe) > 1))):
if (len(err) != len(data)
or isinstance(fe, Sized) and len(fe) > 1):
raise ValueError("err must be [ scalar | N, Nx1 "
"or 2xN array-like ]")
# using list comps rather than arrays to preserve units
Expand Down
6 changes: 2 additions & 4 deletions lib/matplotlib/axes/_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1925,11 +1925,9 @@ def update_datalim(self, xys, updatex=True, updatey=True):
# limits and set the bound to be the bounds of the xydata.
# Otherwise, it will compute the bounds of it's current data
# and the data in xydata

if iterable(xys) and not len(xys):
xys = np.asarray(xys)
if not len(xys):
return
if not isinstance(xys, np.ma.MaskedArray):
xys = np.asarray(xys)
self.dataLim.update_from_data_xy(xys, self.ignore_existing_data_limits,
updatex=updatex, updatey=updatey)
self.ignore_existing_data_limits = False
Expand Down
9 changes: 5 additions & 4 deletions lib/matplotlib/cm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,10 +305,11 @@ def set_clim(self, vmin=None, vmax=None):

ACCEPTS: a length 2 sequence of floats
"""
if (vmin is not None and vmax is None and
cbook.iterable(vmin) and len(vmin) == 2):
vmin, vmax = vmin

if vmax is None:
try:
vmin, vmax = vmin
except (TypeError, ValueError):
pass
if vmin is not None:
self.norm.vmin = vmin
if vmax is not None:
Expand Down
29 changes: 2 additions & 27 deletions lib/matplotlib/collections.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,31 +156,6 @@ def __init__(self,
self.update(kwargs)
self._paths = None

@staticmethod
def _get_value(val):
try:
return (float(val), )
except TypeError:
if cbook.iterable(val) and len(val):
try:
float(cbook.safe_first_element(val))
except (TypeError, ValueError):
pass # raise below
else:
return val

raise TypeError('val must be a float or nonzero sequence of floats')

@staticmethod
def _get_bool(val):
if not cbook.iterable(val):
val = (val,)
try:
bool(cbook.safe_first_element(val))
except (TypeError, IndexError):
raise TypeError('val must be a bool or nonzero sequence of them')
return val

def get_paths(self):
return self._paths

Expand Down Expand Up @@ -486,7 +461,7 @@ def set_linewidth(self, lw):
if lw is None:
lw = mpl.rcParams['lines.linewidth']
# get the un-scaled/broadcast lw
self._us_lw = self._get_value(lw)
self._us_lw = np.atleast_1d(np.asarray(lw))

# scale all of the dash patterns.
self._linewidths, self._linestyles = self._bcast_lwls(
Expand Down Expand Up @@ -608,7 +583,7 @@ def set_antialiased(self, aa):
"""
if aa is None:
aa = mpl.rcParams['patch.antialiased']
self._antialiaseds = self._get_bool(aa)
self._antialiaseds = np.atleast_1d(np.asarray(aa, bool))
self.stale = True

def set_antialiaseds(self, aa):
Expand Down
12 changes: 7 additions & 5 deletions lib/matplotlib/colors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,11 @@

from __future__ import (absolute_import, division, print_function,
unicode_literals)
import re
import six
from six.moves import zip

from collections import Sized
import re
import warnings

import numpy as np
Expand Down Expand Up @@ -693,12 +695,12 @@ def from_list(name, colors, N=256, gamma=1.0):
if not cbook.iterable(colors):
raise ValueError('colors must be iterable')

if cbook.iterable(colors[0]) and len(colors[0]) == 2 and \
not cbook.is_string_like(colors[0]):
if (isinstance(colors[0], Sized) and len(colors[0]) == 2
and not cbook.is_string_like(colors[0])):
# List of value, color pairs
vals, colors = list(zip(*colors))
vals, colors = zip(*colors)
else:
vals = np.linspace(0., 1., len(colors))
vals = np.linspace(0, 1, len(colors))

cdict = dict(red=[], green=[], blue=[], alpha=[])
for val, color in zip(vals, colors):
Expand Down
36 changes: 11 additions & 25 deletions lib/matplotlib/legend.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@

from matplotlib import rcParams
from matplotlib.artist import Artist, allow_rasterization
from matplotlib.cbook import is_string_like, iterable, silent_list, is_hashable
from matplotlib.cbook import is_string_like, silent_list, is_hashable
from matplotlib.font_manager import FontProperties
from matplotlib.lines import Line2D
from matplotlib.patches import Patch, Rectangle, Shadow, FancyBboxPatch
Expand Down Expand Up @@ -408,17 +408,6 @@ def _set_loc(self, loc):
# find_offset function will be provided to _legend_box and
# _legend_box will draw itself at the location of the return
# value of the find_offset.
self._loc_real = loc
if loc == 0:
_findoffset = self._findoffset_best
else:
_findoffset = self._findoffset_loc

# def findoffset(width, height, xdescent, ydescent):
# return _findoffset(width, height, xdescent, ydescent, renderer)

self._legend_box.set_offset(_findoffset)

self._loc_real = loc
self.stale = True

Expand All @@ -427,24 +416,20 @@ def _get_loc(self):

_loc = property(_get_loc, _set_loc)

def _findoffset_best(self, width, height, xdescent, ydescent, renderer):
"Helper function to locate the legend at its best position"
ox, oy = self._find_best_position(width, height, renderer)
return ox + xdescent, oy + ydescent

def _findoffset_loc(self, width, height, xdescent, ydescent, renderer):
"Helper function to locate the legend using the location code"
def _findoffset(self, width, height, xdescent, ydescent, renderer):
"Helper function to locate the legend"

if iterable(self._loc) and len(self._loc) == 2:
# when loc is a tuple of axes(or figure) coordinates.
fx, fy = self._loc
bbox = self.get_bbox_to_anchor()
x, y = bbox.x0 + bbox.width * fx, bbox.y0 + bbox.height * fy
else:
if self._loc == 0: # "best".
x, y = self._find_best_position(width, height, renderer)
elif self._loc in Legend.codes.values(): # Fixed location.
bbox = Bbox.from_bounds(0, 0, width, height)
x, y = self._get_anchored_bbox(self._loc, bbox,
self.get_bbox_to_anchor(),
renderer)
else: # Axes or figure coordinates.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the code set up such that if self._loc is neither 0 nor in Legend.codes.values(), then it has to be iterable of len 2?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition effectively is the same as before.

Previous impl:

  • if loc == 0, use findoffset_best, which directly calls find_best_position.
  • otherwise, use findoffset_loc:
    • if len(loc) == 2, use fixed position
    • otherwise, use get_anchored_bbox (fixed location, assumed to be in Legend.codes.values)

New impl:

  • if loc == 0, use find_best_position
  • if fixed location, in Legend.codes.values, use get_anchored_bbox
  • otherwise, assume fixed position

It is correct that the error on invalid locations would be different though. The previous implementation would raise an AssertionError (assert loc in range(1, 11) # called only internally in get_anchored_bbox), the current one raises a TypeError (when trying to iterate over self._loc).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:/ Should it be raising a ValueError since that's what's actually going wrong? I think I get now why the old code checked for len first...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous version would raise AssertionError both for invalid single values (loc=99) and for invalid length values (loc=(1, 2, 3)). The new implementation now raises TypeError for the former and ValueError for the latter. I think the new version is better, given that most functions can indeed raise both TypeError and ValueError when given an incorrect input, so you need to catch both anyways (whereas catching AssertionError is pretty rare).

fx, fy = self._loc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are these changes related to the title of the PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea was to fix the line a bit below (if iterable(self._loc) and len(self._loc) == 2) but as you can see the new logic is just to see whether _loc is a 0-9 location and otherwise unpack it, so it seemed reasonable to squash the if ... elif ... else into a single function rather than keeping them artificially separated.

bbox = self.get_bbox_to_anchor()
x, y = bbox.x0 + bbox.width * fx, bbox.y0 + bbox.height * fy

return x + xdescent, y + ydescent

Expand Down Expand Up @@ -701,6 +686,7 @@ def _init_legend_box(self, handles, labels, markerfirst=True):
children=[self._legend_title_box,
self._legend_handle_box])
self._legend_box.set_figure(self.figure)
self._legend_box.set_offset(self._findoffset)
self.texts = text_list
self.legendHandles = handle_list

Expand Down
8 changes: 5 additions & 3 deletions lib/matplotlib/markers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,10 +89,12 @@
import six
from six.moves import xrange

from collections import Sized

import numpy as np

from .cbook import is_math_text, is_string_like, is_numlike, iterable
from matplotlib import rcParams
from . import rcParams
from .cbook import is_math_text, is_string_like, is_numlike
from .path import Path
from .transforms import IdentityTransform, Affine2D

Expand Down Expand Up @@ -247,7 +249,7 @@ def get_marker(self):
return self._marker

def set_marker(self, marker):
if (iterable(marker) and len(marker) in (2, 3) and
if (isinstance(marker, Sized) and len(marker) in (2, 3) and
marker[1] in (0, 1, 2, 3)):
self._marker_function = self._set_tuple_marker
elif isinstance(marker, np.ndarray):
Expand Down
9 changes: 2 additions & 7 deletions lib/matplotlib/rcsetup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,18 +18,13 @@

import six

from collections import Iterable, Mapping
from functools import reduce
import operator
import os
import warnings
import re

try:
import collections.abc as abc
except ImportError:
# python 2
import collections as abc

from matplotlib.cbook import mplDeprecation
from matplotlib.fontconfig_pattern import parse_fontconfig_pattern
from matplotlib.colors import is_color_like
Expand Down Expand Up @@ -92,7 +87,7 @@ def f(s):
# Numpy ndarrays, and pandas data structures. However, unordered
# sequences, such as sets, should be allowed but discouraged unless the
# user desires pseudorandom behavior.
elif isinstance(s, abc.Iterable) and not isinstance(s, abc.Mapping):
elif isinstance(s, Iterable) and not isinstance(s, Mapping):
# The condition on this list comprehension will preserve the
# behavior of filtering out any empty strings (behavior was
# from the original validate_stringlist()), while allowing
Expand Down
12 changes: 6 additions & 6 deletions lib/matplotlib/tests/test_collections.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@
import io

import numpy as np
from numpy.testing import assert_array_equal, assert_array_almost_equal
from numpy.testing import assert_equal
from numpy.testing import (
assert_array_equal, assert_array_almost_equal, assert_equal)
import pytest

import matplotlib.pyplot as plt
Expand Down Expand Up @@ -641,9 +641,9 @@ def test_lslw_bcast():
col.set_linestyles(['-', '-'])
col.set_linewidths([1, 2, 3])

assert col.get_linestyles() == [(None, None)] * 6
assert col.get_linewidths() == [1, 2, 3] * 2
assert_equal(col.get_linestyles(), [(None, None)] * 6)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason you're using numpy testing over py.test asserts here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because col.get_linewidths can now return an array rather than a list, due to the switch from
self._us_lw = self._get_value(lw)
to
self._us_lw = np.atleast_1d(np.asarray(lw))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does assert_equal work on arrays? don't you have to use assert_array_equal?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works. See also numpy/numpy#8457.

In [4]: np.testing.assert_equal(np.array([1, 2]), [1, 2])

In [5]: np.testing.assert_equal(np.array([1, 2]), [1, 3])
---------------------------------------------------------------------------
AssertionError                            Traceback (most recent call last)
<ipython-input-5-f3a660c3ad1f> in <module>()
----> 1 np.testing.assert_equal(np.array([1, 2]), [1, 3])

/usr/lib/python3.6/site-packages/numpy/testing/utils.py in assert_equal(actual, desired, err_msg, verbose)
    320     from numpy.lib import iscomplexobj, real, imag
    321     if isinstance(actual, ndarray) or isinstance(desired, ndarray):
--> 322         return assert_array_equal(actual, desired, err_msg, verbose)
    323     msg = build_err_msg([actual, desired], err_msg, verbose=verbose)
    324 

/usr/lib/python3.6/site-packages/numpy/testing/utils.py in assert_array_equal(x, y, err_msg, verbose)
    811     """
    812     assert_array_compare(operator.__eq__, x, y, err_msg=err_msg,
--> 813                          verbose=verbose, header='Arrays are not equal')
    814 
    815 def assert_array_almost_equal(x, y, decimal=6, err_msg='', verbose=True):

/usr/lib/python3.6/site-packages/numpy/testing/utils.py in assert_array_compare(comparison, x, y, err_msg, verbose, header, precision)
    737                                 names=('x', 'y'), precision=precision)
    738             if not cond:
--> 739                 raise AssertionError(msg)
    740     except ValueError:
    741         import traceback

AssertionError: 
Arrays are not equal

(mismatch 50.0%)
 x: array([1, 2])
 y: array([1, 3])

assert_equal(col.get_linewidths(), [1, 2, 3] * 2)

col.set_linestyles(['-', '-', '-'])
assert col.get_linestyles() == [(None, None)] * 3
assert col.get_linewidths() == [1, 2, 3]
assert_equal(col.get_linestyles(), [(None, None)] * 3)
assert_equal(col.get_linewidths(), [1, 2, 3])
14 changes: 9 additions & 5 deletions lib/matplotlib/units.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,11 +157,15 @@ def get_converter(self, x):
converter = self.get_converter(next_item)
return converter

if converter is None and iterable(x) and (len(x) > 0):
thisx = safe_first_element(x)
if classx and classx != getattr(thisx, '__class__', None):
converter = self.get_converter(thisx)
return converter
if converter is None:
try:
thisx = safe_first_element(x)
except (TypeError, StopIteration):
pass
else:
if classx and classx != getattr(thisx, '__class__', None):
converter = self.get_converter(thisx)
return converter
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the converter is always returned at the bottom of this function (it's literally the next line) so is it worth putting here? And a general question/qualm on the multiple returns: while I know PEP8 is lenient on them, is there a good reason for them here where everything is bundled in if statements anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to keep the logical flow of the previous implementation as much as possible -- I think the whole thing probably needs some refactor anyways, so there isn't much in favor of a piecemeal approach.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get...refactoring units (and adding ScalerMappable support) is on my wishlist...


# DISABLED self._cached[idx] = converter
return converter
Expand Down