| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
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). |
Sorry, something went wrong.
|
Yeah, honestly I don't fully understand why is it the responsibility of plot_surface to do the downsampling, since it shouldn't be that difficult to do for the users. On the other hand, it is a bit annoying to downsample while making sure that you still cover the full range of the plot (that's the reason this PR even has to exist). I don't have all the context here, so it's likely best to leave this decision to you or other core developers, but I'm happy to run some benchmarks comparing the performance before and after my PRs if you can come up with some good test cases and conditions under which those arguments should no longer be necessary. 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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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... |
Sorry, something went wrong.
|
Sure, I agree that this should wait. |
Sorry, something went wrong.
|
#16675 has landed, so this is unblocked now. |
Sorry, something went wrong.
Sorry, something went wrong.
|
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 plot_surface on its own. All of those PRs have benefits of their own, but they're magnified when you apply them all. There's no particular order in which they have to get merged. |
Sorry, something went wrong.
|
The point is, we could consider merging all others, and then, if the global speed is improved enough, just deprecate {r,c}{count,stride}. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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 (for n 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