Fixed convtranspose.cc - Githubissues

yskong224 commented 4 months ago

This commit resolves the problem in ConvTranspose where bias is added multiple times. The revised code was tested using ConvTranspose1d in PyTorch.

kraiskil commented 4 months ago

Nice, thank you!

Two small change request:

please remove the two .DS_Store files. I don't think they are needed.
I found two more Convtranspose tests in onnx. Could you enable them in your PR? One of the even fails with master, but works with your branch :) Diff below.

Also, I think the fix is big enough to cause you to be able to claim copyright, so if you want to, please add yourself to the blamelist in LICENCE.txt.

It has been quite a while since I looked into Convtranspose, and I don't quite follow what is happening. To what is exactly is the bias added, and what was wrong in the old code? Why are new for-loops needed? I think the answers to these questions could go into the comments line above your added code. And at the same time you could remove the line that (now wrongly) claims bias is not tested by onnx unit tests... (the docs clearly were not that clear :) )

Much appreciated, thanks.

diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index b01ab87..2384caa 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -337,6 +337,8 @@ ONNX_backend_node_test(convtranspose_pad)
 ONNX_backend_node_test(convtranspose_3d)
 ONNX_backend_node_test(convtranspose_kernel_shape)
 ONNX_backend_node_test(convtranspose_pads)
+ONNX_backend_pytorch_converted_test(ConvTranspose2d)
+ONNX_backend_pytorch_converted_test(ConvTranspose2d_no_bias)

 ONNX_backend_node_test(div)
 ONNX_backend_node_test(div_bcast)

yskong224 commented 4 months ago

Thank you for the positive feedback and creating and managing this amazing project. This is a huge help for a project I am working on. I have addressed all the comments and made changes accordingly. I also appreciate your recognition of the contribution. I agree with the term and have updated the LICENCE.txt.

Regarding what was happening in the previous code, bias was added multiple or zero times to some elements in the output matrix in certain situations. For example, this can happen if stride size and kernel size are different. Therefore, I removed the bias addition part in the previous code and created a new loop to add bias to all elements for each channel. In this way, bias is added only once to all elements for each channel (if there is bias).

Thank you again!

kraiskil commented 4 months ago

LGTM, merged.

I think I see the bug I had in the code now. Admittedly, the source for onnx2c is not always the easiest to read...

Thanks for your contribution!

P.s.

This is a huge help for a project I am working on.

The "show and tell" part of the discussions is sadly empty. Feel free to drop a link to your project there :)

yskong224 commented 4 months ago

Many thanks for merging!

For the "show and tell", I will be able to share something once the research (the work) is published 😁

kraiskil / onnx2c

Fixed convtranspose.cc #40