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

Use conj_physical for torch.conj #174

Merged
merged 2 commits into from
Aug 7, 2024
Merged

Conversation

asmeurer
Copy link
Member

@asmeurer asmeurer commented Aug 6, 2024

torch.conj sets the conjugation bit, which breaks other libraries.

Fixes #173.

torch.conj sets the conjugation bit, which breaks other libraries.

Fixes data-apis#173.
Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good modulo the failure. The more principled fix would be to call torch.resolve_conj only in the interop functions between libraries, however there is no way to wrap __dlpack__ & co, so in practice this isn't doable from outside PyTorch.

The performance loss will be quite limited and specific to a few operations that PyTorch would otherwise be able to keep lazy, and isn't too much of a concern here I'd say.

@@ -145,6 +145,9 @@ def can_cast(from_: Union[Dtype, array], to: Dtype, /) -> bool:
# Basic renames
bitwise_invert = torch.bitwise_not
newaxis = None
# torch.conj sets the conjugation bit, which breaks conversion to other
# libraries. See https://github.com/data-apis/array-api-compat/issues/173
conj = torch.conj_physical
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fails a test for function names. It probably requires def conj(..):` instead? Either that, or special-casing in the test.

@asmeurer asmeurer enabled auto-merge August 7, 2024 18:50
@asmeurer asmeurer merged commit 819842b into data-apis:main Aug 7, 2024
43 checks passed
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.

conj should delegate to torch.conj_physical
2 participants