-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Pad Axes3D.plot_surface inputs to allow for vectorized processing #16699
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Actually, the fundamental reason for the existence of r/c/stride/count is the very bad efficiency of plot_surface (you can look up the old issues for that). Do your earlier changes improve the situation enough to make r/c/stride/count unnecessary anymore from a performance POV? If so, another option could be to just deprecate them (the user can still manually downsample their array if they really want such an effect, as for imshow() or any other 2d plotting function). |
Yeah, honestly I don't fully understand why is it the responsibility of Also note that while my PR speeds up the whole preprocessing/projection pipeline, there are still significant overheads in path creation and processing in the backends because this code is not batched at the moment. Striding certainly limits the number of paths that have to be constructed, so it might still be quite a bit faster to do it this way. |
To me it would be very interesting to try to push backends closer to the possible perf limits. It is quite unreasonable that my PC can render graphics in latest games in 4k resolution at 60fps, but needs a second to draw a surface plot on an 800x800 grid. Interestingly enough I've tried multiple other Python packages for drawing but none of them had good performance for my use case, and matplotlib seemed like the one that would be the easiest to patch. |
If you look at the old discussions, some devs thought that plot_surface should not be in charge of the downsampling, but the perf was so bad that it was left in. We could revisit this now, though... |
Sure, I agree that this should wait. |
168ce3a
to
e19fd1c
Compare
#16675 has landed, so this is unblocked now. |
The full benefit of this PR won't be realized until #16688 and #16689 land, but it still provides significant speedups for the slow path in |
The point is, we could consider merging all others, and then, if the global speed is improved enough, just deprecate {r,c}{count,stride}. |
Sure. As I've said they're independent so I'm ok with landing them in any order you like. We can wait for the other PRs for now. |
Since this Pull Request has not been updated in 60 days, it has been marked "inactive." This does not mean that it will be closed, though it may be moved to a "Draft" state. This helps maintainers prioritize their reviewing efforts. You can pick the PR back up anytime - please ping us if you need a review or guidance to move the PR forward! If you do not plan on continuing the work, please let us know so that we can either find someone to take the PR over, or close it. |
PR Summary
As suggested by @anntzer, inputs which normally cannot be processed by the vectorized patch extraction routines from #16675 can be padded. This works out quite nicely and should extend the speedups I've been working on to all kinds of surface plots.
I'm not 100% sure if I'm handling the face colors correctly, mostly because I cannot find enough documentation to fully understand how to work with this argument, and I also don't think that any of the tests actually uses it. It would be great if someone could tell me how to correctly set up such a test so that I can make sure that I haven't messed it up.
Note that I needed to update one figure, because this patch does change the values of the surface normals for padded faces. This is because normal vectors are currently approximated by taking vertices with indices
0, n//3, 2*n//3
(forn
being the number of vertices on the boundary of this face), and so padding can obviously affect those choices. The color differences are however barely noticeable, and it's an approximation anyway, so I don't think it should be a blocking issue.PR Checklist