-
Notifications
You must be signed in to change notification settings - Fork 435
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
feat: add support for ExtractImagePatches #2188
Conversation
b4e1d24
to
2da669d
Compare
2da669d
to
3aa08e8
Compare
Thanks you for putting that solution into this PR, and it looks great! Rewriter is designed to rewrite the ONNX graph after we transform each tf op into the corresponding onnx op. Each rewriter will search the ONNX graph following a given pattern. Once the pattern is matched, those involved onnx ops will be replaced with some other ops for an optimization in further inference. In this case, ExtractImagePatches is just a tf op which is not supported by tf2onnx yet. So, your implementations should be put into nn.py file instead of adding a rewriter. Please add it into nn.py, just like adding a new tf op support. Please feel free to refer to this comment for more details. |
Hi @fatcat-z,
I'm not entirely sure if this is true. From my understanding, the rewriters are ran before each operation is converted into an ONNX operation: tensorflow-onnx/tf2onnx/tfonnx.py Line 616 in 0152029
where line 622 performs the conversion (?). There do appear to be late rewriters that run after the mapping occurs, but in general, it seems like the rewriting and optimization steps are separate. I chose to implement this as a rewrite in order to avoid duplicating the construction of the Conv2D node but if you would still prefer for this to be implemented in |
No, graphs_from_tf() function will transfer the tf graph to onnx graph meaning each tf op has been converted to onnx op, if possible. Afterwards, process_parsed_graph() will be called to finish those rewriters and optimizations. Yes, please implement this as an op in nn.py instead of creating a new rewriter. Thanks. |
Is this new operator going to be merged into main? |
I don't think this operator can be considered to be new but I'm aiming to get the requested changes done sometime within the week. |
7c7cc34
to
16643fd
Compare
Sorry for the delay! I've implemented this operation inside of |
Signed-off-by: Nanoskript <96655713+nanoskript@users.noreply.github.com>
16643fd
to
36f203b
Compare
@fatcat-z Hi! Apologies for the ping! Do you think this PR could be merged into main at some time? |
tf2onnx/onnx_opset/nn.py
Outdated
rates = node.get_attr_value("rates") | ||
padding = node.get_attr_str("padding") | ||
|
||
# Our constraints. |
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.
Could you please provide more details about this constraint so people know how to improve in future?
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've expanded this comment and given an example of a call that succeeds in Tensorflow but fails for this particular implementation.
Signed-off-by: Nanoskript <96655713+nanoskript@users.noreply.github.com>
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, thanks!
Closes: #436
This rewrite is based on this comment: #436 (comment) with changes to make it more general and translatable into
tf2onnx
.Equivalent TensorFlow function and automated test script (expand)
Output from
pytest convolve.py --hypothesis-show-statistics
(no failures):