Skip to content
This repository has been archived by the owner on Mar 12, 2021. It is now read-only.

WIP: Add wrapper functions for setting/getting convolution group count #388

Closed
wants to merge 2 commits into from
Closed

WIP: Add wrapper functions for setting/getting convolution group count #388

wants to merge 2 commits into from

Conversation

dbadrian
Copy link

@dbadrian dbadrian commented Aug 9, 2019

Based on a discussion over at Flux ( FluxML/Flux.jl#459 and FluxML/Flux.jl#330 ) @avik-pal proposed the inclusion of new functions in libcudnn.jl, namely to set the group count for a group convolution.

Further changes to other functions to actually enable group convolutions (and by that depthwise conv on the gpu), could be added to this PR based on discussion and support.
The latter because this is prolly out of depth at this point in time and will require some more diggin in.

Hopefully @avik-pal can add some more hints on what should be done :).

@avik-pal
Copy link
Contributor

Reposting from the original thread

The next thing would be to modify the ConvDesc call here to call the wrapped function first.

You can pass the group count as a keyword argument maybe, but even I am not certain if this is the right way or we should modify NNlib cdims to incorporate it internally. @dhairyagandhi96 thoughts on the best way to do this?

@dbadrian
Copy link
Author

dbadrian commented Aug 13, 2019

As avik-pal pointed out, there could be a few potential points of integration, but I don't have the overview to see which are feasible and sensible.

Would be great to get some feedback from the library maintainers and I am happy to try draft up an implementation based on it.

Supporting group convolution is highly relevant to efficiently implement several modern NN architectures.

@DhairyaLGandhi
Copy link
Member

I believe the correct way would be to capture it in the cdims object.

Pinging @staticfloat for his thoughts

@staticfloat
Copy link
Contributor

Yes; we can make the DepthwiseConvDims object instead be a special-case of a GroupConvDims object, where you specify the group number (where depthwise conv is when the number of groups == number of channels).

This will require us to write an implementation of grouped convolution so that we can test all our implementations against eachother, but that shouldn't be too hard.

@dbadrian
Copy link
Author

dbadrian commented Aug 15, 2019

True and along the same line of argumentation a regular convolution is simply a special case with groupcount==1 (the default value)?

@staticfloat
Copy link
Contributor

Yes, that's true. So really, we could do away with DepthwiseConvDims entirely, add a groupcount parameter to ConvDims, then have optimized implementations for groupcount == 1, groupcount == numchannels and have a fallback implementation for the general case. I'm not entirely sure how to express the groupcount == numchannels case in the type system, but I'm sure we'll figure it out. ;)

@DhairyaLGandhi
Copy link
Member

I like the sound of that, Avik had a similar thing in mind as well

@maleadt maleadt changed the title Add wrapper functions for setting/getting convolution group count WIP: Add wrapper functions for setting/getting convolution group count Aug 23, 2019
@maleadt
Copy link
Member

maleadt commented Dec 2, 2019

Concerning this PR: the API wrapper is already in CuArrays, so we only need an API: #523

@maleadt maleadt closed this Dec 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants