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 2022/06/11 11:28:28 UTC
[GitHub] [tvm] wzh99 opened a new pull request, #11681: [Bugfix] Shape inference of weight for grouped `nn.conv3d`
wzh99 opened a new pull request, #11681:
URL: https://github.com/apache/tvm/pull/11681
This PR fixes #11664 according to the proposal in that issue. A test case is added as requested by @ganler.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] wzh99 commented on pull request #11681: [Bugfix] Shape inference of weight for grouped `nn.conv3d`
Posted by GitBox <gi...@apache.org>.
wzh99 commented on PR #11681:
URL: https://github.com/apache/tvm/pull/11681#issuecomment-1153073465
@ganler Thanks for your suggestion. However, I am sorry that I cannot completely agree that this test should be placed in [test_type_infer.py](https://github.com/apache/tvm/blob/main/tests/python/relay/test_type_infer.py). The reason is that the tests in this file are concerned with the language features of Relay itself (tuple, ref, let-binding, ADT, etc.) There are some test cases about operators such as `test_add_broadcast_op`, but I think they are mainly used to test whether the type inference in Relay can support different forms of type relations, instead of whether the type relations of a specific operator is correct.
I still think that the test case should be placed in [test_op_level2.py](https://github.com/apache/tvm/blob/main/tests/python/relay/test_op_level2.py), because this is the place for testing specific type relations of `nn.conv3d`. I can add the following lines at the end of `test_conv3d_infer_type`:
```
# Infer with groups
x = relay.var("x", relay.TensorType((1, 16, 224, 224, 224), "float32"))
w = relay.var("w", relay.TensorType((4, 4, 1, 1, 1), "float32"))
y = relay.nn.conv3d(x, w, groups=4, kernel_size=(1, 1, 1), channels=4)
yy = run_infer_type(y)
assert yy.checked_type == relay.TensorType((1, 4, 224, 224, 224), "float32")
```
I am new to contributing to TVM and my understanding may not be thorough enough. Please point out if I make any mistakes.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] ganler commented on pull request #11681: [Bugfix] Shape inference of weight for grouped `nn.conv3d`
Posted by GitBox <gi...@apache.org>.
ganler commented on PR #11681:
URL: https://github.com/apache/tvm/pull/11681#issuecomment-1153001882
Thanks @wzh99! Can you further confirm that the new test will fail before the fix 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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] wzh99 commented on pull request #11681: [Bugfix] Shape inference of weight for grouped `nn.conv3d`
Posted by GitBox <gi...@apache.org>.
wzh99 commented on PR #11681:
URL: https://github.com/apache/tvm/pull/11681#issuecomment-1153043424
> Thanks @wzh99! Can you further confirm that the new test will fail before the fix commit?
I revert the commit but keep the new test in my local repository. The new test in test_topi_conv3d_ncdhw.py does not fail before the commit. It seems that TOPI can handle this case correctly. The bug I reported in #11664 is only concerned with type inference in Relay but not TOPI. Perhaps we need to add a test case in `test_conv3d_infer_type` of test_op_level2.py if we need to test type inference.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] ganler commented on pull request #11681: [Bugfix] Shape inference of weight for grouped `nn.conv3d`
Posted by GitBox <gi...@apache.org>.
ganler commented on PR #11681:
URL: https://github.com/apache/tvm/pull/11681#issuecomment-1152977810
Hi @masahi could you help enable the CI? 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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] wzh99 commented on pull request #11681: [Bugfix] Shape inference of weight for grouped `nn.conv3d`
Posted by GitBox <gi...@apache.org>.
wzh99 commented on PR #11681:
URL: https://github.com/apache/tvm/pull/11681#issuecomment-1153041663
> Thanks @wzh99! Can you further confirm that the new test will fail before the fix commit?
I revert the commit but keep the new test in my local repository. The new test in test_topi_conv3d_ncdhw.py does not fail before the commit. It seems that TOPI can handle this case correctly. The bug I reported in #11664 is only concerned with type inference in Relay but not TOPI. Perhaps we need to add a test case in `test_conv3d_infer_type` of test_op_level2.py if we need to test type inference.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi merged pull request #11681: [Bugfix] Shape inference of weight for grouped `nn.conv3d`
Posted by GitBox <gi...@apache.org>.
masahi merged PR #11681:
URL: https://github.com/apache/tvm/pull/11681
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] ganler commented on pull request #11681: [Bugfix] Shape inference of weight for grouped `nn.conv3d`
Posted by GitBox <gi...@apache.org>.
ganler commented on PR #11681:
URL: https://github.com/apache/tvm/pull/11681#issuecomment-1153061005
@wzh99 Thanks for digging into the issue. Then I think a proper place to put this unit test can be: https://github.com/apache/tvm/blob/main/tests/python/relay/test_type_infer.py
The test can be something like:
```python
def test_conv3d_type_infer():
x = relay.var("x", shape=(1, 4, 8, 8, 8), dtype="float32")
out = relay.nn.conv3d(
x,
relay.var("weight", shape=(2, 2, 1, 1, 1), dtype="float32"),
groups=2,
channels=2,
kernel_size=[1, 1, 1],
)
mod = tvm.IRModule.from_expr(out)
mod = transform.InferType()(mod)
tvm.ir.assert_structural_equal(
mod["main"].body.checked_type,
relay.TensorType((...), "float32") # The correct output shape
)
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] ganler commented on pull request #11681: [Bugfix] Shape inference of weight for grouped `nn.conv3d`
Posted by GitBox <gi...@apache.org>.
ganler commented on PR #11681:
URL: https://github.com/apache/tvm/pull/11681#issuecomment-1153077582
@wzh99 I think what you said makes sense as tests in `test_op_level2.py` shares the similar patterns. Previously I suggest `test_type_infer` as I found the bug-introducing point is in the type inference pass.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] wzh99 commented on pull request #11681: [Bugfix] Shape inference of weight for grouped `nn.conv3d`
Posted by GitBox <gi...@apache.org>.
wzh99 commented on PR #11681:
URL: https://github.com/apache/tvm/pull/11681#issuecomment-1153090465
@ganler I have added the test case in [test_op_level2.py](https://github.com/apache/tvm/blob/main/tests/python/relay/test_op_level2.py). Before the fix is committed, the test fails. As the error message emitted by pytest is long, I only show the part from Relay type inference:
```
The Relay type checker is unable to show the following types match:
Tensor[(16, 0, 1, 1, 1), float32]
Tensor[(4, 4, 1, 1, 1), float32]
In particular:
dimension 0 conflicts: 16 does not match 4. dimension 1 conflicts: 0 does not match 4.
The Relay type checker is unable to show the following types match.
In particular `Tensor[(4, 4, 1, 1, 1), float32]` does not match `Tensor[(16, 0, 1, 1, 1), float32]`
```
After the commit fix, this test is passed.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org