Conversation
jlubo
force-pushed
the
test/busyring-stdp
branch
from
65a4a3a to
70b84aa
Compare
|
The rebase is done. |
thorstenhater
left a comment
thorstenhater
left a comment
There was a problem hiding this comment.
Thanks. If you want to pull out the config thing into a new PR,
feel free do so. I left a few comments on the rest.
T
| } | ||
| }; | ||
|
|
||
| std::string get_arbor_config_str() { |
There was a problem hiding this comment.
This is usually intended only for use in Python. However, I see the usefulness of providing this to the user at runtime.
|
|
||
| // Add a synapse to proximal end of first dendrite. | ||
| decor.place(arb::mlocation{1, 0}, arb::synapse{"expsyn"}, "p"); | ||
| decor.place(arb::mlocation{0, 1}, arb::synapse{"expsyn"}, "p"); |
There was a problem hiding this comment.
These locations are equivalent soma(1) ~ dend(0).
| std::uniform_real_distribution<float> delay_dist(0, 2*min_delay_); | ||
| auto src_gen = std::mt19937(gid); | ||
| for (unsigned i=1; i<ncons; ++i) { | ||
| for (unsigned i=0; i<ncons; ++i) { |
There was a problem hiding this comment.
This seems wrong; we got one connection from the ring, thus n-1 random ones.
There was a problem hiding this comment.
Yeah... I've now adapted the synapses parameter to consistently count the random synapses only. So in this sense it should be correct. But I hope this doesn't conflict with any previous uses of busyring? The thing is that this is how it's described in our paper (and also implemented in the coreneuron example). The alternative would be to change the number of synapses in all *.json files.
There was a problem hiding this comment.
Well, it depends ;) Previously, we did count all synapses and all other instantiations of busyring do remain doing so. I'd prefer consistency, if it's ok.
…and documentation improvement
thorstenhater
left a comment
thorstenhater
left a comment
There was a problem hiding this comment.
Could you match the synapse count to the protocol of all the other examples? I am a bit worried that using two approaches is confusing.
2 participants
Adds a STDP case for the busyring benchmark (also cf. here).