-
Notifications
You must be signed in to change notification settings - Fork 12
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
Splitter refactoring #202
Conversation
df54ce5
to
de2b772
Compare
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 On this branch, On master, |
Added a commit (6c201e0) that increases test coverage since I touched the lines of some un-tested convolution code. |
Just a few minor comments. Otherwise everything LGTM. Thanks for doing all those tests against the benchmarks. |
7132b51
to
a1dd199
Compare
328f0b9
to
820bc77
Compare
b415361
to
f9bff69
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
- 5694c27 Is
Split
a good name? Or should we stick withSplitter
,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?
- 16e322a Is
get_seed
something we want built intoModel
? And if so, do we do it now or make a separate issue for it? - f64b102 Are
BuildError
s sufficient here or do we need to make a newNengoException
subclass?
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.
f9bff69
to
fbd3522
Compare
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.
LGTM!
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.
This LGTM, but we should make a card to go through the rest of the code base? e..g, There are a bunch of |
e30f32c
to
76b8645
Compare
I just noticed that the simulator docstring says the default for |
OK, will do. Running into some issues getting the hardware build to pass so I'll be doing at least one more push anyhow. |
Also corrects an incorrect default in the simulator docstring.
The test was fragile with the old tolerances, failing for many seeds.
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>
Also fix an issue caught by flake8
Covers some previously uncovered lines in `conv.py`.
There were a few places where we were raising errors that were not subclasses of NengoException. They are now appropriate NengoExceptions.
A keyword argument changed in a recent abr_control commit.
3053887
to
2808ea5
Compare
683384e
to
b315cf3
Compare
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.
b315cf3
to
e9cd851
Compare
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:
self
,host
, andhost_pre
) in parallel, by delegating the objects to the appropriate builder methods.Solves the following bugs:
Random improvements:
(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)split_pre_from_host
is no longerO(n^2)
__contains__
is no longerO(n)
Bugs that have been identified that are outside the scope of this PR:
precompute=True
could be smarter.Existing issues that should now be more readily achievable:
sim.data
does not contain chip <-> host connectionsparams
and handle splitter-created objects #4 - Exposeparams
and handle splitter-created objectsOther future work:
network=None
.assert self.model.dt == dt
to an error and then unit test (Simulator dt ignored if model dt specified nengo#1517).