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

Parse ONNX Expand to broadcast_with_dims #2799

Merged
merged 55 commits into from
Apr 16, 2024
Merged

Parse ONNX Expand to broadcast_with_dims #2799

merged 55 commits into from
Apr 16, 2024

Conversation

CharlieL7
Copy link
Collaborator

@CharlieL7 CharlieL7 requested review from turneram and removed request for pfultz2 and turneram March 28, 2024 20:04
@bpickrel
Copy link
Contributor

What's the reason this was created as a separate op instead of extra functionality for the multibroadcast op?

test/onnx/gen_onnx.py Show resolved Hide resolved
test/op_shape_test.cpp Show resolved Hide resolved
test/onnx/parse/expand_test.cpp Show resolved Hide resolved
test/ref/broadcast_with_dims.cpp Show resolved Hide resolved
test/ref/broadcast_with_dims.cpp Show resolved Hide resolved
test/ref/broadcast_with_dims.cpp Show resolved Hide resolved
* input_tensor shape: lens = {2, 3}, strides = {3, 1}
* dims = [4, 1, 3]
* output shape: lens = {4, 2, 3}, strides = {0, 3, 1}
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like this example make this one of the tests so we can have a 1:1 with expected behavior

Copy link
Collaborator

@TedThemistokleous TedThemistokleous left a comment

Choose a reason for hiding this comment

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

@CharlieL7 overall looks good. Move so change the one test with your example you mention in the code. You already have 100% coverage so more just change the test to mirror the docstring.

I've kicked off CI for you on this and the previous runs have been passing/without issue.

@CharlieL7
Copy link
Collaborator Author

What's the reason this was created as a separate op instead of extra functionality for the multibroadcast op?

very messy that way, this way is cleaner

@CharlieL7 CharlieL7 requested a review from bpickrel April 10, 2024 14:55
Copy link
Contributor

@bpickrel bpickrel left a comment

Choose a reason for hiding this comment

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

Did you see the message about merging #2927 into this branch and making it part of this PR?

@causten causten merged commit 6e496c1 into develop Apr 16, 2024
48 checks passed
@causten causten deleted the expand_dyn_op branch April 16, 2024 01:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Onnx Operators Adding or modifying an Onnx Operator in the MIGraphX codebase onnx issues related to onnx support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parse Expand ONNX into a new broadcast_with_dims operator
5 participants