Fix output_shape of a testcase for ConvTranspose #1437
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
As per the documentation of the operator, `output_shape` seems to expect the same rank as other attributes such as `strides` or `pads`. In other words, `output_shape` should not have dimensions for batch size and number of channels. So this changes `output_shape` from [1, 2, 10, 8] to [10, 8] for the 2D ConvTranspose. Note that test_convtranspose_kernel_shape, the other test which uses `output_shape`, is already OK.
|
|
|
I failed to get an attention in #1158 so decided to send a PR to see if my understanding is right. Comments are welcomed, thanks! |
zrphercule
reviewed
Sep 21, 2018
There was a problem hiding this comment.
Hi shinh! Sorry didnt reply you in #1158 .
I have reviewed your commit, and it seems you are right. Altought whether to use the full shape(NCHW instead of HW) is still considerable now, it does not make sense if there is inconsistency.
Could you please update the document by running
onnx/defs/gen_doc.py
onnx/gen/proto.py (I guess you dont have to run this)
onnx/backend/test/stat_coverage.py
and also upload new test data for convtranspose?
Thanks for your contribution to ONNX!
Note there are a few unrelated updates.
|
Thanks for your quick review! I've updated those documents. I think my original commit had updated model.onnx and test_data_set_0 was unchanged since this PR changes inputs/outputs. I've just double-checked there are no update by doing |
|
I'm not sure why stat_coverage.py used vgg16 for me although it was renamed, but anyway, now the tests seem to be passing. |
|
@shinh Hi shinh, I wonder if this is ready to be merged? |
|
Yes, I believe this PR is OK. Thanks again for your review! |
hariharans29
pushed a commit
to hariharans29/onnx
that referenced
this pull request
Aug 15, 2019
* Fix output_shape of a testcase for ConvTranspose As per the documentation of the operator, `output_shape` seems to expect the same rank as other attributes such as `strides` or `pads`. In other words, `output_shape` should not have dimensions for batch size and number of channels. So this changes `output_shape` from [1, 2, 10, 8] to [10, 8] for the 2D ConvTranspose. Note that test_convtranspose_kernel_shape, the other test which uses `output_shape`, is already OK. * Update the document by gen_doc.py * Update test coverage by stat_coverage.py Note there are a few unrelated updates. * Revert unrelated update in the previous commit
jcwchen
pushed a commit
to jcwchen/onnx
that referenced
this pull request
Sep 23, 2020
* Fix output_shape of a testcase for ConvTranspose As per the documentation of the operator, `output_shape` seems to expect the same rank as other attributes such as `strides` or `pads`. In other words, `output_shape` should not have dimensions for batch size and number of channels. So this changes `output_shape` from [1, 2, 10, 8] to [10, 8] for the 2D ConvTranspose. Note that test_convtranspose_kernel_shape, the other test which uses `output_shape`, is already OK. * Update the document by gen_doc.py * Update test coverage by stat_coverage.py Note there are a few unrelated updates. * Revert unrelated update in the previous commit
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jul 10, 2023
…els (#5400) ### Description <!-- - Describe your changes. --> Add a sentence to highlight that output_shape for ConvTranspose should not have batch and channels. ### Motivation and Context <!-- - Why is this change required? What problem does it solve? --> <!-- - If it fixes an open issue, please link to the issue here. --> It's quite confusing that whether output_shape needs to be fully provided. For instance, #1437. An explicit description should help prevent confusion. cc @satyajandhyala --------- Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
adityagoel4512
pushed a commit
to adityagoel4512/onnx
that referenced
this pull request
Jul 10, 2023
…els (onnx#5400) ### Description <!-- - Describe your changes. --> Add a sentence to highlight that output_shape for ConvTranspose should not have batch and channels. ### Motivation and Context <!-- - Why is this change required? What problem does it solve? --> <!-- - If it fixes an open issue, please link to the issue here. --> It's quite confusing that whether output_shape needs to be fully provided. For instance, onnx#1437. An explicit description should help prevent confusion. cc @satyajandhyala --------- Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com> Signed-off-by: Aditya Goel <agoel4512@gmail.com>
hamptonm1
pushed a commit
to hamptonm1/onnx
that referenced
this pull request
Jul 10, 2023
…els (onnx#5400) ### Description <!-- - Describe your changes. --> Add a sentence to highlight that output_shape for ConvTranspose should not have batch and channels. ### Motivation and Context <!-- - Why is this change required? What problem does it solve? --> <!-- - If it fixes an open issue, please link to the issue here. --> It's quite confusing that whether output_shape needs to be fully provided. For instance, onnx#1437. An explicit description should help prevent confusion. cc @satyajandhyala --------- Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com> Signed-off-by: Megan Hampton <hamptonm@us.ibm.com>
adityagoel4512
pushed a commit
to adityagoel4512/onnx
that referenced
this pull request
Jul 28, 2023
…els (onnx#5400) ### Description <!-- - Describe your changes. --> Add a sentence to highlight that output_shape for ConvTranspose should not have batch and channels. ### Motivation and Context <!-- - Why is this change required? What problem does it solve? --> <!-- - If it fixes an open issue, please link to the issue here. --> It's quite confusing that whether output_shape needs to be fully provided. For instance, onnx#1437. An explicit description should help prevent confusion. cc @satyajandhyala --------- Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com> Signed-off-by: Aditya Goel <agoel4512@gmail.com>
adityagoel4512
pushed a commit
to adityagoel4512/onnx
that referenced
this pull request
Jul 28, 2023
…els (onnx#5400) ### Description <!-- - Describe your changes. --> Add a sentence to highlight that output_shape for ConvTranspose should not have batch and channels. ### Motivation and Context <!-- - Why is this change required? What problem does it solve? --> <!-- - If it fixes an open issue, please link to the issue here. --> It's quite confusing that whether output_shape needs to be fully provided. For instance, onnx#1437. An explicit description should help prevent confusion. cc @satyajandhyala --------- Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com> Signed-off-by: Aditya Goel <agoel4512@gmail.com>
adityagoel4512
pushed a commit
to adityagoel4512/onnx
that referenced
this pull request
Jul 28, 2023
…els (onnx#5400) ### Description <!-- - Describe your changes. --> Add a sentence to highlight that output_shape for ConvTranspose should not have batch and channels. ### Motivation and Context <!-- - Why is this change required? What problem does it solve? --> <!-- - If it fixes an open issue, please link to the issue here. --> It's quite confusing that whether output_shape needs to be fully provided. For instance, onnx#1437. An explicit description should help prevent confusion. cc @satyajandhyala --------- Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com> Signed-off-by: Aditya Goel <agoel4512@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As per the documentation of the operator,
output_shapeseemsto expect the same rank as other attributes such as
stridesor
pads. In other words,output_shapeshould not havedimensions for batch size and number of channels. So this
changes
output_shapefrom [1, 2, 10, 8] to [10, 8] for the2D ConvTranspose.
Note that test_convtranspose_kernel_shape, the other test which
uses
output_shape, is already OK.