str_angles and scale_units logic for quiver #8670

t-bltg · 2017-05-26T17:24:00Z

issue 1
d705dbd is introducing issues when using angles='xy', especially at l.660:

if str_angles and (angles == 'xy'):
    theta = angles

, comparing angles (now an array) to a string is throwing a warning

FutureWarning: elementwise comparison failed; returning scalar instead, but in the future will perform elementwise comparison

issue 2
using scale_units='xy' together with an angles array throws an

UnboundLocalError: local variable 'lengths' referenced before assignment

where it shouldn't

reproduce the problem

import numpy as np
import matplotlib.pyplot as plt

# logic issue (angles: str is overwritten with an array), theta is wrong
_, 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()

# scale_units='xy' + angles array issue
_, 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')

this patch fixes both issues

QuLogic · 2017-05-26T22:52:35Z

lib/matplotlib/quiver.py

@@ -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 ''


I'm not seeing why this change is necessary.

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

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.

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 ...

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

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.

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

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

dstansby

This looks good, thanks for fixing what I broke in my original commit! It looks like this has changed one of the test images: https://travis-ci.org/matplotlib/matplotlib/jobs/236581341#L1788 - could you generate a new image and see what the difference is?

dstansby · 2017-05-27T10:04:37Z

lib/matplotlib/quiver.py

            # 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':


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

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

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.

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

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 ...

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

dstansby · 2017-05-27T10:05:05Z

lib/matplotlib/tests/test_quiver.py

+    # 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()


Could these all go on a separate line without semicolons

done. the issue with the test_image is mostly due to different version of freetype on my dev version, I will try to generate it using mpl from the repos or anaconda ... thanks

QuLogic · 2017-05-28T21:56:35Z

Once you've answered the last question, can you rebase and remove the multiple copies of the test image?

added 2 simple tests

QuLogic · 2017-05-31T21:22:48Z

lib/matplotlib/quiver.py

            # 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':


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

tacaswell · 2017-06-01T02:00:28Z

🎉 Congratulations on your first merged PR into Matplotlib @neok-m4700 ! Hopefully we will hear from you again!

t-bltg · 2017-06-02T07:30:58Z

Thanks @tacaswell , I guess there is a start to everything :)

QuLogic reviewed May 26, 2017

View reviewed changes

dstansby reviewed May 27, 2017

View reviewed changes

fixing quiver logic

47825e9

added 2 simple tests

t-bltg force-pushed the upstream branch from 52700e4 to 47825e9 Compare May 29, 2017 00:05

QuLogic approved these changes May 31, 2017

View reviewed changes

tacaswell merged commit da711e1 into matplotlib:master Jun 1, 2017

t-bltg deleted the upstream branch June 2, 2017 20:07

QuLogic added this to the 2.1 (next point release) milestone Aug 7, 2017

jklymak mentioned this pull request Nov 23, 2017

quiver angles array UnboundLocalError: local variable 'lengths' referenced before assignment #9840

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

str_angles and scale_units logic for quiver #8670

str_angles and scale_units logic for quiver #8670

t-bltg commented May 26, 2017

QuLogic May 26, 2017

t-bltg May 26, 2017 •

edited

Loading

QuLogic May 27, 2017

t-bltg May 27, 2017 •

edited

Loading

QuLogic May 27, 2017

t-bltg May 27, 2017

QuLogic May 27, 2017

t-bltg May 27, 2017

dstansby left a comment

dstansby May 27, 2017

t-bltg May 27, 2017

QuLogic May 27, 2017

QuLogic May 27, 2017

t-bltg May 28, 2017 •

edited

Loading

QuLogic May 31, 2017

dstansby May 27, 2017

t-bltg May 27, 2017 •

edited

Loading

QuLogic commented May 28, 2017

QuLogic May 31, 2017

tacaswell commented Jun 1, 2017

t-bltg commented Jun 2, 2017

str_angles and scale_units logic for quiver #8670

str_angles and scale_units logic for quiver #8670

Conversation

t-bltg commented May 26, 2017

Choose a reason for hiding this comment

t-bltg May 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-bltg May 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dstansby left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-bltg May 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-bltg May 27, 2017 • edited Loading

Choose a reason for hiding this comment

QuLogic commented May 28, 2017

Choose a reason for hiding this comment

tacaswell commented Jun 1, 2017

t-bltg commented Jun 2, 2017

t-bltg May 26, 2017 •

edited

Loading

t-bltg May 27, 2017 •

edited

Loading

t-bltg May 28, 2017 •

edited

Loading

t-bltg May 27, 2017 •

edited

Loading