API: Return scalars for scalar inputs to np.real/imag - #8388

Its not obvious that this is correct. doing this implies that putting in a 0-D array will not give you a view like the attribute does.

@seberg : True, but it is consistent with what these other numpy functions do. It would be preferable to choose one (return view) or the other (return scalar).

The other functions all return copies though.

Then in that case we should return scalars across the board, unless there is an advantageous reason why we should return a view for the 0-D array case that would override consistency.

It is a view for all other dimensions, I am just saying that it is an API change, and that I think the comparison to angle or conj is only half valid.

Fair enough, though I did acknowledge that this is an API change if you read the title of this PR.

Also, correct me if I'm wrong, but how many functions in numpy return array when a scalar is passed in? I think the majority behavior is to return a scalar.

Having a view can be useful as the view can be used as the value of an out parameter.

EDIT: Not clear to me that this should treat 0-D arrays as scalars.

@charris: I still return 0-D arrays when one is passed in, but when a scalar is passed in, I return scalar. That seems consistent with what we do with other functions.

@gfyoung you do? In that case, can you make it clear with a test pointing that out? 0-D arrays are rare, but still....

@seberg , @charris : Added test cases to confirm that 0-D array input gets 0-D array output. Ready to merge if there are no other concerns.

Nope, sorry, but me asking for a test was a trick question really.

What type of comment is this? Not really sure why you're trying to be "clever".

Also, you should look at the PR again before making comments like this. I wrote the test case out, and the behavior does return 0-D array upon 0-D input as requested.

Was a bit evil, but anyway... You did not test for array return, because scalars are equal to arrays in those tests.

You did not test for array return, because scalars are equal to arrays in those tests.

Now that's a more useful comment: straightforward and to the point. I've updated the tests to check.

Please refrain from your "trickery" in subsequent comments. Thanks!

@charris , @seberg : Updated test to confirm that 0-D array is returned when we pass in 0-D array, and everything is still passing.

@charris , @seberg : Any updates on this?

@eric-wieser : Made the requested changes. Please have a look.

not sure if this needs a manual release note

IMO probably not This change is relatively minor compared to those that would find themselves in the release notes.

@eric-wieser @seberg : Changes made as requested, and all is green.

Tests look good now, but I'm kinda tempted to duck-type this in the same way we duck-type np.min and np.ndim(...) in fromnumeric

@eric-wieser @seberg : Changes made as requested, and all is green.

Not green for me, but looks good. @seberg, do you agree with the duck-typing I suggested above that @gfyoung has now implemented?

@seberg : Any updates on this? I think @eric-wieser is waiting for your response before merging.

@eric-wieser : @seberg seems to not be responding (either because the message wasn't seen or this is tacit approval). Ideas of how to move forward? This change seems relatively trivial.

It looks good to me now, sorry if I am slow but don't have much brain cycles for numpy right now, and frankly the first implementations looked too tricky to me to just approve off. Even here I would not mind mentioning it in the release notes, but probably it is not necessary.

Ugh, I've just realized that this fails to meet the goal of "Returns scalars for scalar inputs as consistent with functions like np.angle and np.conj".

type(np.angle(1+1j)) == np.float64, but type(np.real(1+1j)) == float

Well, this is subclassing hell.... np.angle I don't understand why that is a goal, there is no angle method after all. np.conjugate could indeed be set up to go via the method in principle, since it is pretty much a well defined method within python. This "function that is basically supposed to be an operator" thing is really annoying.

Overall, I think the approach of try/except is consistent with what we do on most methods at least, so I don't mind the change, and doubt right now it creates much trouble. @mhvk you are a subclass guy, maybe you can give your opinion/final decision on whether this is an improvement or not?

What we had in an earlier iteration actually fails in exactly the same way:

arr = asanyarray(val)
return arr.real.item() if isscalar(val) else arr.real
#               ^ item returns float, not float64

A possible fix would be to avoid .item():

arr = asanyarray(val)
return arr[()].real if isscalar(val) else arr.real

Or possibly

def asanyarray_or_scalar(val):
    arr = asanyarray(val)
    if isscalar(val):
        return arr[()]
    else:
        return arr

def real(val):
    return asanyarray_or_scalar(val).real

Not sure I like that, because it does not return views. Though, I admit, if it creates problems in code, it should at least not be silent problems, since the scalars are immutable.

@seberg: That comment got edited way too much, you should probably check it still says what you were replying to

Yeah, well... we just have to be careful we don't simply replace one problem with another.

I agree. I think the above code now does return a view, if passed a viewable object?

I had a look and I think this is the right implementation. I don't think it makes all that much sense to compare with conj and angle, as both actually do an operation, while real and imag just select a particular part of an operand. (Indeed, angle could use some work - why, o why, this need to asarray all input...).

So, I think this should just go in.

Isn't it a great convenience ;). Ok, then lets just put it in and hopefully we are not missing anything that might get broken here.

Needs a release note...

@seberg , @eric-wieser , @mhvk : Thanks for all of the input!

@charris : Acknowledged. Will put up PR as follow-up.

Take a look at scipy/scipy#7227

Note, in particular, the quirk of numpy.testing.assert_equal() that I commented on there (using numpy 1.12.1):

In [10]: from numpy.testing import assert_equal

In [11]: assert_equal(np.array(-0.0), 0)

In [12]: assert_equal(-0.0, 0)
---------------------------------------------------------------------------
AssertionError                            Traceback (most recent call last)
<ipython-input-12-8315da2bce84> in <module>()
----> 1 assert_equal(-0.0, 0)

/Users/warren/miniconda3scipy/lib/python3.5/site-packages/numpy/testing/utils.py in assert_equal(actual, desired, err_msg, verbose)
    410         elif desired == 0 and actual == 0:
    411             if not signbit(desired) == signbit(actual):
--> 412                 raise AssertionError(msg)
    413     # If TypeError or ValueError raised while using isnan and co, just handle
    414     # as before

AssertionError: 
Items are not equal:
 ACTUAL: -0.0
 DESIRED: 0

I haven't tracked down the ultimate source of the failure in scipy, but I suspect a use of assert_equal that previously passed is now failing because something that was an array containing -0.0 is now the scalar -0.0.

I've narrowed down the problem to the issue reported in #8848, so further discussion should be over there.

@WarrenWeckesser : Still, yikes...what you indicated above is a bug it seems.