ENH: Optional 3d bar shading #7691

phobson · Dec 27, 2016

codecov-io · Dec 27, 2016

Current coverage is 62.11% (diff: 100%)

Merging #7691 into master will increase coverage by <.01%

@@             master      #7691   diff @@
==========================================
  Files           174        174          
  Lines         56022      56024     +2   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          34799      34801     +2   
  Misses        21223      21223          
  Partials          0          0

Powered by Codecov. Last update 159f36e...a99c034

WeatherGod · Dec 29, 2016

examples/mplot3d/plot_3d_bars.py

+# fake data
+_x = np.arange(4)
+_y = np.arange(5)
+_xx, _xx = np.meshgrid(_x, _y)


typo here, should be _xx, _yy = ...

WeatherGod · Dec 29, 2016

examples/mplot3d/plot_3d_bars.py

+bottom = np.zeros_like(top)
+width = depth = 1
+
+ax1.bar3d(x, y, bottom, width, depth, top, shade=True)


simplify to ax1.bar() instead of ax1.bar3d()

I don't understand this one. I'm trying to draw 3-dimensional bars. I thought bar would draw 2-D bars on the 3D Axes object.

ah, sorry, my reply apparently never got sent. For Axes3D objects, ax.bar() is the alias of ax.bar3d(). The bar3d() is an anachronism, much like set_xlim3d().

I take it all back. I could have sworn we separated the flat bar stuff out. You are right.

WeatherGod · Dec 29, 2016

examples/mplot3d/plot_3d_bars.py

+ax2.bar3d(x, y, bottom, width, depth, top, shade=False)
+ax2.set_title('Not Shaded')
+
+fig.canvas.draw()


should be plt.show()

WeatherGod · Dec 29, 2016

lib/mpl_toolkits/mplot3d/axes3d.py

