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

Loihi neuron type nengo_dl builders not used by default #248

Closed
hunse opened this issue Sep 18, 2019 · 3 comments · Fixed by #275
Closed

Loihi neuron type nengo_dl builders not used by default #248

hunse opened this issue Sep 18, 2019 · 3 comments · Fixed by #275
Labels

Comments

@hunse
Copy link
Collaborator

hunse commented Sep 18, 2019

nengo_loihi.builder.nengo_dl has the builders for nengo_dl for LoihiLIF and LoihiSpikingRectifiedLinear.

If you make a nengo_loihi.Simulator and tensorflow is installed, these builders will get properly initialized (see the install_dl_builders) call in Simulator.__init__. However, if you just import these neuron types from nengo_loihi to do training in nengo_dl, before you've created a nengo_loihi.Simulator, the builders won't be initialized.

Even worse, these neuron types are subclasses of LIF and SpikingRectifiedLinear that nengo_dl does know how to build, so it just ends up silently trying to build them but not working.

I think that these builders should be initialized as soon as the neuron types are imported from nengo_loihi.neurons. Then, things will just work as people expect. This is what we had originally, but then we changed it for some reason, and I can't remember why. (@tbekolay, you're a co-author with me on that commit 025cdf9. Do you remember why?)

If for some reason we can't guarantee that the builders will be initialized, then I think we need some way to throw an error if someone tries to use those neuron types in nengo_dl (and that error can point the user to how to initialize them).

@hunse hunse added the bug label Sep 18, 2019
@tbekolay
Copy link
Member

I believe I changed it because, in general, you shouldn't modify global state in an import. I also think that before I reorganized some things, everything was being defined in the else block of an if statement, which raised the C901 complexity warning, so I wanted to get rid of that and I think in making all those changes I moved some stuff to install_dl_builders that was previously happening on import. It was easy to modify tests to work properly through calling install_dl_builders explicitly, and in general we prefer explicit calls over automatic things that might make unexpected changes. But, given that you don't get an error due to those neuron types being subclasses, and it's not clear how you would introduce some kind of error, it's probably fine to call install_dl_builders on import. Where would you add the call? In neurons.py?

@hunse
Copy link
Collaborator Author

hunse commented Sep 18, 2019

We could do it the main part of neurons.py, or we could put it in the __init__ of each neuron type (that way nothing changes on import).

@tbekolay
Copy link
Member

__init__ of the neuron type makes sense, though right now there's just one function for installing all builders, so that would have to be modified to allow for enabling one but not the other.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging a pull request may close this issue.

2 participants