Use np.hypot wherever possible.#10322
Conversation
anntzer
force-pushed
the
hypot
branch
2 times, most recently
from
c86fbc7 to
e097aaf
Compare
afvincent
left a comment
afvincent
left a comment
There was a problem hiding this comment.
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).
| 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) |
| 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) |
|
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 |
|
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" 😉 |
|
|
||
| def _f(xy): | ||
| x, y = xy | ||
| return (x - cx) ** 2 + (y - cy) ** 2 < r2 |
There was a problem hiding this comment.
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.
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 |
anntzer
force-pushed
the
hypot
branch
3 times, most recently
from
5a00e0d to
13cb849
Compare
WeatherGod
left a comment
WeatherGod
left a comment
There was a problem hiding this comment.
Most everything looks good, just one spot that I think is wrong.
| 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.
shouldn't this be np.maximum()?
There was a problem hiding this comment.
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 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).
QuLogic
changed the title
7 participants
Clearer IMO, although perhaps not for the examples?
PR Summary
PR Checklist