MAINT: Use vectorization in plot_trisurf, simplifying greatly by eric-wieser · Pull Request #9991 · matplotlib/matplotlib

eric-wieser · 2017-12-13T06:59:35Z

Cleanup only, no behavior changes.

I'd expect considerably better performance on large arrays, since there are fewer copies being made, and the operations can be vectorized

eric-wieser

codecov/patch is picking up the fact that shade=True was not tested before this patch, and that all the lines that were tested have been made a minority due to being combined into one.

eric-wieser · 2017-12-13T08:56:18Z

lib/mpl_toolkits/mplot3d/axes3d.py

            if shade:
+                v1 = verts[:,0,:] - verts[:,1,:]
+                v2 = verts[:,1,:] - verts[:,2,:]
+                normals = np.cross(v1, v2)


If I combined these into one less-readable line, then your code coverage tests would pass. Aren't metrics great?

The other way of looking at that is the coverage tests would pass if this bit of code was tested!

It's not as if we really cared about what codecov says...

@dstansby: I suppose the way of looking at it is not "here's a dice that's rolled which will randomly reject your PR", but "here's a dice that when rolled might let you off a test".

I've added the test now :)

QuLogic

There are a couple PEP8 issues, though because it's disabled for this (extremely inconsistent) file, there are no complaints from Travis, but that doesn't mean we can't fix them when modifying the file.

Otherwise, I think this PR is good.

QuLogic · 2017-12-14T08:08:39Z

lib/mpl_toolkits/mplot3d/axes3d.py

-            colset = np.array(colset)
-            polyc.set_array(colset)
+            # average over the three points of each triangle
+            avg_z = verts[:,:,2].mean(axis=1)


Space after comma.

I always seem to go with omitting them in indexing expressions. But I guess PEP8 doesn't call out indexing with a tuple specifically, and if that's the style matplotlib is (trying to be) using, then I'll go with it.

QuLogic · 2017-12-14T08:08:49Z

lib/mpl_toolkits/mplot3d/axes3d.py

        else:
            if shade:
+                v1 = verts[:,0,:] - verts[:,1,:]
+                v2 = verts[:,1,:] - verts[:,2,:]


Space after comma.

eric-wieser · 2017-12-14T08:19:58Z

@QuLogic: Updated

QuLogic · 2017-12-14T08:26:03Z

This seems a bit bigger than when I first reviewed it?

QuLogic · 2017-12-14T08:30:03Z

lib/mpl_toolkits/mplot3d/axes3d.py

-            normals = np.cross(v1, v2)
-        else:
-            normals = []
+        # verts = np.stack((xt, yt, zt), axis=-1)


Are they 1D or 2D? np.vstack or np.dstack could also be used in those cases.

Yep, this was a new change since you last reviewed. Both vstack and dstack have undesirable semantics of guessing the array .ndim, so using concatenate is more precise.

This isn't the only place that np.stack is mentioned in a comment above a np.concatenate((a[..., None], ...)), so might be worth backporting to ~~the very simple~~

~~def stack(arrays, axis): return np.concatenate([arr[..., np.newaxis] for arr in arrays], axis)~~

- (oops, not correct)

Not something I want to involve in this PR though

You can just stick the definition in https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/cbook/_backports.py if you change your mind.

Oh, and to answer your question, 2D - (N, edges_in_triangle)

We do have a cbook._backports; not sure how strong the need is for it without having done any looking.

I reckon there're a substantial number of cases - I can put together a PR once this is merged

Looking again, a stack_last function is all that ever seems to be needed, implemented as the above - so that wouldn't belong in _backports. Suggestions of where to put such a helper?

cbook would be the place.
But given that we'd probably just be using stack(..., -1) "if it was available", I'd just backport stack.
Let us know if you want to do this in this PR or a separate one.

Definitely a separate one

Use it to compute surface normals and mean z coord, rather than manual looping

anntzer

quick skim looks fine, second pair of eyes (@QuLogic? :-)) would be good.
edit: Ah, you already approved it.

eric-wieser force-pushed the normals-cleanup branch 2 times, most recently from 5d63231 to 5e1b08b Compare December 13, 2017 07:41

eric-wieser mentioned this pull request Dec 13, 2017

Fix and document lightsource argument in mplot3d #9990

Merged

6 tasks

eric-wieser changed the title ~~MAINT: Use broadcasting to compute surface normals, rather than manual looping~~ MAINT: Use vectorization in plot_trisurf, simplifying greatly Dec 13, 2017

eric-wieser commented Dec 13, 2017

View reviewed changes

tacaswell added this to the v2.2 milestone Dec 13, 2017

QuLogic reviewed Dec 14, 2017

View reviewed changes

eric-wieser force-pushed the normals-cleanup branch from 5e1b08b to f0077d2 Compare December 14, 2017 08:19

eric-wieser force-pushed the normals-cleanup branch from f0077d2 to cb05203 Compare December 14, 2017 08:21

QuLogic reviewed Dec 14, 2017

View reviewed changes

QuLogic approved these changes Dec 14, 2017

View reviewed changes

eric-wieser force-pushed the normals-cleanup branch from cb05203 to 100f167 Compare December 15, 2017 05:15

MAINT: Use vectorization in plot_trisurf

e36bb0b

Use it to compute surface normals and mean z coord, rather than manual looping

eric-wieser force-pushed the normals-cleanup branch from 100f167 to e36bb0b Compare December 15, 2017 08:18

QuLogic requested a review from anntzer February 3, 2018 05:15

anntzer approved these changes Feb 3, 2018

View reviewed changes

anntzer merged commit 2ed292d into matplotlib:master Feb 3, 2018

QuLogic modified the milestones: needs sorting, v2.2.0 Feb 12, 2018

eric-wieser deleted the normals-cleanup branch March 23, 2019 18:07

Uh oh!

Conversation

eric-wieser commented Dec 13, 2017

Uh oh!

eric-wieser left a comment

Choose a reason for hiding this comment

Uh oh!

eric-wieser Dec 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QuLogic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-wieser commented Dec 14, 2017

Uh oh!

QuLogic commented Dec 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-wieser Dec 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anntzer left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

eric-wieser Dec 13, 2017 •

edited

Loading

eric-wieser Dec 14, 2017 •

edited

Loading

anntzer left a comment •

edited

Loading