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

BUG: Fix the implementation of numpy.array_api.vecdot #21928

Merged
merged 2 commits into from Sep 7, 2022

Conversation

@charris charris changed the title Fix the implementation of numpy.array_api.vecdot BUG: Fix the implementation of numpy.array_api.vecdot Jul 6, 2022
@seberg
Copy link
Member

seberg commented Jul 7, 2022

Seems OK, I think I slightly prefer the np.moveaxis() solution:

x1 = np.moveaxis(x1, axis, -1)
x2 = np.moveaxis(x2, axis, -1)

res = x1[..., None, :] @ x2[..., None]
return res[..., 0, 0]

Plus the wrapping/unwrapping stuff. Or use einsum("...i,...i->...", x1, x2).

@asmeurer
Copy link
Member Author

asmeurer commented Jul 7, 2022

OK, made it use moveaxis + matmul instead of einsum.

Comment on lines +382 to +384
ndim = max(x1.ndim, x2.ndim)
x1_shape = (1,)*(ndim - x1.ndim) + tuple(x1.shape)
x2_shape = (1,)*(ndim - x2.ndim) + tuple(x2.shape)
Copy link
Member

Choose a reason for hiding this comment

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

This will go away with the change.

Copy link
Member Author

@asmeurer asmeurer Jul 7, 2022

Choose a reason for hiding this comment

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

Maybe this can be simplified, but without this, the shape checking is not correct (because size 1 dimensions broadcast).

Also, the broadcasting is necessary because, assuming I am understanding the idea in the spec correctly, nonnegative axis should refer to the broadcasted dimensions.

Copy link
Member

Choose a reason for hiding this comment

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

You can move the checking to later and use -1 as axis. That probably gives you better errors for bad axis values anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm sorry, I'm not following your suggestion here.

Copy link
Member

Choose a reason for hiding this comment

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

I am saying that you can do the check after the broadcast+moveaxis at the point where the axis is properly normalized to -1.

Copy link
Member

Choose a reason for hiding this comment

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

I took the liberty to do that change, unless you disagree will merge next time around (or earlier with feedback).

@charris charris added the 09 - Backport-Candidate PRs tagged should be backported label Jul 8, 2022
@charris charris added this to the 1.23.2 release milestone Jul 8, 2022
x1_ = np.moveaxis(x1_, axis, -1)
x2_ = np.moveaxis(x2_, axis, -1)

if x1_shape[-1] != x2_shape[-1]:
Copy link
Member Author

Choose a reason for hiding this comment

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

You missed some dots here.

@asmeurer
Copy link
Member Author

asmeurer commented Aug 1, 2022

I guess the spec's a little ambiguous here, but my understanding is that vecdot should not broadcast along the specified axis. With my code, it does not (e.g., vecdot(empty((1, 3)), empty((2, 3)), axis=0) gives an error), but with your code it does. Not allowing broadcasting along the contracted dimension is inline with other functions like matmul.

We should probably figure out which behavior we want here and update the spec to be clearer.

@charris
Copy link
Member

charris commented Aug 3, 2022

Just to note that we ignore the lint errors in the array_api.

@seberg
Copy link
Member

seberg commented Aug 4, 2022

Yeah, I missed the boardcasting part, lets go with the previous version for now.

@charris charris added the triage review Issue/PR to be discussed at the next triage meeting label Sep 7, 2022
@seberg
Copy link
Member

seberg commented Sep 7, 2022

Thanks @asmeurer, sorry I had lost track of this, lets put it in.

@seberg seberg merged commit 0e960b9 into numpy:main Sep 7, 2022
@InessaPawson InessaPawson removed the triage review Issue/PR to be discussed at the next triage meeting label Sep 7, 2022
charris pushed a commit to charris/numpy that referenced this pull request Sep 7, 2022
@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Sep 7, 2022
@charris charris removed this from the 1.23.3 release milestone Sep 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants