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

Add grouped convolution #1531

Merged
merged 18 commits into from
Jul 22, 2021
Merged

Add grouped convolution #1531

merged 18 commits into from
Jul 22, 2021

Conversation

DhairyaLGandhi
Copy link
Member

Add support for grouped convolutions. Needed for a bunch of things. Depends on an NNlib branch where the support is present in the Dims Helpers, but needs to be chained through to the conv kernel. Although we now can generate the correct shaped kernels and filters here.

@lorenzoh
Copy link
Member

Do the grouped convolutions run on the GPU as well?

@DhairyaLGandhi
Copy link
Member Author

This PR adds the necessary changes to the dims objects that we can use to pass the groups to CuDNN.

@DhairyaLGandhi
Copy link
Member Author

Seems like we are good to merge FluxML/NNlib.jl#289

test/runtests.jl Outdated Show resolved Hide resolved
@CarloLucibello
Copy link
Member

Needs some tests. Manifest can be removed

@DhairyaLGandhi
Copy link
Member Author

Going by how many releases of NNlib were needed to get the Embedding layer merged into Flux, it seemed to me that development could have been a lot faster and releases a lot safer had a Manifest been used. Releases aren't free since they affect unsuspecting users the most.

@CarloLucibello
Copy link
Member

development with a Manifest would have gone at exactly the same speed. Something that did slow down a bit development is the separation of NNlib and NNlibCUDA. Releases are free as long as they are non-breaking, they don't affect users that don't use the new features

@CarloLucibello
Copy link
Member

Anyways, Manifest should not be discussed here, this PR is meant to add groups support not to reinstate the manifest, let's not derail the PR

@DhairyaLGandhi
Copy link
Member Author

DhairyaLGandhi commented Jul 19, 2021

Agreed, that I don't want to derail the PR but:

development with a Manifest would have gone at exactly the same speed.

A manifest would have shown the issues with every CI run, without having to make disruptive releases. Not to mention the time it takes to make them, and the buggy code that is pushed out to users

don't affect users that don't use the new features

Features are features, new or old. Sure bug fixes will come in faster for new ones than the old ones but if it is known that something isn't quite right, then why release?

the separation of NNlib and NNlibCUDA

The issue is the existence of the separation. The release schedule would have been the same even if in the same repo, and with a Manifest, the gap of where packages live is made significantly smaller

@CarloLucibello
Copy link
Member

Agreed, that I don't want to derail the PR but:

but then you do. I disagree with everything you said, but again, this is not the right place to have that discussion

@CarloLucibello
Copy link
Member

a news entry would be good

@DhairyaLGandhi
Copy link
Member Author

bors r+

@bors
Copy link
Contributor

bors bot commented Jul 22, 2021

Build succeeded:

@bors bors bot merged commit eb5077f into master Jul 22, 2021
ven-k added a commit to ven-k/Flux.jl that referenced this pull request Aug 16, 2021
[Diff since v0.12.5](FluxML/Flux.jl@v0.12.5...v0.12.6)

**Merged pull requests:**
- Add grouped convolution (FluxML#1531) (@DhairyaLGandhi)
- fix deprecations of zeros (FluxML#1670) (@DhairyaLGandhi)
- Add GPU activation tests for grouped conv (FluxML#1672) (@DhairyaLGandhi)
@CarloLucibello CarloLucibello deleted the groups branch April 7, 2022 07:03
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.

3 participants