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

Fix shape inference for Loop #198

Merged
merged 5 commits into from
Jan 3, 2025
Merged

Conversation

neNasko1
Copy link
Contributor

@neNasko1 neNasko1 commented Jan 2, 2025

This PR is based on the discussions in #163 and #164 and is a best effort solution to fill the type inferencing hole referenced.

I provided a short summary of the current state to make it easier to follow the current discussion.

Note: This is not a complete implementation as mentioned by the comment in #163.

Checklist

  • Added a CHANGELOG.rst entry

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.

The overall approach looks sound to me.

src/spox/opset/ai/onnx/v17.py Outdated Show resolved Hide resolved
tests/type_inference/test_loop.py Show resolved Hide resolved
src/spox/opset/ai/onnx/v17.py Outdated Show resolved Hide resolved
src/spox/opset/ai/onnx/v17.py Show resolved Hide resolved
CHANGELOG.rst Outdated Show resolved Hide resolved
src/spox/_type_system.py Outdated Show resolved Hide resolved
@neNasko1 neNasko1 changed the title Fix shape inference for Loop@v19 and v21 Fix shape inference for Loop Jan 3, 2025
@neNasko1
Copy link
Contributor Author

neNasko1 commented Jan 3, 2025

Note, that this does not run the test actions, so we should wait before merging.

adityagoel4512
adityagoel4512 previously approved these changes Jan 3, 2025
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.

Thanks, let's fix test CI then merge!

@adityagoel4512 adityagoel4512 dismissed their stale review January 3, 2025 17:25

This is provisionally approved but I'll reapprove once the test action is fixed.

Fix inconsistencies

Signed-off-by: neNasko1 <[email protected]>

Add CHANGELOG

Signed-off-by: neNasko1 <[email protected]>

Fix docstring

Signed-off-by: neNasko1 <[email protected]>

Apply suggestions from code review

Co-authored-by: Aditya Goel <[email protected]>

Comments after code review

Signed-off-by: neNasko1 <[email protected]>

Comments after code review

Signed-off-by: neNasko1 <[email protected]>

Fix license

Signed-off-by: neNasko1 <[email protected]>
@neNasko1 neNasko1 mentioned this pull request Jan 3, 2025
1 task
CHANGELOG.rst Show resolved Hide resolved
Co-authored-by: Aditya Goel <[email protected]>
@neNasko1 neNasko1 merged commit 0741557 into Quantco:main Jan 3, 2025
16 checks passed
@neNasko1
Copy link
Contributor Author

neNasko1 commented Jan 3, 2025

Thanks for the help 🚀

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.

2 participants