Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork8.1k
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 toe097aafCompare
afvincent left a comment
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.
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
| 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 :).
anntzer commentedJan 25, 2018
That's why I said "perhaps not for the examples". |
timhoffm commentedJan 25, 2018
Oh, I wasn't aware of |
jklymak commentedJan 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 |
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). |
QuLogic commentedJan 26, 2018
Pretty sure you had another PR with something similar and we thought it'd be best not to add |
anntzer commentedJan 26, 2018
You were actually suggesting the other way round :p#7562 (comment) |
anntzer commentedJan 26, 2018
reverted the examples |
QuLogic commentedJan 26, 2018
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.
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.
jkseppan commentedJan 27, 2018
I think the point of |
anntzer commentedJan 27, 2018
The real cost is not the square root (~40ns, by timing |
5a00e0d to13cb849Compare
WeatherGod left a comment
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.
Most everything looks good, just one spot that I think is wrong.
lib/matplotlib/projections/geo.py Outdated
| 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.
WeatherGod commentedOct 6, 2018
travis is failing on a streamplot test in all python versions. RMS of 0.0009. Possibly a tiny numerical precision change somewhere? |
anntzer commentedOct 6, 2018
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