-
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
[Frontend][ONNX] Support ONNX Scan operator #9438
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.
LGTM
dtype = output_node.checked_type.dtype | ||
scan_output_vars.append(_expr.var(name, shape=shape, dtype=dtype)) | ||
scan_output_init.append(_op.zeros(shape, dtype)) | ||
|
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.
It seems has a more elegant method(similar as operator Loop existing implementation):
scan_output_vars.append(_expr.var(name, shape=_ty.Any() * len(shape), dtype=dtype))
scan_output_init.append(_op.rehape(_expr.const(np.array[]).astype(dtype)), [0] + [1]*(len(shape)-1))
so the following _op.strided_slice could be removed, as the scan_output_init is empty.
But I did not make it work, could you take a look?
@shengxinhu My apologies, I missed this one, I should have reviewed it three weeks ago, it seems. Any chance you can rebase to resolve the conflicts? Edit: NVM, it was a simple "two prs added lines to the end of the test list" issue, I could fix that in the GUI |
The change seems to have a lot of duplication with |
Hi @masahi , I didn't get your point of duplication with loop converter, could you point it out sepecifically? |
I mean, just looking at the two implementations, it's obvious that you started by copy-pasting Loop converter (even comments are identical). Can we refactor common code into a helper function, or is it difficult to cleanly extract common bits? This is not just for usual code-hygiene reason. I believe there are some issues in Loop converter in preserving static shape information. I've never seen a practical ONNX model with See for example this issue regarding ssd mobilenet v1 #8284 (comment) Note that the original tf ssd mobilenet v1 model compiles and runs fine when imported directly by TF frontend, so I can only assume that this is a ONNX converter issue. |
Hi @masahi , thanks for your explanations. I indeed copy some code in Loop converter implementation. I think only the get_var function could be extracted. do you think is it necessary? If Loop is questionable, Scan is likely to be the same. I also have not seen an ONNX model with scan operator, I realized it just for a task assigned to me. |
ok, no problem. |
* [Frontend][ONNX] Support ONNX Scan operator * fix lint * remove test_scan_sum in unsupported_onnx_tests * support scan opset 8 * fix lint * fix negative axes bug * fix lint Co-authored-by: Matthew Brookhart <mbrookhart@octoml.ai>
* [Frontend][ONNX] Support ONNX Scan operator * fix lint * remove test_scan_sum in unsupported_onnx_tests * support scan opset 8 * fix lint * fix negative axes bug * fix lint Co-authored-by: Matthew Brookhart <mbrookhart@octoml.ai>
* [Frontend][ONNX] Support ONNX Scan operator * fix lint * remove test_scan_sum in unsupported_onnx_tests * support scan opset 8 * fix lint * fix negative axes bug * fix lint Co-authored-by: Matthew Brookhart <mbrookhart@octoml.ai>
* [Frontend][ONNX] Support ONNX Scan operator * fix lint * remove test_scan_sum in unsupported_onnx_tests * support scan opset 8 * fix lint * fix negative axes bug * fix lint Co-authored-by: Matthew Brookhart <mbrookhart@octoml.ai>
* [Frontend][ONNX] Support ONNX Scan operator * fix lint * remove test_scan_sum in unsupported_onnx_tests * support scan opset 8 * fix lint * fix negative axes bug * fix lint Co-authored-by: Matthew Brookhart <mbrookhart@octoml.ai>
* [Frontend][ONNX] Support ONNX Scan operator * fix lint * remove test_scan_sum in unsupported_onnx_tests * support scan opset 8 * fix lint * fix negative axes bug * fix lint Co-authored-by: Matthew Brookhart <mbrookhart@octoml.ai>
support onnx scan operator