Skip to content

Fix plot_wireframe with nonequal rstride, cstride, plus additional speedups #29435

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

Merged
merged 4 commits into from
Jan 24, 2025

Conversation

scottshambaugh
Copy link
Contributor

@scottshambaugh scottshambaugh commented Jan 8, 2025

PR summary

I accidentally introduced a regression in #29399, which broke wireframe plots when there are an unequal number of rows and columns. That case forms ragged data which can not be concatenated into a single numpy array. This fixes that, and adds an image test. But the fix isn't ideal, and there are a few different paths we could take:

  1. The current solution of creating an array large enough to hold both differently-shaped row and column arrays introduces a potentially large number of NaN points in the returned Line3DCollection, which might be an issue for users that then postprocess this data.
  2. An alternative would be to generate and return separate Line3DCollection's for the rows and columns, but that changes the return signature.
  3. Potentially we could modify Line3DCollection to handle groups of several collections internally? Seems messy though.
  4. We could also partially revert the previous PR and return to ragged lists for this data, but lose the vectorization / draw speed improvements.

Note that this issue isn't present for other collections such as Poly3DCollection because those do not plot a higher density of data on the lines in-between grid points like Line3DCollection does.

There is also a performance improvement here. In the previous PR I identified the autoscaling of the axes as a large time sink, but didn't realize that there was lower hanging fruit. Instead of autoscaling based on all the X, Y, Z data passed in, we now only autoscale based on the visible data. For my example test of a 4000x4000 wireframe at the default rstride, cstride = 50, this reduces the data parsed for autoscaling from 3x4000x4000 to 3x4000x100. There is a slight change in behavior in that axis limits will no longer bound data which are skipped over between lines, which changes one of the baseline image tests.

Before, majority of plot time (3.5 seconds) dedicated to autoscaling:
image

After, 20 milliseconds dedicated to autoscaling:
image

PR checklist

@scottshambaugh scottshambaugh added topic: mplot3d Performance PR: bugfix Pull requests that fix identified bugs labels Jan 8, 2025
@scottshambaugh scottshambaugh added this to the v3.11.0 milestone Jan 8, 2025
@scottshambaugh scottshambaugh changed the title Fix plot_wireframe with nonequal rstride, cstride, plus additional speedups Fix plot_wireframe with nonequal rstride, cstride, plus additional speedups Jan 8, 2025
@scottshambaugh
Copy link
Contributor Author

scottshambaugh commented Jan 24, 2025

Thanks to the dev team for talking through this on the call today! It turns out that the approach there works well - keep the original data held as a ragged list in the Line3DCollection, and then just autoscale twice with the vectorized column and row data.

Speed to draw a 8000x8000 wireframe plot:

  • v3.10: 6.82 sec
  • main: 4.00 sec
  • this PR: 0.80 sec

The autoscaling still takes up a decent chunk of time (170 ms) relative to the draw routines, but it's vastly improved.
image

@oscargus oscargus merged commit ac77fea into matplotlib:main Jan 24, 2025
38 of 41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Performance PR: bugfix Pull requests that fix identified bugs topic: mplot3d
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants