nengo-dl
nengo-dl copied to clipboard
Added groups param to convolutions
Motivation and context: There is a (tentatively) accepted pull request for adding grouped convolutions to nengo already. Before adding grouped convolutions to nengo-loihi it is neccessary to add grouped convolutions to nengo-dl.
Related Forum Posts https://forum.nengo.ai/t/adding-a-groups-parameter-for-convolutions-in-nengo-loihi/2035/2 https://forum.nengo.ai/t/trying-to-understand-nengo-transforms-convolution/1844/4?u=mjurado3
Interactions with other PRs: This change is dependent upon this pull requrest in the nengo repository: https://github.com/nengo/nengo/pull/1684
How has this been tested? This is tested through the test_conv in nengo_dl/tests/test_converter.py
How long should this take to review? 30 minutes if there are no additional features that should be added.
Where should a reviewer start?
- First the reviewer should look at ConvIncBuilder and convince themselves that no change is needed in this function for adding grouped convolutions. I was somewhat surprised when the unit tests passed but then I realized that tf.nn.convolution infers the groups parameter from the shape of the weights as shown in this example
- Then the reviewer should verify that test_conv in nengo_dl/tests/test_converter.py provides sufficient coverage.
Types of changes: New-Feature: High Level addition of groups parameter to ConvertConv
Checklist:
- [x] I have read the CONTRIBUTING.rst document.
- [x] I have updated the documentation accordingly.
- [x] I have included a changelog entry.
- [x] I have added tests to cover my changes.
- [x] I have run the test suite locally and all tests passed.
Note to reviewer: when testing things as @Michaeljurado24 has described in the PR description, make sure these things work in the older versions of TF that we support.
Merged, thank you for your contribution!