MAINT: Simplify algebra in LightSource.hillshade #8880
Conversation
| np.cos(az) * np.cos(alt), | ||
| np.sin(az) * np.cos(alt), | ||
| np.sin(alt) | ||
| ]) |
There was a problem hiding this comment.
This trig is unavoidable, but it's also on scalars, so is cheap
lib/matplotlib/colors.py
Outdated
| normal[...,0] = -e_dx | ||
| normal[...,1] = -e_dy | ||
| normal[...,2] = 1 | ||
| normal /= _linalg_norm(normal, keepdims=True, axis=-1) |
There was a problem hiding this comment.
All of this vectorized trig on dx and dy amounts to... concatenating and normalizing.
Expanding np.cos(az - aspect) turns this into a dot product between a bunch of trig stuff, and the light .direction vector.
Moving on from there is done by tedious application of trig identities, or simply by geometric reasoning on how to obtain normal vectors.
lib/matplotlib/colors.py
Outdated
| @@ -1493,6 +1493,11 @@ def hsv_to_rgb(hsv): | |||
| return rgb | |||
|
|
|||
|
|
|||
| # np.linalg.norm doesn't broadcast in numpy 1.7 | |||
| def _linalg_norm(arr, keepdims=False, axis=None): | |||
| return np.sqrt(np.sum(np.square(arr), axis=axis, keepdims=keepdims)) | |||
There was a problem hiding this comment.
I'm correct that we're stuck on 1.7, right? Does this helper exist elsewhere?
There was a problem hiding this comment.
There was a problem hiding this comment.
Wasn't enough here - that doesn't preserve masks on a masked array. See the corrected patch
eric-wieser
force-pushed
the
tidy-LightSource.hillshade
branch
from
d5cff49
to
f409e31
Compare
|
Currently failing on masked arrays due to numpy bugs |
eric-wieser
force-pushed
the
tidy-LightSource.hillshade
branch
from
f409e31
to
9df110d
Compare
Don't use trig functions when vector operations will suffice. Image tests of this function continue to pass
eric-wieser
force-pushed
the
tidy-LightSource.hillshade
branch
from
9df110d
to
2990c78
Compare
|
Seems that this reduced the number of executable lines in |
Don't use trig functions when vector operations will suffice.
This also breaks
hillshadedown into two smaller functions, since shading based on normal vectors is an operation that's likely helpful elsewhere.Image tests of this function continue to pass.
Brought up by #8877, after considering shading for #6404