[ISSUE-7, 10] Moved all model-specific defaults from Global.h to appropriate model classes#928
[ISSUE-7, 10] Moved all model-specific defaults from Global.h to appropriate model classes#928CodersRepo wants to merge 13 commits intoSharedDevelopmentfrom
Conversation
Issue 907 add contributor
…fault-cleanup [ISSUE-7] Moved all model-specific defaults from Global.h to appropriate model classes
There was a problem hiding this comment.
Pull request overview
This PR refactors the simulator’s neuroscience models to remove model-specific default constants from Simulator/Utils/Global.h and place them into the owning neuron/synapse classes, improving encapsulation and addressing inconsistencies noted in #7 and #10.
Changes:
- Moved IF neuron default parameters from
Global.hintoAllIFNeurons(static constexpr). - Moved spiking synapse defaults (
tau,U, delay weight) intoAllSpikingSynapsesand updated host/GPU code paths to reference them. - Removed the corresponding
#define DEFAULT_*macros fromGlobal.h.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Simulator/Vertices/Neuro/AllIFNeurons.h | Adds static constexpr IF neuron defaults to the model class. |
| Simulator/Vertices/Neuro/AllIFNeurons.cpp | Switches neuron initialization to use AllIFNeurons::DEFAULT_* values. |
| Simulator/Utils/Global.h | Removes model-specific default macros from the global header. |
| Simulator/Edges/Neuro/AllSynapsesDeviceFuncs_d.cpp | Updates CUDA device-side synapse creation defaults to use AllSpikingSynapses::DEFAULT_* and adds required include. |
| Simulator/Edges/Neuro/AllSpikingSynapses.h | Introduces static constexpr defaults for spiking synapses. |
| Simulator/Edges/Neuro/AllSpikingSynapses.cpp | Uses the new AllSpikingSynapses::DEFAULT_tau in edge creation. |
| Simulator/Edges/Neuro/AllDynamicSTDPSynapses.cpp | Replaces DEFAULT_U usage with AllSpikingSynapses::DEFAULT_U. |
| Simulator/Edges/Neuro/AllDSSynapses.cpp | Replaces DEFAULT_U usage with AllSpikingSynapses::DEFAULT_U. |
| Contributors.md | Adds a 2026 entry. |
Comments suppressed due to low confidence (2)
Simulator/Edges/Neuro/AllSynapsesDeviceFuncs_d.cpp:120
- In createSpikingSynapse(), the switch(type) has a
default: break;and thentau/delayare used to compute decay/totalDelay. If an unexpected edgeType reaches here, this will use uninitialized values (UB) on the device. Consider asserting in the default case (like the CPU path does) or initializingtau/delayto safe defaults before the switch.
allEdgesDevice->tau_[iEdg] = AllSpikingSynapses::DEFAULT_tau;
BGFLOAT tau;
switch (type) {
case edgeType::II:
Simulator/Edges/Neuro/AllSynapsesDeviceFuncs_d.cpp:267
- In createSTDPSynapse(), the switch(type) has a
default: break;buttau/delayare used immediately afterwards to compute decay/totalDelay. If type is outside {II, IE, EI, EE}, this will use uninitialized values on the device. Recommend asserting in the default branch or settingtau/delaydefaults before the switch.
allEdgesDevice->tau_[iEdg] = AllSpikingSynapses::DEFAULT_tau;
BGFLOAT tau;
switch (type) {
case edgeType::II:
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Initialize DS/dynamic STDP parameter locals to safe defaults and assert in unexpected switch branches to avoid undefined behavior from uninitialized device values.
| delay = 1.5e-3; | ||
| break; | ||
| default: | ||
| assert(false && "Unexpected edgeType in createDSSynapse"); |
There was a problem hiding this comment.
This is kind of tricky. I'll allow it 😀, as we eventually want to move to our own set of exceptions, but that issue hasn't risen to the top yet.
There was a problem hiding this comment.
This is just a comment. You can just resolve it and merge, I believe.
Closes #7 #10
Description
Checklist (Mandatory for new features)
Testing (Mandatory for all changes)
test-medium-connected.xmlPassedtest-large-long.xmlPassed