Skip to content

ENH: add pivot kwarg to 3d quiver plot#3735

Merged
tacaswell merged 6 commits into
matplotlib:masterfrom
perimosocordiae:patch-1
Nov 5, 2014
Merged

ENH: add pivot kwarg to 3d quiver plot#3735
tacaswell merged 6 commits into
matplotlib:masterfrom
perimosocordiae:patch-1

Conversation

@perimosocordiae

Copy link
Copy Markdown
Contributor

Also removing an unused inner function and cleaning up some math.

The default value for pivot is "tip", which preserves the behavior with the existing 3d quiver plot, but differs from the regular 2d version which defaults to "tail".

I suspect this PR will need tests added, but I'll need help setting those up.

Also removing an unused inner function and cleaning up some math.
Comment thread lib/mpl_toolkits/mplot3d/axes3d.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Might as well turn this into for xyz, uvw in zip(zip(xs, ys, zs), zip(us, vs, ws)): Which I could have sworn got done when this came in...

@tacaswell tacaswell added this to the v1.5.x milestone Oct 28, 2014
@tacaswell

Copy link
Copy Markdown
Member

tagged as revision as it needs tests. Probably just png for the image tests.

@perimosocordiae

Copy link
Copy Markdown
Contributor Author

I vectorized and simplified quite a bit, possibly at the expense of readability. I tried to comment the tricky bits, though.

Tests will come soon.

@perimosocordiae

Copy link
Copy Markdown
Contributor Author

Tests are in and Travis is happy. Anything else need to be done for this PR?

@tacaswell

Copy link
Copy Markdown
Member

👍 from me. @efiring Should probably take a look at this.

Is there any noticeable speed up?

Comment thread lib/mpl_toolkits/mplot3d/axes3d.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

spaces after the commas, please

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Curious travis didn't catch that...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That is because mplot3d is not pep8 checked. The toolkits generally are not
checked.

On Wed, Oct 29, 2014 at 1:42 PM, Thomas A Caswell [email protected]
wrote:

In lib/mpl_toolkits/mplot3d/axes3d.py:

     linec = art3d.Line3DCollection(lines, *args[argi:], **kwargs)
     self.add_collection(linec)
  •    self.auto_scale_xyz(xs, ys, zs, had_data)
    
  •    self.auto_scale_xyz(XYZ[:,0], XYZ[:,1], XYZ[:,2], had_data)
    

Curious travis didn't catch that...


Reply to this email directly or view it on GitHub
https://github.com/matplotlib/matplotlib/pull/3735/files#r19556818.

@WeatherGod

Copy link
Copy Markdown
Member

Thank you very much for your feature, and for vectorizing much of the code. In the initial PR, we discussed vectorizing, but the students who developed the feature could only get so far, so thank you for getting it all the way. It is much cleaner to read now.

@perimosocordiae

Copy link
Copy Markdown
Contributor Author

Ok, linting done. I also did a quick benchmark using this setup code:

ax = plt.subplot(projection='3d')
x, y, z = np.meshgrid(np.arange(-0.8, 1, 0.2),
                      np.arange(-0.8, 1, 0.2),
                      np.arange(-0.8, 1, 0.8))
u = np.sin(np.pi * x) * np.cos(np.pi * y) * np.cos(np.pi * z)
v = -np.cos(np.pi * x) * np.sin(np.pi * y) * np.cos(np.pi * z)
w = (np.sqrt(2.0 / 3.0) * np.cos(np.pi * x) * np.cos(np.pi * y) *
     np.sin(np.pi * z))

Before this PR:

In [5]: %timeit ax.quiver(x, y, z, u, v, w, length=0.1)
10 loops, best of 3: 45.7 ms per loop

After this PR:

In [5]: %timeit ax.quiver(x, y, z, u, v, w, length=0.1)
100 loops, best of 3: 8.75 ms per loop

Looks like a 5x speedup! Not bad.

@tacaswell

Copy link
Copy Markdown
Member

@perimosocordiae One last thing, can you add an entry to whats_new to advertise the new pivot feature?

@perimosocordiae

Copy link
Copy Markdown
Contributor Author

Ok, added.

tacaswell added a commit that referenced this pull request Nov 5, 2014
ENH: add pivot kwarg to 3d quiver plot
@tacaswell tacaswell merged commit 6e36226 into matplotlib:master Nov 5, 2014
@tacaswell

Copy link
Copy Markdown
Member

Thank you.

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.

4 participants