Always set filter on post ensemble in connection

[hunse]

This would fix (for example) neuron->ensemble connections
(of which we currently have no test cases).

Unfortunately, this changes the behaviour of node->ensemble connections,
causing node_ens_ens to fail. The synapse set on the connection (None)
overrides the default INTER_TAU synapse, removing the filtering on
the ReLU neurons used to turn the node values into spikes.

Based on #43.

[hunse]

Based on the discussion in #63, I think this is ready for review.

[tcstewar]

I don't quite understand in what conditions the system uses tau_s and in what conditions it uses INTER_TAU, but I can confirm that this change fixes the issue I posted #63 . So perhaps a bit of a comment in builder.py indicating what the conditions are and why we're sometimes using tau_s and sometimes using INTER_TAU would be nice, but other than that it looks fine to me!

[hunse]

@tcstewar: I've added some comments. Let me know what you think.

One situation that we're going to have trouble with in general is that each group of neurons can only have one input synapse. So right now, in the case where we have a recurrent connection (which would have a INTER_TAU filter going back in to the neurons) and an input node (with this change, would now apply its own filter), things would break.

One solution, especially since it seems we don't need filtering on the interneurons (at least on node interneurons, I haven't tested on ensemble ones where we use 20 neurons at 100 Hz instead of 2 at 1000 Hz), would be to only use INTER_TAU as a default for ensembles, and if anything else overrides that, then that would be the new input filter.

[hunse]

So I think I'd added the comments, but not pushed them. Sorry @tcstewar! If you want to take a look now, go ahead.

I think @tbekolay is looking to merge this soon. I was trying to figure out what I was talking about in that last comment about only making INTER_TAU the default for ensembles; it seems like that was done in cb6ba75. As far as I can tell, this is good to go.

[tcstewar]

So I think I'd added the comments, but not pushed them. Sorry @tcstewar! If you want to take a look now, go ahead.

Those comments look good to me! Thank you for clarifying! :)

[tbekolay]

After rebasing, this fails test_examples::test_node_ens_ens and test_ensemble::test_amplitude. Are these failures a result of the rebase or were they failing before? How to fix?

[hunse]

Fails in emulator or on Loihi?

[hunse]

I just rebased myself and found this failure in the emulator for test_node_ens_ens. It did not fail before the rebase (in the emulator). I'll look into these failures.

[tbekolay]

Emulator, therefore probably both. You can see the failures in travis CI.

[hunse]

Ok, I think this should all pass now.

I'm not quite sure what changed about test_node_ens_ens, but essentially I had made the tolerances a bit tighter in one of these commits so I backed them up a bit (though they're still tighter than originally).

Wtih test_amplitude, we were using a filter from an (input) node to an ensemble with no synapse. Before these commits, that would still provide some filtering, now it provides none. So I just removed the synapse=None there. I also went back and removed the synapse=None on the input node in test_node_ens_ens (basically, I would always encourage people to keep filters on their nodes if they're running on nengo_loihi).

I think for merging, all these commits can probably get squashed down into one.

[drasmuss]

Could we add a warning for node->* connections with synapse=None? It'll definitely surprise people that synaptic filters are necessary (or useful) on Nodes.

[tbekolay]

@hunse Could you explain a bit what the issue is with having no filter on nodes? Is it because node inputs are translated to spikes later on? Is there a mechanism for injecting current? Obviously not things to look into in this PR, but a bit of this explanation would be good to have on the warning raised for node->* connections.

[hunse]

So yeah, all node output gets turned into spikes by this ensemble that we introduce in the splitter:

nengo-loihi/nengo_loihi/splitter.py

Line 187 in 619e45b

ens = nengo.Ensemble(

The filter on the node connection is applied to the output spikes of that ensemble. Things work without a filter, they're just a little noisier.

Another option for down the road would be to have the standard INTER_TAU filter on the output of that ensemble (on the chip), and apply our connection filter on the connection going into that ensemble.

[hunse]

Ok, added a warning.

[tbekolay]

Added a commit fixing any warnings about synapse=None raised in the test suite. I am slightly concerned that the RMSE goes from 0.19802 +/- 0.6952 before that commit to 0.21271 +/- 0.7643, but I'll let it slide for now.

[hunse]

We're not good about accounting for synaptic delays in our tests, so I'm guessing that's the cause of the RMSE change.

…

On Wed, Sep 26, 2018, 21:24 Trevor Bekolay ***@***.***> wrote: ***@***.**** approved this pull request. Added a commit fixing any warnings about synapse=None raised in the test suite. I am slightly concerned that the RMSE goes from 0.19802 +/- 0.6952 before that commit to 0.21271 +/- 0.7643, but I'll let it slide for now. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#45 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB3J-i5845JBwcm3tMG5ohYE2iaFY5Ggks5ufCi2gaJpZM4WZrRQ> .