API: Return scalars for scalar inputs to np.real/imag#8388
API: Return scalars for scalar inputs to np.real/imag#8388seberg merged 1 commit intonumpy:masterfrom gfyoung:real-imag-scalar
Conversation
|
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 |
|
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 |
|
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 |
|
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 |
|
Having a view can be useful as the view can be used as the value of an 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.... |
|
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.
|
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. |
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! |
numpy/lib/type_check.py
Outdated
There was a problem hiding this comment.
I STRONGLY beg to differ on this point. I just confirmed it myself. I would encourage you to check it out yourself to confirm.
There was a problem hiding this comment.
Ah, okay. No worries. 😄
numpy/lib/type_check.py
Outdated
There was a problem hiding this comment.
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?
numpy/lib/tests/test_type_check.py
Outdated
There was a problem hiding this comment.
This doesn't actually check that the scalar is returned - all of these tests would have passed before this patch, I think.
There was a problem hiding this comment.
Ah, that is true. Done.
|
@eric-wieser : Made the requested changes. Please have a look. |
eric-wieser
left a comment
There was a problem hiding this comment.
Generally looks good, just some minor nitpicks on doc comments - not sure if this needs a manual release note
numpy/lib/type_check.py
Outdated
There was a problem hiding this comment.
Nit-pick: "component of the ..."
numpy/lib/type_check.py
Outdated
numpy/lib/type_check.py
Outdated
There was a problem hiding this comment.
Not really float: complex128 -> float64, complex32 -> float32. But that was here before, so out of scope for this PR, I think
There was a problem hiding this comment.
Actually, the exact float type seems platform-dependent. On my machine, it actually returns float128 when I pass in complex128.
There was a problem hiding this comment.
That sounds like a bug if its true?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I am a little surprised that complex128 would return float64
Why? 64 bits of real + 64 bits of imaginary = 128 bits of complex
There was a problem hiding this comment.
Okay...but then why would I get float128 returned on my machine then?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Ah, okay. I should have clarified. For the scalar case, I get float128, but for the array case, I do indeed get float64.
There was a problem hiding this comment.
By "the scalar case", you presumably mean complex not np.complex128
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. |
numpy/lib/type_check.py
Outdated
There was a problem hiding this comment.
Just thinking about duck typing here... How about:
try:
return val.real
except AttributeError:
return asanyarray(val).realWhich has the nice bonus that builtin complex returns builtin float not np.float64
There was a problem hiding this comment.
Although now this is inconsistent with np.conj, which does not return a native complex...
What was the previous implementation?
|
Tests look good now, but I'm kinda tempted to duck-type this in the same way we duck-type |
|
@eric-wieser @seberg : Changes made as requested, and all is green. |
|
@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".
|
|
Well, this is subclassing hell.... 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 float64A possible fix would be to avoid arr = asanyarray(val)
return arr[()].real if isscalar(val) else arr.realOr 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 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 I haven't tracked down the ultimate source of the failure in scipy, but I suspect a use of |
|
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. |
Returns scalars for scalar inputs for the functions
np.realandnp.imag, as consistent with functions likenp.angleandnp.conj.Closes #8386.