[ISSUE-7] Moved all model-specific defaults from Global.h to appropriate model classes#910
Conversation
stiber
left a comment
stiber
left a comment
There was a problem hiding this comment.
Mostly comments, possibly not requiring change.
| // Default synapse parameter values | ||
| static constexpr BGFLOAT DEFAULT_tau = 3e-3; // The default synaptic time constant | ||
| static constexpr BGFLOAT DEFAULT_U = 0.4; // The default synaptic efficiency | ||
| static constexpr BGFLOAT DEFAULT_delay_weight = 0; // The default delay weight |
There was a problem hiding this comment.
Why constexpr and not just const? Just curious.
DEFAULT_U should, I think, be a Dynamic Spiking Synapse member.
There was a problem hiding this comment.
Or is the DEFAULT_U placement a "feature" of the fact that AllDynamicSTDPSynapses doesn't inherit from AllDSSynapses?
There was a problem hiding this comment.
Why
constexprand not justconst? Just curious.
It just made more sense in my head to have them compile at compile-time instead of run-time. constexpr guarantees it, so I used it.
DEFAULT_Ushould, I think, be a Dynamic Spiking Synapse member.
I will check and get back to you.
There was a problem hiding this comment.
Or is the
DEFAULT_Uplacement a "feature" of the fact thatAllDynamicSTDPSynapsesdoesn't inherit fromAllDSSynapses?
Just quickly double checked. Yes, DEFAULT_U lives in AllSpikingSynapses because it’s shared across multiple spiking synapse models, and AllDynamicSTDPSynapses doesn’t inherit from AllDSSynapses, so this avoids duplication and keeps the default at the appropriate abstraction level. It made sense to keep the code clean this way.
There was a problem hiding this comment.
OK, the DEFAULT_U placement makes sense.
Regarding constexpr with variables that are initialized to a constant value, I don't think that it makes a difference, especially since this is static and so only done once (though only one instance of this class is ever made, so it's a distinction without a difference). I vote for using const to avoid confusing people.
| W_[iEdg] = edgSign(type) * 10.0e-9; | ||
| type_[iEdg] = type; | ||
| tau_[iEdg] = DEFAULT_tau; | ||
| tau_[iEdg] = AllSpikingSynapses::DEFAULT_tau; |
There was a problem hiding this comment.
I don't think this needs the scope specification here.
2 participants
…classes
Closes #7
Description
Moved all model-specific defaults from Global.h to appropriate model classes
Checklist (Mandatory for new features)
Testing (Mandatory for all changes)
test-medium-connected.xmlPassedtest-large-long.xmlPassed