Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DepthWiseConv2dImplicitGEMM has no 'padding' class attribute(actually zero) #48

Open
zzzc18 opened this issue Nov 19, 2022 · 9 comments

Comments

@zzzc18
Copy link

zzzc18 commented Nov 19, 2022

When I ran directly replknet.py with using DepthWiseConv2dImplicitGEMM, I got this error:

RuntimeError: The size of tensor a (56) must match the size of tensor b (26) at non-singleton dimension 3

which seems likely a padding issuse with large kernel size. Then I checked replknet.py and depthwise_conv2d_implicit_gemm.py and found there is no padding parameter.

With debugging, I find out that DepthWiseConv2dImplicitGEMM has no attribute of padding(actually zero), which leads to when calling model.structural_reparam() and ReparamLargeKernelConv.merge_kernel(), incorrect conv2d class will be created (miss the using condtion of DepthWiseConv2dImplicitGEMM and fallback to create a nn.Conv2d with 0 padding).

@zzzc18 zzzc18 changed the title DepthWiseConv2dImplicitGEMM has no padding parameter? DepthWiseConv2dImplicitGEMM has no 'padding' class attribute(actually zero) Nov 19, 2022
@StarTherlyn
Copy link

I got the same problem, have you solved it yet?

@StarTherlyn
Copy link

I got the same problem, have you solved it yet?
I add this at line56:
result.conv.padding = padding

@zzzc18
Copy link
Author

zzzc18 commented Dec 17, 2022

I got the same problem, have you solved it yet?
I add this at line56:
result.conv.padding = padding

I didn't investigate it further, my exp finds author's Conv2d fails to converge on my mission, while nn.Conv2d is fine, so I just ignore them.

@jsczzzk
Copy link

jsczzzk commented Dec 28, 2022

I also got the same problem. Have you solved it yet? @StarTherlyn

@jsczzzk
Copy link

jsczzzk commented Dec 28, 2022

I changed this at line122:
image
and got the results:
image.

Is it right?

@zzzc18
Copy link
Author

zzzc18 commented Jan 1, 2023

I changed this at line122: image and got the results: image.

Is it right?

I think the difference is too large, I recommend you to use nn.Conv2d Instead

@jsczzzk
Copy link

jsczzzk commented Feb 4, 2023

Thank you for your suggestion :)

@liyiersan
Copy link

I changed this at line122: image and got the results: image.

Is it right?

I guess it is right. As can be seen in depthwise_conv2d_implicit_gemm.py in cutlass, nn.Conv2d needs padding while DepthWiseConv2dImplicitGEMM does not.
fig1

I modified line 122 as you did, and the difference is quite small, just 1e-5.
fig2
The difference is accepted to me.
fig3

@liyiersan
Copy link

I changed this at line122: image and got the results: image.

Is it right?

Actually, another modification to convert tuple to int is needed. see #59

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

No branches or pull requests

4 participants