add docstring for set_3d_properties for PatchCollection3d #19572
Conversation
There was a problem hiding this comment.
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 plt
import numpy as np
fig, (ax1, ax2) = plt.subplots(1, 2, subplot_kw={'projection': '3d'}, figsize=(12, 6))
x = np.linspace(-1, 1, 51)
y = 1 - x**2
z = 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 data
ax2.plot(x, y, z, zdir='x')
l, = ax2.plot(x, y, z, '--', zdir='x')
l.set_3d_properties(0.5*z)
plt.show()
More generally, this whole API is awkward and inconsistent between different Artists.
I'd say that set_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 has
set_position_3d() - It internally stores
self._dir_vecso thatzdiris 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 store self._dir_vec in set_3d_projection and make zdir there default to None.
| 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. |
There was a problem hiding this comment.
| 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. | |
| Update the offset values for the 3rd dimension. If there are more | |
| XY points in the offsets than given here, these additional points | |
| are not drawn. |
Hope this makes it a little more clear. Z is a little ambiguous.
There was a problem hiding this comment.
👍 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 used will not be but are switches to the present tense which I found confusing. Not a big deal either way.
|
Removed documentation tag, because as of now this also changes the function signature. |
I don't think this is exactly what you mean but see also: #19573
That's a compelling example. Note that many of the other artists |
Why almost? What more would you want there? Privatizing |
|
For now I removed the addition of default argument for |
|
One other thing that this PR should definitely do make sure all of the But for that I think there should be a consistent phrase used to describe |
IIRC that and not having a default |
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.
We promote artists from 2D to 3D by adding a dimension. Special case Special case |
|
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. |
|
The docstring here was added in #23536, so closing this PR. Thank you for the initial work on this! |
PR Summary
zdira default to be consistent with the rest of the file.PR Checklist
flake8on changed files to check).flake8-docstringsand runflake8 --docstring-convention=all).