BUG: Fix double-wrapping of object scalars #8643

eric-wieser · 2017-02-19T14:07:34Z

eric-wieser · 2017-02-23T13:00:53Z

(since you reviewed and merged #8441)

shoyer · 2017-02-23T17:26:34Z

numpy/lib/shape_base.py

@@ -134,9 +134,9 @@ def apply_along_axis(func1d, axis, arr, *args, **kwargs):
        buff = res.__array_prepare__(buff)

    # save the first result, then compute and save all remaining results
-    buff[ind0] = res
+    buff[ind0] = res[()]


I didn't realize that indexing like this was valid on non-scalar arrays. This needs a comment, or better yet, maybe a helper function. Something like asanyarray_or_scalar? This is related to #5353, though in this case we want fallback behavior.

Alternatively, maybe we should change indexing behavior for cases like this in general? I don't see any situation in which a user would want an object array of scalar object arrays.

Sure is possible, though you must be sure you have an array. Otherwise, object dtype might blow up in your face.

I didn't realize that indexing like this was valid on non-scalar arrays

Yep, in these cases it's just a no-op, as if it were :, but repeated 0 times.

I'll add the comment

Perhaps asanyarray(or_scalar=True)?

Ok, comment added. I think that a helper function would be more widely applicable, so belongs in a separate MAINT PR. I've created #8681 to discuss what it should be named

ahaldane · 2017-02-28T17:06:57Z

Tangent idea:

Maybe we should define a new helper method to convert 0d arrays to scalars. In C we have PyArray_Return to do this, which is used all over the place, demonstrating its usefulness.

I think the current workaround in python using empty tuples is a bit opaque, and there are weird edge cases involving it. See #7267, or this comment

eric-wieser · 2017-02-28T17:43:04Z

This isn't quite the same problem as #7267, as here we know we have a 0d array and not a scalar. And that comment you link to is no longer applicable

ahaldane · 2017-02-28T17:47:59Z

I agree #7267 is not a problem here, I should have said so more clearly.

I didn't realize that comment was no longer applicable... I guess empty tuple behavior changed pretty recently...

eric-wieser · 2017-02-28T17:51:22Z

@ahaldane : Perhaps a regression in 1.12? I'm testing on 1.10.4, 1.11.1, and master, and all are fine

ahaldane · 2017-02-28T18:15:50Z

Hmm maybe that comment was just wrong, I also don't notice any changes going back to 1.9.3.

eric-wieser · 2017-03-06T15:01:23Z

numpy/lib/shape_base.py

    for ind in inds:
-        buff[ind] = asanyarray(func1d(inarr_view[ind], *args, **kwargs))
+        buff[ind] = asanyarray(func1d(inarr_view[ind], *args, **kwargs))[()]


This could also have been fixed by changing this line to buff[ind + np.index_exp[...]], and leaving the rest untouched. Probably better for not going through a scalar, and avoiding #7267?

Yeah, this is definitely the right approach, in light of #8902. Updating...

Fixes numpy#8642

eric-wieser · 2017-04-06T19:14:04Z

Adding this to the milestone, since it would be a shame to introduce a feature already broken

mhvk · 2017-04-06T19:31:27Z

I wondered slightly about adding (slice(None),) instead of (Ellipsis,) but really it does not matter (and this way you may be ready for extening this to apply over multiple axis at the same time...

So, good to go as far as I'm concerned.

eric-wieser · 2017-04-06T19:37:47Z

(slice(None),) would error when the operation is a reduction, as 3darray[1, 2, 3, :] is not valid, but 3darray[1, 2, 3, ...] is

Remember that these indices are used for input and output

mhvk · 2017-04-06T19:40:26Z

Duh, should have seen that.

eric-wieser · 2017-04-11T13:19:27Z

@shoyer, happy with the new implementation?

Digging a little deeper, the rules for object array nesting seem to be:

obj_array_0d = np.array(None)
obj_array = np.array([[1, 2], [3, 4]], object)
ind_producing_scalar = (0, 0)
ind_producing_0d = (0, 0, Ellipsis)

# nests object arrays
obj_array[ind_producing_scalar] = obj_array_0d

# does not nest object arrays
obj_array[ind_producing_0d] = obj_array_0d

I think these rules make sense, as they allow you to choose between nesting arrays and not doing so, although perhaps need more documentation elsewhere (but that's well out of scope for this PR).

Worth noting that obj_array[ind_producing_0d] is broken on masked arrays (#8684), but I've a fix for that in #8905, and we don't care about that in np.apply_along_axis till #8511 anyway.

shoyer

This looks great to me. @eric-wieser please go ahead and merge!

eric-wieser added 00 - Bug component: numpy.lib labels Feb 19, 2017

eric-wieser requested a review from shoyer February 23, 2017 13:00

shoyer reviewed Feb 23, 2017

View reviewed changes

eric-wieser force-pushed the fix-8642 branch from 7b6d2f7 to 31d00e3 Compare February 23, 2017 23:59

eric-wieser commented Mar 6, 2017

View reviewed changes

eric-wieser force-pushed the fix-8642 branch 2 times, most recently from eb73409 to b3d7e69 Compare April 6, 2017 18:05

BUG: Fix double-wrapping of object scalars

fa56437

Fixes numpy#8642

eric-wieser force-pushed the fix-8642 branch from b3d7e69 to fa56437 Compare April 6, 2017 18:06

eric-wieser added this to the 1.13.0 release milestone Apr 6, 2017

shoyer approved these changes Apr 11, 2017

View reviewed changes

eric-wieser merged commit f368286 into numpy:master Apr 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Fix double-wrapping of object scalars #8643

BUG: Fix double-wrapping of object scalars #8643

eric-wieser commented Feb 19, 2017

eric-wieser commented Feb 23, 2017 •

edited

Loading

shoyer Feb 23, 2017

seberg Feb 23, 2017

eric-wieser Feb 23, 2017 •

edited

Loading

eric-wieser Feb 23, 2017

eric-wieser Feb 24, 2017

ahaldane commented Feb 28, 2017

eric-wieser commented Feb 28, 2017 •

edited

Loading

ahaldane commented Feb 28, 2017

eric-wieser commented Feb 28, 2017 •

edited

Loading

ahaldane commented Feb 28, 2017

eric-wieser Mar 6, 2017 •

edited

Loading

eric-wieser Apr 6, 2017

eric-wieser commented Apr 6, 2017

mhvk commented Apr 6, 2017

eric-wieser commented Apr 6, 2017 •

edited

Loading

mhvk commented Apr 6, 2017

eric-wieser commented Apr 11, 2017 •

edited

Loading

shoyer left a comment

BUG: Fix double-wrapping of object scalars #8643

BUG: Fix double-wrapping of object scalars #8643

Conversation

eric-wieser commented Feb 19, 2017

eric-wieser commented Feb 23, 2017 • edited Loading

shoyer Feb 23, 2017

Choose a reason for hiding this comment

seberg Feb 23, 2017

Choose a reason for hiding this comment

eric-wieser Feb 23, 2017 • edited Loading

Choose a reason for hiding this comment

eric-wieser Feb 23, 2017

Choose a reason for hiding this comment

eric-wieser Feb 24, 2017

Choose a reason for hiding this comment

ahaldane commented Feb 28, 2017

eric-wieser commented Feb 28, 2017 • edited Loading

ahaldane commented Feb 28, 2017

eric-wieser commented Feb 28, 2017 • edited Loading

ahaldane commented Feb 28, 2017

eric-wieser Mar 6, 2017 • edited Loading

Choose a reason for hiding this comment

eric-wieser Apr 6, 2017

Choose a reason for hiding this comment

eric-wieser commented Apr 6, 2017

mhvk commented Apr 6, 2017

eric-wieser commented Apr 6, 2017 • edited Loading

mhvk commented Apr 6, 2017

eric-wieser commented Apr 11, 2017 • edited Loading

shoyer left a comment

Choose a reason for hiding this comment

eric-wieser commented Feb 23, 2017 •

edited

Loading

eric-wieser Feb 23, 2017 •

edited

Loading

eric-wieser commented Feb 28, 2017 •

edited

Loading

eric-wieser commented Feb 28, 2017 •

edited

Loading

eric-wieser Mar 6, 2017 •

edited

Loading

eric-wieser commented Apr 6, 2017 •

edited

Loading

eric-wieser commented Apr 11, 2017 •

edited

Loading