Don't check iterable() before len().
#7767
Conversation
anntzer
force-pushed
the
iterables-may-be-unsized
branch
2 times, most recently
from
c8beba1
to
aeb1898
Compare
Current coverage is 62.11% (diff: 100%)@@ master #7767 diff @@
==========================================
Files 174 174
Lines 56028 56011 -17
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
- Hits 34805 34793 -12
+ Misses 21223 21218 -5
Partials 0 0
|
| self.stale = True | ||
|
|
||
| def _get_loc(self): | ||
| return self._loc_real | ||
|
|
||
| _loc = property(_get_loc, _set_loc) | ||
|
|
||
| def _findoffset_best(self, width, height, xdescent, ydescent, renderer): |
There was a problem hiding this comment.
are these changes related to the title of the PR?
There was a problem hiding this comment.
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 = 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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
:/ 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...
There was a problem hiding this comment.
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).
| @@ -646,12 +646,12 @@ 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) | |||
There was a problem hiding this comment.
any reason you're using numpy testing over py.test asserts here?
There was a problem hiding this comment.
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))
There was a problem hiding this comment.
Does assert_equal work on arrays? don't you have to use assert_array_equal?
There was a problem hiding this comment.
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])
| else: | ||
| if classx and classx != getattr(thisx, '__class__', None): | ||
| converter = self.get_converter(thisx) | ||
| return converter |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I get...refactoring units (and adding ScalerMappable support) is on my wishlist...
... because the former does not imply the latter anyways, e.g. generators are iterables but unsized. Now `plot(zip([1, 2], [3, 4]))` (py3) raises `RuntimeError: matplotlib does not support generators` (from `cbook.safe_first_element`) which is probably the intended exception, rather than `TypeError: object of type 'zip' has no len()`. Perhaps this exception should be changed to a `TypeError`, by the way...
anntzer
force-pushed
the
iterables-may-be-unsized
branch
from
aeb1898
to
199a87a
Compare
NelleV
changed the title
iterable() before len().iterable() before len().
QuLogic
changed the title
iterable() before len().iterable() before len().
... because the former does not imply the latter anyways, e.g.
generators are iterables but unsized.
Now
plot(zip([1, 2], [3, 4]))(py3) raisesRuntimeError: matplotlib does not support generators(fromcbook.safe_first_element) which isprobably the intended exception, rather than
TypeError: object of type 'zip' has no len(). Perhaps this exception should be changed to aTypeError, by the way...