Skip to content

str_angles and scale_units logic for quiver #8670

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

Merged
merged 1 commit into from
Jun 1, 2017
Merged

str_angles and scale_units logic for quiver #8670

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
12 changes: 6 additions & 6 deletions lib/matplotlib/quiver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -626,19 +626,19 @@ def _angles_lengths(self, U, V, eps=1):

def _make_verts(self, U, V, angles):
uv = (U + V * 1j)
str_angles = isinstance(angles, six.string_types)
if str_angles and (angles == 'xy' and self.scale_units == 'xy'):
str_angles = angles if isinstance(angles, six.string_types) else ''
Copy link
Member

Choose a reason for hiding this comment

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

I'm not seeing why this change is necessary.

Copy link
Author

@t-bltg t-bltg May 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Build mpl on latest commit 1144e8c on branch master

_, ax = plt.subplots(subplot_kw=dict(aspect='equal'))
ax.quiver(0, 0, 1, 1, angles='xy', scale_units='xy', scale=1)
ax.set_xlim(0, 1.1); ax.set_ylim(0, 1.1); ax.grid()

before.pdf
after.pdf

this block l. 668 is now wrong:

if str_angles and (angles == 'xy'):
    theta = angles
elif str_angles and (angles == 'uv'):
    theta = np.angle(uv)

since variable angles is overridden by angles, lengths = self._angles_lengths(U, V, eps=1)

the cited commit d705dbd screws up the whole quiver logic

Copy link
Member

Choose a reason for hiding this comment

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

Ah, okay. In that case, if str_angles and str_angles == 'some string': is redundant now; it can just be if str_angles == 'some string':.

Please add a test as well; something similar to your example will probably work.

Copy link
Author

@t-bltg t-bltg May 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I guess not, str_angles is here to avoid the FutureWarning: elementwise comparison failed cited above and was introduced in #6971

I tried to stick with the existant ...

Copy link
Member

Choose a reason for hiding this comment

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

But this line has changed it so that str_angles is always a string.

Copy link
Author

Choose a reason for hiding this comment

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

yes, but the empty string evaluates to False (as in print('foo' if '' else 'bar')), thus short-circuiting the angles == 'xy' test, which can eventually throw a warning.

Copy link
Member

Choose a reason for hiding this comment

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

But you've replaced all the angles == with str_angles ==.

Copy link
Author

Choose a reason for hiding this comment

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

ha yes I see it now, stupid me it's getting late here ...

if str_angles == 'xy' and self.scale_units == 'xy':
# Here eps is 1 so that if we get U, V by diffing
# the X, Y arrays, the vectors will connect the
# points, regardless of the axis scaling (including log).
angles, lengths = self._angles_lengths(U, V, eps=1)
elif str_angles and (angles == 'xy' or self.scale_units == 'xy'):
elif str_angles == 'xy' or self.scale_units == 'xy':
# Calculate eps based on the extents of the plot
# so that we don't end up with roundoff error from
# adding a small number to a large.
eps = np.abs(self.ax.dataLim.extents).max() * 0.001
angles, lengths = self._angles_lengths(U, V, eps=eps)
if self.scale_units == 'xy':
if str_angles and self.scale_units == 'xy':
Copy link
Member

Choose a reason for hiding this comment

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

Why does this extra bit need to go in - if str_angles is a string does this even make sense?

Copy link
Author

Choose a reason for hiding this comment

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

this should avoid throwing issue 2 is str_angles == '', doesn't it ?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it should be if str_angles == 'xy' as well, to match the others? I'm not sure what the behaviour should be if you pass angles = 'some garbage' instead though.

Copy link
Member

Choose a reason for hiding this comment

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

I guess the real question is whether this should apply to str_angles == 'uv' as well?

Copy link
Author

@t-bltg t-bltg May 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

well, all the scalings except 'xy' are based on U and V, so I guess I it sufficient to leave it like that. throwing an exception on str_angles not in ('xy', 'uv') seems to be an overkill since a and theta will fallback to using U and V values ...

Copy link
Member

Choose a reason for hiding this comment

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

Appears that angles='uv' is the default, so the existing tests not being changed shows that it's probably fine.

a = lengths
else:
a = np.abs(uv)
Expand All @@ -665,9 +665,9 @@ def _make_verts(self, U, V, angles):
self.scale = scale * widthu_per_lenu
length = a * (widthu_per_lenu / (self.scale * self.width))
X, Y = self._h_arrows(length)
if str_angles and (angles == 'xy'):
if str_angles == 'xy':
theta = angles
elif str_angles and (angles == 'uv'):
elif str_angles == 'uv':
theta = np.angle(uv)
else:
theta = ma.masked_invalid(np.deg2rad(angles)).filled(0)
Expand Down
Binary file added lib/matplotlib/tests/baseline_images/test_quiver/quiver_xy.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
20 changes: 20 additions & 0 deletions lib/matplotlib/tests/test_quiver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,26 @@ def test_bad_masked_sizes():
ax.barbs(x, y, u, v)


def test_angles_and_scale():
# angles array + scale_units kwarg
fig, ax = plt.subplots()
X, Y = np.meshgrid(np.arange(15), np.arange(10))
U = V = np.ones_like(X)
phi = (np.random.rand(15, 10) - .5) * 150
ax.quiver(X, Y, U, V, angles=phi, scale_units='xy')


@image_comparison(baseline_images=['quiver_xy'],
extensions=['png'], remove_text=True)
def test_quiver_xy():
# simple arrow pointing from SW to NE
fig, ax = plt.subplots(subplot_kw=dict(aspect='equal'))
ax.quiver(0, 0, 1, 1, angles='xy', scale_units='xy', scale=1)
ax.set_xlim(0, 1.1)
ax.set_ylim(0, 1.1)
ax.grid()


def test_quiverkey_angles():
# Check that only a single arrow is plotted for a quiverkey when an array
# of angles is given to the original quiver plot
Expand Down