Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
c86fbc7 toe097aafCompareThere 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.
Not 100% sure thatnp.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) | |||
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.
np.hypot(X, Y)?
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.
y
| 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) |
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.
Nice catch :).
That's why I said "perhaps not for the examples". |
Oh, I wasn't aware of |
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 |
anntzer commentedJan 25, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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). |
Pretty sure you had another PR with something similar and we thought it'd be best not to add |
You were actually suggesting the other way round :p#7562 (comment) |
reverted the examples |
That's why I said "we" 😉 |
lib/matplotlib/bezier.py Outdated
| def _f(xy): | ||
| x, y = xy | ||
| return (x - cx) ** 2 + (y - cy) ** 2 < r2 |
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.
I'm not sure that this change is an improvement. The original is probably faster, since there is a square-root computation inside hypot.
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.
the real cost is actually setting up the numpy ufunc, but sure.
I think the point of |
The real cost is not the square root (~40ns, by timing |
5a00e0d to13cb849CompareThere 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.
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) | |||
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.
shouldn't this benp.maximum()?
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.
It's a bit scary that we don't error on this, but heh, fixed.
travis is failing on a streamplot test in all python versions. RMS of 0.0009. Possibly a tiny numerical precision change somewhere? |
Indeed, looks like an accuracy issue on streamplot, can repro locally. Switched the relevant line back to |
Except examples, where it may be too obscure?Also added -T to CI sphinx-build invocation, to help troubleshootinge.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).
Clearer IMO, although perhaps not for the examples?
PR Summary
PR Checklist