-
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][Frontend][Tensorflow]Add conv2d_transpose #4300
Conversation
@yongwww @srkreddy1238 could you help to review? |
@@ -349,6 +361,14 @@ def test_forward_convolution(): | |||
_test_convolution('depthwise', [4, 124, 17, 17], [1, 1, 124, 1], [1, 1], [1, 1], 'SAME', 'NCHW') | |||
_test_convolution('depthwise', [4, 12, 17, 17], [3, 3, 12, 1], [1, 1], [2, 2], 'VALID', 'NCHW') | |||
_test_convolution('depthwise', [4, 12, 17, 17], [3, 3, 12, 2], [1, 1], [2, 2], 'VALID', 'NCHW') | |||
_test_convolution('conv_transpose', [4, 32, 8, 8], [1, 1, 176, 32], [1, 1], [1, 1], 'SAME', |
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.
please add test case for cpu
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.
NHWC layout is not supported, see nn.conv2d_transpose. Is it still needed to add test cases for cpu?
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.
yeah, test cases for cpu are needed. How aboud adding test cases for NCHW input, and adding layout transformation to get the expected NCHW layout for NHWC input?
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.
Do you mean adding the transformation from NHWC to NCHW if 'conv2d_transpose' op at the beginning of _conv ?
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.
yeah, if the layout is NCHW, the nn.conv2d_transpose should work well. If layout is NHWC, then we can consider transforming the layout to NCHW to be compatible with nn.conv2d_transpose
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 have added transformation and the cpu test cases, could you please review again? Thanks!
08e3fef
to
5acdaee
Compare
The check failure is not due to the changes in this PR. And I can run through the failed tests in my local environment. So I 'd like to re-start the check, but don't know how to do that. Can anyone advice? Thanks! |
Your test case failed due to "TypeError: conv2d_transpose() got an unexpected keyword argument 'dilations'", try to use different tf version (1.2, 1.3, or 1.4) to reproduce it. |
tf1.4 doesn't have this error while tf1.3 does. |
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!
@srkreddy1238 @yzhliu any further comments? Thanks! |
Thanks @optima2005 @yongwww |
* [Relay][Frontend][Tensorflow]Add conv2d_transpose * add transformation from NHWC to NCHW to compatible with TVM conv2d_transpose implementation * remove 'dilations' paramater to compitable with TF1.3
* [Relay][Frontend][Tensorflow]Add conv2d_transpose * add transformation from NHWC to NCHW to compatible with TVM conv2d_transpose implementation * remove 'dilations' paramater to compitable with TF1.3
Why we only support kernel (1,1) for TF conv2d_transpose with padding SAME? |
@yongwww I am wondering whether there might be some issue in padding for 'SAME'. This PR seems not match the padding in topi.nn.conv2d_transpose . I need a little more time to invectigate this and will get back a additional PR if any issue would be needed to fix this. |
In any case please add For now I'd open PR to make kernel size check and show users message saying that "Currently we support SAME padding for kernel with size 1x1 only" |
Hi, This PR is to fix #4263
After investigation, I found this operation has been implemented in relay cc codes and been working in keras frontend. For tensorflow frontend, only python part is missing. So this PR only to fix the python part.
The conv2d_transpose implementation is similar with the conv2d op, beside the following differences,
The PR implementation is reusing _conv() function and adding the conditions to handle the below differences.