Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Static synapse with transmission probability #747

Merged
merged 12 commits into from Jun 23, 2017
Merged

Conversation

@hyukz
Copy link
Contributor

@hyukz hyukz commented Jun 9, 2017

A new synapse model built over static_synapse introducing a transmission probability p_transmit used to calculate whether the spike reaches the target or not.
Inspired by the results described in [1] of greater transmission probability for stronger excitatory connections and previously applied in [2] and [3].

[1] Sandrine Lefort, Christian Tomm, J.-C. Floyd Sarria, Carl C.H. Petersen, The Excitatory Neuronal Network of the C2 Barrel Column in Mouse Primary Somatosensory Cortex, Neuron, Volume 61, Issue 2, 29 January 2009, Pages 301-316, DOI: 10.1016/j.neuron.2008.12.020.

[2] Jun-nosuke Teramae, Yasuhiro Tsubo & Tomoki Fukai, Optimal spike-based communication in excitable networks with strong-sparse and weak-dense links, Scientific Reports 2, Article number: 485 (2012), DOI: 10.1038/srep00485

[3] Yoshiyuki Omura, Milena M. Carvalho, Kaoru Inokuchi, Tomoki Fukai, A Lognormal Recurrent Network Model for Burst Generation during Hippocampal Sharp Waves, Journal of Neuroscience 28 October 2015, 35 (43) 14585-14601; DOI: 10.1523/JNEUROSCI.4944-14.2015

…apse.sli. new

synapse and new parameter name "p_transmit" were added to listings in other files.
@heplesser
Copy link
Contributor

@heplesser heplesser commented Jun 9, 2017

@hyukz Thank you for your contribution. We will try to review it without too much delay. Before we can integrate your code into NEST, we require that you transfer copyright to the NEST Initiative. Please download our Contributor License Agreement from http://nest.github.io/nest-simulator/, and returned it to me signed (by email to my @nmbu.no address).

Improve documentation of synapse class and test
@mschmidt87
Copy link

@mschmidt87 mschmidt87 commented Jun 9, 2017

Actually, I was involved in creating this pull-request, so I guess it would make more sense if someone else reviewed the code. The original implementation was done by @suku248 back in the old days of the 10kcollaps branch. We basically moved this implementation into the current framework and wrote the test. I therefore added her now as an author to the synapse file. I hope that's fine.

As alternative reviewer, I would thus suggest either @suku248 as a reviewer or @jakobj .

@jakobj
jakobj approved these changes Jun 9, 2017
Copy link
Contributor

@jakobj jakobj left a comment

Very nice work! I've added a few comments that should be fixed before merging, but since these are just minor points, I'll already approve now.

