Skip to content

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

Closed
wants to merge 3 commits into from

Conversation

apaszke
Copy link
Contributor

@apaszke apaszke commented Mar 6, 2020

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

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

@anntzer
Copy link
Contributor

anntzer commented Mar 6, 2020

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

@apaszke
Copy link
Contributor Author

apaszke commented Mar 6, 2020

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.

@apaszke
Copy link
Contributor Author

apaszke commented Mar 6, 2020

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.

@anntzer
Copy link
Contributor

anntzer commented Mar 6, 2020

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...
(In think I'd rather leave this specific PR for until all other optimizations go in, so that we can better evaluate the need for it.)

@apaszke
Copy link
Contributor Author

apaszke commented Mar 6, 2020

Sure, I agree that this should wait.

@apaszke apaszke force-pushed the surface_plot_padding branch from 168ce3a to e19fd1c Compare March 13, 2020 20:44
@apaszke
Copy link
Contributor Author

apaszke commented Mar 13, 2020

#16675 has landed, so this is unblocked now.

@anntzer
Copy link
Contributor

anntzer commented Mar 13, 2020

Should we also wait for #16688 and #16689 first?

@apaszke
Copy link
Contributor Author

apaszke commented Mar 13, 2020

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.

@anntzer
Copy link
Contributor

anntzer commented Mar 13, 2020

The point is, we could consider merging all others, and then, if the global speed is improved enough, just deprecate {r,c}{count,stride}.

@apaszke
Copy link
Contributor Author

apaszke commented Mar 14, 2020

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.

@github-actions
Copy link

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.

@github-actions github-actions bot added the status: inactive Marked by the “Stale” Github Action label Jul 12, 2023
@github-actions github-actions bot removed the status: inactive Marked by the “Stale” Github Action label Jan 11, 2024
@scottshambaugh
Copy link
Contributor

scottshambaugh commented Jan 28, 2025

After implementing the other plot_surface speedups, this is what the profiler looks like for a 1000x1000 surface plot:
image

The patching routine here is a very small fraction of the overall runtime, which is dominated by drawing. It's also a one-time cost which won't be re-incurred during plot interaction. And when I test the new code, there is minimal improvement in the speed of that part of the routine (it's still operating with a list comprehension).

So I think this is very low impact, and since this PR is orphaned am closing as won't do. The good news is that the other recent speedups to 3D plotting have resulted in a significant improvement to plot_surface without this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants