-
Notifications
You must be signed in to change notification settings - Fork 3
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
Array API support - continuation #48
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need tests for everything here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just a few tweaks to bitwise ops, the scalar changes, and the one change in full
The PR is ready from my side - I added tests and addressed all comments. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #48 +/- ##
=======================================
Coverage ? 91.25%
=======================================
Files ? 7
Lines ? 709
Branches ? 0
=======================================
Hits ? 647
Misses ? 62
Partials ? 0 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just add a warning for the temporary reshape. Otherwise LGTM!
Hi @willow-ahrens @hameerabbasi Can we merge this PR? Do you have any other comments? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments to finish up, seems like the larger discussions are resolved.
Hi @hameerabbasi,
This PR add a couple of simple Array API functions:
Adds
__bool__
, etc. toTensor
class.It also introduces a few modifications to correctly run
array-api-test
suite.The tricky part is
reshape
which we don't have right now but it's required by most ofarray-api-tests
suites to even run them.Maybe one option could be another env var
TEST_MODE
that when set enables temporary implementation (that moves to numpy, reshapes, and moves back to Tensor). OtherwiseNotImplementedError
is raised. WDYT?