nengo-dl icon indicating copy to clipboard operation
nengo-dl copied to clipboard

Added groups param to convolutions

Open Michaeljurado42 opened this issue 2 years ago • 1 comments

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?

  1. 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
  2. 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.

Michaeljurado42 avatar Mar 20 '22 19:03 Michaeljurado42

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.

tbekolay avatar May 13 '22 15:05 tbekolay

Merged, thank you for your contribution!

drasmuss avatar Jan 24 '23 00:01 drasmuss