+        """Generate a 3D barplot.
+
+        This method creates three dimensional barplot where the width,
+        depth, height, and color of the bars can all ne uniquely set.


typo: "ne" --> "be"

WeatherGod · Dec 29, 2016

lib/mpl_toolkits/mplot3d/axes3d.py

+        zsort : str, optional
+            The z-axis sorting scheme passed onto
+            :func:`~mpl_toolkits.mplot3d.art3d.Poly3DCollection`
+        shape : bool, optional


typo: 'shape' --> 'shade'

WeatherGod · Dec 29, 2016

lib/mpl_toolkits/tests/test_mplot3d.py

+    x2d, y2d = x2d.ravel(), y2d.ravel()
+    z = x2d + y2d
+    ax.bar3d(x2d, y2d, x2d * 0, 1, 1, z, shade=True)
+    fig.canvas.draw()


remove the unneeded draw()

WeatherGod · Dec 29, 2016

lib/mpl_toolkits/tests/test_mplot3d.py

+    remove_text=True,
+    extensions=['png']
+)
+def test_bar3d_dflt_actually3d():


I prefer test names and baseline image names that are the same or very nearly the same. It makes it easy to match a bad image file with the test in the code.

fair enough. I'll change them.

WeatherGod · Dec 29, 2016

lib/mpl_toolkits/tests/test_mplot3d.py

+    remove_text=True,
+    extensions=['png']
+)
+def test_bar3d_dflt_actually3d_no_shade ():


extraneous space between function name and parens.

WeatherGod · Dec 29, 2016

lib/mpl_toolkits/tests/test_mplot3d.py

    fig = plt.figure()
    ax = fig.add_subplot(111, projection='3d')
    x = np.arange(4)
    y = np.arange(5)
    x2d, y2d = np.meshgrid(x, y)
    x2d, y2d = x2d.ravel(), y2d.ravel()
    z = x2d + y2d
-    ax.bar3d(x2d, y2d, x2d * 0, 1, 1, z)
+    ax.bar3d(x2d, y2d, x2d * 0, 1, 1, z, shade=False)
    fig.canvas.draw()


go ahead and take it out here, too

WeatherGod · Dec 29, 2016

lib/mpl_toolkits/tests/test_mplot3d.py

@@ -20,16 +20,37 @@ def test_bar3d():
        ax.bar(xs, ys, zs=z, zdir='y', color=cs, alpha=0.8)


-@cleanup
-def test_bar3d_dflt_smoke():


heh... I wonder why this was a smoke test?

WeatherGod · Jan 2, 2017

Nope. Look at axes3d.py. You can't do 2d bar() with an Axes3d object. It overrides the 2d version. The 3d part of many of the methods and attributes are anachronisms and are only kept for back-compat. On Jan 1, 2017 5:08 PM, "Paul Hobson" <notifications@github.com> wrote: *@phobson* commented on this pull request. ------------------------------ In examples/mplot3d/plot_3d_bars.py <#7691>:

+# setup the figure and axes

+fig = plt.figure(figsize=(8, 3)) +ax1 = fig.add_subplot(121, projection='3d') +ax2 = fig.add_subplot(122, projection='3d') + +# fake data +_x = np.arange(4) +_y = np.arange(5) +_xx, _xx = np.meshgrid(_x, _y) +x, y = _xx.ravel(), _yy.ravel() + +top = x + y +bottom = np.zeros_like(top) +width = depth = 1 + +ax1.bar3d(x, y, bottom, width, depth, top, shade=True) I don't understand this one. I'm trying to draw 3-dimensional bars. I thought bar would draw 2-D bars on the 3D Axes object. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#7691>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARy-GgAFULsGoScOm_0C4EUa-c00xhhks5rOCP0gaJpZM4LWa0h> .

phobson · Jan 2, 2017

@WeatherGod Oh, I see. Should I deprecated bar3d then?

phobson · Jan 2, 2017

Wait, I take it back, I'll still confused.

In axes3d.py, I see:

    def bar(self, left, height, zs=0, zdir='z', *args, **kwargs):
        '''
        Add 2D bar(s).
        ...
        '''

And then:

    def bar3d(self, x, y, z, dx, dy, dz, color=None,
              zsort='average', shade=True, *args, **kwargs):
        """Generate a 3D barplot.
        ...
        """

These seem like two very different and current methods. In the example that I created I wanted to demonstrate the difference that using shading makes, so 3D bars need to be generated in both cases.

The plot looks like this:

Perhaps this would be clarified if the example was renamed to plot_bars3d

NelleV

LGTM 👍

NelleV · Jan 13, 2017

lib/mpl_toolkits/mplot3d/axes3d.py

+            The coordinates of the anchor point of the bars.
+
+        dx, dy, dz : scalar or array-like
+            The width, depth, and height of the bars, resprectively.


typo: respectively.

NelleV · Jan 13, 2017

lib/mpl_toolkits/mplot3d/axes3d.py

+        zsort : str, optional
+            The z-axis sorting scheme passed onto
+            :func:`~mpl_toolkits.mplot3d.art3d.Poly3DCollection`
+        shade : bool, optional


what's the default?

NelleV · Jan 13, 2017

Thanks for the example and the docstring update. It looks great!

NelleV · Jan 16, 2017

I hadn't seen the previous comments. I can have a look at it if you'd like

QuLogic · Feb 24, 2017

Any chance you will get back to this soon @phobson?

phobson · Feb 24, 2017

@QuLogic happy to rebase, but I'm still unclear what @WeatherGod meant with his comments.

phobson · Feb 27, 2017

@QuLogic rebased and squashed everything.
@WeatherGod could you take a second look at this. I'm still not sure what meant by this

Despite the name of the example, ax.bar puts 2D bars in a 3D space while ax.bar3d draws actual prisms

WeatherGod · Feb 27, 2017

We still have the other things I have noticed to fix, though.

phobson · Feb 27, 2017

@WeatherGod understood. I'm looking for clarity on what they might be.

phobson · Feb 27, 2017

@WeatherGod oops, sorry. I didn't see your other inline comments 🐑

I took care of them and force-pushed

WeatherGod

almost there... just noticed a couple more typos and 1 code question.

WeatherGod · Feb 28, 2017

lib/mpl_toolkits/mplot3d/axes3d.py

+            :func:`~mpl_toolkits.mplot3d.art3d.Poly3DCollection`
+        shape : bool, optional (default = True)
+            When true, the dark sides of the bars (relative to the
+            plot's source of light), shaded.


typo: shape --> shade

Also, the description is missing a verb. Additionally, I think there is supposed to be a blank line between the zsort entry and the shade entry?

Still missing a verb for this sentence, and possibly a blank line between the zsort entry and the shade entry.

WeatherGod · Feb 28, 2017

lib/mpl_toolkits/mplot3d/axes3d.py

@@ -2511,9 +2536,13 @@ def bar3d(self, x, y, z, dx, dy, dz, color=None,
            facecolors = list(mcolors.to_rgba_array(color))
            if len(facecolors) < len(x):
                facecolors *= (6 * len(x))
+                normals = self._generate_normals(polys)


why is this here? wouldn't it make a lot more sense if it was in the if shade block?

WeatherGod · Mar 8, 2017

Ah, I almost completely forgot... we need a "what's new" entry for this.

NelleV · Mar 19, 2017

@phobson Do you think you'll have time to write this what's new entry?

NelleV · Mar 20, 2017

doc/users/whats_new/toggle_3d_bar_shading.rst

+--------------------------------------------
+
+A new ``shade`` parameter has been added the 3D bar plotting method.
+The default behavior remains to the shade the bars, but now users


I think it should be "to shade the bars"

gahhh i read that like 5 times. i'll fix :)

(shakes fist at self)

closes matplotlib#7683 Also expands the testing and documentation of bar3d

WeatherGod · Mar 21, 2017

doc/users/whats_new/toggle_3d_bar_shading.rst

+
+A new ``shade`` parameter has been added the 3D bar plotting method.
+The default behavior remains to shade the bars, but now users
+have to the option of setting ``shade`` to ``False``.


extraneous "to"

phobson · Mar 22, 2017

ping @WeatherGod

phobson added the topic: mplot3d label Dec 27, 2016

phobson self-assigned this Dec 28, 2016

phobson force-pushed the optional-3d-bar-shading branch from 907bb0b to 67d1642 Compare December 29, 2016 02:09

WeatherGod requested changes Dec 29, 2016

View reviewed changes

phobson force-pushed the optional-3d-bar-shading branch from 67d1642 to a99c034 Compare January 2, 2017 00:27

phobson changed the title ~~WIP: Optional 3d bar shading~~ ENH: Optional 3d bar shading Jan 2, 2017

phobson added the status: needs review label Jan 2, 2017

NelleV approved these changes Jan 13, 2017

View reviewed changes

NelleV changed the title ~~ENH: Optional 3d bar shading~~ [MRG+1] ENH: Optional 3d bar shading Jan 13, 2017

phobson force-pushed the optional-3d-bar-shading branch from a99c034 to 43bb1c1 Compare February 27, 2017 20:20

phobson force-pushed the optional-3d-bar-shading branch from bb4834c to fac8a25 Compare February 27, 2017 21:15

phobson force-pushed the optional-3d-bar-shading branch from fac8a25 to 333e444 Compare February 27, 2017 21:18

WeatherGod requested changes Feb 28, 2017

View reviewed changes

phobson force-pushed the optional-3d-bar-shading branch from 13639c1 to 448e5d7 Compare February 28, 2017 18:05

WeatherGod approved these changes Mar 7, 2017

View reviewed changes

NelleV reviewed Mar 20, 2017

View reviewed changes

ENH: Make 3d bar shading optional

ffab7c5

closes matplotlib#7683 Also expands the testing and documentation of bar3d

phobson force-pushed the optional-3d-bar-shading branch from 397198d to a218cb1 Compare March 20, 2017 18:09

WeatherGod reviewed Mar 21, 2017

View reviewed changes

DOC: add 3D bar shade blurb to what's new

8e535e6

phobson force-pushed the optional-3d-bar-shading branch from a218cb1 to 8e535e6 Compare March 21, 2017 15:46

WeatherGod merged commit ee61ee4 into matplotlib:master Mar 22, 2017

phobson deleted the optional-3d-bar-shading branch March 22, 2017 21:21

QuLogic changed the title ~~[MRG+1] ENH: Optional 3d bar shading~~ ENH: Optional 3d bar shading Mar 23, 2017

QuLogic removed the status: needs review label Mar 23, 2017

QuLogic added this to the 2.1 (next point release) milestone Mar 23, 2017

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

Uh oh!

ENH: Optional 3d bar shading #7691

ENH: Optional 3d bar shading #7691

Uh oh!

Conversation

phobson commented Dec 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Dec 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 62.11% (diff: 100%)

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!

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!

WeatherGod commented Jan 2, 2017 via email

Uh oh!

phobson commented Jan 2, 2017

Uh oh!

phobson commented Jan 2, 2017

Uh oh!

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

NelleV commented Jan 13, 2017

Uh oh!

NelleV commented Jan 16, 2017

Uh oh!

QuLogic commented Feb 24, 2017

Uh oh!

phobson commented Feb 24, 2017

Uh oh!

phobson commented Feb 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WeatherGod commented Feb 27, 2017

Uh oh!

phobson commented Feb 27, 2017

Uh oh!

phobson commented Feb 27, 2017

Uh oh!

WeatherGod left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phobson commented Dec 27, 2016 •

edited

Loading

codecov-io commented Dec 27, 2016 •

edited

Loading

phobson commented Feb 27, 2017 •

edited

Loading