Fix issue with first spike passing through adaptive synapse #3384
Conversation
clinssen
force-pushed
the
first_syn_spike
branch
from
edc9f65 to
b907e0d
Compare
terhorstd
added
S: High
|
@tomtetzlaff Ping! |
There was a problem hiding this comment.
@clinssen Thanks for this PR. It looks mostly good to me, but see my small suggestions inline. Could you add a little more text to the PR description to explain why/how things go wrong with the current implementation? Just for future reference.
| tsodyks_params = dict(fac_params, synapse_model="tsodyks_synapse") # for tsodyks_synapse | ||
| tsodyks2_params = dict(fac_params, synapse_model="tsodyks2_synapse") # for tsodyks2_synapse | ||
| tsodyks_params = dict(dep_params, synapse_model="tsodyks_synapse") # for tsodyks_synapse | ||
| tsodyks2_params = dict(dep_params, synapse_model="tsodyks2_synapse") # for tsodyks2_synapse |
There was a problem hiding this comment.
I find it strange that both dep and fac parameters are given above, but only one of the sets is used, without any further explanation. It is also not clear to me why the default is now changed from fac to dep. Wouldn't it be most illustrative to show the effect of both facilitation and depression by creating a network with in total four plastic synapses?
| if self.synapse_parameters["tau_fac"] < 1e-10: | ||
| u_decay = 0.0 | ||
| else: | ||
| u_decay = np.exp(-h / self.synapse_parameters["tau_fac"]) |
There was a problem hiding this comment.
Based on the comment in the C++ code above, I would suggest here
| if self.synapse_parameters["tau_fac"] < 1e-10: | |
| u_decay = 0.0 | |
| else: | |
| u_decay = np.exp(-h / self.synapse_parameters["tau_fac"]) | |
| if self.synapse_parameters["tau_fac"] == 0: | |
| u_decay = 0.0 | |
| else: | |
| u_decay = math.exp( - h / self.synapse_parameters["tau_fac"] ) |
Do we actually test for both cases, i.e. tau_fac == 0 and tau_fac != 0?
Also, since the exponent looks like a scalar, math.exp() is faster than the NumPy version.
| double x_decay = std::exp( -h / tau_rec_ ); | ||
| double u_decay = ( tau_fac_ < 1.0e-10 ) ? 0.0 : std::exp( -h / tau_fac_ ); |
There was a problem hiding this comment.
| double x_decay = std::exp( -h / tau_rec_ ); | |
| double u_decay = ( tau_fac_ < 1.0e-10 ) ? 0.0 : std::exp( -h / tau_fac_ ); | |
| const double x_decay = std::exp( -h / tau_rec_ ); | |
| const double u_decay = ( tau_fac_ == 0 ) ? 0.0 : std::exp( -h / tau_fac_ ); // tau_fac == 0 disables facilitation |
I find this check here a bit confusing. Since tau_fac_ == 0 is allowed to turn off facilitation (actually the default), I think it is most sensible to actually test for zero.
There was a problem hiding this comment.
Thanks @clinssen. This solved the issue with the order of update steps. After a period of silence, the synapse now fully recovers, as it should.
The fix for first spike passing through an adaptive synapse in #1672 was erroneous and affects more than just the first spike. This PR fixes the fix.