-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[Relay/TOPI][ONNX/TFLite] Refactor MATRIX_SET_DIAG Operator for Relay… #9329
Conversation
…/TOPI to support ONNX Trilu operator
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.
Looks mostly right to me, have a few comments. Will take a closer look later though.
Also please fix the lint issues so we can run CI
// Determining which diagonal/sub-diagonal/super-diagonal it is | ||
k = iter_vars[ndim] - iter_vars[ndim - 1]; | ||
diagonal_indices.push_back(k2 - k); | ||
diagonal_indices.push_back(k2(0) - k); |
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 believe (though please check) that you can just do k2() for 0-D tensor access.
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.
ok
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.
k2() will report error
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.
Eh ok
python/tvm/relay/frontend/onnx.py
Outdated
"""Operator converter for Trilu""" | ||
|
||
@classmethod | ||
def _impl_v1(cls, inputs, attr, params): |
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 should be opset14 so _impl_v14
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.
done
python/tvm/relay/frontend/onnx.py
Outdated
|
||
@classmethod | ||
def _impl_v1(cls, inputs, attr, params): | ||
upper = attr.get("upper") |
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.
suggest attr.get("upper", 1) since this appears to be the default in onnx spec
onnx and onnxruntime version need to be updated to version 1.10.1 and 1.9.0 respectively for Trilu operator, |
Let me talk to some folks about doing this. |
Hmm this will eventually get merged, I might get to upgrading CI next week |
Working on upgrading onnx right now #9511 |
wait for #9882 |
The new image with updated ONNX should become available by this week. |
New CI image with updated onnx is (finally up)! Please jostle ci by sending an empty commit |
@shengxinhu do you still have interest in working to get this PR merged? If not then I can try to shepard this in new branch. |
Thanks, please shepard it. |
…/TOPI to support ONNX Trilu operator This commit is based on PR apache#9329 proposed by @shengxinhu. Refactor MATRIX_SET_DIAG operator in Relay/TOPI to support ONNX Trilu operator; + Fixed issues related to shape transformation of inputs in tflite and onnx frontend ops.
Refactor MATRIX_SET_DIAG operator in Relay/TOPI to support ONNX Trilu operator