Input synapses are currently ignored

[tcstewar]

This came up while writing tests for #26 . That PR changes how inputs are handled when precompute=False, so I wanted to test that the signals are being passed and filtered correctly. It turns out that it looks like the synapse on an input Connection (i.e. one that goes from a Node to an Ensemble) is currently being ignored. Here's the test:

@pytest.mark.parametrize('precompute', [False, True])
def test_input_interneurons_running(Simulator, allclose, plt, precompute):
    synapse = 0.1
    with nengo.Network() as model:
        stim = nengo.Node(lambda t: 1 if t % 0.5 < 0.25 else 0)
        ens = nengo.Ensemble(n_neurons=1, dimensions=1,
                             encoders=[[1]],
                             intercepts=[0],
                             max_rates=[40])
        c = nengo.Connection(stim, ens, synapse=synapse)   # This is the synapse that is being ignored
        p_stim = nengo.Probe(stim)
        p_neurons = nengo.Probe(ens.neurons, synapse=0.1)
    with Simulator(model, precompute=precompute) as sim:
        sim.run(1.0)
    with nengo.Simulator(model) as ref:
        ref.run(1.0)

    plt.plot(sim.trange(), sim.data[p_neurons], label='nengo_loihi')
    plt.plot(sim.trange(), ref.data[p_neurons], label='nengo')
    plt.legend(loc='best')
    plt.twinx()
    plt.plot(sim.trange(), sim.data[p_stim])

Here is the result (for both precompute=True and precompute=False):

If I modify the test such that reference nengo ignores the input synapse by setting c.synapse=None, then we get this

The synapse is being set on the Connection that's being passed to the builder ( https://github.com/nengo/nengo-loihi/blob/master/nengo_loihi/splitter.py#L181 ), but I'm not sure how that ends up being used.

This affects both the emulator (target='sim') and the chip itself (target='loihi')

Note that we could resolve this either by introducing the synapse on loihi, or we could do the synapse on the host side by setting the synapse here https://github.com/nengo/nengo-loihi/blob/master/nengo_loihi/splitter.py#L207 instead of here https://github.com/nengo/nengo-loihi/blob/master/nengo_loihi/splitter.py#L181. I believe the intent was to have this synapse on the chip, but there may be reasons not to do that.

[hunse]

So I think #45 addresses this issue.

My problem there right now is that if a synapse is not set (i.e. synapse=None), then there's no filtering on the interneuron spikes coming from the node (whereas before, we have an INTER_TAU filter). In core Nengo, Nodes typically don't need filters, so some users (like myself) might be in the habit of putting a synapse=None filter on Node->Ensemble connections.

Thoughts about what to do in this situation? Some options are:

1. Having no filter on the interneurons, which makes things kind of noisy (it caused our node_ens_ens test to fail).
2. Special-case things when synapse=None, and use INTER_TAU in that case (conveniently, the default synapse of 0.005 is currently the same as INTER_TAU).
3. Take the largest synapse time constant. So if the user sets anything below INTER_TAU, we'd use INTER_TAU, otherwise we use what they set.

[hunse]

It turns out, having no filter on spiking node inputs is not actually that bad, since these have high firing rates (whereas for ensemble->ensemble interneurons, where we have 10 neurons firing at 100 Hz each, filtering is more necessary). Removing the synapse only caused a slight change in test_node_ens_ens. In #45, I've redone test_node_ens_ens with a lower-frequency input signal, and the output appears close to the target with or without filtering on the input node.

This doesn't mean that option 2 or 3 isn't still the best solution. It just means that option 1 is more viable than I had originally thought.

[tcstewar]

Hmm... my inclination is to not introduce new synapses that the user isn't expecting. I could see that being more confusing than it's worth. So I'd say we stick with option 1 for now (i.e. if the user specifies synapse=None for a Node->Ensemble Connection, then we leave it with synapse=None)

[hunse]

I'm fine with that. Though my argument for option 1 would be that it gives users the most control. We're already introducing stuff that they might not expect (i.e., neurons that turn the node values into spikes), so introducing something else to help make this work more like Nengo doesn't seem too insidious to me.

[hunse]

Fixed by #45.

[tbekolay]

Does #45 explicitly test this? I was looking at it and some tests are modified, but there are no specific tests added for it. The OP in this issue includes a test, so we should add it to the test suite before closing this issue.

[drasmuss]

Test added in #250, which wraps this up.