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

Conjugate the first argument in vecdot #201

Merged
merged 2 commits into from
Nov 20, 2024
Merged

Conversation

asmeurer
Copy link
Member

Note that PyTorch uses its own vecdot directly for complex inputs,, which already does conjugation.

This is currently untested by the test suite (data-apis/array-api-tests#312)

Fixes #200.

Note that PyTorch uses its own vecdot directly for complex inputs,, which
already does conjugation.

This is currently untested by the test suite (data-apis/array-api-tests#312)

Fixes data-apis#200.
@asmeurer asmeurer requested a review from Copilot November 19, 2024 22:45

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no suggestions.

Comments skipped due to low confidence (1)

array_api_compat/common/_aliases.py:482

  • The new behavior of conjugating the first argument in vecdot should be covered by tests.
res = xp.conj(x1_[..., None, :]) @ x2_[..., None]
ev-br
ev-br previously requested changes Nov 20, 2024
Copy link
Contributor

@ev-br ev-br left a comment

Choose a reason for hiding this comment

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

The spec stance on rejecting real arguments to complex functions is a bit bizzare. I realize it's probably because xp.imag(real_array) would otherwise have to allocate an array of zeros but still.

array_api_compat/common/_aliases.py Show resolved Hide resolved
@asmeurer
Copy link
Member Author

FWIW, I agree with you that real arguments really should be accepted in conj. We agreed to fix this in a consortium meeting, but it hasn't been updated in the standard yet (and even then, then strict would only use the updated behavior when the version flag is set to 2024). See data-apis/array-api#824 (comment)

At any rate, this isn't a problem here. The reason is that the code in array_api_compat/common/_aliases.py only applies to the libraries numpy, cupy, and dask (and in a small number of cases, torch), all of which allow conj on real inputs.

This is something to be aware of generally. In a lot of the wrappers, we use specific implementation details either of torch or of numpy/cupy/dask when using xp, which are often not things allowed by the array API. It does mean that if we ever want to reuse the same wrapper for another library, we'd have to carefully check that it still works. But the wrappers themselves do not need to follow the array API (otherwise it would be circular, and we wouldn't get anywhere).

@ev-br ev-br dismissed their stale review November 20, 2024 21:30

resolved

@asmeurer
Copy link
Member Author

And also you don't really ever have to worry about array-api-strict in array-api-compat. The only place it's ever used is it can be returned from array_namespace.

@asmeurer
Copy link
Member Author

I'm going to merge this now, but I'm hopeful we can get testing for this in array-api-tests before we do a release.

@asmeurer asmeurer merged commit ee25aae into data-apis:main Nov 20, 2024
42 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.

BUG: vecdot does not conjugate first element
2 participants