Refactor for issue 28444 [Introduce _Bbox3D to represent 3D axes limits] #30115
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.) |
This reverts commit 7b8c5e1.
|
I fixed the logic in my code and now the tests are passing correctly! Please tell me if it's implemented as expected |
| raise ValueError("Expected array of shape (N, 3)") | ||
| xmin, xmax = np.min(arr[:, 0]), np.max(arr[:, 0]) | ||
| ymin, ymax = np.min(arr[:, 1]), np.max(arr[:, 1]) | ||
| zmin, zmax = np.min(arr[:, 2]), np.max(arr[:, 2]) |
There was a problem hiding this comment.
Should be slightly faster to do these in one pass:
mins = np.min(arr, axis=0)
maxs = np.max(arr, axis=0)
xmin, ymin, zmin = mins
xmax, ymax, zmax = maxsThere was a problem hiding this comment.
May also need to handle nan values? In which cases nanmin and nanmax should be used. Not clear to me if those are filtered out by this point.
| @@ -963,6 +963,28 @@ def update_from_data_xy(self, xy, ignore=None, updatex=True, updatey=True): | |||
| self.update_from_path(path, ignore=ignore, | |||
| updatex=updatex, updatey=updatey) | |||
|
|
|||
| def update_from_bbox(self, bbox, ignore=False, updatex=True, updatey=True): | |||
| """ | |||
| Update the Bbox to include another Bbox. | |||
There was a problem hiding this comment.
Could you add a little more clarity to the docstring here? My read is that this will strictly increase the size of the initial Bbox, or keep it the same if it already bounds the input bbox. Unless the ignore flag is True, in which case it will set the values to the input bbox.
| @@ -97,6 +98,16 @@ def _viewlim_mask(xs, ys, zs, axes): | |||
| return mask | |||
|
|
|||
|
|
|||
| def create_bbox3d_from_array(arr): | |||
| arr = np.asarray(arr) | |||
| if arr.ndim != 2 or arr.shape[1] != 3: | |||
There was a problem hiding this comment.
Should also handle the case of a shape (0, 3) array
| # FIXME: Implement auto-scaling function for Patch3DCollection | ||
| # Currently unable to do so due to issues with Patch3DCollection | ||
| # See https://github.com/matplotlib/matplotlib/issues/14298 for details | ||
| if autolim and hasattr(col, "_get_datalim3d"): |
There was a problem hiding this comment.
hasattr(col, "_get_datalim3d")
Are there any which do not? Should raise a warning in an elif case
| @@ -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.
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.
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.
PR summary
This PR adresses issue #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