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/07/15 17:19:51 UTC
[GitHub] [incubator-tvm] abergeron opened a new pull request #6065: Change the meaning of output_padding to correspond to conv{1,2}d_transpose
abergeron opened a new pull request #6065:
URL: https://github.com/apache/incubator-tvm/pull/6065
This is the follow-up to #5758 for conv3d.
All tests pass and work, but I noticed that there aren't any non-symmetrical shapes in the conv3d tests and adding such shapes gives a failing test, because the ref and output will have different shapes like (1, 16, 73, 72, 74) and (1, 16, 74, 73, 72). This is why one of the tests is commented out.
Ping: @vinx13 @siju-samuel
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] abergeron commented on pull request #6065: Change the meaning of conv3d_transpose output_padding to match conv{1,2}d_transpose
Posted by GitBox <gi...@apache.org>.
abergeron commented on pull request #6065:
URL: https://github.com/apache/incubator-tvm/pull/6065#issuecomment-659158891
Something is weird with the mac build, I'll try an empty commit.
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] abergeron commented on pull request #6065: Change the meaning of conv3d_transpose output_padding to match conv{1,2}d_transpose
Posted by GitBox <gi...@apache.org>.
abergeron commented on pull request #6065:
URL: https://github.com/apache/incubator-tvm/pull/6065#issuecomment-663891403
The CI problem appears to be fixed, so I'm trying again.
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] vinx13 commented on a change in pull request #6065: Change the meaning of conv3d_transpose output_padding to match conv{1,2}d_transpose
Posted by GitBox <gi...@apache.org>.
vinx13 commented on a change in pull request #6065:
URL: https://github.com/apache/incubator-tvm/pull/6065#discussion_r464046988
##########
File path: topi/tests/python/test_topi_conv3d_transpose_ncdhw.py
##########
@@ -86,15 +86,18 @@ def check_device(device):
def test_conv3d_transpose_ncdhw():
- verify_conv3d_transpose_ncdhw(1, 3, (24, 24, 24), 1, (1, 1, 1), (1, 1, 1), (0, 0, 0, 0, 0, 0))
- verify_conv3d_transpose_ncdhw(1, 3, (24, 24, 24), 2, (3, 3, 3), (1, 1, 1), (0, 0, 0, 0, 0, 0))
- verify_conv3d_transpose_ncdhw(1, 3, (24, 24, 24), 16, (3, 3, 3), (1, 1, 1), (0, 0, 0, 0, 0, 0))
- verify_conv3d_transpose_ncdhw(1, 3, (24, 24, 24), 16, (3, 3, 3), (3, 3, 3), (0, 0, 0, 0, 0, 0))
- verify_conv3d_transpose_ncdhw(1, 3, (24, 24, 24), 16, (3, 3, 3), (1, 1, 1), (0, 0, 0, 0, 0, 0))
- verify_conv3d_transpose_ncdhw(1, 3, (24, 24, 24), 16, (3, 3, 3), (2, 2, 2), (1, 1, 1, 1, 1, 1))
- verify_conv3d_transpose_ncdhw(1, 3, (24, 24, 24), 16, (2, 2, 2), (2, 2, 2), (0, 0, 0, 0, 0, 0))
- verify_conv3d_transpose_ncdhw(1, 8, (32, 32, 32), 32, (5, 5, 5), (1, 1, 1), (0, 0, 0, 0, 0, 0))
- verify_conv3d_transpose_ncdhw(1, 8, (32, 32, 32), 64, (5, 5, 5), (2, 2, 2), (1, 1, 1, 1, 1, 1))
+ verify_conv3d_transpose_ncdhw(1, 3, (24, 24, 24), 1, (1, 1, 1), (1, 1, 1), (0, 0, 0, 0, 0, 0), (0, 0, 0))
+ verify_conv3d_transpose_ncdhw(1, 3, (24, 24, 24), 2, (3, 3, 3), (1, 1, 1), (0, 0, 0, 0, 0, 0), (0, 0, 0))
+ verify_conv3d_transpose_ncdhw(1, 3, (24, 24, 24), 16, (3, 3, 3), (1, 1, 1), (0, 0, 0, 0, 0, 0), (0, 0, 0))
+ verify_conv3d_transpose_ncdhw(1, 3, (24, 24, 24), 16, (3, 3, 3), (3, 3, 3), (0, 0, 0, 0, 0, 0), (0, 0, 0))
+ verify_conv3d_transpose_ncdhw(1, 3, (24, 24, 24), 16, (3, 3, 3), (3, 3, 3), (0, 0, 0, 0, 0, 0), (2, 2, 2))
+ #verify_conv3d_transpose_ncdhw(1, 3, (24, 24, 24), 16, (3, 3, 3), (3, 3, 3), (0, 0, 0, 0, 0, 0), (1, 0, 2))
Review comment:
is this broken?
##########
File path: topi/python/topi/nn/conv3d_transpose.py
##########
@@ -25,7 +25,7 @@
from ..util import simplify
-def conv3d_transpose_ncdhw(Input, Filter, strides, padding, out_dtype):
+def conv3d_transpose_ncdhw(Input, Filter, strides, padding, out_dtype, output_padding):
Review comment:
please update doc of param here
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] abergeron commented on a change in pull request #6065: Change the meaning of conv3d_transpose output_padding to match conv{1,2}d_transpose
Posted by GitBox <gi...@apache.org>.
abergeron commented on a change in pull request #6065:
URL: https://github.com/apache/incubator-tvm/pull/6065#discussion_r465176524
##########
File path: topi/tests/python/test_topi_conv3d_transpose_ncdhw.py
##########
@@ -86,15 +86,18 @@ def check_device(device):
def test_conv3d_transpose_ncdhw():
- verify_conv3d_transpose_ncdhw(1, 3, (24, 24, 24), 1, (1, 1, 1), (1, 1, 1), (0, 0, 0, 0, 0, 0))
- verify_conv3d_transpose_ncdhw(1, 3, (24, 24, 24), 2, (3, 3, 3), (1, 1, 1), (0, 0, 0, 0, 0, 0))
- verify_conv3d_transpose_ncdhw(1, 3, (24, 24, 24), 16, (3, 3, 3), (1, 1, 1), (0, 0, 0, 0, 0, 0))
- verify_conv3d_transpose_ncdhw(1, 3, (24, 24, 24), 16, (3, 3, 3), (3, 3, 3), (0, 0, 0, 0, 0, 0))
- verify_conv3d_transpose_ncdhw(1, 3, (24, 24, 24), 16, (3, 3, 3), (1, 1, 1), (0, 0, 0, 0, 0, 0))
- verify_conv3d_transpose_ncdhw(1, 3, (24, 24, 24), 16, (3, 3, 3), (2, 2, 2), (1, 1, 1, 1, 1, 1))
- verify_conv3d_transpose_ncdhw(1, 3, (24, 24, 24), 16, (2, 2, 2), (2, 2, 2), (0, 0, 0, 0, 0, 0))
- verify_conv3d_transpose_ncdhw(1, 8, (32, 32, 32), 32, (5, 5, 5), (1, 1, 1), (0, 0, 0, 0, 0, 0))
- verify_conv3d_transpose_ncdhw(1, 8, (32, 32, 32), 64, (5, 5, 5), (2, 2, 2), (1, 1, 1, 1, 1, 1))
+ verify_conv3d_transpose_ncdhw(1, 3, (24, 24, 24), 1, (1, 1, 1), (1, 1, 1), (0, 0, 0, 0, 0, 0), (0, 0, 0))
+ verify_conv3d_transpose_ncdhw(1, 3, (24, 24, 24), 2, (3, 3, 3), (1, 1, 1), (0, 0, 0, 0, 0, 0), (0, 0, 0))
+ verify_conv3d_transpose_ncdhw(1, 3, (24, 24, 24), 16, (3, 3, 3), (1, 1, 1), (0, 0, 0, 0, 0, 0), (0, 0, 0))
+ verify_conv3d_transpose_ncdhw(1, 3, (24, 24, 24), 16, (3, 3, 3), (3, 3, 3), (0, 0, 0, 0, 0, 0), (0, 0, 0))
+ verify_conv3d_transpose_ncdhw(1, 3, (24, 24, 24), 16, (3, 3, 3), (3, 3, 3), (0, 0, 0, 0, 0, 0), (2, 2, 2))
+ #verify_conv3d_transpose_ncdhw(1, 3, (24, 24, 24), 16, (3, 3, 3), (3, 3, 3), (0, 0, 0, 0, 0, 0), (1, 0, 2))
Review comment:
Yes it is broken.
I don't remember how exactly but I think the original function doesn't work correctly when the shapes are not all the same. None of the current tests check for that. I might take some time to figure out why right now.
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] abergeron commented on pull request #6065: Change the meaning of conv3d_transpose output_padding to match conv{1,2}d_transpose
Posted by GitBox <gi...@apache.org>.
abergeron commented on pull request #6065:
URL: https://github.com/apache/incubator-tvm/pull/6065#issuecomment-659070859
It seems the mac build actually succeeded but there was a fluke failure. Should I push an empty commit to restart or is it possible to restart manually?
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] vinx13 merged pull request #6065: Change the meaning of conv3d_transpose output_padding to match conv{1,2}d_transpose
Posted by GitBox <gi...@apache.org>.
vinx13 merged pull request #6065:
URL: https://github.com/apache/incubator-tvm/pull/6065
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] abergeron commented on pull request #6065: Change the meaning of conv3d_transpose output_padding to match conv{1,2}d_transpose
Posted by GitBox <gi...@apache.org>.
abergeron commented on pull request #6065:
URL: https://github.com/apache/incubator-tvm/pull/6065#issuecomment-668691399
I've rebased this on top of the topi refactor.
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] abergeron commented on pull request #6065: Change the meaning of conv3d_transpose output_padding to match conv{1,2}d_transpose
Posted by GitBox <gi...@apache.org>.
abergeron commented on pull request #6065:
URL: https://github.com/apache/incubator-tvm/pull/6065#issuecomment-667194431
@vinx13 This is ready for review/merging.
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] abergeron commented on a change in pull request #6065: Change the meaning of conv3d_transpose output_padding to match conv{1,2}d_transpose
Posted by GitBox <gi...@apache.org>.
abergeron commented on a change in pull request #6065:
URL: https://github.com/apache/incubator-tvm/pull/6065#discussion_r465175659
##########
File path: topi/python/topi/nn/conv3d_transpose.py
##########
@@ -25,7 +25,7 @@
from ..util import simplify
-def conv3d_transpose_ncdhw(Input, Filter, strides, padding, out_dtype):
+def conv3d_transpose_ncdhw(Input, Filter, strides, padding, out_dtype, output_padding):
Review comment:
ok
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] abergeron commented on a change in pull request #6065: Change the meaning of conv3d_transpose output_padding to match conv{1,2}d_transpose
Posted by GitBox <gi...@apache.org>.
abergeron commented on a change in pull request #6065:
URL: https://github.com/apache/incubator-tvm/pull/6065#discussion_r465201160
##########
File path: topi/tests/python/test_topi_conv3d_transpose_ncdhw.py
##########
@@ -86,15 +86,18 @@ def check_device(device):
def test_conv3d_transpose_ncdhw():
- verify_conv3d_transpose_ncdhw(1, 3, (24, 24, 24), 1, (1, 1, 1), (1, 1, 1), (0, 0, 0, 0, 0, 0))
- verify_conv3d_transpose_ncdhw(1, 3, (24, 24, 24), 2, (3, 3, 3), (1, 1, 1), (0, 0, 0, 0, 0, 0))
- verify_conv3d_transpose_ncdhw(1, 3, (24, 24, 24), 16, (3, 3, 3), (1, 1, 1), (0, 0, 0, 0, 0, 0))
- verify_conv3d_transpose_ncdhw(1, 3, (24, 24, 24), 16, (3, 3, 3), (3, 3, 3), (0, 0, 0, 0, 0, 0))
- verify_conv3d_transpose_ncdhw(1, 3, (24, 24, 24), 16, (3, 3, 3), (1, 1, 1), (0, 0, 0, 0, 0, 0))
- verify_conv3d_transpose_ncdhw(1, 3, (24, 24, 24), 16, (3, 3, 3), (2, 2, 2), (1, 1, 1, 1, 1, 1))
- verify_conv3d_transpose_ncdhw(1, 3, (24, 24, 24), 16, (2, 2, 2), (2, 2, 2), (0, 0, 0, 0, 0, 0))
- verify_conv3d_transpose_ncdhw(1, 8, (32, 32, 32), 32, (5, 5, 5), (1, 1, 1), (0, 0, 0, 0, 0, 0))
- verify_conv3d_transpose_ncdhw(1, 8, (32, 32, 32), 64, (5, 5, 5), (2, 2, 2), (1, 1, 1, 1, 1, 1))
+ verify_conv3d_transpose_ncdhw(1, 3, (24, 24, 24), 1, (1, 1, 1), (1, 1, 1), (0, 0, 0, 0, 0, 0), (0, 0, 0))
+ verify_conv3d_transpose_ncdhw(1, 3, (24, 24, 24), 2, (3, 3, 3), (1, 1, 1), (0, 0, 0, 0, 0, 0), (0, 0, 0))
+ verify_conv3d_transpose_ncdhw(1, 3, (24, 24, 24), 16, (3, 3, 3), (1, 1, 1), (0, 0, 0, 0, 0, 0), (0, 0, 0))
+ verify_conv3d_transpose_ncdhw(1, 3, (24, 24, 24), 16, (3, 3, 3), (3, 3, 3), (0, 0, 0, 0, 0, 0), (0, 0, 0))
+ verify_conv3d_transpose_ncdhw(1, 3, (24, 24, 24), 16, (3, 3, 3), (3, 3, 3), (0, 0, 0, 0, 0, 0), (2, 2, 2))
+ #verify_conv3d_transpose_ncdhw(1, 3, (24, 24, 24), 16, (3, 3, 3), (3, 3, 3), (0, 0, 0, 0, 0, 0), (1, 0, 2))
Review comment:
Apparently I was way too tired when I read that code last, and I was the one to introduce problems.
I have fixed those and uncommented the test since it now works.
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] abergeron commented on pull request #6065: Change the meaning of conv3d_transpose output_padding to match conv{1,2}d_transpose
Posted by GitBox <gi...@apache.org>.
abergeron commented on pull request #6065:
URL: https://github.com/apache/incubator-tvm/pull/6065#issuecomment-659742964
It seems the master is broken, because some rust test is failing and I didn't touch any of the rust code.
It also seems that the macOS build is flaky because it passed with the first empty commit and failed with the second.
I will hold off on the re-triggers of the CI until I hear the problem has been fixed.
----------------------------------------------------------------
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