Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Conversation

apaszke
Copy link
Contributor

@apaszkeapaszke commentedMar 5, 2020
edited
Loading

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

  • Has Pytest style unit tests
  • Code isFlake 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

@apaszke
Copy link
ContributorAuthor

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()

Copy link
Contributor

@anntzeranntzer left a 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.

@apaszke
Copy link
ContributorAuthor

Also note that I haven't added any tests because I've checked that existingplot_surface tests trigger both code paths.

@apaszke
Copy link
ContributorAuthor

@anntzer I added the padding in#16699.

@anntzer
Copy link
Contributor

let's keep that discussion in the corresponding PR then.

@apaszke
Copy link
ContributorAuthor

@anntzer Please see if the_array_patch_perimeters comment is satisfactory now. I think all your concerns should be addressed now.

@anntzer
Copy link
Contributor

looks great, just needs a second positive review.
you may also consider squashing your commits.

@apaszke
Copy link
ContributorAuthor

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?

@apaszkeapaszkeforce-pushed thevectorize_surface_plot_perimeters branch from50e2157 to5a8a5f4CompareMarch 6, 2020 14:23
@anntzer
Copy link
Contributor

I think it's mostly habit, though perhaps@tacaswell will have an opinion there :)

@QuLogic
Copy link
Member

Personally, I dislike GitHub squash merge because it breaks contributors'git prune +git branch -d.

@QuLogic
Copy link
Member

Small correction, parameter names in docstrings should be*var*, not**var**.

@apaszke
Copy link
ContributorAuthor

@QuLogic I've applied the suggested changes and added an example for_unfold.

@QuLogic
Copy link
Member

Thanks, the example is quite helpful.

@QuLogic
Copy link
Member

Do you want to squash this?

@apaszkeapaszkeforce-pushed thevectorize_surface_plot_perimeters branch from6af6818 toae5c7b3CompareMarch 13, 2020 09:54
When 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.
@apaszkeapaszkeforce-pushed thevectorize_surface_plot_perimeters branch fromae5c7b3 to907235dCompareMarch 13, 2020 09:55
@apaszke
Copy link
ContributorAuthor

@QuLogic done!

@tacaswelltacaswell added this to thev3.3.0 milestoneMar 13, 2020
@tacaswell
Copy link
Member

Failure looks like disk / filesystem related problems.

@tacaswelltacaswell merged commit8094fb1 intomatplotlib:masterMar 13, 2020
@tacaswell
Copy link
Member

Thanks@apaszke ! Congratulations on your first Matplotlib PR 🎉 hopefully we will hear from you again.

@QuLogic
Copy link
Member

Failure was#16735.

@apaszke
Copy link
ContributorAuthor

@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!

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@anntzeranntzeranntzer approved these changes

@QuLogicQuLogicQuLogic approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
v3.3.0
Development

Successfully merging this pull request may close these issues.

4 participants
@apaszke@anntzer@QuLogic@tacaswell

[8]ページ先頭

©2009-2025 Movatter.jp