Add lightsource parameter to bar3d #15099

fourpoints · Aug 22, 2019

Bar3d now allows the user to define the user to define the origin of the lightsource.

PR Summary

Note: I have little experience with pull requests, and I have not thouroughly checked the correctness of the code (even though it is two lines of code, anything can go wrong), so I cannot advise a merge just yet. I would like to hear if this change is even needed (is this even the best way to go about it?). I will make the proper adjustments if needed, any advice is appreciated.

This adds a lightsource parameter to bar3d. This is already a documented feature without any implementation (AFAIK). The documentation for the shade parameter says:

When true, this shades the dark sides of the bars (relative to the plot's source of light).

However, there is currently no apparent way to set this, as bar3d calls self._shade_colors(facecolors, normals) without a lightsource parameter, which by default uses LightSource(azdeg=225, altdeg=19.4712) if no lightsource parameter is set.

PR Checklist

Has Pytest style unit tests
Code is Flake 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
~~Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way~~

Bar3d now allows the user to define the user to define the origin of the lightsource.

ImportanceOfBeingErnest · Aug 22, 2019

lib/mpl_toolkits/mplot3d/axes3d.py

@@ -2375,6 +2375,9 @@ def bar3d(self, x, y, z, dx, dy, dz, color=None,
        shade : bool, optional (default = True)
            When true, this shades the dark sides of the bars (relative
            to the plot's source of light).
+
+        lightsource : matplotlib.color.LightSource, optional (default = None)
+            The lightsource defines the plot's source of light.


The lightsource defines the plot's source of light, if shade=True is set.

Thanks, I noticed that plot_trisurf already uses this parameter so I copied its line instead:

lightsource : `~matplotlib.colors.LightSource` The lightsource to use when *shade* is True.

Uses `plot_trisurface`'s documentation for `bar3d`'s lightsource parameter.

Failed Travis CI test for flake8.

ImportanceOfBeingErnest · Aug 22, 2019

In general it would be nice to see the effect of this new parameter. Do you have a small example code with an image you can show in the PR?

Also there should be a test that checks that the new parameter does what it should. In the simplest case this can just be a smoke-test, or one could check if a particular face of the bar really has the expected color.

Fixed over- and under-indentation.

fourpoints · Aug 22, 2019

sample code

import matplotlib.pyplot as plt
from mpl_toolkits.mplot3d import Axes3D
import matplotlib.colors as mcolors
import numpy as np

fig = plt.figure()
ax = fig.add_subplot(1, 1, 1, projection="3d")

ls = mcolors.LightSource(azdeg=0, altdeg=90)
cmap = plt.get_cmap("coolwarm")

length, width = 3, 4
area = length * width

norm = mcolors.Normalize(0, area-1)

x, y = np.meshgrid(np.arange(length), np.arange(width))
x = x.ravel()
y = y.ravel()
dz = x + y

color = np.array([cmap(norm(i)) for i in range(area)])

collection = ax.bar3d(x=x, y=y, z=0,
         dx=1, dy=1, dz=dz,
         color=color, shade=True, lightsource=ls)

ax.set_title(f"Azimutal degree = {ls.azdeg}°, altitude degree = {ls.altdeg}°")

plt.show()

# simple color test
assert np.all(color == collection._facecolors3d[1::6]),\
    "bar colors doesn't match"

Issues:

I am still not sure why Travis CI is failing. Apparently due to flake8, but this I cannot reproduce.
I am not sure where I am supposed to add the test?

ImportanceOfBeingErnest · Aug 22, 2019

lib/mpl_toolkits/mplot3d/axes3d.py

@@ -2375,6 +2375,9 @@ def bar3d(self, x, y, z, dx, dy, dz, color=None,
        shade : bool, optional (default = True)
            When true, this shades the dark sides of the bars (relative
            to the plot's source of light).
+


This is the line that causes the flake hickup. A blank like cannot contain any whitespaces.

ImportanceOfBeingErnest · Aug 22, 2019

A test would need to go into lib/mpl_toolkits/tests/test_mplot3d.py; there are already 3 test for bars.

Compares the computed colors of the bar tops with the user-defined colors.

ImportanceOfBeingErnest · Aug 23, 2019

lib/mpl_toolkits/tests/test_mplot3d.py

+                          dx=1, dy=1, dz=dz,
+                          color=color, shade=True, lightsource=ls)
+
+    np.testing.assert_array_equal(color, collection._facecolors3d[1::6])


What I don't understand here is, if the facecolors are actually the same as the colors taken from the colormap, are we really testing that the lightsource works? That is, if there is a lightsource, shouldn't the colors be different from the ones from the coolwarm colormap?

._facecolors3d[1::6] should be taking the color of the top surfaces, which with a lightsource coming from the top (altdeg=90), should leave these unchanged (fully illuminated), as expected.

Changing the altdeg (e.g. default value of 19.4712) or selecting different sides (e.g. .facecolors3d[2::6]) fails this test.

A test could be made which computes to expected colors given different angles, or compares to an image, but I thought this simple approach would suffice, as it ensures that the default lightsource is not used.

It's worth mentioning that the test would also pass if no shading were done, but test_bar3d_shaded should already test for this.

I see. So imagine in the future someone unintentionally removes lightsource from the line self._shade_colors(facecolors, normals, lightsource). The test would not detect this, right? But that is what we have tests for.

The test would detect this.

Calling self._shade_colors(facecolors, normals) without a lightsource argument defaults to LightSource(azdeg=225, altdeg=19.4712), which would fail the test. This would shade the top surfaces with a different color. It will only pass if the top colors are unchanged, which requires a LightSource(altdeg=90) to be passed through.

Oh, that is not very obvious. I'd suggest to add that as a comment in the test; else I fear in the future people might be as confused as I was by it.

May I suggest the following as comment in the test:

# Testing that the custom 90° lightsource produces different shading than # the default, and that those colors are precisely the colors from the # colormap, due to the illumination parallel to the z-axis.

Thanks, that sounds much better. I also added that it compares the top facecolors.

#(...) different shading on the top facecolors compared to the default (...)

ImportanceOfBeingErnest · Aug 23, 2019

One final thing. If you think this is a notable feature, people should be made be aware of, you can add a "What's new entry"

Make the comment more obvious

Add lightsource parameter to bar3d

8012595

Bar3d now allows the user to define the user to define the origin of the lightsource.

ImportanceOfBeingErnest reviewed Aug 22, 2019

View reviewed changes

fourpoints added 2 commits August 22, 2019 14:22

Copy lightsource parameter documentation

74c49eb

Uses `plot_trisurface`'s documentation for `bar3d`'s lightsource parameter.

Removed redundant #'s for flake8 complicancy

6575740

Failed Travis CI test for flake8.

Changed indentation for flake8 compliancy

ecb8811

Fixed over- and under-indentation.

ImportanceOfBeingErnest reviewed Aug 22, 2019

View reviewed changes

fourpoints added 2 commits August 23, 2019 09:57

Remove whitespace in blank line

e19babf

Add test for bar3d lightsource

0215995

Compares the computed colors of the bar tops with the user-defined colors.

dstansby added the topic: mplot3d label Aug 23, 2019

ImportanceOfBeingErnest reviewed Aug 23, 2019

View reviewed changes

fourpoints added 5 commits August 23, 2019 12:41

Add support for lightsource parameter in bar3d

a8a6b83

Extend line length in rst

cf778f6

Fix rst formatting

5dd5f22

Fix rst formatting

aae9b9f

Add test description

429d090

tacaswell added this to the v3.3.0 milestone Aug 24, 2019

timhoffm approved these changes Aug 25, 2019

View reviewed changes

fourpoints added 2 commits August 25, 2019 20:24

Update test description

51029fb

Make the comment more obvious

Fix typo

dd2f0dd

ImportanceOfBeingErnest approved these changes Aug 25, 2019

View reviewed changes

ImportanceOfBeingErnest modified the milestones: v3.3.0, v3.2.0 Aug 25, 2019

ImportanceOfBeingErnest merged commit 1cf7539 into matplotlib:master Aug 25, 2019

fourpoints deleted the patch-2 branch August 25, 2019 19:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Add lightsource parameter to bar3d #15099

Add lightsource parameter to bar3d #15099

Uh oh!

fourpoints commented Aug 22, 2019 •

edited

Loading

Uh oh!

ImportanceOfBeingErnest Aug 22, 2019

Uh oh!

fourpoints Aug 22, 2019

Uh oh!

ImportanceOfBeingErnest commented Aug 22, 2019

Uh oh!

fourpoints commented Aug 22, 2019 •

edited

Loading

Uh oh!

ImportanceOfBeingErnest Aug 22, 2019

Uh oh!

ImportanceOfBeingErnest commented Aug 22, 2019

Uh oh!

ImportanceOfBeingErnest Aug 23, 2019

Uh oh!

fourpoints Aug 23, 2019

Uh oh!

ImportanceOfBeingErnest Aug 23, 2019

Uh oh!

fourpoints Aug 23, 2019 •

edited

Loading

Uh oh!

ImportanceOfBeingErnest Aug 23, 2019

Uh oh!

ImportanceOfBeingErnest Aug 25, 2019 •

edited

Loading

Uh oh!

fourpoints Aug 25, 2019

Uh oh!

ImportanceOfBeingErnest commented Aug 23, 2019

Uh oh!

Uh oh!

Search code, repositories, users, issues, pull requests...

Uh oh!

Add lightsource parameter to bar3d #15099

Add lightsource parameter to bar3d #15099

Uh oh!

Conversation

fourpoints commented Aug 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ImportanceOfBeingErnest commented Aug 22, 2019

Uh oh!

fourpoints commented Aug 22, 2019 • 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!

ImportanceOfBeingErnest commented Aug 22, 2019

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!

fourpoints Aug 23, 2019 • 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!

ImportanceOfBeingErnest Aug 25, 2019 • 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!

ImportanceOfBeingErnest commented Aug 23, 2019

Uh oh!

Uh oh!

fourpoints commented Aug 22, 2019 •

edited

Loading

fourpoints commented Aug 22, 2019 •

edited

Loading

fourpoints Aug 23, 2019 •

edited

Loading

ImportanceOfBeingErnest Aug 25, 2019 •

edited

Loading