for ( ulong n = 0; n < n_spikes_in; n++ )
{
if ( rng->drand() < p_transmit_ )
n_spikes_out++;

This comment has been minimized.

@jakobj

jakobj Jun 9, 2017
Contributor

please add curly braces

updateValue< double >( d, names::p_transmit, p_transmit_);

if ( p_transmit_ < 0 || p_transmit_ > 1 )
throw BadProperty("Spike transmission probability must be in [0, 1].");

This comment has been minimized.

@jakobj

jakobj Jun 9, 2017
Contributor

missing curly braces


% build
/spike_generator Create /sg Set
/parrot_neuron 10 Create

This comment has been minimized.

@jakobj

jakobj Jun 9, 2017
Contributor

did you mean to use n_parrots instead of 10 here?

This comment has been minimized.

@hyukz

hyukz Jun 9, 2017
Author Contributor

yes, thanks for the input!

pn [sd] Connect

% simulates for 1010ms
N_spikes 10. add Simulate

This comment has been minimized.

@jakobj

jakobj Jun 9, 2017
Contributor

Why do you need to simulate for 1010ms, not 1000ms?

This comment has been minimized.

@hyukz

hyukz Jun 9, 2017
Author Contributor

while we were testing we wanted to make sure that the simulation had enough time to record all spikes and quit successfully, but 10ms does seem like too much right now. thanks for the input!

@mschmidt87
Copy link

@mschmidt87 mschmidt87 commented Jun 9, 2017

Thanks for the quick review.

One thing that we discussed, is the name of the synapse. We were not 100 % happy with lossy_synapse, but couldn't come up with a better name. stochastic_synapse seems ambiguous, because this could also mean a stochastic variation of e.g. the weight.

style changes
@hyukz
Copy link
Contributor Author

@hyukz hyukz commented Jun 9, 2017

New commit includes the suggestions pointed by @jakobj and tentatively solves the problems with Travis build (wrong model name in documentation).

As @mschmidt87 mentioned, we are not greatly fond of the current name for this synapse model, so suggestions are highly appreciated. My second option is (static_)stochtrans_synapse, with stochtrans meaning Stochastic Transmission.

@heplesser heplesser removed the request for review from mschmidt87 Jun 9, 2017
Copy link
Contributor

@heplesser heplesser left a comment

I added some comments on the test and will review more later.


% test parameters -------------------------------------------------
/N_spikes 1000 def % Number of spikes send via the lossy synapse to each parrot neuron
/n_parrots 10 def % Number of parrot neurons receiving spikes

This comment has been minimized.

@heplesser

heplesser Jun 9, 2017
Contributor

A single parrot neuron suffices, adding more just complicates the test without adding anything to the quality of the test.

This comment has been minimized.

@hyukz

hyukz Jun 9, 2017
Author Contributor

Initially, we were checking only one random seed with many parrot neurons to see the variability. Eventually we did implement the test across many random seeds, so indeed this could be too much at this point. Thanks for the input.

% test parameters -------------------------------------------------
/N_spikes 1000 def % Number of spikes send via the lossy synapse to each parrot neuron
/n_parrots 10 def % Number of parrot neurons receiving spikes
/p 0.5 def % Transmission probability of the lossy synapse

This comment has been minimized.

@heplesser

heplesser Jun 9, 2017
Contributor

p==0.5 is the one value one should avoid in the test: it will not detect whether the "drop probability" is interpreted the wrong way around. With p=0.25 we get a much better test.

This comment has been minimized.

@hyukz

hyukz Jun 9, 2017
Author Contributor

True. We tested other probability values and I ended up submitting this one, which is probably not the best. I am now wondering if checking a couple of different probabilities would make this a more comprehensive test or if it's just overdoing it.

This comment has been minimized.

@mschmidt87

mschmidt87 Jun 12, 2017

I agree with @heplesser . But let's stick to 0.25. I don't see a reason to believe that the synapse should work with one probability but not with another (with the exception of 0.5, as @heplesser pointed out).

/N_spikes 1000 def % Number of spikes send via the lossy synapse to each parrot neuron
/n_parrots 10 def % Number of parrot neurons receiving spikes
/p 0.5 def % Transmission probability of the lossy synapse
/margin 0.05 def % allowed relative deviation of measured mean from the target mean

This comment has been minimized.

@heplesser

heplesser Jun 9, 2017
Contributor

The margin should not be chosen "ad hoc", but should be based on statistics, e.g., choosing the limit to be three standard deviations for the given N and p.

This comment has been minimized.

@hyukz

hyukz Jun 12, 2017
Author Contributor

This is now changed in the latest commit. The standard deviation is calculated from N_spikes and p and an error margin of three standard deviations is used. Thanks!

@heplesser
Copy link
Contributor

@heplesser heplesser commented Jun 9, 2017

How about bernoulli_synapse, since the synapse performs a Bernoulli trial on each spike?

@mschmidt87
Copy link

@mschmidt87 mschmidt87 commented Jun 12, 2017

Currently, the regression test "ticket-478.sli" fails. It tests whether NEST fails when the user tries to connect devices using plastic synapses. It distinguishes synapses into plastic and static synapses by checking the parameters of their status dictionaries. If there is any additional parameter beyond the parameters of static_synapse, it classifies the synapse model as plastic.
Consequently, lossy_synapse is classified as plastic, but with the current implementation, we can connect devices using this synapse --> test fails.

The question: Is there a reasonable use case, where one wants to connect a device to a neuron using a lossy_synapse?
For recording devices, I would say that there is no use case. For stimulating devices, however, I could imagine that one would like to simulate a network with a common stimulus but let neurons receive stochastic samples of the stimulus.
What do you think, should we enable such a use case?

Then the test has to reformulated. However, as far as I can see, the test is currently broken, anyway, because it falsely classifies static synapses if and only if they have exactly the same parameters as static_synapse AND static_synapse_lbl. This is not the case for any synapse model, so that the tests does not classify any synapse as being static, which doesn't make sense.

@hyukz
Copy link
Contributor Author

@hyukz hyukz commented Jun 12, 2017

The newest commit changes the synapse name to Bernoulli synapse and fixes all previously mentioned problems with the unit test.

About ticket-478.sli, I am not familiar with the test per se but I also believe that bernoulli_synapse could be used to connect a stimulating device to a neuron in some circunstances. Actually, in my naïve point of view, there may be other synaptic mechanisms which are plausible to connect a spiking stimulus device to a neuron, though I don't know if there are implementation problems that come with that.

@heplesser
Copy link
Contributor

@heplesser heplesser commented Jun 12, 2017

The NEST Open Developer VC on 12 June has favoured bernoulli_synapse as name of this model.

Copy link
Contributor

@heplesser heplesser left a comment

@hyukz This looks mostly fine, I commented on some details.

handles_test_event( SpikeEvent&, rport )
{
return invalid_port_;
}

This comment has been minimized.

@heplesser

heplesser Jun 12, 2017
Contributor

This synapse should only transmit normal spikes. Therefore, only handles_test_event( SpikeEvent&, rport ) must be implemented, all other overloaded variants below should be deleted. This will solve the problems with the ticket-478 test.


if ( n_spikes_out > 0 )
{
e_spike.set_multiplicity(n_spikes_out);

This comment has been minimized.

@heplesser

heplesser Jun 12, 2017
Contributor

Spaces around argument required.

SpikeEvent e_spike = static_cast<SpikeEvent &>(e);

librandom::RngPtr rng = kernel().rng_manager.get_rng( t );
ulong n_spikes_in = e_spike.get_multiplicity();

This comment has been minimized.

@heplesser

heplesser Jun 12, 2017
Contributor

  • This can be const.
  • Please do not use ulong. Either write out unsigned long or use size_t. This applies in several places below as well.

This comment has been minimized.

@hyukz

hyukz Jun 12, 2017
Author Contributor

Just to make sure, you were talking about e_spike being const, correct?

This comment has been minimized.

@heplesser

heplesser Jun 12, 2017
Contributor

No, n_spikes_in will not be changed in the code below, and

const size_t n_spikes_in = e_spike.get_multiplicity();

will inform the compiler about this and allow further optimization. It also tells a reader of the code that this value will remain const and thus reduces the code complexity for the reader.

ulong n_spikes_in = e_spike.get_multiplicity();
ulong n_spikes_out = 0;

for ( ulong n = 0; n < n_spikes_in; n++ )

This comment has been minimized.

@heplesser

heplesser Jun 12, 2017
Contributor

Please use ++n here and ++n_spikes_out? below.

private:
double weight_;
double p_transmit_;

This comment has been minimized.

@heplesser

heplesser Jun 12, 2017
Contributor

Remove empty line.

void
BernoulliConnection< targetidentifierT >::get_status( DictionaryDatum& d ) const
{

This comment has been minimized.

@heplesser

heplesser Jun 12, 2017
Contributor

Remove empty line.

@@ -306,6 +306,7 @@ const Name overwrite_files( "overwrite_files" );

const Name p( "p" );
const Name P( "P" );
const Name p_transmit( "p_transmit" );

This comment has been minimized.

@heplesser

heplesser Jun 12, 2017
Contributor

Names should be in alphabetical order, move this below p_copy.

% test parameters -------------------------------------------------
/N_spikes 1000 def % number of spikes send via the bernoulli synapse to each parrot neuron
/p 0.25 def % transmission probability of the bernoulli synapse
/margin N_spikes p mul 1 p sub mul sqrt 3 mul def % allowed absolute deviation: three standard deviations of the binomial distribution with (N_spikes, p)

This comment has been minimized.

@heplesser

heplesser Jun 12, 2017
Contributor

Move this comment above the line it documents, it is too long to be stretch out to the right.

public:
// this line determines which common properties to use
typedef CommonSynapseProperties CommonPropertiesType;

This comment has been minimized.

@heplesser

heplesser Jun 12, 2017
Contributor

Remove this empty line.

@heplesser
Copy link
Contributor

@heplesser heplesser commented Jun 12, 2017

@hyukz @mschmidt87 The reason that the ticket-478 test fails, is that bernoulli_synapse currently allows all kinds of events to pass, while it should only transmit SpikeEvents. I added a comment in the code indicating what to change. We cannot use bernoulli_synapse to transmit spikes from devices using DSSpikeEvents, because these devices use a callback mechanism that will not work properly if spikes may be dropped in transit.

@heplesser
Copy link
Contributor

@heplesser heplesser commented Jun 13, 2017

@hyukz Travis reveals two problems with you code:

  1. The bernoulli_synapse test now fails, because you cannot connect spike_generator to neurons using bernoulli_synapse any more. You need to connect the generator to a parrot_neuron using static_synapse and then connect that parrot to another parrot with bernoulli_synapse.
  2. There are some C++ code-formatting problems. Please run, from within the NEST top-level source directory extras/check_code_style.sh and fix the problems reported (most likely clang-format-3.6 -i <filename> can fix issues for you.
@heplesser
Copy link
Contributor

@heplesser heplesser commented Jun 20, 2017

@hyukz Thank you for fixing the test. The only remaining issue now is the format of the code. Please see the instructions in the NEST Developer Space on how to install the necessary tools and run them. The crucial one to make the CI test system on Travis happy is clang-format-3.6.

@hyukz
Copy link
Contributor Author

@hyukz hyukz commented Jun 20, 2017

I am aware of that issue but I wasn't able to get clang-format-3.6 to run on my system to this point. I use Arch-Linux and even the source code from LLVM 3.6 is giving me cmake issues. As I get it to work I shall update the code.

Maximilian Schmidt and others added 3 commits Jun 20, 2017
fix formatting issues in bernoulli_connection.h
@mschmidt87
Copy link

@mschmidt87 mschmidt87 commented Jun 22, 2017

Do I understand the output of the static code analysis correctly that clang only complains about nest_names.h and modelsmodule.cpp and in these files, it complains only about lines that are not changed in this PR. Why doesn't this fail in all other pull-requests as well? Is it because clang only checks files that are touched by a pull-request?
Should we fix these issues then in this pull-request although they actually seem unrelated to the bernoulli_synapse?

@heplesser
Copy link
Contributor

@heplesser heplesser commented Jun 22, 2017

@mschmidt87 I am not entirely sure why the formatting problems in nest_names.h have not been flagged earlier, given that we have worked quite a bit in that file previously. But please fix them, even though they are on lines that you did not touch.

I am surprised that modelsmodule.cpp is flagged as failing clang format. If I look at the log for Travis #1944.1, line 4099 seems to indicate that clang-format found a problem with modelsmodule.cpp, but l2045f shows that the file passed clang-format without a diff. Or maybe I am reading the report wrong, and the fact that modelsmodule.cpp appears on line 4099 does not mean there was a problem? @gtrensch, could you comment?

@gtrensch
Copy link
Collaborator

@gtrensch gtrensch commented Jun 22, 2017

@heplesser @mschmidt87 I went through the commits and it seems that the formatting issue in nest_names.h was introduced with commit a929906. I did not check the old build logs (not sure if they are still available on Travis). So I have no idea why this was not detected before.

modulesmodule.cpp is flagged correctly. It passed the "clang diff" but failed the cppcheck. Currently we ignore those message (line 4162) but do the reporting (line 4099).

Maximilian Schmidt and others added 2 commits Jun 23, 2017
solve formatting errors in nest_names.h by running clang-format-3.6
@heplesser
Copy link
Contributor

@heplesser heplesser commented Jun 23, 2017

@hyukz Thank you for contributing this model!

@heplesser
Copy link
Contributor

@heplesser heplesser commented Jun 23, 2017

Release notes: New bernoulli_synapse model: a static synapse propagating spikes with probability p.

@heplesser heplesser merged commit 2a0f238 into nest:master Jun 23, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants