brian-team / brian2cuda

A brian2 extension to simulate spiking neural networks on GPUs
https://brian2cuda.readthedocs.io/
GNU General Public License v3.0
60 stars 12 forks source link

Changing `dt` between runs is not working #136

Open denisalevi opened 6 years ago

denisalevi commented 6 years ago

Implement this brian2 test for brian2cuda. A first test did not show any spikes in cuda, but some in cpp (but also not all that are tested for).

I just came across this section in the CPPSpikeQueue, where in case of a changed dt the spikes in the queues are sorted into new queues. I don't think we are doing anything like this at all?

moritzaugustin commented 6 years ago

I don't think we necessarily should support variable time steps in brian2cuda

@mstimberg do you think differently?

denisalevi commented 6 years ago

This is actually related to #86. We are just rerunning the synapses_initialise_queue template as soon as any synapse variable (I guess?) is changed between run calls (maybe same for dt?). We will have take care of that anyways and while doing it, we can copy the code I linked above from the cspikequeue.cpp.

mstimberg commented 6 years ago

I don't think we necessarily should support variable time steps in brian2cuda

I agree that this is a bit of a niche feature that I did not come across "in the wild" yet. The only thing that is important to me (this was an improvement over Brian 1) is that dt should be changeable until the run call -- i.e. if a user specifies defaultclock.dt = ... not at the start of the script but later, it shouldn't screw up things. I would not mind at all throwing a NotImplementedError if the dt changes between runs.

denisalevi commented 2 years ago

I just implemented the Brian2 test mentioned above for cpp_standalone and cuda_standalone (see here). The test looks like this:

from numpy.testing import *
from brian2 import *
import brian2cuda

#device_name = 'cuda_standalone'
device_name = 'cpp_standalone'
set_device(device_name, build_on_run=False)

defaultclock.dt = .5*ms
G1 = NeuronGroup(1, 'v:1', threshold='v>1', reset='v=0')
G1.v = 1.1
G2 = NeuronGroup(10, 'v:1', threshold='v>1', reset='v=0')
S = Synapses(G1, G2, on_pre='v+=1.1')
S.connect(True)
S.delay = 'j*ms'
mon = SpikeMonitor(G2)
net = Network(G1, G2, S, mon)
net.run(5*ms)
defaultclock.dt = 1*ms
net.run(3*ms)
defaultclock.dt = 0.1*ms
net.run(2*ms)

device.build(direct_call=False, **device.build_options)
# Spikes should have delays of 0, 1, 2, ... ms and always
# trigger a spike one dt later
expected = [0.5, 1.5, 2.5, 3.5, 4.5, # dt=0.5ms
            6, 7, 8, #dt = 1ms
            8.1, 9.1 #dt=0.1ms
            ] * ms
assert_allclose(mon.t[:], expected)

This test fails for both, cpp_standalone and cuda_standalone. All spikes after the first run are lost.

I only wanted to add a test that fails expectedly (since this is a known issue). But I'm surprised is also fails for C++ Standalone. This test is not part of the test suite though and was implemented 9 years ago, so it might be expected to fail? But I don't see why.

@mstimberg Could you have a look? Is this a Brian2 bug or am I missing something?

mstimberg commented 2 years ago

Hmm, this seems indeed to be a bug in Brian. For some reason, we never made this test "standalone-compatible", so it was only running for the runtime targets (where it obviously works). Maybe as a short-term solution, one could just throw an error if dt changes between runs in standalone mode (at least when it affects the spike queue)?

denisalevi commented 2 years ago

Makes sense. But maybe the fix is not so hard, there is some code that looks like it should take care of this, here in the cspikequeue implementation.

I will raise an error now anyways for changing dt between runs for the spike queue in Brian2CUDA.