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.

Sign up for GitHub

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

Jump to bottom

Update connections to work with new Transform API #142

Merged
merged 4 commits into from
Feb 20, 2019
Merged

Update connections to work with new Transform API #142

merged 4 commits into from
Feb 20, 2019

Conversation

drasmuss
Copy link
Member

@drasmuss drasmuss commented Nov 19, 2018 •
edited
Loading

This gets all the tests passing with nengo master. Note that this does not support the new Convolution transforms, it just brings the existing nengo-loihi functionality up-to-date with nengo core. There are still several open PRs related to the nengo-loihi convolution implementation, so I figured I would wait for those to be resolved before trying to apply those changes.

Also note that right now I've done this in a non-backwards-compatible way (i.e. it does not work with nengo 2.8.0). Since nengo-loihi is pretty "in development", requiring the most up-to-date version of Nengo (assuming there is a new nengo release before the next nengo-loihi release) seems reasonable. But we could make it backwards compatible, it just adds a little extra complexity and maintenance burden.

TODO:

  • improve test coverage
  • update conv example notebook

hunse
hunse reviewed Nov 19, 2018
View reviewed changes
nengo_loihi/splitter.py Outdated Show resolved Hide resolved
nengo_loihi/builder.py Outdated Show resolved Hide resolved
nengo_loihi/builder.py Outdated Show resolved Hide resolved
nengo_loihi/builder.py Outdated Show resolved Hide resolved
@arvoelke arvoelke mentioned this pull request Dec 6, 2018
Closed
@drasmuss
Copy link
Member Author

drasmuss commented Dec 28, 2018 •
edited
Loading

Rebased this to master, and added support for nengo.Convolution transforms. All the tests are passing, but I'm not confident that that means things are working, as the test coverage is a bit sparse. For example, it looks like a lot of the tests were set up as if they should test different conditions, but then those conditions aren't being tested (e.g. e39c54b#diff-2b5472730de7e72d3c4c167f986643d2R46, e39c54b#diff-2b5472730de7e72d3c4c167f986643d2R285, e39c54b#diff-2b5472730de7e72d3c4c167f986643d2R359, e39c54b#diff-2b5472730de7e72d3c4c167f986643d2R397). But I'm not sure if those un-tested conditions are expected to be working or not. So need @hunse to look it over, since he is most familiar with the convolution code (and might have other examples that could be made into tests).

hunse
hunse reviewed Jan 4, 2019
View reviewed changes
nengo_loihi/conv.py Outdated Show resolved Hide resolved
nengo_loihi/conv.py Show resolved Hide resolved
@hunse hunse mentioned this pull request Feb 1, 2019
Merged
@hunse
Copy link
Collaborator

hunse commented Feb 1, 2019 •
edited
Loading

  • The xfail tests in test_conv.py should all work on the emulator, and currently they're not. I think it's just syntax changes related to this PR that were never fixed. So a reminder to do that before we merge.

@hunse hunse self-assigned this Feb 1, 2019
@drasmuss drasmuss force-pushed the nengo-update branch 2 times, most recently from 72afaa7 to 28adc10 Compare February 8, 2019 21:12
hunse
hunse requested changes Feb 19, 2019
View reviewed changes
.travis.yml Show resolved Hide resolved
docs/examples/mnist_convnet.ipynb
" layer, conv = conv_layer(layer, 6, conv.output_shape, strides=2)\n",
" layer, conv = conv_layer(layer, 24, conv.output_shape, strides=2)\n",
" layer, conv = conv_layer(layer, 6, conv.output_shape, \n",
" strides=(2, 2))\n",
Copy link
Collaborator

Choose a reason for hiding this comment

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

It'd be nice to be able to pass an int and have it broadcast like before, since in my experience almost all the time you want the same strides in both directions. I realize that a) this would be a Nengo change and b) it would make it different than kernel_size which needs a tuple (since that specifies the number of dimensions for the convolution). I just wanted to raise the point here.

nengo_loihi/builder/connection.py Show resolved Hide resolved
nengo_loihi/compat.py Show resolved Hide resolved
@drasmuss @hunse
Update connections to work with new Transform API
5e221cd 
Also fix the passthrough logic for non-array transforms.

This is backwards compatibile with Nengo 2.8 transforms,
where we did not have Transform types (all transforms were arrays).
@hunse hunse force-pushed the nengo-update branch 2 times, most recently from 0271a86 to 803dded Compare February 20, 2019 21:46
@drasmuss @hunse
Add support for Nengo convolution transforms
a9d5df1 
And update the MNIST example with new syntax and parameter files.

This is not backwards compatible: We do not support the new
convolution code with older Nengo, since much of the supporting code
is in new Nengo. However, this is not a significant regression,
since convolution only worked briefly with old Nengo.
@drasmuss @hunse
Add nengo dev version builds to travis-ci
072bad3 
Also separate builds into basic and advanced stages.

This also makes test_learning.test_multiple_pes more robust;
lenthening the runtime seems to make it fail much less often.
@drasmuss @hunse
Upload coverage reports even if tests fail
7176179
@hunse hunse merged commit 7176179 into master Feb 20, 2019
@hunse hunse deleted the nengo-update branch February 20, 2019 23:10
@drasmuss drasmuss mentioned this pull request Feb 22, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hunse hunse hunse approved these changes

Assignees

@hunse hunse

Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@drasmuss @hunse