-
Notifications
You must be signed in to change notification settings - Fork 22
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
More memory optimizations #129
Conversation
dbfe079
to
8c21849
Compare
b7747ae
to
398a694
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 PR is admittedly harder to review than others in terms of whether the code is correct, so I'm mostly relying on the tests. Readability-wise, all looks good, save the the one inline question.
If the signal is set then the initial value is never being read, so no point keeping it around.
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.
On a second look, decided to keep the label as a bunch of other sections had similar labels. Merging when CI passes!
The main change here is avoiding storing simulator state for signals targeted by
set
operations (since that state will never be read).I also added some more graph simplification functions. One (
remove_constant_copies
), I had previously disabled because it could change which signals are stored bysave_params
/load_params
. But thinking about this more, that doesn't seem worth disabling it for. As long as the user is consistently using (or not using)remove_constant_copies
in both networks, then save/load params will work fine. So better to have it enabled by default (users can still disable it if they run into problems).The second one
remove_reset_incs
, I had previously completely removed because in general it seemed to make things worse. But I realized that this was largely to do with the kinds of networks we often build in Nengo, which have a lot of horizontal parallelism. In more deep learning style networks, where you just have a big long chain of objects,remove_reset_incs
can still be helpful. And then with some further testing, I didn't really see any significant performance decreases when enabling it by default, so I just went with that.