Conversation
|
This does not address all the comments I left on the prior PR. |
|
Thanks for the quick feedback. I tried to address the points from the prior PR as follows:
If I am still missing one or more of your prior points, could you please point out which specific ones are still unresolved? I am happy to revise. |
|
|
Thanks for clarifying. I updated the PR description to mark this as a partial fix (no longer claiming it closes #784). For follow-up coverage, could you point me to the specific 3D artist classes you want included in this PR? I can extend it accordingly. |
|
Could you first please clarify if you are using AI assistance to write your comments here? |
|
Yes. For comments, I use AI only for translation/grammar because I'm a Brazilian Portuguese speaker and wanna communicate clearly in English. The technical reasoning is mine: I reviewed the code changes myself, ran the tests locally and I can explain each change in detail. I also disclosed AI assistance in the PR template. :) |
|
Ok, translation is fine. We would want to bring this in for all 3D artists at the same time, so that there is consistent behavior. We would also want to include I also do not understand the purpose of the array modification tests, could you please explain why those are needed? |
|
Thanks for clarifying. Understood on consistency. I'll revise this PR to add a consistent 3D offset API across all relevant mplot3d artists, including a getter counterpart ( About the If you prefer this PR to stay strictly on offsets APIs, I can remove the |
scottshambaugh
added
the
status: autoclose candidate
|
⏰ This pull request might be automatically closed in two weeks from now. Thank you for your contribution to Matplotlib and for the effort you have put into this PR. This pull request does not yet meet the quality and clarity standards needed for an effective review. Project maintainers have limited time for code reviews, and our goal is to prioritize well-prepared contributions to keep Matplotlib maintainable. Matplotlib maintainers cannot provide one-to-one guidance on this PR. However, if you ask focused, well-researched questions, a community member may be willing to help. 💬 To increase the chance of a productive review:
As the author, you are responsible for driving this PR, which entails doing necessary background research as well as presenting its context and your thought process. If you are a new contributor, or do not know how to fulfill these requirements, we recommend that you familiarize yourself with Matplotlib's development conventions or engage with the community via our Discourse or one of our meetings before submitting code. If you substantially improve this PR within two weeks, leave a comment and a team member may remove the |
|
Thanks again for the detailed guidance on the previous review. I pushed a focused revision for the two collection classes in this PR ( In this revision I added After CI completed, the remaining failures appear unrelated to these mplot3d changes:
I did not find failures in the modified mplot3d tests in CI logs. I also see If you prefer, I can also extend this PR to additional 3D artist classes to match a fully consistent offsets API across mplot3d. |
|
Again, we would want to bring this in for all 3D artists at the same time, so that there is consistent behavior. |
scottshambaugh
left a comment
scottshambaugh
left a comment
There was a problem hiding this comment.
All 3D artists need to be covered by these changes.
- Add explicit set/get_verts_and_codes3d support to Collection3D and use it in collection_2d_to_3d. - Preserve axlim_clip in Patch3D/PathPatch3D geometry setters unless explicitly overridden. - Harden get_offsets3d legacy fallback when _zdir is missing in older instances. - Add focused regression tests for Collection3D, legacy offsets fallback, and axlim_clip preservation; fix ruff line-length failure in tests.
|
Hi @scottshambaugh, thanks for the review. I moved this PR to draft while I fixed issues I had missed in the previous revision. I just pushed a single atomic update focused on API consistency and compatibility:
I marked the PR as ready for review again. |
scottshambaugh
removed
the
status: autoclose candidate
a7cb6d6 to
e98fe2c
Compare
|
Marking this PR ready for review again. Current remaining CI failures appear unrelated to this mplot3d change:
Core mplot3d jobs are passing. |
scottshambaugh
added
the
status: autoclose candidate
This comment was marked as resolved.
This comment was marked as resolved.
scottshambaugh
removed
the
status: autoclose candidate
There was a problem hiding this comment.
Didn't mean to add the autoclose tag again, you can ignore that. And CI can be flaky - don't worry about failures that are unrelated to changes you make.
Some comments to address, but zooming out I think there is still a lot of work that needs to go into thinking through the API here. The high level goal is to have 3D getters/setters for positional data that mirrors the 2D ones. Some of the artists have their primary data coordinates as segments / verts / offsets, and some set offsets on top of that. I think some of the 3D artists should accept offsets, but currently don't.
I would recommend stepping back and making a table mapping out the holes in the API before implementing further changes. (You may want to pick an easier issue for your first contribution to the repo)
| """ | ||
| if hasattr(self, "_offsets3d_data"): | ||
| return self._offsets3d_data | ||
| # Backward compatibility for instances that pre-date get_offsets3d. |
There was a problem hiding this comment.
get_offsets3d is a new API and shouldn't need backwards compatibility, here and elsewhere
There was a problem hiding this comment.
Good point — agreed. get_offsets3d is a new API and should not carry backward-compatibility fallbacks. I'll remove this fallback here and align the same behavior in the similar getters.
| codes : array-like or None | ||
| The path codes. | ||
| """ | ||
| if hasattr(self, "_codes3d_data"): |
There was a problem hiding this comment.
Why the fallbacks here?
There was a problem hiding this comment.
Good catch. This fallback is unnecessary here. I added it to preserve behavior for older internal state, but for this API we should avoid extra fallback paths. I'll remove this fallback and keep a single canonical source for codes.
| return xs, ys, zs | ||
|
|
||
|
|
||
| def _backjuggle_axes(xs, ys, zs, zdir): |
There was a problem hiding this comment.
Why is y/-y split but not x and z, and why _str_equal instead of ==? Please match the logic in juggle_axes and put in tests to verify each case.
There was a problem hiding this comment.
You're right — this inversion logic should mirror juggle_axes exactly. I will rewrite _backjuggle_axes to follow the same case handling (including signed axis directions) and switch to the same comparison style. I will also add explicit tests for each supported zdir case to verify round-trip behavior.
| self._axlim_clip = axlim_clip | ||
| self.stale = True | ||
|
|
||
| def set_offsets3d(self, xs, ys, zs, *, zdir='z'): |
There was a problem hiding this comment.
The 2D case calls with Collection.set_offsets(offsets) rather than breaking out x and y as separate args, and we should match that pattern here
There was a problem hiding this comment.
Makes sense, agreed. I will align this with Collection.set_offsets(offsets) and remove the split xs, ys signature.
| if axlim_clip is None: | ||
| axlim_clip = getattr(self, "_axlim_clip", False) | ||
| self._verts3d = np.asanyarray(verts) | ||
| self._segment3d = self._verts3d |
There was a problem hiding this comment.
Why do we have two private attrs pointing at the same data?
There was a problem hiding this comment.
Agreed, this is redundant state. I’ll remove the duplicate attr and keep one canonical source of truth.
| self.set_verts3d(verts, closed=False) | ||
| # And set our own codes instead. | ||
| self._codes3d = codes | ||
| self._codes3d_data = codes |
There was a problem hiding this comment.
Why have two attrs here pointing towards the same thing?
There was a problem hiding this comment.
Yes, agreed — this duplicates state unnecessarily. I'll keep a single canonical codes attribute and remove the second private attr.
| segments : sequence of (N, 3) array-like | ||
| The 3D line segments to draw. | ||
| """ | ||
| self._segments3d = segments |
There was a problem hiding this comment.
Should check the shape on all set_segments3d calls.
There was a problem hiding this comment.
Good point, agreed. I’ll enforce shape checks in set_segments3d and add tests for both valid and invalid inputs.
| If *None*, preserve the current setting. | ||
| """ | ||
| self.set_verts3d(verts, axlim_clip=axlim_clip) | ||
| self._code3d = codes |
There was a problem hiding this comment.
Should check that shape of verts and codes match (or codes can be broadcast to match).
There was a problem hiding this comment.
Good catch. I’ll add compatibility checks between verts and codes (including broadcast support) plus tests for valid/invalid cases.
| @check_figures_equal() | ||
| def test_scatter3d_offsets3d_modification(fig_ref, fig_test): |
There was a problem hiding this comment.
Could you please add similar check_figures_equal tests for all artists?
There was a problem hiding this comment.
Yes, agreed. I'll add corresponding check_figures_equal post-creation update tests across the relevant 3D artists touched by this API consistency pass.
| self._axlim_clip = axlim_clip | ||
| self.stale = True | ||
|
|
||
| def set_offsets3d(self, xs, ys, zs, *, zdir='z'): |
There was a problem hiding this comment.
I'm not sure that we should use this to re-set zdir?
There was a problem hiding this comment.
Makes sense — agreed. I will avoid using this setter to reconfigure zdir implicitly and keep axis-direction changes explicit in the appropriate 3D-property path. I’ll update the implementation/tests accordingly.
|
Hi @scottshambaugh, stepping back as suggested, I mapped the current positional API surface in This matrix is only about positional-data APIs for now; I am intentionally separating the Single-artist classes
Geometry-first collection classes
Offset-first collection classes
My current reading of the main unresolved design question is:
Cross-cutting follow-ups from inline review, to apply consistently across whichever artist set is in scope:
This is a preliminary matrix from the current |
|
Yeah, the existing API is all a bit of a mess. In your tables please have columns for the existing 3D API and the proposed new API, rather than a current snapshot of this PR draft. And also include the full details for 2D |
|
Hi @scottshambaugh, thanks for the clarification. I rebuilt this matrix from current 2D baseline:
|
| 2D type | Canonical positional data | Existing 2D API | Shape / semantic constraints |
|---|---|---|---|
Path |
vertices + optional codes are the canonical geometry representation |
Constructor: Path(vertices, codes=None, ...); mutable properties: path.vertices, path.codes |
vertices must be (N, 2); codes must be None or (N,); if codes is provided, length must match vertices and first code must be MOVETO |
PathPatch |
Geometry is defined by an embedded Path object |
get_path(), set_path(path) |
Positional geometry and path codes come from path.vertices / path.codes; geometry updates are path replacement/update operations |
3D artist matrix: existing vs proposed
| 3D artist | Existing 3D API (origin/main) |
Proposed new API |
|---|---|---|
Text3D |
get_position_3d, set_position_3d, set_z |
No change |
Line3D |
get_data_3d, set_data_3d |
No change |
Collection3D |
No explicit public 3D geometry getter/setter (internal _3dverts_codes is populated by conversion path) |
Add explicit get_verts_and_codes3d() / set_verts_and_codes3d(...) |
Line3DCollection |
set_segments(...) currently carries 3D semantics, but no explicit 3D-named API pair |
Add get_segments3d() / set_segments3d(...); keep set_segments(...) as compatibility alias |
Patch3D |
set_3d_properties(...) writes internal 3D segment state; no explicit public 3D verts getter/setter |
Add get_verts3d() / set_verts3d(...) |
PathPatch3D |
set_3d_properties(path, ...) + internal verts/codes state; no explicit public geometry pair |
Add get_verts_and_codes3d() / set_verts_and_codes3d(...) (plus get_codes3d()) |
Patch3DCollection |
set_3d_properties(zs, zdir, ...) writes internal 3D offsets; no explicit 3D offsets pair |
Add get_offsets3d() / set_offsets3d(offsets, *, zdir='z') |
Path3DCollection |
set_3d_properties(zs, zdir, ...) writes internal 3D offsets; no explicit 3D offsets pair |
Add get_offsets3d() / set_offsets3d(offsets, *, zdir='z') |
Poly3DCollection |
set_verts(...) and set_verts_and_codes(...) already take 3D geometry but through 2D-style names; no explicit 3D getters |
Add get_verts3d() / set_verts3d(...), get_verts_and_codes3d() / set_verts_and_codes3d(...), and get_codes3d(); keep existing names as compatibility aliases |
If this target surface matches your intent, I will implement this exact scope first, then apply the cross-cutting consistency items across the same approved artist set (shape checks, canonical internal state, and matching post-update figure-equality tests).
|
Sorry, could you please edit your last comment to re-add the base 2D api info for each artist in the table? Also, I thought you agreed that we should leave As a note - your comments seem heavily AI-written beyond direct translation. What was the extent of its usage here? |
|
Hi @scottshambaugh, thanks for the follow-up. You are right; I updated the matrix below to re-add the 2D base API per artist, and I removed 2D baseline details (
|
| 2D type | Canonical positional data | Existing 2D API | Shape / semantic constraints |
|---|---|---|---|
Path |
vertices + optional codes |
Path(vertices, codes=None, ...); mutable path.vertices, path.codes |
vertices: (N, 2); codes: None or (N,); if provided, length must match vertices, first code must be MOVETO |
PathPatch |
Geometry is an embedded Path |
get_path(), set_path(path) |
Geometry/codes come from path.vertices / path.codes; updates happen by replacing/updating the Path |
Artist matrix (origin/main): 2D base vs existing 3D vs proposed 3D
| 3D artist | 2D base positional API | Existing 3D API (origin/main) |
Proposed new API |
|---|---|---|---|
Text3D |
Text.get_position / set_position |
get_position_3d, set_position_3d, set_z |
No change |
Line3D |
Line2D.get_data / set_data |
get_data_3d, set_data_3d |
No change |
Collection3D |
Collection.get_paths / set_paths; Collection.get_offsets / set_offsets |
No explicit public 3D geometry pair (internal _3dverts_codes) |
Add get_verts_and_codes3d() / set_verts_and_codes3d(...) |
Line3DCollection |
LineCollection.get_segments / set_segments; inherited Collection.get_offsets / set_offsets |
set_segments(...) has 3D semantics; no explicit 3D-named pair |
Add get_segments3d() / set_segments3d(...); keep set_segments(...) alias |
Patch3D |
Patch.get_path (geometry via path/verts) |
set_3d_properties(...); no explicit public 3D verts pair |
Add get_verts3d() / set_verts3d(...) |
PathPatch3D |
PathPatch.get_path / set_path (path vertices/codes) |
set_3d_properties(path, ...); internal verts/codes state |
Add get_verts_and_codes3d() / set_verts_and_codes3d(...) and get_codes3d() |
Patch3DCollection |
PatchCollection + inherited Collection.get_offsets / set_offsets |
set_3d_properties(zs, zdir, ...); internal _offsets3d |
Add get_offsets3d() / set_offsets3d(offsets3d) |
Path3DCollection |
PathCollection + inherited Collection.get_offsets / set_offsets |
set_3d_properties(zs, zdir, ...); internal _offsets3d |
Add get_offsets3d() / set_offsets3d(offsets3d) |
Poly3DCollection |
PolyCollection.set_verts / set_verts_and_codes; inherited Collection.get_offsets / set_offsets |
set_verts(...) and set_verts_and_codes(...) already take 3D geometry, but no explicit 3D getters |
Add get_verts3d() / set_verts3d(...), get_verts_and_codes3d() / set_verts_and_codes3d(...), get_codes3d(); keep existing names as aliases |
On AI usage: I used AI for language polishing and table formatting, but I manually reviewed origin/main APIs and made the technical mapping/proposal myself. I can explain each row and rationale directly.
|
Hi @scottshambaugh, quick FUP on the matrix update I posted on March 25. I’m ready to implement that exact scope next. Before I start coding, could you confirm whether that table is aligned with what you want, or point out which rows you want changed first? |
|
Sorry for the delay. I'm a little hesitant to add such a large amount of additional public API that is all mismatched among the 3D artists. Better this than no ability to change things, but I do wonder if there's a more consistent way to structure it. We try to be a pretty careful with the public API since we'd much rather not have to do deprecations to change things after the fact. I'll ping in the discourse to see if other devs have opinions on this. |
|
Thanks for the update, and no worries on timing. Your concern about public API stability makes sense, especially to avoid deprecations later. I also agree that I’ll pause further code changes until there is feedback on the Discourse thread so we can align on a consistent structure first. After that, I can update the matrix to reflect the agreed direction and implement only that scoped API. One candidate direction from my side is to mirror each artist’s 2D primary positional API, and only add |
|
Hi, quick FUP on this PR. I’m still holding off on code changes until there is alignment on the public API direction. Is there a discourse thread or other discussion I should follow, or should I keep this PR paused for now @scottshambaugh? |
|
I haven't seen any discussion on discourse. On the 3D API: This is unfortunately quite a mess historically. Deriving 3D from 2D is a questionable API choice as a lot is inherited and it is not clear which functionality still works and which not. In general, I would be hesitant to add a lot of new |
|
Here's the discourse thread, I'll bring it up during the Thursday meeting to get some discussion going: https://discourse.matplotlib.org/chat/c/-/2/t/129/654 |
|
Hi @xndvaz, we talked about this during the july 2 meeting for a bit and think it's best to defer this work. We are planning to move to a new data containerization approach, which will commonize a lot of the underlying data structures across artists, and should make it possible to have a more unified API for these getters and setters. You can follow along with that effort here: #30865 Thank you for your work on this! I am going to close for now, with an eye on revisiting when that other work is done. |
github-project-automation
Bot
moved this from Needs decision
to Done
in First Time Contributors
5 participants
PR summary
This PR improves
mplot3doffset updates for 3D collections.Path3DCollectionandPatch3DCollectionproject 3D offsets to 2D during draw, so updating throughset_offsets()(2D data) is lossy and does not update the 3D source offsets. This PR adds an explicitset_offsets3d(xs, ys, zs, *, zdir='z')method to both classes and routesset_3d_properties()through that path.Tests added:
set_offsets3d()updates onPath3DCollectionset_offsets3d()updates onPatch3DCollectionset_array()updates onPath3DCollectionset_array()updates onPatch3DCollectionLocal test command run:
python -m pytest -q lib/mpl_toolkits/mplot3d/tests/test_axes3d.pyPartially addresses #784.
AI Disclosure
I used AI tooling to help draft and iterate the code and tests. I manually reviewed the implementation, validated behavior locally, and ran the affected test suite before opening this PR.
PR checklist