Skip to content

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

Merged
seberg merged 2 commits intonumpy:mainfrom
asmeurer:fix-array-api-vecdot
Sep 7, 2022
Merged

BUG: Fix the implementation of numpy.array_api.vecdot#21928
seberg merged 2 commits intonumpy:mainfrom
asmeurer:fix-array-api-vecdot

Conversation

@asmeurer
Copy link
Copy Markdown
Member

@asmeurer asmeurer commented Jul 5, 2022

@asmeurer asmeurer mentioned this pull request Jul 5, 2022
Merged
@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
Copy Markdown
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
Copy Markdown
Member Author

asmeurer commented Jul 7, 2022

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

seberg
seberg reviewed Jul 7, 2022
View reviewed changes
numpy/array_api/linalg.py
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
Copy Markdown
Member

@seberg seberg Jul 7, 2022

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
Copy Markdown
Member Author

@asmeurer asmeurer Jul 7, 2022 •
edited
Loading

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
Copy Markdown
Member

@seberg seberg Jul 7, 2022

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
Copy Markdown
Member Author

@asmeurer asmeurer Jul 8, 2022

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
Copy Markdown
Member

@seberg seberg Jul 8, 2022

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
Copy Markdown
Member

@seberg seberg Aug 1, 2022

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
asmeurer
asmeurer commented Aug 1, 2022
View reviewed changes
numpy/array_api/linalg.py Outdated
x1_ = np.moveaxis(x1_, axis, -1)
x2_ = np.moveaxis(x2_, axis, -1)

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

@asmeurer asmeurer Aug 1, 2022

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
Copy Markdown
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
Copy Markdown
Member

charris commented Aug 3, 2022

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

@seberg seberg force-pushed the fix-array-api-vecdot branch from 9d61d5e to 285084a Compare August 4, 2022 02:04
@seberg
Copy link
Copy Markdown
Member

seberg commented Aug 4, 2022

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

@asmeurer asmeurer mentioned this pull request Aug 4, 2022
Closed
@charris charris added the triage review Issue/PR to be discussed at the next triage meeting label Sep 7, 2022
@seberg
Copy link
Copy Markdown
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
@asmeurer @charris
BUG: Fix the implementation of numpy.array_api.vecdot (numpy#21928)
6cc4183 
* Fix the implementation of numpy.array_api.vecdot

See https://data-apis.org/array-api/latest/API_specification/generated/signatures.linear_algebra_functions.vecdot.html

* Use moveaxis + matmul instead of einsum in vecdot
@charris charris mentioned this pull request Sep 7, 2022
Merged
@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
@AnirudhDagar AnirudhDagar mentioned this pull request Dec 15, 2022
Closed
@leofang leofang mentioned this pull request Jan 16, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@seberg seberg seberg left review comments

Assignees

No one assigned

Labels

00 - Bug component: numpy.array_api

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@asmeurer @seberg @charris @InessaPawson