MAINT/BUG: Don't use 5-sided quadrilaterals in Axes3D.plot_surface by eric-wieser · Pull Request #10001 · matplotlib/matplotlib

eric-wieser · 2017-12-14T07:40:27Z

Previously:

"cell" perimeters were clumsily calculated with duplicates, which were then removed at runtime
code to calculate normals was spread into multiple places
average z was calculated even if not used
repeated conversion between stride and count was done

Should have no visible behavior changes

This should be up to 25% faster, since the quadrilateral polygons now have 4 points, rather than 5 (!)

eric-wieser · 2017-12-14T07:41:00Z

lib/mpl_toolkits/mplot3d/axes3d.py

+                    ztop   = a[rs,             cs:cs_next   ]
+                    zleft  = a[rs:rs_next,     cs_next      ]
+                    zbase  = a[rs_next,        cs_next:cs:-1]
+                    zright = a[rs_next:rs:-1,  cs           ]


This is the first time I've ever found myself using a:b:-1 and it doing exactly what I wanted!

eric-wieser · 2017-12-14T07:46:17Z

lib/mpl_toolkits/mplot3d/axes3d.py

+            for poly_i, ps in enumerate(polys):
+                # pick three points around the polygon to find the normal at
+                # hard to vectorize, since len(ps) is different at the edges
+                i1, i2, i3 = 0, int(len(ps)/3), int(2*len(ps)/3)


I feel like there's a much better approach here, but this is at least cheap, I suppose. Taking the middle point or even mean of each edge, and taking the cross product of the vectors joining opposite edges would be be a marginally better heuristic.

Either way, this is just cleanup, so I'm leaving it as is.

Note that if I want to make the image output unchanged, then this needs to become (len(ps) + 1). Is per-pixel compatibility something that is cared about?

QuLogic · 2017-12-14T08:15:29Z

lib/mpl_toolkits/mplot3d/axes3d.py

+        row_inds = list(xrange(0, rows-1, rstride)) + [rows-1]
+        col_inds = list(xrange(0, cols-1, cstride)) + [cols-1]




Remove extra blank line.

QuLogic · 2017-12-14T08:15:38Z

lib/mpl_toolkits/mplot3d/axes3d.py



-        #colset contains the data for coloring: either average z or the facecolor
+        #colset contains the sampled facecolors


Space after #.

QuLogic · 2017-12-14T08:17:19Z

lib/mpl_toolkits/mplot3d/axes3d.py

-                    zbase = a[min(rows-1, rs+rstride), cs:min(cols, cs+cstride+1):][::-1]
-                    zright = a[rs:min(rows-1, rs+rstride):, cs][::-1]
+                    # the edges of the projected quadrilateral
+                    # note we use pythons half-open ranges to avoid repeating


QuLogic · 2017-12-14T08:18:35Z

lib/mpl_toolkits/mplot3d/axes3d.py

+                    # the edges of the projected quadrilateral
+                    # note we use pythons half-open ranges to avoid repeating
+                    # the corners
+                    ztop   = a[rs,             cs:cs_next   ]


Spacing is against PEP8.

Adds greatly to the clarity here though, so I'd maybe make an exception

QuLogic · 2017-12-14T08:22:18Z

lib/mpl_toolkits/mplot3d/axes3d.py

-                avgzsum = sum(p[2] for p in ps2)
-                polys.append(ps2)
+                # ps = np.stack(ps, axis=-1)
+                ps = np.array(ps).T


I wonder if there's some way of pre-allocating this with the lengths of row_inds and col_inds.

No, because ps varies in length throughout the iteration

Err, I might have had the wrong loop; the lengths of bits sliced out of X, Y, Z are not known?

No. They correspond the perimeters of the "square" "cells", which are not always the same size - the cells look like:

+---+---+-+ | | | | | | | | +---+---+-+ | | | | | | | | +---+---+-+ | | | | +---+---+-+

where the trailing edges contain non-uniformly sized cells. Perhaps we could vectorize across the uniform ones, and do the rest separately, but that's more work.

I think we're talking about different things; I mean ps itself which should be uniform since you're calling np.array on it, no?

Ah, I see. Yes, we could preallocate to (2*(rs_next - rs) + 2*(cs_next - cs), 3)

Ok, here's the answer - preallocating doesn't help, because concatenate has no out argument until numpy 1.14

QuLogic · 2017-12-14T08:34:48Z

