x=np.arange(3)y=np.arange(3)u=np.ma.array(15.*np.ones((4,)),mask=[False,True,False,False])v=np.ma.array(15.*np.ones_like(u),mask=[False,True,False,False])plt.barbs(x,y,u,v)

The error would end up with something inscrutable (I lost over an hour figuring out what was wrong):

---------------------------------------------------------------------------TypeError                                 Traceback (most recent call last)<SNIP>/Users/rmay/repos/matplotlib/lib/matplotlib/quiver.py in __init__(self, ax, *args, **kw)942self.set_transform(transforms.IdentityTransform())943 --> 944         self.set_UVC(u, v, c)945946def_find_tails(self,mag,rounding=True,half=5,full=10,flag=50):/Users/rmay/repos/matplotlib/lib/matplotlib/quiver.py in set_UVC(self, U, V, C)   1138         plot_barbs = self._make_barbs(u, v, flags, barbs, halves, empty,   1139                                       self._length, self._pivot, self.sizes,-> 1140                                       self.fill_empty, self.flip)   1141         self.set_verts(plot_barbs)   1142 /Users/rmay/repos/matplotlib/lib/matplotlib/quiver.py in _make_barbs(self, u, v, nflags, nbarbs, half_barb, empty_flag, length, pivot, sizes, fill_empty, flip)   1071    1072             # Add vertices for each flag-> 1073             for i in range(nflags[index]):   1074                 # The spacing that works for the barbs is a little to much for   1075                 # the flags, but this only occurs when we have more than 1TypeError:only integer arrays with one element can be converted to an index

It also only happens when masked arrays are involved--otherwise it silently succeeds, which seems poor as well. With this change now the error is easy to identify:

---------------------------------------------------------------------------ValueError                                Traceback (most recent call last)<SNIP>/Users/rmay/repos/matplotlib/lib/matplotlib/quiver.py in __init__(self, ax, *args, **kw)948self.set_transform(transforms.IdentityTransform())949 --> 950         self.set_UVC(u, v, c)951952def_find_tails(self,mag,rounding=True,half=5,full=10,flag=50):/Users/rmay/repos/matplotlib/lib/matplotlib/quiver.py in set_UVC(self, U, V, C)   1135             x, y, u, v = delete_masked_points(self.x.ravel(), self.y.ravel(),   1136                                               self.u, self.v)-> 1137             check_array_shapes(x, y, u, v)   1138    1139         magnitude = np.hypot(u, v)/Users/rmay/repos/matplotlib/lib/matplotlib/quiver.py in check_array_shapes(*arrays)396     all_shapes=set(a.shapefor ain arrays)397iflen(all_shapes)!=1:--> 398         raise ValueError('The shapes of the passed in arrays do not match.')399400ValueError:The shapes of the passed in arrays do not match.

dopplershift modified the milestones:2.0 (style change major release),2.0.1 (next bug fix release)

Nov 4, 2016

NelleV requested changes

Nov 4, 2016

View reviewed changes

Copy link

Member

NelleV left a comment

Choose a reason for hiding this comment

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

The error message looks good, but I've got a couple of comments on the API.

lib/matplotlib/quiver.py

		@@ -392,6 +392,12 @@ def _parse_args(*args):
		return X, Y, U, V, C


		def check_array_shapes(*arrays):

Copy link

Member

NelleVNov 4, 2016

Choose a reason for hiding this comment

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

Can you make this function private? I don't think it should be part of our public API.

I also think the name of the function is not very explicit on what it does. How aboutcheck_consistent_shape?

lib/matplotlib/quiver.py

		def check_array_shapes(*arrays):
		all_shapes = set(a.shape for a in arrays)
		if len(all_shapes) != 1:
		raise ValueError('The shapes of the passed in arrays do not match.')

Copy link

Member

NelleVNov 4, 2016

Choose a reason for hiding this comment

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

👍

ENH: Improve barbs() error message for badly-sized arrays.

b915ae1

With this change, when given masked arrays with incompatible sizes,barbs() will raise a ValueError with a message stating as much.Previously, it would error with a TypeError that did not make it clearat all the source of the problem.

dopplershift force-pushed thebarbs-error branch from9ad1c9a to11c9aebCompare

November 4, 2016 20:47

Copy link

ContributorAuthor

dopplershift commentedNov 4, 2016

Changed the name of the helper to_check_consistent_shapes.

NelleV approved these changes

Nov 4, 2016

View reviewed changes

Copy link

Member

NelleV commentedNov 4, 2016

👍 Thanks for the patch!
I recently spend a couple of hours understanding why scatter wasn't happy because of a cryptic error message so this patch makes me very happy :)

NelleV changed the title~~Improve barbs() error message~~[MRG+1] Improve barbs() error message

Nov 4, 2016

Copy link

ContributorAuthor

dopplershift commentedNov 4, 2016

Well considering Iwrotebarbs() and it still cost me time, I'd say we need to do better. 😬

NelleV reviewed

Nov 4, 2016

View reviewed changes

lib/matplotlib/quiver.py

		def _check_consistent_shapes(*arrays):
		all_shapes = set(a.shape for a in arrays)
		if len(all_shapes) != 1:
		raise ValueError('The shapes of the passed in arrays do not match.')

Copy link

Member

NelleVNov 4, 2016

Choose a reason for hiding this comment

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

I think that line is too long (pep8 is complaining somewhere).

Copy link

ContributorAuthor

dopplershiftNov 4, 2016

Choose a reason for hiding this comment

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

Nope, even worse--it was the docstring typo I fixed. I rewrapped and pushed.

MNT: Fix docstring typo.

65d818d

dopplershift force-pushed thebarbs-error branch from11c9aeb to65d818dCompare

November 4, 2016 22:29

phobson reviewed

Nov 4, 2016

View reviewed changes

Copy link

Member

phobson left a comment

Choose a reason for hiding this comment

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

I don't want to hold this up, but I would prefer if we leftnose out of the test

lib/matplotlib/tests/test_quiver.py

		@@ -133,6 +134,20 @@ def test_barbs():
		cmap='viridis')


		@cleanup
		@raises(ValueError)

Copy link

Member

phobsonNov 4, 2016

Choose a reason for hiding this comment

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

Just a thought: we could avoid falling back tonose with pytest like this:

@cleanupdeftest_bad_masked_sizes():'Test error handling when given differing sized masked arrays'x=np.arange(3)y=np.arange(3)u=np.ma.array(15.*np.ones((4,)))v=np.ma.array(15.*np.ones_like(u))u[1]=np.ma.maskedv[1]=np.ma.maskedfig,ax=plt.subplots()withpytest.raises(ValueError):ax.barbs(x,y,u,v)

Copy link

ContributorAuthor

dopplershiftNov 4, 2016

Choose a reason for hiding this comment

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

If we're allowed to explicitly use pytest now, I will absolutely do that. I just tried to be consistent with what's already around.

Copy link

ContributorAuthor

dopplershiftNov 4, 2016

Choose a reason for hiding this comment

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

I'm not seeing anywhere that we're explicitly using pytest yet...I sense this shouldn't be the first. 😞

Copy link

Member

NelleVNov 4, 2016

Choose a reason for hiding this comment

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

oh, I thought it was official!

Copy link

Member

NelleVNov 4, 2016

Choose a reason for hiding this comment

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

I personally don't care… I much prefer nose's API than pytests, so I have a tendency to use naturally pytest. I'd be interested in knowing whether we should write everything pytest-like.