Addset andget_offsets3d to the PatchCollection3d object. The casting back to numpy array and transpose in theget method is for consistency with what the 2d version that returns a numpy array with shape (N, 2).

Does this need an example or can that be left to#19520?

PR Checklist

Has pytest style unit tests (andpytest passes).
IsFlake 8 compliant (runflake8 on changed files to check).
New features are documented, with examples if plot related.
Documentation is sphinx and numpydoc compliant (the docs shouldbuild without error).
Conforms to Matplotlib style conventions (installflake8-docstrings and runflake8 --docstring-convention=all).
New features have an entry indoc/users/next_whats_new/ (follow instructions in README.rst there).
API changes documented indoc/api/next_api_changes/ (follow instructions in README.rst there).

create set_offsets3d for PathCollection3d

7f70c15

ianhi force-pushed theoffsets3d branch from3a59107 to7f70c15Compare

February 24, 2021 21:20

ianhi added the topic: mplot3d label

Feb 24, 2021

ianhi mentioned this pull request

Feb 27, 2021

add docstring for set_3d_properties for PatchCollection3d#19572

Closed

3 tasks

timhoffm requested changes

Feb 27, 2021

View reviewed changes

Copy link

Member

timhoffm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Sorry to be so critical, but 3D is a mess and without a clear picture where we want to go we risk making it only worse.

We need a bit more thinking to get a consistent 3D API. We currently have,

Text3D.set_positions_3d, having optionalzdir
Lines3D.set_data_3d , not havingzdir`

Do we want azdir parameter for data setters?

Also the 3D upcasting is solved differently. Text3D takeszdir but defers the data adaption todraw. In contrastLine3D doesn't even acceptzdir in__init__ instead,Axes3D.plot callsline2d_to_3d() which passeszdir toset_3d_properties() and immediately modifies the data. ThusText3D.get_positions_3d andLine3D.get_data_3d have different interpretations of the returned data, wrt. ifzdir is applied.

When and how do we want to applyzdir? What interpretation should the getter return?

Copy link

ContributorAuthor

ianhi commentedFeb 28, 2021

hmm yeah. Maybe it's best to close this PR and have that worked out on an issue?

One other thing to note is that looking at thetutorial the descriptions ofzdir in the docstrings vary wildly:

When plotting 2D data, the direction to use as z ('x', 'y' or 'z').

The direction to use.

The axis direction for the zs. This is useful when plotting 2D data on a 3D Axes. The data must be passed as xs, ys. Setting zdir to 'y' then plots the data to the x-z-plane.

So until now I didn't even really understand what it was for - which I think is evidence for it's currently confused state of API. It's also weird to me that changing thezdir will affect what happens to plots where I fully specify xyz, it seems like it only ought to affect 2D data (or at least only calls where at least of xyz is a scalar).

Sorry to be so critical,

I think we have different definitions of "so critical" :)

and without a clear picture where we want to go we risk making it only worse.

Agreed, I was being irresponsible by jumping the gun to scratch the single itch I had rather than the larger picture.

Copy link

Member

QuLogic commentedMar 2, 2021

Text3D takeszdir but defers the data adaption todraw.

This is something I implemented in#18525 when adding the getters/setters on purpose, and I would do the same for the other 3D artists (#18932) except that it requires some work to fix legends.

I can't say exactly what the API should be, but we should try to defer as much as possible the conversion to 2D todraw. Eventually, if we have a better 3D renderer, then it will be able to ingest the 3D values directly there instead.

timhoffm added the status: needs comment/discussionneeds consensus on next step label

Mar 6, 2021

jklymak marked this pull request as draft

May 15, 2021 15:26

ianhi mentioned this pull request

Jul 19, 2021

3D plotsmpl-extensions/mpl-interactions#89

Open

github-actionsbot added the status: needs rebase label

Nov 12, 2022

Copy link

github-actionsbot commentedNov 20, 2023

Since this Pull Request has not been updated in 60 days, it has been marked "inactive." This does not mean that it will be closed, though it may be moved to a "Draft" state. This helps maintainers prioritize their reviewing efforts. You can pick the PR back up anytime - please ping us if you need a review or guidance to move the PR forward! If you do not plan on continuing the work, please let us know so that we can either find someone to take the PR over, or close it.