Update lib/mpl_toolkits/mplot3d/axes3d.py #1294

jgehrcke · 2012-09-21T14:44:13Z

Axes3D.scatter: raise exception in case z.size neither being 1 nor equaling x.size and y.size (according to documentation)

Axes3D.scatter: raise exception in case z.size neither being 1 nor equaling x.size and y.size (according to documentation).

dmcdougall · 2012-09-21T18:37:33Z

On Friday, September 21, 2012, Jan-Philip Gehrcke wrote:

Axes3D.scatter: raise exception in case z.size neither being 1 nor

equaling x.size and y.size (according to documentation)

You can merge this Pull Request by running:

git pull https://github.com/jgehrcke/matplotlib patch-1

Or view, comment on, or merge it at:

#1294
Commit Summary

Update lib/mpl_toolkits/mplot3d/axes3d.py

File Changes

M lib/mpl_toolkits/mplot3d/axes3d.py (7)

Patch Links

https://github.com/matplotlib/matplotlib/pull/1294.patch

https://github.com/matplotlib/matplotlib/pull/1294.diff

—
Reply to this email directly or view it on GitHubhttps://github.com/Update lib/mpl_toolkits/mplot3d/axes3d.py #1294.

Seems reasonable. I'll try it out when I get home. If it feels ok, I'll
leave it up for a day for others to comment. If there are no objections, I
think this is a sensible addition.

Thanks for your contribution!

Damon McDougall
http://www.damon-is-a-geek.com
B2.39
Mathematics Institute
University of Warwick
Coventry
West Midlands
CV4 7AL
United Kingdom

dmcdougall · 2012-09-21T22:31:09Z

I tried this out; it's totally reasonable.

I'll merge this in 24 hours.

pelson · 2012-09-22T20:13:49Z

lib/mpl_toolkits/mplot3d/axes3d.py

-            zs = np.array(zs[0] * xs.size)
+        if xs.size != zs.size:
+            if zs.size == 1:
+                zs = np.array(zs[0] * xs.size)


Was the logic faulty in the first place? I can't see that multiplying the z value by the size of x is a sensible thing to be doing... I would have thought the purpose of this line was more along the lines of: np.array([z[0]] * x.size).

Better yet would be np.tile(z[0], x.size).

@WeatherGod : Any thoughts?

On Saturday, September 22, 2012, Phil Elson wrote:

In lib/mpl_toolkits/mplot3d/axes3d.py:

@@ -1937,8 +1937,11 @@ def scatter(self, xs, ys, zs=0, zdir='z', s=20, c='b', _args, *_kwargs):
zs = np.ma.ravel(zs)
if xs.size != ys.size:
raise ValueError("x and y must be the same size")

if xs.size != zs.size and zs.size == 1:

zs = np.array(zs[0] \* xs.size)

if xs.size != zs.size:

if zs.size == 1:

zs = np.array(zs[0] \* xs.size)

Was the logic faulty in the first place? I can't see that multiplying the
z value by the size of x is a sensible thing to be doing... I would have
thought the purpose of this line was more along the lines of: np.array([z[0]]

x.size).

Better yet would be np.tile(z[0], x.size).

@WeatherGod https://github.com/WeatherGod : Any thoughts?

Good catch. Probably should use something like np.broadcast_arrays()?

Oh wow. Yes, good eye. Well spotted. np.repeat is another option.

Edit: Or, np.ones(xs.size) * zs[0]. That does the broadcasting for you.

Right, should've also looked closer; who wrote that in the first place? :)

zs, xs = np.broadcast_arrays(zs, xs) zs = np.tile(zs[0], xs.size) zs = np.ones(xs.size) * zs[0]

all work (and there probably are a lot more solutions). I'd vote for tile in this case. It is well-readable and super fast (enough for this purpose). In my opionion broadcasting is OTT at this point. However, I am quite inexperienced with numpy and matplotlib, so the decision is up to you and I'll respect it.

Can I somehow change the pull request or should I revoke it?

The PR is a request to merge this branch. So if you add commits and push them to your fork, the PR will automatically be updated appropriately.

@jgehrcke You don't need to revoke it! Just add another commit to your patch-1 branch:

git checkout patch-1
.. hack hack hack..
git add axes3d.py
git commit
git push origin patch-1

And this page will automagically update to reflect your branch's commit history.

I'd vote for either np.tile or np.ones. Let's see what @pelson and @WeatherGod have to say first, though.

My vote:

Fixed > Broken

