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

Implement tensordot and repeat #95

Merged
merged 12 commits into from
Jan 17, 2025

Conversation

neNasko1
Copy link
Contributor

@neNasko1 neNasko1 commented Jan 7, 2025

This PR adds implementations to tensordot and repeat.

Note they currently segfault on the latest onnx release, so the arrayapi tests are disabled. Coverage is provided instead on our side with some tests.

Copy link
Member

@adityagoel4512 adityagoel4512 left a comment

Choose a reason for hiding this comment

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

I'll need to play around with the einsum a little more but here's a first pass.

ndonnx/_core/_interface.py Show resolved Hide resolved
ndonnx/_core/_shapeimpl.py Outdated Show resolved Hide resolved
ndonnx/_funcs.py Outdated Show resolved Hide resolved
ndonnx/_funcs.py Outdated Show resolved Hide resolved
ndonnx/_core/_shapeimpl.py Show resolved Hide resolved
xfails.txt Outdated Show resolved Hide resolved
tests/test_core.py Outdated Show resolved Hide resolved
xfails.txt Outdated
Comment on lines 2 to 4
array_api_tests/test_manipulation_functions.py::test_repeat
# segmentation fault: due to onnxruntime not handling einsum with dim 0
array_api_tests/test_linalg.py::test_tensordot
Copy link
Collaborator

Choose a reason for hiding this comment

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

If these tests segfault then we should put them into skips.txt rather than here. That the array-api-tests are green nonetheless is an indication that something else is fishy. That feeling is confounded by the fact that they finish within 20s. Could you take a look, please?

xfails.txt Outdated Show resolved Hide resolved
Copy link
Member

@adityagoel4512 adityagoel4512 left a comment

Choose a reason for hiding this comment

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

I think we're almost there. Not sure I get what your lazy_repeats test is doing.

(np.arange(60).reshape(3, 4, 5), np.arange(60), None),
],
)
def test_repeat(a, repeats, axis, lazy_repeats):
Copy link
Member

Choose a reason for hiding this comment

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

What is "lazy" about the lazy_repeats case here? Even when repeats is int, it's immediately wrapped in ndx.asarray(repeats) in your implementation of repeat.

Copy link
Member

@adityagoel4512 adityagoel4512 Jan 16, 2025

Choose a reason for hiding this comment

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

For a test that exercises shape inference for repeat, ideally you should be using a lazy array and just building the ONNX model.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea of the test is to test that if we actually supply the argument as a python integer, we wrap it correctly. I rewrote the test to be cleaner.

Copy link
Member

Choose a reason for hiding this comment

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

@neNasko1 please rebase

skips.txt Outdated Show resolved Hide resolved
@neNasko1
Copy link
Contributor Author

Thanks for merging into main!

Signed-off-by: Atanas Dimitrov <[email protected]>
@adityagoel4512 adityagoel4512 merged commit c919f11 into Quantco:main Jan 17, 2025
16 checks passed
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.

3 participants