Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
qlinearadd operator #2188
qlinearadd operator #2188
Changes from 2 commits
b795462
7abb3e6
f506678
e5c9a10
d2f567a
35214db
67a042f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This requires some modifications of
add_common_op
underonnx_parser.cpp/hpp, common.cpp/hpp
, but this would encourage reuse of existing functions that aim to do the same thing. I've tested by making the convert optional inadd_common_op
by adding a flag to the end of the function (on by default to keep other functional calls intact). Seems like it passes both tests when trying this approach.@pfultz2 @umangyadav feel free to chime in if this approach is cleaner or not.
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.
Thanks. Please note, this new api uses
Broadcast
andMultibroadcast
. The other one doesn't useBroadcast
. I am not sure if we want to make it a kitchen sink there..;There are other Quant operators that I am adding which shall use Broadcast, and so that part isn't tested by your new patch.
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.
I dont think
add_common_op
will work because we compare from the right. In this case we would need to compare from the left, and that only works because it will broadcast on axis 0. For axis 1 this wouldn't work. We could extend theadd_common_op
so an optional axis could be passed to handle 1d tensors.Either way we may want to address this another PR.
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.
I think we should address in another PR. Effectively we want to create reusable functions that will automatically handle broadcaasting shapes to be compatible for any operation.
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.
I agree that the common_op updates should be in another PR. We need to discuss how we want to design such an API for these cases.
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.
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 just merge this suggestion.