lib/mpl_toolkits/mplot3d/axes3d.py

+                # pick three points around the polygon to find the normal at
+                # hard to vectorize, since len(ps) is different at the edges
+                i1, i2, i3 = 0, int(len(ps)/3), int(2*len(ps)/3)
+                v1[poly_i] = ps[i1] - ps[i2]


I prefer to be explicit with known 2D things, i.e., v1[poly_i, :] = ps[i1, :] - ps[i2, :]; I don't think there's any penalty for it.

There is a small penalty associated with parsing tuple indices over scalar indices, but I suppose it's probably not significant.

eric-wieser · 2017-12-14T08:41:00Z

Looks like I got something wrong here...

eric-wieser · 2017-12-14T09:02:37Z

Style fixes made, will investigate failures once I get my test setup working again

eric-wieser · 2017-12-15T04:59:19Z

@QuLogic: Worked out why it was failing - previously, we were plotting every quadrilateral with 5 points, which changed the normal vector! I've updated the images, and added another test that makes shading more obvious, and tests striding.

eric-wieser · 2017-12-15T05:00:00Z

lib/mpl_toolkits/mplot3d/axes3d.py

-                # are removed here.
-                ps = list(zip(*ps))
-                lastp = np.array([])
-                ps2 = [ps[0]] + [ps[i] for i in xrange(1, len(ps)) if ps[i] != ps[i-1]]


This didn't detect the fact that ps[-1] == ps[0], which was always true

eric-wieser · 2017-12-15T07:22:07Z

Failures are PEP8 failures - one line that's 80 instead of 79, and then the extra spacing that I argue is worth keeping

eric-wieser · 2018-03-16T08:12:26Z

Ping.

The failure is the following PEP8 violation in these lines

/home/travis/build/matplotlib/matplotlib/lib/mpl_toolkits/mplot3d/axes3d.py:1686:25: E221 multiple spaces before operator
                    ztop   = a[rs,             cs:cs_next   ]
                        ^
/home/travis/build/matplotlib/matplotlib/lib/mpl_toolkits/mplot3d/axes3d.py:1686:60: E202 whitespace before ']'
                    ztop   = a[rs,             cs:cs_next   ]
                                                           ^
/home/travis/build/matplotlib/matplotlib/lib/mpl_toolkits/mplot3d/axes3d.py:1687:26: E221 multiple spaces before operator
                    zleft  = a[rs:rs_next,     cs_next      ]
                         ^
/home/travis/build/matplotlib/matplotlib/lib/mpl_toolkits/mplot3d/axes3d.py:1687:60: E202 whitespace before ']'
                    zleft  = a[rs:rs_next,     cs_next      ]
                                                           ^
/home/travis/build/matplotlib/matplotlib/lib/mpl_toolkits/mplot3d/axes3d.py:1688:26: E221 multiple spaces before operator
                    zbase  = a[rs_next,        cs_next:cs:-1]
                         ^
/home/travis/build/matplotlib/matplotlib/lib/mpl_toolkits/mplot3d/axes3d.py:1689:60: E202 whitespace before ']'
                    zright = a[rs_next:rs:-1,  cs           ]

Does this seem like a reasonable exception to spacing rules
Do I need to mark it as such?

anntzer · 2018-03-16T08:57:05Z

I'm fine with putting a noqa.

eric-wieser · 2018-03-16T23:45:47Z

Updated with:

A helper function to enable a list comprehension
~~Flake8 ignore~~
Much shorter lines to make alignment no longer needed to make things readable

eric-wieser · 2018-03-17T00:06:16Z

lib/mpl_toolkits/mplot3d/axes3d.py


