Add set_data_3d and get_data_3d to Line3d #10111
Conversation
lib/mpl_toolkits/mplot3d/art3d.py
Outdated
| """ | ||
| Return the xdata, ydata, zdata. | ||
| """ | ||
| return self._verts3d |
There was a problem hiding this comment.
Does this need to un-juggle the order of the data?
There was a problem hiding this comment.
It probably should. We're calling set_3d_properties with no zdir so we shouldn't do any juggling but the user could change the zdir, I'll probably also allow an optional kwarg to allow the user to also set zdir right from set_data_3d.
There was a problem hiding this comment.
Both should probably get the zdir kwarg?
There was a problem hiding this comment.
The juggle axis feature is for converting a 2D object into 3D. It essentially states which dimension is going to be a constant. It is not applicable here.
There was a problem hiding this comment.
👍 Looks like a reasonable feature and something we should support.
This needs a test (a round-trip test showing you can set (x, y, z) and then get the values back out is enough, I do not think this needs an image test) and a whats_new entry (so people know this exists).
Could you also change the docstrings to numpydoc format (but not going to block PR on that because the rest of this module is not converted yet)?
lib/mpl_toolkits/mplot3d/art3d.py
Outdated
| x, y, z = args | ||
|
|
||
| self.set_xdata(x) | ||
| self.set_ydata(y) |
There was a problem hiding this comment.
This is incorrect. set_xdata() and set_ydata() are for the 2D projected values. self._verts3d is just a tuple of the 3 1D arrays.
There was a problem hiding this comment.
Ah, fair enough. Using this method is very suggestive due to
matplotlib/lib/mpl_toolkits/mplot3d/art3d.py
Lines 117 to 129 in 37978a8
but on a bit closer inspection it seems that these methods are used in the 2D class -> 3D class upcasting, not as a standard part of the setup.
lib/mpl_toolkits/mplot3d/art3d.py
Outdated
| """ | ||
| Return the xdata, ydata, zdata. | ||
| """ | ||
| return self._verts3d |
There was a problem hiding this comment.
The juggle axis feature is for converting a 2D object into 3D. It essentially states which dimension is going to be a constant. It is not applicable here.
|
I think I've addressed all issues here but if anyone wants to let me know why Travis is complaining I'd love to hear. From the log it looks like it failed in |
|
Try rebasing on master and force-pushing. There have been some fixes to
Travis testing the past few weeks.
…On Sun, Feb 11, 2018 at 11:57 PM, Rob Harrigan ***@***.***> wrote:
I think I've addressed all issues here but if anyone wants to let me know
why Travis is complaining I'd love to hear. From the log it looks like it
failed in lib/matplotlib/tests/test_compare_images.py:222 which is a file
not changed in this PR.
—
You are receiving this because your review was requested.
Reply to this email directly, vie
8000
w it on GitHub
<#10111 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AARy-GVB65SiP8Fdsxh3xzQu-5sepSU1ks5tT8S_gaJpZM4RNLt3>
.
|
a6c0e3f to
43d7b6f
Compare
|
Thanks for the tip @WeatherGod, that seems to have fixed Travis but broken circleci... Again, definitely not something changed in this PR. |
|
Ping @tacaswell and @WeatherGod, rebasing on master seems to have fixed it this time. |
|
Can this be merged? |
| if len(args) == 1: | ||
| self._verts3d = args[0] | ||
| else: | ||
| self._verts3d = args |
There was a problem hiding this comment.
I think you need to add self.stale = True here.
|
ping @tacaswell , the tests were added |
PR Summary
Fixes #8914 by adding set_data_3d and get_data_3d methods to Line3D. This allows for setting and getting of 3d data without overriding the widely used set_data and get_data methods.
I looked for testing and documentation and did not see any testing or documentation related to Line3D would require updating. Please let me know if this is incorrect.
PR Checklist