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

gfyoung · 2016-12-17T09:07:14Z

Returns scalars for scalar inputs for the functions np.real and np.imag, as consistent with functions like np.angle and np.conj.

Closes #8386.

seberg · 2016-12-18T16:12:40Z

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.

gfyoung · 2016-12-18T17:45:28Z

@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).

seberg · 2016-12-18T17:51:53Z

The other functions all return copies though.

gfyoung · 2016-12-18T19:17:50Z

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.

seberg · 2016-12-18T20:35:20Z

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.

gfyoung · 2016-12-18T20:36:03Z

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.

charris · 2016-12-29T04:08:53Z

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.

gfyoung · 2016-12-29T04:25:39Z

@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.

seberg · 2016-12-29T12:36:47Z

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

gfyoung · 2016-12-29T19:16:52Z

@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.

seberg · 2016-12-29T19:45:22Z

Nope, sorry, but me asking for a test was a trick question really. Because you do not preserve 0-D arrays to return 0-D arrays (or views) as it is. Not saying I know this can't be OK anyway, but I have my doubts.

gfyoung · 2016-12-29T20:34:58Z

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.

seberg · 2016-12-29T20:39:46Z

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

gfyoung · 2016-12-29T20:48:24Z

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!

gfyoung · 2016-12-29T23:04:28Z

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

seberg · 2016-12-30T09:57:31Z

numpy/lib/type_check.py


    """
-    return asanyarray(val).real
+    arr = asanyarray(val)
+    return arr.real.item() if isscalar(val) else arr.real


Now it never returns scalars.

I STRONGLY beg to differ on this point. I just confirmed it myself. I would encourage you to check it out yourself to confirm.

oops, misread.

Ah, okay. No worries. 😄

gfyoung · 2017-01-11T08:03:59Z

@charris , @seberg : Any updates on this?

eric-wieser · 2017-02-19T15:28:26Z

numpy/lib/type_check.py

-from numpy.core.numeric import asarray, asanyarray, array, isnan, \
-                obj2sctype, zeros
+from numpy.core.numeric import (asarray, asanyarray, array, isnan,
+                                obj2sctype, zeros, isscalar)


Given that we imported _nx on the line above, and much of this file uses it already, is there any real point importing isscalar here?

Fair enough. Done.

eric-wieser · 2017-02-19T15:31:52Z

numpy/lib/tests/test_type_check.py

+        assert_(isinstance(out, np.ndarray))
+
+        y = 1
+        assert_equal(y, np.real(y))


This doesn't actually check that the scalar is returned - all of these tests would have passed before this patch, I think.

Ah, that is true. Done.

gfyoung · 2017-02-20T05:55:00Z

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

eric-wieser

Generally looks good, just some minor nitpicks on doc comments - not sure if this needs a manual release note

eric-wieser · 2017-02-20T09:59:18Z

numpy/lib/type_check.py

-        Output array. If `val` is real, the type of `val` is used for the
-        output.  If `val` has complex elements, the returned type is float.
+    out : ndarray or scalar
+        The real component of a complex argument. If `val` is real, the type


Nit-pick: "component of the ..."

eric-wieser · 2017-02-20T10:01:06Z

numpy/lib/type_check.py

-        Output array. If `val` is real, the type of `val` is used for the
-        output.  If `val` has complex elements, the returned type is float.
+    out : ndarray or scalar
+        The imaginary component of a complex argument. If `val` is real,


(same here)

eric-wieser · 2017-02-20T10:01:36Z

numpy/lib/type_check.py

+    out : ndarray or scalar
+        The imaginary component of a complex argument. If `val` is real,
+        the type of `val` is used for the output.  If `val` has complex
+        elements, the returned type is float.


Not really float: complex128 -> float64, complex32 -> float32. But that was here before, so out of scope for this PR, I think

Actually, the exact float type seems platform-dependent. On my machine, it actually returns float128 when I pass in complex128.

That sounds like a bug if its true?

Can't tell at this point, though @eric-wieser I am a little surprised that complex128 would return float64 if the architecture is capable of supporting complex128.

I am a little surprised that complex128 would return float64

Why? 64 bits of real + 64 bits of imaginary = 128 bits of complex

Okay...but then why would I get float128 returned on my machine then?

Since the result is a view (unless it happens to be scalar), it frankly seems pretty improbable to me at least for the array case. Can you double check? And if, what system exactly?

Ah, okay. I should have clarified. For the scalar case, I get float128, but for the array case, I do indeed get float64.

By "the scalar case", you presumably mean complex not np.complex128

@eric-wieser : That is correct.

gfyoung · 2017-02-20T10:19:21Z

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.

gfyoung · 2017-02-21T08:51:32Z

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

eric-wieser · 2017-02-21T11:45:49Z

numpy/lib/type_check.py


    """
