UWB-Biocomputing / BrainGrid

A project to facilitate construction of high-performance neural simulations.
https://uwb-biocomputing.github.io/BrainGrid/
Apache License 2.0
32 stars 17 forks source link

Bug in STDP delay computation #291

Closed stiber closed 4 years ago

stiber commented 4 years ago

Lines of code like the following:

delta = ((int64_t)g_simulationStep - spikeHistory) * deltaT;
delta = (spikeHistory - (int64_t)g_simulationStep) * deltaT;

Appear 4 times in AllSTDPSynapses::advanceSynapse() and in advanceSTDPSynapsesDevice() in the file AllSynapsesDeviceFuncs_d.cu. This is incorrect, because the subtraction will convert an int to a uint if one of the operands is a uint. Since we know that the current simulation time step will always be greater than the time step when a spike in the history occurred (since that spike happened in the past), it is better to perform an unsigned subtraction, multiply by deltaT, then negate if necessary. So, the above two lines would be:

delta = static_cast<BGFLOAT>(g_simulationStep - spikeHistory) * deltaT;
delta = -static_cast<BGFLOAT>(g_simulationStep - spikeHistory) * deltaT;

Note that this change needs to be captured for the multi-cluster simulation, too.

https://github.com/UWB-Biocomputing/BrainGrid/blob/4f4e674e3a673fcc52eeaaabc92e8f5d5b9bc890/Synapses/AllSTDPSynapses.cpp#L326

fumik commented 4 years ago

Fixed AllSTDPSynapses.cpp on the master branch.

stiber commented 4 years ago

We subsequently made some additional tweaks to the code. This is now reflected in the single threaded and multi cluster code base.