+        def boundary_edge(arr):
+            """
+            Get the boundary elements of the rectangle ``arr``,


Happy to rename / extract this function if someone else can suggest a name / location.

heh, I might call it "nibble()", because it nibbles around the edge of the 2D array. At the very least, I would expand the explanation a bit to get that point across. It took me a few minutes to realize that was what was done.

Not a fan of nibble. How about one of

[array_](perimeter|edge|boundary|border)[_(elements|values)]

I think I like array_perimeter most.

Can you suggest a top-level place to put this in?

Moved to cbook, and added an example

WeatherGod

This code is cleaner than before, which is definitely an improvement. Might be nice to figure out if there is a way to keep the arrays from getting ragged, but I don't think that is possible by the nature of the problem.

There still remains a little bit more to do here, particularly fixing the py3k issue.

WeatherGod · 2018-03-17T13:27:42Z

lib/mpl_toolkits/mplot3d/axes3d.py

+
+        # evenly spaced, and including both endpoints
+        row_inds = list(xrange(0, rows-1, rstride)) + [rows-1]
+        col_inds = list(xrange(0, cols-1, cstride)) + [cols-1]


this isn't py3k compatible. xrange doesn't exist, right?

Maybe np.array_split() would be cleaner? Don't know when it was added to numpy, but I do see it in v1.12.

array_split produces non-overlapping chunks, but an interesting idea.

I assume that there was xrange = range or something, but I guess not. I'll switch to range

It looks like I this came in after rebasing on #10525. Does matplotlib no longer support python 2?

Not on master, it doesn't.

Am I right to target this against master?

Yes, we can always backport if necessary (I don't think it is).

WeatherGod · 2018-03-17T13:48:36Z

lib/mpl_toolkits/mplot3d/axes3d.py


+        def boundary_edge(arr):
+            """
+            Get the boundary elements of the rectangle ``arr``,


heh, I might call it "nibble()", because it nibbles around the edge of the 2D array. At the very least, I would expand the explanation a bit to get that point across. It took me a few minutes to realize that was what was done.

eric-wieser · 2018-03-18T17:54:00Z

@WeatherGod: Updated, tests now all pass

eric-wieser · 2018-04-29T06:10:49Z

@WeatherGod: Anything else needed here?

WeatherGod · 2018-05-02T00:27:16Z

lib/matplotlib/cbook/__init__.py

                delattr(obj, attr)
            else:
                setattr(obj, attr, orig)
+


Looks like a PEP8 check is not liking the blank line here? I thought we were supposed to have a blank line at the end of files?

A new line, not a full blank line.

Ah, I couldn't tell that with the github diffs. I like how git diff shows you those trailing blank lines.

Bad merge I guess - will fix

WeatherGod · 2018-05-02T00:33:24Z

lib/mpl_toolkits/mplot3d/axes3d.py

+                i1, i2, i3 = 0, len(ps)//3, 2*len(ps)//3
+                v1[poly_i, :] = ps[i1, :] - ps[i2, :]
+                v2[poly_i, :] = ps[i2, :] - ps[i3, :]
+            normals = np.cross(v1, v2)


We need an else: normals = [] so that "normals" is always defined for the code blocks ahead.

I don't think this happens in any real cases, but will fix instead of trying to work out which cases.

Better would be a local function that's only called when the result is needed.

Setting to [] is an (existing) bug too, which I guess caused normals to be ignored when face colors were specified.

WeatherGod · 2018-05-02T00:44:14Z

Besides those two points, I think this is ready to go. It is certainly easier to understand than before.

Previously: * "cell" perimeters were clumsily calculated with duplicates, which were then (badly) removed at runtime. As a result, every quadrilateral was drawn with 5 vertices! * code to calculate normals was spread into multiple places * average z was calculated even if not used * normals were sometimes not calculated even when needed * repeated conversion between stride and count was done This will affect shading of plots very slightly, hence the image tests changing in this commit. Adds a `cbook._array_perimeter` function for use here.

eric-wieser · 2018-05-02T05:55:01Z

@WeatherGod: All fixed - thanks for catching that normals bug

eric-wieser · 2018-05-12T19:24:26Z

@WeatherGod: Starting to undergo bitrot here. I've merged and solved conflicts again, but I'd rather not keep having to

WeatherGod · 2018-05-13T00:19:08Z

lib/mpl_toolkits/mplot3d/axes3d.py

-        else :
-            normals = []

+        def get_normals(polygons):


Just noticed the existing _generate_normals() further down. We should make sure we don't have such duplication of near-similar things without at least justifying the duplication. Or figure out how to get rid of the duplicate logic.

I'd argue that that's out of scope for this PR - I've not introduced that duplication, I just made it clearer that it exists (which makes it easier for someone else to remove later). Of course, that someone else might be me, but I'd rather do it in a separate unit of work.

fair enough

In particular, deciding which version to keep (assemble then cross vs cross then assemble) would require some profiling

This is addressed in #12136

WeatherGod · 2018-05-13T00:21:58Z

it is amazing how much cleaner this code has gotten over the revisions!

eric-wieser · 2018-05-23T04:43:17Z

