-
-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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.
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
Fix for issue #3930:ConnectionPatch with fancy arrow of length zero produces no plot #4173
Conversation
@@ -316,7 +317,7 @@ def _f(xy): | |||
|
|||
def get_cos_sin(x0, y0, x1, y1): | |||
dx, dy = x1 - x0, y1 - y0 | |||
d = (dx * dx + dy * dy) ** .5 | |||
d = (dx * dx + dy * dy) ** .5 or 1 # Account for divide by zero |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a matter of style, I would much rather see more explicit handling of the zero case via an if statement:
if d == 0:
return 0.0, 0.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 This works and is beautifully elegant, but given the number of people who work on this library we should err on the side of verbose and clarity (unless there is a performance reason, and yes you can take verbosity too far).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I'll make the changes now. The reason for this implementation is that this fix was already present in class Arrow(Patch)
:
Patch.__init__(self, **kwargs)
L = np.sqrt(dx ** 2 + dy ** 2) or 1 # account for div by zero
cx = float(dx) / L
sx = float(dy) / L
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a side note, my negative reaction to it reflected my earlier days working with Perl--it's a typical Perl-style construct, compact but a bit tricky, relying on evaluating an expression in two distinct ways (numerical and Boolean) in a single statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I like that compact statement when I am selecting one of two
items: "somevar = foo or bar". Particularly useful for situations like the
following: "foo, bar = zip(somelist or [[], []])" to prevent errors if
somelist is empty.
But I agree here... having an "or" towards the end of a mathematical
expression just seems wrong. I am not looking for an "or" there, and it is
easy to miss.
On Fri, Feb 27, 2015 at 1:46 PM, Eric Firing notifications@github.com
wrote:
In lib/matplotlib/bezier.py
#4173 (comment):@@ -316,7 +317,7 @@ def _f(xy):
def get_cos_sin(x0, y0, x1, y1):
dx, dy = x1 - x0, y1 - y0
- d = (dx * dx + dy * dy) ** .5
- d = (dx * dx + dy * dy) ** .5 or 1 # Account for divide by zero
As a side note, my negative reaction to it reflected my earlier days
working with Perl--it's a typical Perl-style construct, compact but a bit
tricky, relying on evaluating an expression in two distinct ways (numerical
and Boolean) in a single statement.—
Reply to this email directly or view it on GitHub
https://github.com/matplotlib/matplotlib/pull/4173/files#r25528281.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, can we change that expression to use np.hypot()?
On Fri, Feb 27, 2015 at 2:08 PM, Benjamin Root ben.root@ou.edu wrote:
Personally, I like that compact statement when I am selecting one of two
items: "somevar = foo or bar". Particularly useful for situations like the
following: "foo, bar = zip(somelist or [[], []])" to prevent errors if
somelist is empty.But I agree here... having an "or" towards the end of a mathematical
expression just seems wrong. I am not looking for an "or" there, and it is
easy to miss.On Fri, Feb 27, 2015 at 1:46 PM, Eric Firing notifications@github.com
wrote:In lib/matplotlib/bezier.py
#4173 (comment)
:@@ -316,7 +317,7 @@ def _f(xy):
def get_cos_sin(x0, y0, x1, y1):
dx, dy = x1 - x0, y1 - y0
- d = (dx * dx + dy * dy) ** .5
- d = (dx * dx + dy * dy) ** .5 or 1 # Account for divide by zero
As a side note, my negative reaction to it reflected my earlier days
working with Perl--it's a typical Perl-style construct, compact but a bit
tricky, relying on evaluating an expression in two distinct ways (numerical
and Boolean) in a single statement.—
Reply to this email directly or view it on GitHub
https://github.com/matplotlib/matplotlib/pull/4173/files#r25528281.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like this?
L = np.hypot(dx, dy)
# Account for divide by zero
if L == 0:
L = 1
This would also need to be changed in Curve(_Base)
and FancyArrow(Polygon)
, for consistency.
@@ -1113,6 +1118,11 @@ def __init__(self, x, y, dx, dy, width=0.001, length_includes_head=False, | |||
head_length = 1.5 * head_width | |||
|
|||
distance = np.sqrt(dx ** 2 + dy ** 2) | |||
|
|||
# Account for divide by zero | |||
if distance == 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels wrong to me, shouldn't be we special casing it where we divide by it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that makes sense. Do you want to just move the if statement above the division, or handle it without changing distance?
if distance != 0:
cx = float(dx) / distance
sx = float(dy) / distance
else:
cx = float(dx)
sx = float(dy)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but it would be clearer and more direct to replace those last two lines with cx, sx = 0, 1
. All of this is not fundamentally a matter of handling a possible "divide by zero", it is handling a situation in which the length of a vector is zero so there is no information about orientation; the orientation is arbitrary.
BUG : fix zero length ConnectionPatch closes #3930
@basharovV Thank you! |
Awesome, thanks! Yes that PR is similar, but doesn't solve all the possible errors (some arrow styles will still cause division by zero, as well as creating a FancyArrow directly). |
Fixes 'division by zero' errors in patches.py and bezier.py to address issue #3930. Now allows to plot fancy arrows of length 0 using either FancyArrow directly or by using a ConnectionPatch.
The test_fancyarrow() case in test_arrowpatches.py now tests for arrows of length 0, and the expected images were updated accordingly.