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:
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.
An alternative would be to generate and return separate Line3DCollection's for the rows and columns, but that changes the return signature.
Potentially we could modify Line3DCollection to handle groups of several collections internally? Seems messy though.
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:
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
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.
timhoffm reacted with hooray emoji
rcomer reacted with rocket emoji
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
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:

After, 20 milliseconds dedicated to autoscaling:

PR checklist