Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Refactor for issue 28444 [Introduce _Bbox3D to represent 3D axes limits]#30115
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
I'm currently getting errors in these two tests. I tried fixing them but i didn't get the idea of these specific values |
The values are most likely observations of what the current algorithm gives. However, looking at the last failure, the updated method determines the limits -0.02, 1.02, while the old version was -0.08, 4.0833 (basically 0, 1 now, earlier 0, 4, but with some margins). Looking at the input data for the last test, there are z-values that are 4, so in that case it seems like the new approach is not working. (Now that I understand it a bit more, in the first test it has changed from 0, 2 to 0, 1. But one line has an x-value of 2, so, again, the new approach seems to be off. While probably not the case, it seems like the new approach is always return 0, 1, at least based on the working and failing tests, although the code seems to make sense from a quick check.) |
I fixed the logic in my code and now the tests are passing correctly! Please tell me if it's implemented as expected |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| @@ -2887,19 +2906,9 @@ def add_collection3d(self, col, zs=0, zdir='z', autolim=True, *, | |||
| axlim_clip=axlim_clip) | |||
| col.set_sort_zpos(zsortval) | |||
| if autolim: | |||
| if isinstance(col, art3d.Line3DCollection): | |||
| self.auto_scale_xyz(*np.array(col._segments3d).transpose(), | |||
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.
Does anything use auto_scale_xyz internally after this? Should still keep it since it's a public method, but should make sure everything is switched over
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.
I see in your initial comment that this is meant to lay the groundwork for switching everything over in a follow-on PR - I think that incrementalism is fine but also feel free to put everything into one if you prefer.
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.
Hi Scott! I did a full review of the remaining auto_scale_xyz() usages in axes3d.py. I noticed that:
1 - Some cases are tied to 3D collections like Line3DCollection and Poly3DCollection, which now have _get_datalim3d() implemented. These can be cleanly refactored to use auto_scale_lim() instead.
2 - Other cases operate directly on raw X, Y, Z array inputs. In these, there's no associated artist or _Bbox3d abstraction, so converting to auto_scale_lim() would require constructing temporary bounding boxes manually — which feels like a larger shift in design.
Before I proceed, I wanted to check:
Would you prefer that I refactor only the collection-based uses in this PR, and leave the raw-data ones as they are? Or should I aim for a broader unification?
Thanks in advance for the guidance!
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.
Hm, I think it makes sense to move to bounding boxes for the cases where they're currently operating on raw X, Y, Z array inputs. It will make clearer what's going on, and since the array data is not being copied should not introduce any overhead.
PR summary
This PR adressesissue #28444 and introduces a new internal 3D bounding box system to improve the way data limits are computed for 3D artists in mpl_toolkits.mplot3d.
Why is this change necessary?
Currently, autoscaling for 3D plots requires extracting and copying x, y, and z data from each artist, which is inefficient and error-prone. The system relies on auto_scale_xyz(), which is tied to manual data unpacking.
What’s implemented:
Added _Bbox3d, a 3D bounding box class with conversion methods to 2D Bbox
Added _get_datalim3d() to several 3D artist types: Line3D, Line3DCollection, Poly3DCollection, Patch3D, etc.
Implemented Axes3D.auto_scale_lim(bbox3d) using Bbox.union()
Updated add_collection3d() to use _get_datalim3d() and auto_scale_lim()
This lays the groundwork for a full refactor and replacement of auto_scale_xyz() across the codebase.
Let me know if it is workin as intended