Skip to content
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

Fix sign() for torch and cupy #137

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

asmeurer
Copy link
Member

Neither propagate nans correctly, and torch does not support complex numbers.

Fixes #136

Neither propagate nans correctly, and torch does not support complex numbers.

Fixes data-apis#136
@asmeurer
Copy link
Member Author

#136 should be resolved before this is merged, specifically, we should decide if it's worth fixing the sign(nan) special case, and if we want to keep that special case at all. Regardless of that, though, we should keep the torch complex handling, as it's very straightforward to implement.

@asmeurer
Copy link
Member Author

It seems that torch has gained quite a few new test failures since the last time we ran them. I don't know if that's because of a test suite update or a torch update.

@asmeurer
Copy link
Member Author

So based on a simple timing test on PyTorch CPU, is 3-10x slower than torch.sign, depending on how many nans are in the tensor. Although sign itself is a fast operation to begin with. But it would definitely be better for this to be fixed upstream.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sign special case implementations
1 participant