[Relay/TOPI][ONNX/TFLite] Refactor MATRIX_SET_DIAG Operator for Relay/TOPI to support ONNX Trilu operator #10873
Conversation
Added hardswish support to TVM CI and fixed unit test. - Add class HardSwish and added its reference to convert_map in onnx.py; - Removed test_hardswish entry from test_forward.py;
Fixing onnx.py format.
…/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.
jwfromm
left a comment
jwfromm
left a comment
There was a problem hiding this comment.
Thanks for this change, it overall seems excellent. I left a few nitpicks and questions.
| for dtype in ["float32", "int32"]: | ||
| verify_matrix_set_diag((2, 2), (2,), dtype) | ||
| verify_matrix_set_diag((4, 3, 3), (4, 3), dtype) | ||
| verify_matrix_set_diag((2, 3, 4), (2, 3), dtype, 1) |
There was a problem hiding this comment.
Just curious, why are these tests removed?
There was a problem hiding this comment.
This has been removed in the original PR. I ended up not putting it back and testing it again. Thanks for pointing it out since the test_matrix_set_diag is outputting an error for these three cases. I will check it out.
There was a problem hiding this comment.
Josh, these three TOPI test cases are still not passing. I am debugging it to solve the Tensor dimension mismatch issue.
src/relay/op/tensor/transform.cc
Outdated
| .set_num_inputs(4) | ||
| .add_argument("input", "Tensor", "Input Tensor.") | ||
| .add_argument("diagonal", "Tensor", "Values to be filled in the diagonal.") | ||
| .add_argument("k1", "Tensor", "ILower limit (included) of the range of diagonals.") |
There was a problem hiding this comment.
Small typo here "ILower" -> "Lower"
There was a problem hiding this comment.
Thanks, I will fix it.
| matrix_set_diag_result = topi.transform.matrix_set_diag(input, diagonal, (k1, k2), align) | ||
|
|
||
| k_one, k_two = None, None | ||
| if isinstance(k, (tuple, list)): |
There was a problem hiding this comment.
Can you add some comments to this test? It's a little hard to follow.
There was a problem hiding this comment.
Sure. I will add some comments and update the PR.
-- Fixed typo in transform.cc; -- Adding comments to verify_matrix_set_diag on test_topi_transform.py; -- Rollback TOPI tests set in test_matrix_set_diag; -- Rollback TFLite frontend convert_matrix_diag method; --- For some reason (maybe some package version), removing the last dimension was necessary (on Macbook M1); --- When moving to Linux, the old version was running smoothly and test passed; - Andrew's comments: -- Small change into matrix_set_diag of include/tvm/topi/transform.h; -- Fixed comment in relay/op/tensor/transform.cc
|
@bfgoldstein what is the status on this one? |
@mikepapadim, it is on hold. I will try to work on that by early next week. |
no worries, I was looking into the |
No problem! I bump into two special test cases which the TOPI is not passing. I had to put it on hold due to priorities. |
areusch
added
needs-triage
6 participants
This commit is based on PR #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.
@AndrewZhaoLuo PTAL