Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update x,y.z values for an existing Line3D object #1629

Closed
wants to merge 5 commits into from

Conversation

tacaswell
Copy link
Member

There does not seem to be a clean way to update the x,y,z data for a Line3D object.

Added set_3d_data(x,y,z,zdir) which will update the data for an existing line.

added support for 'zdir` to init

Thomas A Caswell added 3 commits December 30, 2012 23:15
spirit as set_xdata and set_ydata for Line2D, but requires _all_
of the data to be set in one go (to not have to write the caching
layer of Line2D again yet).
@@ -112,6 +117,21 @@ def set_3d_properties(self, zs=0, zdir='z'):
pass
self._verts3d = juggle_axes(xs, ys, zs, zdir)

def set_3d_data(self, xs=None, ys=None, zs=None, zdir='z'):
x_old,y_old,z_old = self._verts3d
Copy link
Member

Choose a reason for hiding this comment

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

For PEP8 compliance, could you put spaces after the commas in this tuple?

@dmcdougall
Copy link
Member

Those Travis failures are false negatives.

@WeatherGod
Copy link
Member

I raised an issue related to this in #1483. This is certainly a step in the right direction.

@tacaswell
Copy link
Member Author

Sorry about my poor style, pep8 is not ingrained in my brain yet.

@dmcdougall
Copy link
Member

Sorry about my poor style, pep8 is not ingrained in my brain yet.

No worries. That's what we're here for.

@mdboom
Copy link
Member

mdboom commented Jan 16, 2013

Question for the masses: This is debatable, but do we think this is enough like a bugfix to backport to the v1.2.x branch?

@WeatherGod
Copy link
Member

Actually, I think there are enough problems with this code that I want to hold off on this. Plus, I have uncovered some other mplot3d bugs recently which solving may conflict with this one (it has to do with juggling axes). I would like to close this PR for now, but flag it for re-examination later. If others disagree, we can always re-open.

@WeatherGod WeatherGod closed this Jan 16, 2013
@tacaswell
Copy link
Member Author

@WeatherGod by this code do you mean mplot3d or this particular patch? If it's the patch, what should I have done better?

@WeatherGod
Copy link
Member

The patch. You should never do bare exceptions (they hide RuntimeExceptions, among others). Also, the list comprehension to build the zs could be done better (maybe [z] * len(xs)), and some other smaller issues.

But, really, the thing that I see as blocking this is another bug that I am currently investigating on my own. My current thoughts on how to deal with it may conflict with your patch. So, if we can hold off for a week while I figure this other thing out, we can re-examine this one then.

@tacaswell
Copy link
Member Author

ok.

I copied both of those things (exception, list comprehension) from the existing set_3d_properties function.

[I am trying really hard to to sound defensive and spent far too long writing this note]

@WeatherGod
Copy link
Member

Yes, the current mplot3d code is not PEP8-compliant at all, and really shouldn't be considered an example for others to follow. That is another upcoming project. We are currently trying to make sure that all new code going in is PEP8 compliant.

Also, a slight issue I see. If one were to directly create the Line3D object, wouldn't this break because the self._verts3D wouldn't be created yet?

@tacaswell
Copy link
Member Author

You are correct about breaking __init__, although I wouldn't have called that a slight issue.

@tacaswell tacaswell deleted the set_3d_data branch August 14, 2014 00:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants