Skip to content

Allow 'same' padding for convolution#297

Merged
tbekolay merged 5 commits intomasterfrom
same-padding
Dec 13, 2021
Merged

Allow 'same' padding for convolution#297
tbekolay merged 5 commits intomasterfrom
same-padding

Conversation

@hunse
Copy link
Copy Markdown
Contributor

@hunse hunse commented Sep 10, 2020 •
edited
Loading

This allows "same" padding to be used with nengo.Convolution.

TODO:

  • More testing (e.g. in test_conv_deepnet)

@hunse hunse force-pushed the same-padding branch 2 times, most recently from bed1098 to c724ed7 Compare October 13, 2020 20:48
@hunse
Copy link
Copy Markdown
Contributor Author

hunse commented Oct 13, 2020

Added an error for the case shown in #298

@hunse
Copy link
Copy Markdown
Contributor Author

hunse commented Oct 14, 2020

This depends on #296

@hunse hunse mentioned this pull request Oct 23, 2020
Merged
@hunse hunse force-pushed the same-padding branch 2 times, most recently from db1ed40 to b244ec4 Compare January 11, 2021 20:41
@hunse
Copy link
Copy Markdown
Contributor Author

hunse commented Jan 11, 2021

Requires nengo/nengo#1648 for the fix to npconv2d padding for the test, so I think it's easier to just merge this as part of #300.

@hunse hunse mentioned this pull request Nov 25, 2021
Merged
@hunse hunse force-pushed the same-padding branch 2 times, most recently from 2b119be to 9225ca3 Compare December 3, 2021 20:40
tbekolay
tbekolay approved these changes Dec 13, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@tbekolay tbekolay left a comment

Choose a reason for hiding this comment

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

Made some fixups so that we always test on both Nengo master and release, and changed the require_partition helper to a pytest marker as we were only using require_partition to have multichip snips work.

We are getting some random test failures even with these changes, but I've run those tests manually on INRC and they pass when things don't get desynced, so I'm going to merge even if we get some desyncs in this final test run.

hunse and others added 5 commits December 13, 2021 10:55
@hunse @tbekolay
Allow 'same' padding for convolution
3c8c7b0 
Also change test_pop_tiny with ``channels_last=False`` to properly
order the biases and thus properly test with the channels first.
@hunse @tbekolay
Error for non-dense chip2host connections
a5b4861 
These are not supported, but were failing with cryptic errors.
@hunse @tbekolay
Fix bug with convolution for unit kernel size
ed78e00 
The padding now matches `nengo._vendor.npconv2d` exactly.

Also clean up convolution code.
@hunse @tbekolay
Test against NengoCore master branch
9bf6803 
We test against master and release for the emulator, and
only master for hardware since it's expensive to run.
@tbekolay
Add requires_multichip_snips marker
f896b72
@tbekolay
Copy link
Copy Markdown
Member

tbekolay commented Dec 13, 2021

It's still intermittently failing and/or stalling, but I'm at least convinced that it's (mostly) INRC environment issues, so I'm going to merge regardless. We can clean up any issues when all the v1.1.0 PRs are in.

@tbekolay tbekolay merged commit f896b72 into master Dec 13, 2021
@tbekolay tbekolay deleted the same-padding branch December 13, 2021 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tbekolay tbekolay tbekolay approved these changes

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@hunse @tbekolay