Skip to content
New issue

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

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

Already on GitHub? # to your account

Added groups param to convolutions #223

Merged
merged 2 commits into from
Jan 24, 2023

Conversation

Michaeljurado42
Copy link

@Michaeljurado42 Michaeljurado42 commented Mar 20, 2022

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: nengo/nengo#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:

  • I have read the CONTRIBUTING.rst document.
  • I have updated the documentation accordingly.
  • I have included a changelog entry.
  • I have added tests to cover my changes.
  • I have run the test suite locally and all tests passed.

@Michaeljurado42 Michaeljurado42 changed the title Added groups param to regular convolutions Added groups param to convolutions Mar 20, 2022
CHANGES.rst Outdated
@@ -20,6 +20,9 @@ Release history

3.4.5 (unreleased)
------------------
**Added**

- Added ``groups`` parameter to ConvertConv
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should have a period at the end, and link to the pull request. See the other entries in this file for examples.

@tbekolay
Copy link
Member

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.

@drasmuss drasmuss force-pushed the Convolution-groups branch 2 times, most recently from d66fad7 to 8af99e2 Compare January 23, 2023 23:36
Michael Jurado and others added 2 commits January 23, 2023 20:26
Co-authored-by: Daniel Rasmussen <daniel.rasmussen@appliedbrainresearch.com>
@drasmuss drasmuss merged commit 216d4d0 into nengo:main Jan 24, 2023
@drasmuss
Copy link
Member

Merged, thank you for your contribution!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants