-
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
Passing dilation argument to account for API change. #3510
Conversation
@@ -112,8 +112,8 @@ def run_inference(data_dtype, kernel_dtype, out_dtype, im_height, im_width, in_f | |||
|
|||
with tvm.target.create(TARGET_NAME): | |||
conv = topi.nn.conv2d_NCHWc(data, kernel, stride=hstride, | |||
padding=hpad, layout='NCHWc', | |||
out_layout='NCHWc', out_dtype=out_dtype) | |||
padding=hpad, dilation=(1, 1), |
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.
is dilation=(1,1) not already a default value in the topi.nn.conv2d_NCHWc
API? if not should it be?
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 is a low-level topi operator and not the Relay operator. Relay operator has dilation default arguments which will be passed on to TOPI, if dilation was not provided in the framework graph. So, I think not having a default here is fine.
@tqchen The change is in the test case itself. Please let me know if I should add more testing to it. |
Thanks for contributing to TVM! Please refer to guideline https://docs.tvm.ai/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from Reviewers.