-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sign
special case implementations
#136
Comments
array_api_compat
sign
special case implementationssign
special case implementations
I haven't tried to do any special-cases workarounds here yet. I guess if this is causing issues for you we can add a workaround. |
We definitely should add complex support to torch.sign. It sounds like that will just be an easy wrapper of torch.sgn. |
Yeah I actually did run into this special case while converting code from NumPy to array API. The purpose of the code is to produce a different status flag that depends on whether an element is positive, negative, zero, or NaN, so no wonder I ran into it. So it would be helpful to add the special case, but support for 2023.12 would be higher priority to me. |
We can add it. The main concern with adding special-case handling is it means adding a mask to the function, so it could be a minor hit to performance. I would at least make sure there are upstream issues about this to the libraries that don't implement it correctly. |
I think that won't be a minor hit. For element-wise functions, adding In general, these special cases have not been well validated yet, so I'd be quite reluctant to assume they are all correctly specified or support them in the compat layer. In this case, it may be possible to fix in NumPy. There's a bunch of special-casing to make Changing it would be a minor backwards-compat break, but probably an improvement. Much larger changes were made to |
NumPy is the one that agrees with what the array API says. If we think sign(nan) should not return NaN, then the array API should change. |
I'm honestly not sure. These special cases are a pain, it requires a lot of time investment to figure out what was discussed before, and why different libraries end up with different return values. I suspect the following:
|
I think one normal rule of NaNs is that they are propagated unless there is a clear reason why not. Further, returning NaN preserves the full "partial order" (e.g. in C++20) of So IMO, NumPy does the right thing unless there is a good argument why typical use-cases would expect a TBH, I would lean towards torch should fix this, but if there is a good reason why they don't want to (what is it?), then it has to stay undefined which seems unfortunate for the actual use-case above. |
I found this old discussion https://mail.python.org/archives/list/numpy-discussion@python.org/thread/A2JFHOZZOF634CNZ7E27THQEBU4EZFTS/#F3KS7QWVPIXSYB7CSSY37OXYM4JVZTZQ
It does seem valuable to figure this out, since there's at least one real-world use-case. Maybe we should move this discussion to the array-api repo. |
I have a fix at #137, which we can at least use to check the performance implications. |
By the way, you can see the other special cases that are not being followed in the xfails files (we do not yet attempt to fix any of them, and most are on operators which can't be fixed anyways): torch array-api-compat/torch-xfails.txt Lines 89 to 179 in 376038e
cupy array-api-compat/cupy-xfails.txt Lines 61 to 169 in 376038e
numpy array-api-compat/numpy-xfails.txt Lines 18 to 41 in 376038e
I will say that even though there are quite a few of these, the |
@rgommers The addition of the special case for The specification is correct on this, as returning and where This was not a pre-C99 oversight and was intentional. We shouldn't expect the |
Since its maybe interesting. Turns out getting NaNs may make this faster on the GPU: cupy/cupy#8327 |
Just thought I'd mention that I ran into this again in a different context. |
According to v2022.12 (and v2023.12) of the array API standard, the special cases of
sign
include:However,
There may be other special cases that are not yet implemented. I haven't done a complete review, but I noticed that
torch
gives an error when the input is complex.The text was updated successfully, but these errors were encountered: