-
-
Notifications
You must be signed in to change notification settings - Fork 8k
Fix 3D set_aspect error cases #25481
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
Conversation
The test code is a bit messier now, and the image is more crammed. What if the test image had two rows, where the first row is the original set of aspect displays, and the second shows off the same set but with "datalim" as the adjustable?
7bde732 to
e407716
Compare
Sure, I cleaned up the tests code a bit and made the images clearer.
e407716 to
dc8a7c4
Compare
Most of the original five cubes have now changed their appearance. Not sure if that was expected. Also, maybe I'm not understanding something, but why just add a single new subplot? I was thinking one row would be the original 5, and a second row would be the same aspects, but under the new condition. It would make it easy to visualize the effect of setting the datalim as the "adjustable".
Hi @WeatherGod, I’m not following the two rows idea. What do you mean by "the new condition"? This PR isn’t adding anything new.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Over here. The first five subplots are done with 5x1x1 cuboid, but then the last one is done as a 1x1x1 regular cube. What I'm saying is that the first row can be done as 5x1x1, and the second row as 1x1x1, with each column being a different aspect mode.
We also still have to explain why some of the original five plots now look different. Was that supposed to be expected? I want to make sure the results make sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah originally it was a 1x1x5 and got switched as I was iterating, I switched it back to that just now so it should match what was there before.
After plotting it up I'm not getting too much from the second row... the cube was added because that was a failure case in the linked issue, but I don't think it helps too much to demonstrate the behavior to whoever is looking at tests rather than docs. But I'm happy to add it in if you find it useful.
aspects
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think you are right, it isn't really all that illuminating. What you have now is good.
dc8a7c4 to
1e34062
Compare
Since this was introduced in 3.7(?), I think it makes sense to fix it for 3.7.2.
...481-on-v3.7.x Backport PR #25481 on branch v3.7.x (Fix 3D set_aspect error cases)
PR Summary
Closes #25443
PR Checklist
Documentation and Tests
pytestpasses)Release Notes
.. versionadded::directive in the docstring and documented indoc/users/next_whats_new/.. versionchanged::directive in the docstring and documented indoc/api/next_api_changes/next_whats_new/README.rstornext_api_changes/README.rst