-    return asanyarray(val).real
+    arr = asanyarray(val)
+    return arr.real.item() if _nx.isscalar(val) else arr.real


Just thinking about duck typing here... How about:

try: return val.real except AttributeError: return asanyarray(val).real

Which has the nice bonus that builtin complex returns builtin float not np.float64

Good point. Done.

Although now this is inconsistent with np.conj, which does not return a native complex...

What was the previous implementation?

eric-wieser · 2017-02-21T11:47:39Z

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

gfyoung · 2017-02-21T19:33:42Z

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

eric-wieser · 2017-02-21T19:41:43Z

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

gfyoung · 2017-03-08T17:56:15Z

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

gfyoung · 2017-03-25T11:23:05Z

@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.

Closes gh-8386.

seberg · 2017-03-26T12:33:09Z

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.

eric-wieser · 2017-03-26T12:47:32Z

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

seberg · 2017-03-26T12:53:38Z

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?

eric-wieser · 2017-03-26T13:07:37Z

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

seberg · 2017-03-26T13:11:04Z

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.

eric-wieser · 2017-03-26T13:11:53Z

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

seberg · 2017-03-26T13:14:21Z

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

eric-wieser · 2017-03-26T13:15:07Z

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

mhvk · 2017-03-26T17:53:49Z

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.

seberg · 2017-03-26T18:11:37Z

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.

charris · 2017-03-26T19:46:10Z

Needs a release note...

gfyoung · 2017-03-26T20:01:56Z

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

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

WarrenWeckesser · 2017-03-27T00:41:29Z

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.

WarrenWeckesser · 2017-03-27T01:15:29Z

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

gfyoung · 2017-03-27T07:40:56Z

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

charris added the component: numpy.lib label Dec 29, 2016

seberg reviewed Dec 30, 2016

View reviewed changes

eric-wieser requested changes Feb 19, 2017

View reviewed changes

eric-wieser approved these changes Feb 20, 2017

View reviewed changes

eric-wieser reviewed Feb 21, 2017

View reviewed changes

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

d268966

Closes gh-8386.

seberg merged commit 82e0860 into numpy:master Mar 26, 2017

gfyoung deleted the real-imag-scalar branch March 26, 2017 20:01

gfyoung mentioned this pull request Mar 26, 2017

DOC: Add release note about np.real and np.conj #8845

Merged

WarrenWeckesser mentioned this pull request Mar 27, 2017

special.wrightomega test failure using numpy's master branch scipy/scipy#7227

Closed

Uh oh!

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

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

Uh oh!

Conversation

gfyoung commented Dec 17, 2016

Uh oh!

seberg commented Dec 18, 2016

Uh oh!

gfyoung commented Dec 18, 2016

Uh oh!

seberg commented Dec 18, 2016

Uh oh!

gfyoung commented Dec 18, 2016

Uh oh!

seberg commented Dec 18, 2016

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

seberg commented Dec 29, 2016

Uh oh!

gfyoung commented Dec 29, 2016

Uh oh!

seberg commented Dec 29, 2016 via email

Uh oh!

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

Uh oh!

Uh oh!

seberg commented Dec 29, 2016

Uh oh!

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

Uh oh!

Uh oh!

gfyoung commented Dec 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gfyoung Dec 30, 2016 • 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!

Choose a reason for hiding this comment

Uh oh!

gfyoung commented Jan 11, 2017

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!

gfyoung commented Feb 20, 2017

Uh oh!

eric-wieser 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-wieser Feb 20, 2017 • 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!

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

gfyoung commented Dec 18, 2016 •

edited

Loading

charris commented Dec 29, 2016 •

edited

Loading

gfyoung commented Dec 29, 2016 •

edited

Loading

gfyoung commented Dec 29, 2016 •

edited

Loading

gfyoung commented Dec 29, 2016 •

edited

Loading

gfyoung Dec 30, 2016 •

edited

Loading

eric-wieser Feb 20, 2017 •

edited

Loading

gfyoung Feb 20, 2017 •

edited

Loading

gfyoung Feb 20, 2017 •

edited

Loading

gfyoung commented Feb 20, 2017 •

edited

Loading

eric-wieser Feb 21, 2017 •

edited

Loading

eric-wieser commented Feb 21, 2017 •

edited

Loading

gfyoung commented Mar 25, 2017 •

edited

Loading