You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2020/01/29 08:26:16 UTC
[GitHub] [incubator-tvm] zxy844288792 opened a new pull request #4787:
[Relay] Conv2D padding representation
zxy844288792 opened a new pull request #4787: [Relay] Conv2D padding representation
URL: https://github.com/apache/incubator-tvm/pull/4787
As discussed here: https://discuss.tvm.ai/t/rfc-conv2d-padding-representation/5394. We agree we should enforce topi.nn.conv2d to accept 4-way padding. We also should have relay.nn.conv2d legalizes the padding to 4-way.
get_pad_tuple is from topi util.py. I deleted some unuseful code and reuse it for relay.op.nn.conv2d.
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](https://github.com/apache/incubator-tvm/blob/master/CONTRIBUTORS.md#reviewers) by @ them in the pull request thread.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] kevinthesun merged pull request #4787: [Relay]
Conv2D padding representation
Posted by GitBox <gi...@apache.org>.
kevinthesun merged pull request #4787: [Relay] Conv2D padding representation
URL: https://github.com/apache/incubator-tvm/pull/4787
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] comaniac commented on a change in pull request
#4787: [Relay] Conv2D padding representation
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #4787: [Relay] Conv2D padding representation
URL: https://github.com/apache/incubator-tvm/pull/4787#discussion_r373085568
##########
File path: topi/python/topi/nn/conv2d.py
##########
@@ -62,6 +61,8 @@ def conv2d(input, filter, strides, padding, dilation, layout='NCHW', out_dtype=N
output : tvm.Tensor
4-D with shape [batch, out_channel, out_height, out_width]
"""
+ #only accepts 4-way padding
+ assert len(padding) == 4, "only accepts 4-way padding"
Review comment:
I'm afraid that you have to add this assertion to all conv2d compute functions. Specifically, all conv2d functions with `@autotvm.register_topi_compute(nn.conv2d, ...)` decorator should have this assertion. @icemelon9 could you help confirm?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] icemelon9 commented on issue #4787: [Relay] Conv2D
padding representation
Posted by GitBox <gi...@apache.org>.
icemelon9 commented on issue #4787: [Relay] Conv2D padding representation
URL: https://github.com/apache/incubator-tvm/pull/4787#issuecomment-580874675
You should also add `get_pad_tuple2d` to these contrib conv2d ops, e.g., contrib_conv2d_nchwc
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] zxy844288792 commented on a change in pull request
#4787: [Relay] Conv2D padding representation
Posted by GitBox <gi...@apache.org>.
zxy844288792 commented on a change in pull request #4787: [Relay] Conv2D padding representation
URL: https://github.com/apache/incubator-tvm/pull/4787#discussion_r373776360
##########
File path: python/tvm/relay/op/nn/util.py
##########
@@ -0,0 +1,56 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# pylint: disable=invalid-name, unused-variable
+"""NN operator common utilities"""
+from __future__ import absolute_import
+from .... import container
+
+def get_pad_tuple2d(padding):
+ """Common code to get the pad option
+ Parameters
+ ----------
+ padding : Union[int, Tuple[int, ...]]
+ Padding size
+ Returns
+ -------
+ pad_top : int
+ Padding size on top
+ pad_left : int
+ Padding size on left
+ pad_down : int
+ Padding size on down.
+ pad_right : int
+ Padding size on right.
+ """
+ # compute the padding size
+ if isinstance(padding, container.Array):
+ padding = list(padding)
+ if isinstance(padding, (tuple, list)):
+ if len(padding) == 2:
+ pad_h = padding[0] * 2
+ pad_w = padding[1] * 2
+ elif len(padding) == 4:
+ return padding[0], padding[1], padding[2], padding[3]
Review comment:
Thanks! I will address this
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] zxy844288792 commented on issue #4787: [Relay]
Conv2D padding representation
Posted by GitBox <gi...@apache.org>.
zxy844288792 commented on issue #4787: [Relay] Conv2D padding representation
URL: https://github.com/apache/incubator-tvm/pull/4787#issuecomment-581036790
> You should also add `get_pad_tuple2d` to these contrib conv2d ops, e.g., contrib_conv2d_nchwc
I have added get_pad_tuple2d for contrib_conv2d_nchwc and contrib_conv2d_nchwc_int8, contrib_conv2d_winograd_without_weight_transform and contrib_conv2d_winograd_nnpack_without_weight_transform
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] comaniac commented on a change in pull request
#4787: [Relay] Conv2D padding representation
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #4787: [Relay] Conv2D padding representation
URL: https://github.com/apache/incubator-tvm/pull/4787#discussion_r373193490
##########
File path: topi/python/topi/nn/conv2d.py
##########
@@ -62,6 +61,8 @@ def conv2d(input, filter, strides, padding, dilation, layout='NCHW', out_dtype=N
output : tvm.Tensor
4-D with shape [batch, out_channel, out_height, out_width]
"""
+ #only accepts 4-way padding
+ assert len(padding) == 4, "only accepts 4-way padding"
Review comment:
@zxy844288792 how about we revert this file first and add a note in `python/tvm/relay/op/nn/nn.py` to remind us to add it back when #4644 is merged?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] comaniac commented on a change in pull request
#4787: [Relay] Conv2D padding representation
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #4787: [Relay] Conv2D padding representation
URL: https://github.com/apache/incubator-tvm/pull/4787#discussion_r372525767
##########
File path: python/tvm/relay/op/nn/util.py
##########
@@ -0,0 +1,53 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# pylint: disable=invalid-name, unused-variable
+"""NN operator common utilities"""
+from __future__ import absolute_import
+
+def get_pad_tuple(padding):
+ """Common code to get the pad option
+ Parameters
+ ----------
+ padding : int or str
+ Padding size, or ['VALID', 'SAME']
Review comment:
The type should be `Union[int, Tuple[int, ...]]`. Also please change the description accordingly.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] comaniac commented on a change in pull request
#4787: [Relay] Conv2D padding representation
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #4787: [Relay] Conv2D padding representation
URL: https://github.com/apache/incubator-tvm/pull/4787#discussion_r372526415
##########
File path: python/tvm/relay/op/nn/nn.py
##########
@@ -202,6 +203,8 @@ def conv2d(data,
dilation = (dilation, dilation)
if isinstance(padding, int):
padding = (padding, padding)
Review comment:
Consider removing this logic since `get_pad_tuple` can also accept `int`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] zxy844288792 commented on a change in pull request
#4787: [Relay] Conv2D padding representation
Posted by GitBox <gi...@apache.org>.
zxy844288792 commented on a change in pull request #4787: [Relay] Conv2D padding representation
URL: https://github.com/apache/incubator-tvm/pull/4787#discussion_r372718146
##########
File path: python/tvm/relay/op/nn/nn.py
##########
@@ -202,6 +203,8 @@ def conv2d(data,
dilation = (dilation, dilation)
if isinstance(padding, int):
padding = (padding, padding)
Review comment:
thanks, removed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] zxy844288792 commented on a change in pull request
#4787: [Relay] Conv2D padding representation
Posted by GitBox <gi...@apache.org>.
zxy844288792 commented on a change in pull request #4787: [Relay] Conv2D padding representation
URL: https://github.com/apache/incubator-tvm/pull/4787#discussion_r373255718
##########
File path: topi/python/topi/nn/conv2d.py
##########
@@ -62,6 +61,8 @@ def conv2d(input, filter, strides, padding, dilation, layout='NCHW', out_dtype=N
output : tvm.Tensor
4-D with shape [batch, out_channel, out_height, out_width]
"""
+ #only accepts 4-way padding
+ assert len(padding) == 4, "only accepts 4-way padding"
Review comment:
@comaniac Agree
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] icemelon9 commented on a change in pull request
#4787: [Relay] Conv2D padding representation
Posted by GitBox <gi...@apache.org>.
icemelon9 commented on a change in pull request #4787: [Relay] Conv2D padding representation
URL: https://github.com/apache/incubator-tvm/pull/4787#discussion_r373643774
##########
File path: python/tvm/relay/op/nn/util.py
##########
@@ -0,0 +1,56 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# pylint: disable=invalid-name, unused-variable
+"""NN operator common utilities"""
+from __future__ import absolute_import
+from .... import container
+
+def get_pad_tuple2d(padding):
+ """Common code to get the pad option
+ Parameters
+ ----------
+ padding : Union[int, Tuple[int, ...]]
+ Padding size
+ Returns
+ -------
+ pad_top : int
+ Padding size on top
+ pad_left : int
+ Padding size on left
+ pad_down : int
+ Padding size on down.
+ pad_right : int
+ Padding size on right.
+ """
+ # compute the padding size
+ if isinstance(padding, container.Array):
+ padding = list(padding)
+ if isinstance(padding, (tuple, list)):
+ if len(padding) == 2:
+ pad_h = padding[0] * 2
+ pad_w = padding[1] * 2
+ elif len(padding) == 4:
+ return padding[0], padding[1], padding[2], padding[3]
Review comment:
```suggestion
return padding[0], padding[1], padding[2], padding[3]
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] comaniac commented on a change in pull request
#4787: [Relay] Conv2D padding representation
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #4787: [Relay] Conv2D padding representation
URL: https://github.com/apache/incubator-tvm/pull/4787#discussion_r372523989
##########
File path: python/tvm/relay/op/nn/nn.py
##########
@@ -202,6 +203,8 @@ def conv2d(data,
dilation = (dilation, dilation)
if isinstance(padding, int):
padding = (padding, padding)
+ # convert 2-way padding to 4-way padding
+ padding = get_pad_tuple(padding)
Review comment:
Since we also have conv1d and conv3d, maybe it's better to rename this function to `get_2d_pad_tuple`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] comaniac commented on issue #4787: [Relay] Conv2D
padding representation
Posted by GitBox <gi...@apache.org>.
comaniac commented on issue #4787: [Relay] Conv2D padding representation
URL: https://github.com/apache/incubator-tvm/pull/4787#issuecomment-580836237
LGTM.
@icemelon9 @yzhliu could you help review and merge? Thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] icemelon9 commented on issue #4787: [Relay] Conv2D
padding representation
Posted by GitBox <gi...@apache.org>.
icemelon9 commented on issue #4787: [Relay] Conv2D padding representation
URL: https://github.com/apache/incubator-tvm/pull/4787#issuecomment-582063672
@zxy844288792 I see that you didn't modify the `contrib_depthwise_conv2d_nchwc`. Should we also use `get_pad_tuple2d` for it?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] icemelon9 commented on a change in pull request
#4787: [Relay] Conv2D padding representation
Posted by GitBox <gi...@apache.org>.
icemelon9 commented on a change in pull request #4787: [Relay] Conv2D padding representation
URL: https://github.com/apache/incubator-tvm/pull/4787#discussion_r373190892
##########
File path: topi/python/topi/nn/conv2d.py
##########
@@ -62,6 +61,8 @@ def conv2d(input, filter, strides, padding, dilation, layout='NCHW', out_dtype=N
output : tvm.Tensor
4-D with shape [batch, out_channel, out_height, out_width]
"""
+ #only accepts 4-way padding
+ assert len(padding) == 4, "only accepts 4-way padding"
Review comment:
Yes, @comaniac is correct. Probably directly contribute to #4644? 😄
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] zxy844288792 commented on a change in pull request
#4787: [Relay] Conv2D padding representation
Posted by GitBox <gi...@apache.org>.
zxy844288792 commented on a change in pull request #4787: [Relay] Conv2D padding representation
URL: https://github.com/apache/incubator-tvm/pull/4787#discussion_r372702055
##########
File path: python/tvm/relay/op/nn/nn.py
##########
@@ -202,6 +203,8 @@ def conv2d(data,
dilation = (dilation, dilation)
if isinstance(padding, int):
padding = (padding, padding)
+ # convert 2-way padding to 4-way padding
+ padding = get_pad_tuple(padding)
Review comment:
in topi/nn/util, we have get_pad_tuple1d and get_pad_tuple3d, so maybe I will rename it as get_pad_tuple2d for consistency
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] kevinthesun commented on issue #4787: [Relay]
Conv2D padding representation
Posted by GitBox <gi...@apache.org>.
kevinthesun commented on issue #4787: [Relay] Conv2D padding representation
URL: https://github.com/apache/incubator-tvm/pull/4787#issuecomment-582664192
Thanks @zxy844288792 @icemelon9 @comaniac
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] comaniac commented on a change in pull request
#4787: [Relay] Conv2D padding representation
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #4787: [Relay] Conv2D padding representation
URL: https://github.com/apache/incubator-tvm/pull/4787#discussion_r372528150
##########
File path: topi/python/topi/nn/conv2d.py
##########
@@ -62,6 +62,8 @@ def conv2d(input, filter, strides, padding, dilation, layout='NCHW', out_dtype=N
output : tvm.Tensor
4-D with shape [batch, out_channel, out_height, out_width]
"""
+ #only accepts 4-way padding
+ assert len(padding) == 4, "only accepts 4-way padding"
Review comment:
Change the function docstring accordingly.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services