Skip to content

Conversation

sdua-nv
Copy link
Contributor

@sdua-nv sdua-nv commented Mar 9, 2023

Hi, I noticed a problem in the tf2onnx converter. This is regarding ConvTranspose converter (https://github.com/onnx/tensorflow-onnx/blob/main/tf2onnx/onnx_opset/nn.py#L428). When ConvTranspose is quantized, the converter should take a ConvTranspose layer and convert it as follows.

X --->  ConvTranspose
W -----^

now becomes

X ----Q_x -> DQ_x ----->  ConvTranspose
W --> Q_w -> DQ_w -> ----------^

These Q_w/DQ_w nodes that are inserted have the wrong axis set when they are modified by the tf2onnx converter. For ConvTranspose, the axis should be set to 1. This is the number of filters, which is Cout. (The kernel shape here is Cin x Cout x kH x kW)

For Conv, the axis should be (and is correctly) set to 0. (The kernel shape here is Cout x Cin x kH x kW)

Instead, this axis is unconditionally set to 0 for all conv layers that use the conv_convert_inputs() routine.

I've provided a fix in this PR. I had some trouble with the tests I added so I needed to add a mechanism to skip checking shapes in Tf2OnnxBackendTestBase.run_test_case; it seems that no other test sets check_shape=False so this shouldn't change the behavior of anything else.

@sdua-nv sdua-nv force-pushed the dev-sdua-fix-contranspose-qdq-axis branch 4 times, most recently from a25de24 to a78e1d2 Compare March 10, 2023 00:28
@fatcat-z
Copy link
Collaborator

The unit_test CI failure was not caused by your change, will fix it soon.

@sdua-nv sdua-nv force-pushed the dev-sdua-fix-contranspose-qdq-axis branch 3 times, most recently from ecbdc78 to 429ecb0 Compare March 14, 2023 17:05
The quantization axis of QDQ nodes that are being inserted
before the kernel weights of all Conv nodes is currently 0.
This is incorrect; ConvTranspose requires axis=1.

Signed-off-by: Sirej Dua <[email protected]>
@sdua-nv sdua-nv force-pushed the dev-sdua-fix-contranspose-qdq-axis branch from 429ecb0 to e8413c7 Compare March 14, 2023 17:07
Copy link
Collaborator

@fatcat-z fatcat-z left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contributions!

@fatcat-z fatcat-z enabled auto-merge (squash) March 15, 2023 07:14
@fatcat-z fatcat-z merged commit ce29107 into onnx:main Mar 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants