nengo / nengo-loihi

Run Nengo models on Intel's Loihi chip
https://www.nengo.ai/nengo-loihi/
Other
35 stars 12 forks source link

Splitter refactoring #202

Closed arvoelke closed 5 years ago

arvoelke commented 5 years ago

Closes #80. Refactors the splitter logic for adding / removing connections to occur within the builder. This consolidates the connection logic into one place (builder/connection.py) so that backend-relevant logic and objects are manipulated at the same level of abstraction.

Overarching ideas:

Solves the following bugs:

Random improvements:

Bugs that have been identified that are outside the scope of this PR:

Existing issues that should now be more readily achievable:

Other future work:

arvoelke commented 5 years ago

I ran @tcstewar's benchmark notebook with:

conditions = [
    'nengo',                                         # the normal nengo backend
    'nengo_loihi(precompute=False, target="loihi")', # using Loihi itself
    'nengo_loihi(precompute=True, target="loihi")',  # using Loihi itself with precompute
]

on both master and this branch.

On this branch, precompute=False and precompute=True gave identical accuracy (but with different speeds) across all benchmarks and so I modified a simple unit test (45eced7) to verify this. The learning benchmarks aren't supposed to work with precompute=True (#214) and so I added a commit (433b489) to check this and improve the error message.

On master, precompute=False and precompute=True gave different results for ['CircularConvolution', 'MatrixMultiply', 'SPASequence'], which I presume has something to do with some interaction between remove_passthroughs=True and precompute with the seeding of the network in the old splitter code (also see issue #210).

arvoelke commented 5 years ago

Pushed two more commits (1711044, 0ca11d8) to move LoihiInput and SpikeInput from builder/inputs.py back into inputs.py and move build_conv2d_connection from conv.py to builder/connections.py. This should now address all your comments @hunse.

arvoelke commented 5 years ago

Added a commit (6c201e0) that increases test coverage since I touched the lines of some un-tested convolution code.

hunse commented 5 years ago

Just a few minor comments. Otherwise everything LGTM. Thanks for doing all those tests against the benchmarks.

arvoelke commented 5 years ago

I added a couple very minor commits for coverage / sanity. I also rebased onto master again to pick up the testing fixes from the current/rates sync-up.

5694c27 Is Split a good name? Or should we stick with Splitter, SplitterDirective, or some other option? 15890be Similarly, is PassthroughSplit a good name? ea6d52e Should this be a separate PR or is it OK to include here?

LGTM!

16e322a Is get_seed something we want built into Model? And if so, do we do it now or make a separate issue for it?

I think it's fine as is. A bunch of the weirdness with seeding can be resolved when we eventually drop support for nengo<3. This could end up simplifying a bunch of this code.

f64b102 Are BuildErrors sufficient here or do we need to make a new NengoException subclass?

This LGTM, but we should make a card to go through the rest of the code base? e..g, There are a bunch of ValueError exceptions in hardware/builder.py.

arvoelke commented 5 years ago

I just noticed that the simulator docstring says the default for precompute=True. We should fix that here IMO.

tbekolay commented 5 years ago

We should fix that here IMO.

OK, will do. Running into some issues getting the hardware build to pass so I'll be doing at least one more push anyhow.