Conversation
|
I don't think this is correct. And this is by design: see "Avoid Restricting Behavior that is Outside the Scope of the Standard" section in |
|
Ah, ok. |
crusaderky
changed the title
clip() should not have out= parameterclip(out=...) is broken
| out = wrapped_xp.asarray(xp.broadcast_to(x, result_shape), | ||
| copy=True, device=device(x)) | ||
| out = wrapped_xp.empty(result_shape, dtype=x.dtype, device=dev) | ||
| out[()] = x |
There was a problem hiding this comment.
When out is None I expect this to have exactly the same performance as before
| a = xp.broadcast_to(a, result_shape) | ||
| ia = (out < a) | xp.isnan(a) | ||
| # torch requires an explicit cast here | ||
| out[ia] = wrapped_xp.astype(a[ia], out.dtype) |
There was a problem hiding this comment.
Removed unnecessary deep copy
| a = wrapped_xp.asarray(min, dtype=x.dtype, device=dev) | ||
| a = xp.broadcast_to(a, result_shape) | ||
| ia = (out < a) | xp.isnan(a) | ||
| # torch requires an explicit cast here |
There was a problem hiding this comment.
Could not reproduce
This reverts commit 94be1c8.
|
Okay, the added test looks convincing, the CI gives a green light, the patch looks like a nice fix and a simplification. Let's roll with it. Thanks @crusaderky |
reviewed at data-apis#261
2 participants
Fix issue where clip() would accept an out= parameter in numpy (wrapped or not), cupy(wrapped or not), and wrapped torch, whereas in all other namespaces it would (correctly) reject it.Fix non-standard out= parameter for clip and add unit test.
Also speed up all cases by removing one unnecessary deep copy.
I checked scipy and there are no use cases for this.