Skip to content

mplot3d: add set_offsets3d for 3D patch/path collections#31279

Open
xndvaz wants to merge 2 commits intomatplotlib:mainfrom
xndvaz:fix-784-offsets3d-clean
Open

mplot3d: add set_offsets3d for 3D patch/path collections#31279
xndvaz wants to merge 2 commits intomatplotlib:mainfrom
xndvaz:fix-784-offsets3d-clean

Conversation

@xndvaz
Copy link

@xndvaz xndvaz commented Mar 11, 2026

PR summary

This PR improves mplot3d offset updates for 3D collections.

Path3DCollection and Patch3DCollection project 3D offsets to 2D during draw, so updating through set_offsets() (2D data) is lossy and does not update the 3D source offsets. This PR adds an explicit set_offsets3d(xs, ys, zs, *, zdir='z') method to both classes and routes set_3d_properties() through that path.

Tests added:

  • visual equality test for post-creation set_offsets3d() updates on Path3DCollection
  • visual equality test for post-creation set_offsets3d() updates on Patch3DCollection
  • visual equality test for post-creation set_array() updates on Path3DCollection
  • visual equality test for post-creation set_array() updates on Patch3DCollection

Local test command run:

  • python -m pytest -q lib/mpl_toolkits/mplot3d/tests/test_axes3d.py

Partially 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

@scottshambaugh
Copy link
Contributor

This does not address all the comments I left on the prior PR.

@xndvaz
Copy link
Author

xndvaz commented Mar 11, 2026

Thanks for the quick feedback.

I tried to address the points from the prior PR as follows:

  • used the PR template (including AI disclosure/checklist),
  • created a clean branch from upstream/main with only related commits,
  • introduced set_offsets3d(...) for Path3DCollection and Patch3DCollection (no 2D->3D round-trip through set_offsets()),
  • added check_figures_equal tests for post-creation set_offsets3d() and set_array() updates on both collection types.

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.

@scottshambaugh
Copy link
Contributor

@xndvaz
Copy link
Author

xndvaz commented Mar 11, 2026

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.

@scottshambaugh
Copy link
Contributor

Could you first please clarify if you are using AI assistance to write your comments here?

@xndvaz
Copy link
Author

xndvaz commented Mar 11, 2026

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. :)

@scottshambaugh
Copy link
Contributor

scottshambaugh commented Mar 11, 2026

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 get_offsets3d functions or similar to mirror the setters.

I also do not understand the purpose of the array modification tests, could you please explain why those are needed?

@xndvaz
Copy link
Author

xndvaz commented Mar 11, 2026

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 (get_offsets3d or equivalent) for each setter.

About the set_array tests: I added them because issue #784 explicitly mentions post-creation set_array() behavior (including color-update problems), so I wanted a regression check while touching the same classes. The intent was to guard existing behavior, not to broaden scope.

If you prefer this PR to stay strictly on offsets APIs, I can remove the set_array tests and keep them for a separate follow-up PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants