Skip to content

Use np.hypot wherever possible. #10322

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

Jump to bottom
Merged
merged 1 commit into from
Oct 7, 2018
Merged

Use np.hypot wherever possible. #10322

merged 1 commit into from
Oct 7, 2018

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Jan 25, 2018

Clearer IMO, although perhaps not for the examples?

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@anntzer anntzer force-pushed the hypot branch 2 times, most recently from c86fbc7 to e097aaf Compare January 25, 2018 22:41
afvincent
afvincent reviewed Jan 25, 2018
View reviewed changes
Copy link
Contributor

@afvincent afvincent left a comment

Choose a reason for hiding this comment

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

Not 100% sure that np.hypot is clearer for most users (I personally remember that I had to have a look at Numpy's doc the first time I saw it in a commit). But maybe it is better known than what I think.

One small typo (I think).

examples/mplot3d/surface3d.py Outdated
@@ -24,7 +24,7 @@
X = np.arange(-5, 5, 0.25)
Y = np.arange(-5, 5, 0.25)
X, Y = np.meshgrid(X, Y)
R = np.sqrt(X**2 + Y**2)
R = np.sqrt(X, Y)
Copy link
Contributor

Choose a reason for hiding this comment

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

np.hypot(X, Y)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

y

lib/matplotlib/patches.py
xm = x1 + self.frac * r * math.cos(theta)
ym = y1 + self.frac * r * math.sin(theta)
xm = x1 + self.frac * (x2 - x1)
ym = y1 + self.frac * (y2 - y1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch :).

@anntzer
Copy link
Contributor Author

anntzer commented Jan 25, 2018

That's why I said "perhaps not for the examples".

@timhoffm
Copy link
Member

Oh, I wasn't aware of hypot either. Maybe not in the examples?

@jklymak
Copy link
Member

jklymak commented Jan 25, 2018

Not particularly a fan of this suggestion. Its a tiny bit shorter, and substantially less readable. I'd have to go a google to figure out exactly what np.hypot means as well.

@anntzer
Copy link
Contributor Author

anntzer commented Jan 25, 2018 •
edited
Loading

I'll revert the examples. Vote on this message w.r.t. the lib itself (+1 to use hypot, -1 to not use it). IMO once you know the function (whose name is not completely silly) it does make things more readable (plus it should be a tiny bit more efficient as it will avoid allocating a bunch of intermediate temporary arrays).

@QuLogic
Copy link
Member

QuLogic commented Jan 26, 2018

Pretty sure you had another PR with something similar and we thought it'd be best not to add hypot in examples.

@anntzer
Copy link
Contributor Author

anntzer commented Jan 26, 2018

You were actually suggesting the other way round :p #7562 (comment)

@anntzer
Copy link
Contributor Author

anntzer commented Jan 26, 2018

reverted the examples

@QuLogic
Copy link
Member

QuLogic commented Jan 26, 2018

That's why I said "we" 😉

jkseppan
jkseppan reviewed Jan 27, 2018
View reviewed changes
lib/matplotlib/bezier.py Outdated
def _f(xy):
x, y = xy
return (x - cx) ** 2 + (y - cy) ** 2 < r2
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 sure that this change is an improvement. The original is probably faster, since there is a square-root computation inside hypot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the real cost is actually setting up the numpy ufunc, but sure.

@jkseppan
Copy link
Member

I think the point of hypot is to avoid numerical problems if one of the arguments is big or small. Perhaps it is slightly easier to read as well, so I think replacing sqrt(x**2+y**2) or the equivalents to use hypot is usually a good idea. I wouldn't replace comparisons of x**2+y**2 against r**2 with a call to hypot, because that's a common performance optimization that avoids computing a square root.

@anntzer
Copy link
Contributor Author

anntzer commented Jan 27, 2018

The real cost is not the square root (~40ns, by timing x**2+y**2 vs math.sqrt(x**2+y**2)), but actually the ufunc machinery (~1us, by timing math.hypot(x, y) vs np.hypot(x, y)). In any case I restored the explicit calculation in these cases.

@anntzer anntzer force-pushed the hypot branch 3 times, most recently from 5a00e0d to 13cb849 Compare March 29, 2018 18:48
WeatherGod
WeatherGod requested changes Oct 6, 2018
View reviewed changes
Copy link
Member

@WeatherGod WeatherGod left a comment

Choose a reason for hiding this comment

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

Most everything looks good, just one spot that I think is wrong.

lib/matplotlib/projections/geo.py Outdated
@@ -497,8 +494,7 @@ def transform_non_affine(self, xy):
y = xy[:, 1:2]
clong = self._center_longitude
clat = self._center_latitude
p = np.sqrt(x*x + y*y)
p = np.where(p == 0.0, 1e-9, p)
p = np.max(np.hypot(x, y), 1e-9)
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this be np.maximum()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a bit scary that we don't error on this, but heh, fixed.

@WeatherGod
Copy link
Member

travis is failing on a streamplot test in all python versions. RMS of 0.0009. Possibly a tiny numerical precision change somewhere?

@anntzer
Copy link
Contributor Author

anntzer commented Oct 6, 2018

Indeed, looks like an accuracy issue on streamplot, can repro locally. Switched the relevant line back to sqrt(x**2+y**2) as I don't think it's worth a baseline image update.

@anntzer
Use np.hypot whereever possible.
dd1c386 
Except examples, where it may be too obscure?

Also added -T to CI sphinx-build invocation, to help troubleshooting
e.g. https://circleci.com/gh/anntzer/matplotlib/2505?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link
(show traceback on sphinx failure).
@WeatherGod WeatherGod merged commit ea031c2 into matplotlib:master Oct 7, 2018
@QuLogic QuLogic added this to the v3.1 milestone Oct 7, 2018
@anntzer anntzer deleted the hypot branch October 7, 2018 02:01
@QuLogic QuLogic changed the title Use np.hypot whereever possible. Use np.hypot wherever possible. Apr 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkseppan jkseppan jkseppan left review comments

@afvincent afvincent afvincent left review comments

@WeatherGod WeatherGod WeatherGod approved these changes

@jklymak jklymak jklymak approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.1.0
Development

Successfully merging this pull request may close these issues.
7 participants
@anntzer @timhoffm @jklymak @QuLogic @jkseppan @WeatherGod @afvincent