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

Extend our type signatures of inner and dot to match NumPy's types. #285

Merged
merged 3 commits into from
Mar 22, 2022

Conversation

adamreichold
Copy link
Member

Closes #284

@adamreichold adamreichold requested a review from kngwyu March 5, 2022 07:59
Copy link
Member

@kngwyu kngwyu left a comment

Choose a reason for hiding this comment

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

Oops, I didn't notice that this can cause an error, thanks.
Could you please some test cases that extract floats/integers also in (non-doc) tests?

Also, can we do the same for einsum?

@adamreichold
Copy link
Member Author

Oops, I didn't notice that this can cause an error, thanks.

Yeah, I think the effective signature of the capsule API is quite unfortunate considering a zero-dimensional array can be used almost exactly like a scalar in Python code.

Could you please some test cases that extract floats/integers also in (non-doc) tests?

Added more tests for inner and dot as a separate commit.

Also, can we do the same for einsum?

Indeed this implies here too depending on the summation performed. Added as a separate commit.

@adamreichold
Copy link
Member Author

As this break the API anyway, I took the liberty of renaming the einsum_impl function to einsum. There is no danger of conflicts as macros and functions inhabit distinct namespaces and most people will invoke the macro anyway. But I think the _impl suffix makes the name look unnecessary "internal."

@adamreichold
Copy link
Member Author

Slipped in a further change to make use of PyArray::item to basically game our coverage up... |-\

@adamreichold
Copy link
Member Author

@kngwyu Are you satisfied with the adjustments?

@adamreichold adamreichold force-pushed the fix-inner-dot-out branch 2 times, most recently from af505de to 9bcce43 Compare March 19, 2022 14:01
Copy link
Member

@kngwyu kngwyu left a comment

Choose a reason for hiding this comment

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

Sorry for being late. Thank you for addressing my requests!

@kngwyu kngwyu merged commit 7a5e011 into main Mar 22, 2022
@adamreichold adamreichold deleted the fix-inner-dot-out branch March 22, 2022 06:53
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.

Type signature of numpy::innner does not cover all cases
3 participants