Once this goes in, I'll pick back up #9990

eric-wieser · 2018-06-19T07:45:41Z

@QuLogic, @anntzer, is there anything else you'd like to see here (that wouldn't fit better in a future PR)?

anntzer · 2018-06-19T12:21:46Z

(I haven't actually followed the logic of the code, so I'll leave the review to others for now.)

jklymak

Approving largely based on @WeatherGod approval and the fact that it passes the tests, and doesn't introduce any obvious bugs.

eric-wieser · 2018-07-10T07:49:33Z

Thanks! I'll get back to #9990 in the next few weeks

eric-wieser commented Dec 14, 2017

View reviewed changes

QuLogic added the topic: mplot3d label Dec 14, 2017

QuLogic reviewed Dec 14, 2017

View reviewed changes

eric-wieser force-pushed the more-normals branch from 86e1518 to 893801f Compare December 14, 2017 09:00

eric-wieser force-pushed the more-normals branch from 893801f to c38e675 Compare December 15, 2017 04:58

eric-wieser commented Dec 15, 2017

View reviewed changes

eric-wieser force-pushed the more-normals branch from c38e675 to 383f34d Compare December 15, 2017 05:03

eric-wieser changed the title ~~MAINT: Simplify logic in Axes3D.plot_surface~~ MAINT/BUG: Don't use 5-sided quadrilaterals in Axes3D.plot_surface Dec 15, 2017

eric-wieser force-pushed the more-normals branch from 383f34d to cc92c08 Compare December 15, 2017 05:43

eric-wieser force-pushed the more-normals branch from cc92c08 to 12517c8 Compare December 15, 2017 08:15

eric-wieser mentioned this pull request Feb 9, 2018

Fix and document lightsource argument in mplot3d #9990

Merged

6 tasks

eric-wieser force-pushed the more-normals branch 2 times, most recently from ca04037 to 866135c Compare March 16, 2018 23:45

eric-wieser force-pushed the more-normals branch 2 times, most recently from 1a734d8 to bbc417e Compare March 16, 2018 23:56

eric-wieser commented Mar 17, 2018

View reviewed changes

WeatherGod requested changes Mar 17, 2018

View reviewed changes

eric-wieser force-pushed the more-normals branch from bbc417e to ac8f801 Compare March 18, 2018 01:25

WeatherGod reviewed May 2, 2018

View reviewed changes

MAINT: Reduce duplication of stride / count calculations

98c627d

eric-wieser force-pushed the more-normals branch from 937d5be to 4ed3bd9 Compare May 2, 2018 04:16

eric-wieser force-pushed the more-normals branch from 4ed3bd9 to b84c83d Compare May 2, 2018 05:29

Merge branch 'master' into more-normals

b0d7e4b

WeatherGod reviewed May 13, 2018

View reviewed changes

WeatherGod approved these changes May 15, 2018

View reviewed changes

tacaswell added this to the v3.0 milestone Jun 19, 2018

jklymak approved these changes Jul 9, 2018

View reviewed changes

jklymak merged commit 2887c49 into matplotlib:master Jul 9, 2018

eric-wieser deleted the more-normals branch March 23, 2019 18:06

		row_inds = list(xrange(0, rows-1, rstride)) + [rows-1]
		col_inds = list(xrange(0, cols-1, cstride)) + [cols-1]



		#colset contains the data for coloring: either average z or the facecolor
		#colset contains the sampled facecolors

Uh oh!

Conversation

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

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QuLogic 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!

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!

eric-wieser commented Dec 14, 2017

Uh oh!

eric-wieser commented Dec 14, 2017

Uh oh!

eric-wieser commented Dec 15, 2017

Uh oh!

eric-wieser Dec 15, 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!

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

Uh oh!

Uh oh!

eric-wieser commented Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anntzer commented Mar 16, 2018

Uh oh!

eric-wieser commented Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

WeatherGod 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

eric-wieser commented Dec 14, 2017 •

edited

Loading

QuLogic Dec 14, 2017 •

edited

Loading

eric-wieser Dec 14, 2017 •

edited

Loading

eric-wieser Dec 15, 2017 •

edited

Loading

eric-wieser commented Dec 15, 2017 •

edited

Loading

eric-wieser commented Mar 16, 2018 •

edited

Loading

eric-wieser commented Mar 16, 2018 •

edited

Loading

eric-wieser May 2, 2018 •

edited

Loading