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.

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

Splitter refactoring #202

Merged
merged 13 commits into from Apr 26, 2019

Conversation

Projects
None yet
3 participants
@arvoelke
Copy link
Contributor

commented Mar 26, 2019

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:

  • The splitter returns "directives" (i.e., sets of instructions) to guide the builder.
  • The builder fills in all three "sub-models" (self, host, and host_pre) in parallel, by delegating the objects to the appropriate builder methods.

Solves the following bugs:

  • #205 - Splitter handles sliced probes.
  • #206 - Passthrough removal handles sliced probes.
  • #207 - Decoder cache is not globally disabled.
  • #208 - Improved chip -> chip learning signal message.
  • #209 - Improved chip -> chip error signal message.
  • #211 - Splitter won't mutate original network.

Random improvements:

  • split_pre_from_host is no longer O(n^2) (This was a misunderstanding on my part, fueled by the fact the variable named "queue" was actually a stack -- it is now actually a queue)
  • __contains__ is no longer O(n)

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

  • #210 - Passthrough connections aren’t seeded, and the order is non-deterministic.
  • #212 - Passthrough removal is too aggressive.
  • #213 - Passthrough removal is broken.
  • #214 - precompute=True could be smarter.

Existing issues that should now be more readily achievable:

  • #163 - Passing instance config parameters to builder
  • #154 - sim.data does not contain chip <-> host connections
  • #145 - Efficient passthrough nodes
  • #118 - Indicate which parts of a model are running on/off chip
  • #4 - Expose params and handle splitter-created objects

Other future work:

  • Ability to pass in network=None.
  • Change assert self.model.dt == dt to an error and then unit test (nengo/nengo#1517).

@arvoelke arvoelke requested a review from hunse Apr 1, 2019

@hunse hunse force-pushed the splitter-refactoring branch 2 times, most recently from df54ce5 to de2b772 Apr 3, 2019

Show resolved Hide resolved nengo_loihi/builder/node.py Outdated
Show resolved Hide resolved nengo_loihi/builder/tests/test_connection.py
Show resolved Hide resolved nengo_loihi/splitter.py Outdated
Show resolved Hide resolved nengo_loihi/splitter.py Outdated
Show resolved Hide resolved nengo_loihi/splitter.py Outdated
@arvoelke

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

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

Show resolved Hide resolved nengo_loihi/builder/connection.py Outdated
Show resolved Hide resolved nengo_loihi/builder/builder.py Outdated
@hunse

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

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

@hunse hunse force-pushed the splitter-refactoring branch from 7132b51 to a1dd199 Apr 9, 2019

@hunse

hunse approved these changes Apr 9, 2019

@tbekolay tbekolay force-pushed the splitter-refactoring branch 4 times, most recently from 328f0b9 to 820bc77 Apr 15, 2019

@tbekolay tbekolay force-pushed the splitter-refactoring branch 2 times, most recently from b415361 to f9bff69 Apr 23, 2019

@tbekolay
Copy link
Member

left a comment

This is a big improvement, will make future development much easier. I made some changes within the history here, so best to see the commits one-by-one for my suggested changes. There are definitely some discussions points here, so it would be great to get @arvoelke and @hunse's thoughts and a signoff.

  1. 5694c27 Is Split a good name? Or should we stick with Splitter, SplitterDirective, or some other option?
  2. 15890be Similarly, is PassthroughSplit a good name?
  3. ea6d52e Should this be a separate PR or is it OK to include here?
  4. 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?
  5. f64b102 Are BuildErrors sufficient here or do we need to make a new NengoException subclass?
Move some inputs.py classes to builder/inputs.py
From nengo_loihi.inputs. These classes are only used as part of
the build process and should not be made alongside Block and
LoihiInput instances.

@arvoelke arvoelke force-pushed the splitter-refactoring branch from f9bff69 to fbd3522 Apr 23, 2019

@arvoelke

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

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.

@tbekolay tbekolay force-pushed the splitter-refactoring branch 3 times, most recently from e30f32c to 76b8645 Apr 24, 2019

@arvoelke

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

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

@tbekolay

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

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.

arvoelke and others added some commits Mar 27, 2019

Disallow passing network=None to simulator
Also corrects an incorrect default in the simulator docstring.
Combine splitter into builder
Most of the work done by the splitter is now done in the builder.
This should give more clarity and control over the mapping between
pre-build and post-build objects. The `Split` class takes on the
organizational tasks of the old `Splitter`, giving directives to
the builder about what should be on- or off-chip.

Several tests were added or modified to ensure that these changes
did not affect built models in adverse ways. Some tests were added
to ensure that the refactoring made improvements; for example, the
refactored splitter no longer mutates networks, which closes #211.
Simulations should now be identical whether precompute is True
or False, so the tolerance in test_precompute is now set to zero.

The passthrough removal process was also modified to act in the
same way as `Split`, and is now named `PassthroughSplit` accordingly.

Co-authored-by: Trevor Bekolay <tbekolay@gmail.com>
Pass no decoder cache to sub-models
The decoder cache does not work for host models due to having
no context manager, which is normally constructed by the
top-level network builder.

This commit fixes #207 as it does not globally disable the
decoder cache. We also add some asserts to existing tests
for ensure that #207 is fixed.
Raise tolerance for test_pes_comm_channel
The test was fragile with the old tolerances, failing for many seeds.
Test conv weights without lowpass
Covers some previously uncovered lines in `conv.py`.
Fix flake8 checking
Also fix an issue caught by flake8
Change raised exceptions to NengoExceptions
There were a few places where we were raising errors that were
not subclasses of NengoException. They are now appropriate
NengoExceptions.
Fix adaptive control notebook
A keyword argument changed in a recent abr_control commit.

@tbekolay tbekolay force-pushed the splitter-refactoring branch 2 times, most recently from 3053887 to 2808ea5 Apr 24, 2019

@arvoelke arvoelke force-pushed the splitter-refactoring branch 3 times, most recently from 683384e to b315cf3 Apr 26, 2019

Revert "Parallelize hardware tests"
This partially reverts commit
70ecb8a.

The patch to graph.Graph seems to be incomplete, as one process
could be writing to the file to be copied while another process
is attempting to read it. The includes are also not copied, so
likely we need a more robust solution for writing snip files to
temporary directories.

The command was only changed to request one worker instead of two.

@tbekolay tbekolay force-pushed the splitter-refactoring branch from b315cf3 to e9cd851 Apr 26, 2019

@tbekolay tbekolay merged commit e9cd851 into master Apr 26, 2019

3 checks passed

Travis CI - Branch Build Passed
Details
codecov/patch 100% of diff hit (target 100%)
Details
codecov/project Absolute coverage decreased by -0.01% but relative coverage increased by +0.67% compared to c5e72bc
Details

@tbekolay tbekolay deleted the splitter-refactoring branch Apr 26, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.