Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Vectorize patch extraction in Axes3D.plot_surface#16675
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
Vectorize patch extraction in Axes3D.plot_surface#16675
Uh oh!
There was an error while loading.Please reload this page.
Conversation
I just did a quick benchmark, and this PR reduces the runtime of the following script from 16s to 10s on my old MBP (speedup might be higher on systems with more cores): importnumpyasnpimportmatplotlib.pyplotaspltfig=plt.figure()ax=fig.gca(projection='3d')x=y=np.linspace(-1,1,400)x,y=np.meshgrid(x,y)z=x**2+y**2ax.plot_surface(x,y,z,rstride=1,cstride=1)fig.savefig('dump.png')plt.close() |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
Some minor comments which need to be addressed but are not actually really blocking, this basically looks great.
Also note that I haven't added any tests because I've checked that existing |
let's keep that discussion in the corresponding PR then. |
@anntzer Please see if the |
Uh oh!
There was an error while loading.Please reload this page.
looks great, just needs a second positive review. |
Is there any reason to prefer squashing on my side to merge + squash on GitHub? I've usually done the latter for most projects. What's the policy here? |
50e2157
to5a8a5f4
CompareI think it's mostly habit, though perhaps@tacaswell will have an opinion there :) |
Personally, I dislike GitHub squash merge because it breaks contributors' |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Small correction, parameter names in docstrings should be |
@QuLogic I've applied the suggested changes and added an example for |
Thanks, the example is quite helpful. |
Do you want to squash this? |
6af6818
toae5c7b3
CompareWhen rstride and cstride divide the numbers of rows and columns minus 1,all polygons have the same number of vertices which can be recoveredusing some stride tricks on NumPy arrays. On a toy benchmark of a800x800 grid this allows to reduce the time it takes to produce thislist of polygons from ~10s to ~60ms.
ae5c7b3
to907235d
Compare@QuLogic done! |
Failure looks like disk / filesystem related problems. |
Thanks@apaszke ! Congratulations on your first Matplotlib PR 🎉 hopefully we will hear from you again. |
Failure was#16735. |
@tacaswell Thanks! I already havea few followup PRs posted, so I'd appreciate reviews on those! Also thanks to reviewers for helping to get this in! |
Uh oh!
There was an error while loading.Please reload this page.
PR Summary
First of the improvements suggested in#16659.
When rstride and cstride divide the numbers of rows and columns minus 1,
all polygons have the same number of vertices which can be recovered
using some stride tricks on NumPy arrays. On a toy benchmark of a
800x800 grid this allows to reduce the time it takes to produce this
list of polygons from ~10s to ~60ms.
The formatting gets a bit awkward in some places due to the 79 column limit 😕 Also, Flake 8 seems to report many errors in the files I've modified, but I've only tried to make sure the lines I've added don't show up to minimize the diff.
PR Checklist