Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

add docstring for set_3d_properties for PatchCollection3d#19572

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

Closed
ianhi wants to merge1 commit intomatplotlib:mainfromianhi:3d-scatter

Conversation

ianhi
Copy link
Contributor

PR Summary

  1. Added a docstring
  2. Gavezdir a default to be consistent with the rest of the file.

PR Checklist

  • IsFlake 8 compliant (runflake8 on changed files to check).
  • Documentation is sphinx and numpydoc compliant (the docs shouldbuild without error).
  • Conforms to Matplotlib style conventions (installflake8-docstrings and runflake8 --docstring-convention=all).

Copy link
Member

@timhoffmtimhoffm left a comment
edited
Loading

Choose a reason for hiding this comment

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

I'm sceptical on the default zdir.

It works when using the default zdir='z' at artist creation time, but can be surprising otherwise. Note that not specifying zdir in set_3d_properties in the second plot rotates the data.

import matplotlib.pyplot as pltimport numpy as npfig, (ax1, ax2) = plt.subplots(1, 2, subplot_kw={'projection': '3d'}, figsize=(12, 6))x = np.linspace(-1, 1, 51)y = 1 - x**2z = np.exp(-np.abs(2*x))# plot a reference line and a second one with half scaling# in the 3rd dim. using set_3d_properties()ax1.plot(x, y, z)l, = ax1.plot(x, y, z, '--')l.set_3d_properties(0.5*z)# same but using zdir='x' for the original dataax2.plot(x, y, z, zdir='x')l, = ax2.plot(x, y, z, '--', zdir='x')l.set_3d_properties(0.5*z)plt.show()

grafik

More generally, this whole API is awkward and inconsistent between different Artists.

I'd say thatset_3d_properties() should be a private helper. Unfortunately, for most Artists it's currently the only way to update 3d data. But splitting the update in two functions is insane for a user-facing API.Text3D does it almost right:

  • It hasset_position_3d()
  • It internally storesself._dir_vec so thatzdir is optional inset_position_3d()

IMHO the correct way forward would be to create similar data setters for all Artists.
The minimal step would be to storeself._dir_vec inset_3d_projection and makezdir there default to None.

Comment on lines +468 to +470
Update the z values of the offsets. If there are more XY points in
the offsets than there are Z points then points without a Z offset
will not be drawn.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Updatethezvaluesoftheoffsets.IftherearemoreXYpointsin
theoffsetsthanthereareZpointsthenpointswithoutaZoffset
willnotbedrawn.
Updatetheoffsetvaluesforthe3rddimension.Iftherearemore
XYpointsintheoffsetsthangivenhere,theseadditionalpoints
arenotdrawn.

Hope this makes it a little more clear. Z is a little ambiguous.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

👍 that's definitely an improvement. My only question is what tense is the docstring speaking in? Since the draw will happen after this function is called I usedwill not be butare switches to the present tense which I found confusing. Not a big deal either way.

@timhoffm
Copy link
Member

Removed documentation tag, because as of now this also changes the function signature.

ianhi reacted with thumbs up emoji

@ianhi
Copy link
ContributorAuthor

IMHO the correct way forward would be to create similar data setters for all Artists.

I don't think this is exactly what you mean but see also:#19573

I'm sceptical on the default zdir.

It works when using the default zdir='z' at artist creation time, but can be surprising otherwise. Note that not specifying zdir in set_3d_properties in the second plot rotates the data.

That's a compelling example. Note that many of the other artistsset_3d_properties do default tozdir='z' so perhaps it is those that should be changed?

@ianhi
Copy link
ContributorAuthor

Text3D does it almost right:

Why almost? What more would you want there? Privatizingset_3d_properties?

@ianhi
Copy link
ContributorAuthor

For now I removed the addition of default argument forzdir. But something probably should be done about the inconsistency of the variousset_3d_properties function signatures. Though what is not clear to me.

@ianhi
Copy link
ContributorAuthor

ianhi commentedFeb 28, 2021
edited
Loading

One other thing that this PR should definitely do make sure all of theset_3d_properties have docstrings (ideally similar ones) so that I don't become culpable for the aforementioned inconsistency.

But for that I think there should be a consistent phrase used to describezdir. I didn't add one because I'm confused by what it should be, and as I noted in#19573 (comment) there are several different descriptions of it already floating around.

@timhoffm
Copy link
Member

Text3D does it almost right:

Why almost? What more would you want there? Privatizingset_3d_properties?

IIRC that and not having a defaultzdir='z' onset_3d_properties().

@timhoffm
Copy link
Member

But something probably should be done about the inconsistency of the various set_3d_properties function signatures. Though what is not clear to me.

Let's make them private first. Otherwise, this will become quite a song and dance with deprecations.

But that'S is for a separate PR.

But for that I think there should be a consistent phrase used to describezdir. I didn't add one because I'm confused by what it should be, and as I noted in#19573 (comment) there are several different descriptions of it already floating around.

We promote artists from 2D to 3D by adding a dimension.zdir defines how that is done. The effect is most obvious for artists that do not have a real third dimension, e.g. the bars inhttps://matplotlib.org/devdocs/gallery/mplot3d/bars3d.html. Thezdir is the normal to the plane in which this 2D-representation is drawn. Forbar(x, y, z) the first two values are interpreted in this 2D subspace, the third value defines the position of that 2D-image alongzdir. That means that forzdir='x', the valuesx, y are on the axis-plane yz.

Special caseLine3D: As this is not pseudo-3D, there should usually not be a reason to use a differentzdir. But it might be helpful if you plot it along with other data, e.g.bar(x, y, z, zdir='x'); plot(x, y, z, zdir='x').

Special caseText3D: The text direction is in the plane normal tozdir. The characters are rendered along the text direction, but always facing the camera; i.e. you cannot look at the text "from the side/back".

@github-actionsGitHub Actions
Copy link

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.

@github-actionsgithub-actionsbot added the status: inactiveMarked by the “Stale” Github Action labelNov 17, 2023
@scottshambaugh
Copy link
Contributor

The docstring here was added in#23536, so closing this PR. Thank you for the initial work on this!

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

@timhoffmtimhoffmtimhoffm left review comments

Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The various set_3d_properties() methods are undocumented.
4 participants
@ianhi@timhoffm@scottshambaugh@jklymak

[8]ページ先頭

©2009-2025 Movatter.jp