| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
This needs a test, and is the issue present in other 3D artists? We would want to solve them all at once, and this check probably belongs in a helper function. |
Sorry, something went wrong.
Hi @scottshambaugh, that makes total sense! You are absolutely right—this is definitely affecting plot_wireframe, plot_trisurf, and likely others as well. I will abstract the masking logic into a dedicated helper function so it can be applied cleanly across the relevant 3D artists. I'll also get a unit test added to explicitly cover the negative/zero log scale masking. I'll ping you once the refactor and tests are pushed! |
Sorry, something went wrong.
|
You're on the right track, but per our policy please don't use AI to write your pull request description or comments except for direct translation. |
Sorry, something went wrong.
since this is my first PR ,i leaned on it for the formatting part for my text , so it sounds professional . |
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks, your English is very understandable and we would rather hear your direct voice than an AI.
Sorry, something went wrong.
|
|
||
| X = np.where(val_in_range_X(X), X, np.nan) | ||
| Y = np.where(val_in_range_Y(Y), Y, np.nan) | ||
| Z = np.where(val_in_range_Z(Z), Z, np.nan) |
There was a problem hiding this comment.
This is fine for now since we're working a bug fix and want to limit changes. Being able to pass numpy arrays into val_in_range will be more efficient in the future.
Sorry, something went wrong.
| ax.set_zscale('log') | ||
|
|
||
| # Drawing forces evaluation and confirms the absence of crashes | ||
| fig.canvas.draw() |
There was a problem hiding this comment.
This is an inadequate test, as the plot was not crashing previously. Tests need to fail on main and pass on your branch to demonstrate the improvement
Sorry, something went wrong.
There was a problem hiding this comment.
@scottshambaugh thanks 😅i will remove that double/duplicate import and update the test to explicitly assert that the invalid polygons are dropped and i will push these and ensure these updates now!
Sorry, something went wrong.
There was a problem hiding this comment.
@scottshambaugh thanks 😅i will remove that double/duplicate import and update the test to explicitly assert that the invalid polygons are dropped and i will push these and ensure these updates now!
@scottshambaugh just pushed the updates!
I think some Windows CI tests timed out again, but the actual 3D logic fix green okay right?. Let me know how it looks and if we need any final fixes before merging!
Sorry, something went wrong.
There was a problem hiding this comment.
@rahulrathnavel this updated test also passes when I run it on main. You need to actually run the new test yourself on both branches - it wastes our time to have to double check your work. Do not delegate this to an AI. You need to understand all changes you are proposing.
Also, please check exact values in general rather than <, it's a bit more protective.
Sorry, something went wrong.
You've only touched 3 of the axes3d plotting methods, and this really should be applied at the artists3d level instead. I appreciate your enthusiasm and work so far, and if you want to take one more try that's fine. But with the time pressure to get this in 3.11 and the errors in not addressing my comments I would not recommend choosing this as your first issue to solve. |
Sorry, something went wrong.
@scottshambaugh, thank you for the honest feedback! I completely understand. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
PR summary
Closes #31726
Why is this change necessary & What problem does it solve?
Currently, creating a 3D surface plot (ax.plot_surface) with a non-linear scale (e.g., ax.set_zscale('log')) completely corrupts the surface rendering if the data contains zero or negative values. The new 3D scale transforms (introduced in #30980) were allowing mathematically invalid values to pass through to Poly3DCollection, resulting in NaN/-inf during projection and breaking the renderer.
What is the reasoning for this implementation?
Following the guidance of @scottshambaugh, this PR utilizes the new Scale.val_in_range method introduced in #31306.
Immediately after broadcasting X, Y, and Z in plot_surface, we use np.where to check if the coordinates are valid for their respective axis scales. Any invalid values are replaced with np.nan. This elegantly hooks into the existing downstream NaN filtering logic inside plot_surface, allowing the invalid polygons to be gracefully discarded without corrupting the rest of the valid surface.
AI Disclosure
I used an AI coding assistant to help analyze the 3D projection pipeline and draft the val_in_range masking logic for this PR.
PR checklist