:-) i.e. I don't really mind which goes. They are both readable enough in my eyes, and I don't expect performance to make any difference in choosing in this case (compared to other operations, microseconds aren't worth counting).

Match argument names in error messages to argument names in documentation.

Make Axes3D.scatter properly deal with `zs` being of size 1.

jgehrcke · 2012-09-23T18:43:15Z

Thanks for your help. I added two new changesets to this pull request. Have tested those with the following code:

from mpl_toolkits.mplot3d import Axes3D
import matplotlib.pyplot as plt
import traceback

testsets = [([1,2], [1,2], [1,2]), # pass
            ([1,2], [1,2], [1]), # pass
            ([1,2], [1,2], 1), # pass
            ([1,2], [1,2], [1,2,3]), # error
            ([1,2], [1,2], []), # error 
            ([1],   [1,2], [1,2])] # error

for x, y, z in testsets:
    ax = plt.figure().add_subplot(111, projection='3d')
    try:
        ax.scatter(x, y, z)
        plt.show()
    except ValueError:
        print traceback.format_exc()
    finally:
        plt.close()

dmcdougall · 2012-09-23T19:10:01Z

lib/mpl_toolkits/mplot3d/axes3d.py

@@ -1936,9 +1936,13 @@ def scatter(self, xs, ys, zs=0, zdir='z', s=20, c='b', *args, **kwargs):
        ys = np.ma.ravel(ys)
        zs = np.ma.ravel(zs)
        if xs.size != ys.size:


Let xs = [1, 2] and ys = [[1], [2]]. This will evaluate to true. Does it make sense to pass xs and ys with different shapes?

I am not entirely sure if that would make sense and did not look at the internals of this method so far. Let's for now suppose that input with a higher dimensionality than 1 does not make sense here. If so, I would add a dimensionality check and document that x, y, z are required to be 1D. This would have to be done for all the other plotting methods in the Axes3D module as well.

.ravel() flatten things out, iirc.

@WeatherGod Good spot, I didn't notice those.

@jgehrcke As Ben points out, it looks like it passes because they're already raveld, so you can disregard my previous concern.

dmcdougall · 2012-09-24T12:10:58Z

@jgehrcke Are you happy for this change to be merged or are there other things you would like to add?

jgehrcke · 2012-09-24T12:22:22Z

If you are happy now, I am also happy :)

Update lib/mpl_toolkits/mplot3d/axes3d.py

dmcdougall · 2012-09-24T12:48:36Z

@pelson @WeatherGod Should I also merge this into v1.2.x? I guess it technically fixed a bug, but also provided an enhancement (the warning). If so, how do I do that? Will the following do it?

git checkout -b v1.2.x usptream/v1.2.x
git pull https://github.com/jgehrcke/matplotlib patch-1
git push core v1.2.x

I added a remote called core for the ssh protocol. I wanted to keep the pushing stuff separate so I don't accidentally mess anything up.

mdboom · 2012-09-24T13:04:07Z

The above lines will not do the right thing, since it will also pull in things on master that jgehrcke may have been based on and we don't want on the maintenance branch. You need to cherry-pick all of the commits involved. (I keep hoping for an easier way, but I haven't found one -- share it if you do). This is why, in general, it's easier to commit bugfixes to the maintenance branch (e.g. v1.2.x) and then merge everything to master after, since everything on the maintenance branch generally goes into master whereas the inverse is not always true.

dmcdougall · 2012-09-24T15:13:54Z

@mdboom Cheers!

dmcdougall · 2012-09-24T15:15:08Z

So one solution might be to rebase before merging. Cherry-picking sounds easier, though.

dmcdougall · 2012-09-24T15:31:58Z

Ok, I think that worked.

Update lib/mpl_toolkits/mplot3d/axes3d.py

c106b75

Axes3D.scatter: raise exception in case z.size neither being 1 nor equaling x.size and y.size (according to documentation).

pelson reviewed Sep 22, 2012
View reviewed changes

jgehrcke added 2 commits September 23, 2012 21:07

Update lib/mpl_toolkits/mplot3d/axes3d.py

a2fa253

Match argument names in error messages to argument names in documentation.

Update lib/mpl_toolkits/mplot3d/axes3d.py

29e556a

Make Axes3D.scatter properly deal with `zs` being of size 1.

dmcdougall reviewed Sep 23, 2012
View reviewed changes

dmcdougall added a commit that referenced this pull request Sep 24, 2012

Merge pull request #1294 from jgehrcke/patch-1

bdfa2cb

Update lib/mpl_toolkits/mplot3d/axes3d.py

dmcdougall merged commit bdfa2cb into matplotlib:master Sep 24, 2012

Uh oh!

Update lib/mpl_toolkits/mplot3d/axes3d.py #1294

Update lib/mpl_toolkits/mplot3d/axes3d.py #1294

Uh oh!

Conversation

jgehrcke commented Sep 21, 2012

Uh oh!

dmcdougall commented Sep 21, 2012

equaling x.size and y.size (according to documentation)

Uh oh!

dmcdougall commented Sep 21, 2012

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!

jgehrcke commented Sep 23, 2012

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!

dmcdougall commented Sep 24, 2012

Uh oh!

jgehrcke commented Sep 24, 2012

Uh oh!

dmcdougall commented Sep 24, 2012

Uh oh!

mdboom commented Sep 24, 2012

Uh oh!

dmcdougall commented Sep 24, 2012

Uh oh!

dmcdougall commented Sep 24, 2012

Uh oh!

dmcdougall commented Sep 24, 2012

Uh oh!

Uh oh!