-
Notifications
You must be signed in to change notification settings - Fork 360
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
tools(opset_coverage): Map default ops to unoverloaded ops #2292
Conversation
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.
Code conforms to Python style guidelines
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.
Code conforms to C++ style guidelines
Signed-off-by: Naren Dasan <naren@narendasan.com> Signed-off-by: Naren Dasan <narens@nvidia.com>
d56025d
to
784fa4b
Compare
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.
In line with the syntax used in native_functions.yaml, this modification gives more clarity to the operator coverage set. A suggestion for a future improvement might be to provide coverage percentages for just the default operators, since this could give a more accurate view of what's covered.
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.
Code conforms to Python style guidelines
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.
Code conforms to C++ style guidelines
Why do we need to distinguish default operators? Because certain overloads are also part of core_aten so I'd think we would need to treat them equally? |
The only reason was to indicate partial support for an operator, even if we don't support every case of it. I assumed we would almost always have the |
Description
Updates the coverage tool to treat converters for default overloads to the root op name
Fixes # (issue)
Type of change
Please delete options that are not relevant and/or add your own.
Checklist: