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

Array API support - continuation #48

Merged
merged 1 commit into from
May 17, 2024
Merged

Array API support - continuation #48

merged 1 commit into from
May 17, 2024

Conversation

mtsokol
Copy link
Collaborator

@mtsokol mtsokol commented May 13, 2024

Hi @hameerabbasi,

This PR add a couple of simple Array API functions:

    "finfo",  # ported from numpy
    "iinfo",  # ported from numpy
    "isnan",  # the rest is just element-wise functions
    "isinf",
    "isfinite",
    "equal",
    "not_equal",
    "less",
    "less_equal",
    "greater",
    "greater_equal",
    "square",
    "logaddexp",
    "logical_and",
    "logical_or",
    "logical_xor",
    "trunc",

Adds __bool__, etc. to Tensor 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 of array-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). Otherwise NotImplementedError is raised. WDYT?

@mtsokol mtsokol added the enhancement New feature or request label May 13, 2024
@mtsokol mtsokol requested a review from hameerabbasi May 13, 2024 16:31
@mtsokol mtsokol self-assigned this May 13, 2024
Copy link
Collaborator

@hameerabbasi hameerabbasi left a 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.

src/finch/tensor.py Outdated Show resolved Hide resolved
src/finch/tensor.py Outdated Show resolved Hide resolved
src/finch/tensor.py Show resolved Hide resolved
src/finch/__init__.py Outdated Show resolved Hide resolved
Copy link
Owner

@willow-ahrens willow-ahrens left a 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

src/finch/tensor.py Outdated Show resolved Hide resolved
src/finch/tensor.py Outdated Show resolved Hide resolved
src/finch/tensor.py Outdated Show resolved Hide resolved
@mtsokol
Copy link
Collaborator Author

mtsokol commented May 15, 2024

The PR is ready from my side - I added tests and addressed all comments.

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 87.30159% with 8 lines in your changes are missing coverage. Please review.

❗ No coverage uploaded for pull request base (main@8342c52). Click here to learn what that means.

Files Patch % Lines
src/finch/tensor.py 89.47% 6 Missing ⚠️
src/finch/dtypes.py 60.00% 2 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

Copy link
Owner

@willow-ahrens willow-ahrens left a 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!

src/finch/tensor.py Show resolved Hide resolved
src/finch/tensor.py Show resolved Hide resolved
@mtsokol
Copy link
Collaborator Author

mtsokol commented May 16, 2024

Hi @willow-ahrens @hameerabbasi Can we merge this PR? Do you have any other comments?

Copy link
Owner

@willow-ahrens willow-ahrens left a comment

Choose a reason for hiding this comment

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

Lgtm!

Copy link
Collaborator

@hameerabbasi hameerabbasi left a 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.

src/finch/dtypes.py Show resolved Hide resolved
src/finch/tensor.py Show resolved Hide resolved
src/finch/tensor.py Show resolved Hide resolved
@mtsokol mtsokol merged commit 4950921 into main May 17, 2024
5 checks passed
@mtsokol mtsokol deleted the array-api-cont branch May 17, 2024 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants