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

Device for recording weights of plastic synapses #497

Merged
merged 9 commits into from Oct 17, 2016

Conversation

Projects
None yet
7 participants
@weidel-p
Contributor

weidel-p commented Sep 28, 2016

Hi,

with this PR I'd like to add a device (weight_recorder) to NEST allowing the user to record the weight-changes of plastic synapses during the simulation.

Until now, it was necessary to stop the simulation, read the synaptic weights using GetConnections and GetStatus and resume the simulation in order to record the changes of the synaptic strengths during a simulation. With the weight_recorder the changes of synaptic strengths are recorded on the fly and without the need to stop the simulation.

To achieve a consistent user experience and for minimizing the memory overhead, the usage of the weight_recorder is based on the volume_transmitter; the weight_recorder is registered to a synapse in the CommonSynapsePorperties using SetDefaults or CopyModel.

Here is an example pyNEST script using the weight_recorder with the stdp_synapse:

import nest

wr = nest.Create('weight_recorder', 1, {"to_file": True})
nest.CopyModel("stdp_synapse", "stdp_synapse_rec", {"weight_recorder": wr[0]})

sg = nest.Create("spike_generator", params={"spike_times": [10., 15., 55., 70.]})
pre = nest.Create("parrot_neuron", 1)
post = nest.Create("parrot_neuron", 1)

nest.Connect(pre, post, syn_spec="stdp_synapse_rec")
nest.Connect(sg, pre)

nest.Simulate(100)

Internally, the weight_recorder is based on the spike_detector but handles WeightRecorderEvent instead of SpikeEvent and can not be used as a global receiver.
As a synapse has no id in NEST, the WeightRecorderEvent has an additional field receiver_gid containing the gid of the postynaptic neuron, which allows the user to identify a synapse by the gid of the pre- and postsynaptic neuron. (This, of course, can not work for multapses)

Here is the output of the weight_recorder running the example script above:
The columns containing the sender_gid, receiver_gid, time, weight in this order.

3       4       11.000  1.000   
3       4       16.000  1.879   
3       4       56.000  3.450   
3       4       71.000  4.504

As mentioned above, the weight_recorder is registered to a synapse in the CommonSynapseProperties. Also worth mentioning is that the WeightRecorderEvent is sent to the weight_recorder in connector_base.h. This implementation leads to an easy and general solution: every existing and future synapse can make use of the weight_recorder without changing the implementation or caring about it. The drawback of this solution is one additional if statement in the send function, which might slow down the simulation.

This PR also contains a pyNEST unittest, testing for consistent results of the weights recorded by the weight_recorder using a single and multiple threads.

Show outdated Hide outdated pynest/nest/tests/test_weight_recorder.py
"""Weight Recorder Single Threaded"""
nest.ResetKernel()
nest.SetKernelStatus({"local_num_threads": 2})

This comment has been minimized.

@heplesser

heplesser Sep 29, 2016

Contributor

I thought this was testSingleThread?

@heplesser

heplesser Sep 29, 2016

Contributor

I thought this was testSingleThread?

Show outdated Hide outdated pynest/nest/tests/test_weight_recorder.py
weights = np.round(weights, 6)
wr_weights = np.round(nest.GetStatus(wr, "events")[0]["weights"], 6)
self.assertTrue(all([w in weights for w in wr_weights]))

This comment has been minimized.

@heplesser

heplesser Sep 29, 2016

Contributor

I think unittest provides smarter asserts that compare collections elementwise within a given numerical precision. They provide more informative output on failure.

@heplesser

heplesser Sep 29, 2016

Contributor

I think unittest provides smarter asserts that compare collections elementwise within a given numerical precision. They provide more informative output on failure.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Sep 29, 2016

Contributor

@weidel-p Great work! I suggest @jougs and @jakobj as reviewers.

I guess plasticity could also change delays (at least in principle, I think we have no connections in NEST at the moment doing this), so I think it would be useful if the weight_recorder could also record delays.

Contributor

heplesser commented Sep 29, 2016

@weidel-p Great work! I suggest @jougs and @jakobj as reviewers.

I guess plasticity could also change delays (at least in principle, I think we have no connections in NEST at the moment doing this), so I think it would be useful if the weight_recorder could also record delays.

@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Sep 29, 2016

Contributor

@abigailm: can you please comment on plasticity changing delays?

Contributor

jougs commented Sep 29, 2016

@abigailm: can you please comment on plasticity changing delays?

@abigailm

This comment has been minimized.

Show comment
Hide comment
@abigailm

abigailm Sep 29, 2016

Contributor

yes, I can. I think it's not necessary now, as we have no plasticity models that do this, and we would have to think carefully before adding them as the minimum synaptic delay is kind of important. I would recommend letting this PR through without this change, and adapted the device later, if such models are added.

Contributor

abigailm commented Sep 29, 2016

yes, I can. I think it's not necessary now, as we have no plasticity models that do this, and we would have to think carefully before adding them as the minimum synaptic delay is kind of important. I would recommend letting this PR through without this change, and adapted the device later, if such models are added.

@abigailm

This comment has been minimized.

Show comment
Hide comment
@abigailm

abigailm Sep 29, 2016

Contributor

The "advantage" of the GetConnections method is that you can select pre- and post-synaptic neurons in the query. So for example, I can connect a recurrent network randomly, and then record, say, all the outgoing connections from a selected pop1, or all the incoming connections to a selected pop2, or all the connections between pop1 and pop2. Is this functionality covered by this PR? It would be important for large scale simulations to be able to pick out specific groups to be recorded after the network has been wired.

Contributor

abigailm commented Sep 29, 2016

The "advantage" of the GetConnections method is that you can select pre- and post-synaptic neurons in the query. So for example, I can connect a recurrent network randomly, and then record, say, all the outgoing connections from a selected pop1, or all the incoming connections to a selected pop2, or all the connections between pop1 and pop2. Is this functionality covered by this PR? It would be important for large scale simulations to be able to pick out specific groups to be recorded after the network has been wired.

@weidel-p

This comment has been minimized.

Show comment
Hide comment
@weidel-p

weidel-p Sep 29, 2016

Contributor

@abigailm: Recording the weight-changes of all synapses in a large scale simulation will lead to a large amount of data. To limit the amount of recorded data, the user can specify subpopulations using a weight_recorder before/during the wiring of the network using CopyModel.

So in general this PR covers this aspect, but I agree that the workflow could be nicer. I would prefer to register a weight_recorder to a subset ob synapses after wiring the network using SetStatus on selected connections. This means that the weight_recorder would have to be registered in the properties of every synapse instead of in the CommonSynapseProperties resulting in an increased memory overhead. I don't know how bad this would be for HPC applications, but I didn't want to limit the HPC capacity of NEST with this device.

I am open for suggestions to improve the implementation. Maybe I missed a smart way to implement a nicer workflow without increasing the memory overhead.

Contributor

weidel-p commented Sep 29, 2016

@abigailm: Recording the weight-changes of all synapses in a large scale simulation will lead to a large amount of data. To limit the amount of recorded data, the user can specify subpopulations using a weight_recorder before/during the wiring of the network using CopyModel.

So in general this PR covers this aspect, but I agree that the workflow could be nicer. I would prefer to register a weight_recorder to a subset ob synapses after wiring the network using SetStatus on selected connections. This means that the weight_recorder would have to be registered in the properties of every synapse instead of in the CommonSynapseProperties resulting in an increased memory overhead. I don't know how bad this would be for HPC applications, but I didn't want to limit the HPC capacity of NEST with this device.

I am open for suggestions to improve the implementation. Maybe I missed a smart way to implement a nicer workflow without increasing the memory overhead.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Sep 29, 2016

Contributor

@jougs @abigailm You are right, plastic delays would raise a number of questions. So let's proceed with this PR recording weights only.

Contributor

heplesser commented Sep 29, 2016

@jougs @abigailm You are right, plastic delays would raise a number of questions. So let's proceed with this PR recording weights only.

@abigailm

This comment has been minimized.

Show comment
Hide comment
@abigailm

abigailm Sep 29, 2016

Contributor

@weidel-p I see how that would work in many instances, but I think it is problematic. Often the user will not be able to determine pre-connection the synapses that he or she wants to record. For example, I connect a network with fixed indegree, and then I want to record all outgoing connections of a particular sub-population. There would be no way of labelling those particular ones ahead of time with a CopyModel, because they were selected randomly. Would it be possible to supplement the weight_recorder with properties identifying the pre- and post-synaptic populations, enabling it to do filtering of results on the fly? The default behaviour would be to record all synapses it is attached to, but the user could specify those variables if needed.

Contributor

abigailm commented Sep 29, 2016

@weidel-p I see how that would work in many instances, but I think it is problematic. Often the user will not be able to determine pre-connection the synapses that he or she wants to record. For example, I connect a network with fixed indegree, and then I want to record all outgoing connections of a particular sub-population. There would be no way of labelling those particular ones ahead of time with a CopyModel, because they were selected randomly. Would it be possible to supplement the weight_recorder with properties identifying the pre- and post-synaptic populations, enabling it to do filtering of results on the fly? The default behaviour would be to record all synapses it is attached to, but the user could specify those variables if needed.

for ( std::vector< WeightRecorderEvent >::iterator e = B_.events_.begin();
e != B_.events_.end();
++e )
{

This comment has been minimized.

@abigailm

abigailm Sep 29, 2016

Contributor

So, for example, couldn't we test here whether the sender is one of the desired presynaptic population (if specified), ditto receiver, and only record it if it was?

@abigailm

abigailm Sep 29, 2016

Contributor

So, for example, couldn't we test here whether the sender is one of the desired presynaptic population (if specified), ditto receiver, and only record it if it was?

Show outdated Hide outdated models/weight_recorder.cpp
if ( device_.is_active( e.get_stamp() ) )
{
WeightRecorderEvent* event = e.clone();
B_.events_.push_back( *event );

This comment has been minimized.

@abigailm

abigailm Sep 29, 2016

Contributor

It might be better to check for sender/receiver here, to avoid events_ getting swamped in a large simulation. I suggest testing for performance in a large network.

@abigailm

abigailm Sep 29, 2016

Contributor

It might be better to check for sender/receiver here, to avoid events_ getting swamped in a large simulation. I suggest testing for performance in a large network.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Sep 29, 2016

Contributor

I like @abigailm 's suggestions about selecting by senders/targets. An interesting questions is the most efficient way to implement this. From @abigailm 's use-case description, I assume that the senders/targets one would want to select are not contiguous ranges of GIDs. Thus, STL set is probably the most useful data structure to keep the "wanted" nodes and check membership. But given that the set of senders/targets to select from does not change over time (or only at large intervals when the user reconfigures the recorder), we could also put them in a sorted vector and then do a binary search, maybe even with linear interpolation to determine the starting point (see SparseNodeArray).

@abigailm How many senders and targets would you typically select, just a ballpark number?

Contributor

heplesser commented Sep 29, 2016

I like @abigailm 's suggestions about selecting by senders/targets. An interesting questions is the most efficient way to implement this. From @abigailm 's use-case description, I assume that the senders/targets one would want to select are not contiguous ranges of GIDs. Thus, STL set is probably the most useful data structure to keep the "wanted" nodes and check membership. But given that the set of senders/targets to select from does not change over time (or only at large intervals when the user reconfigures the recorder), we could also put them in a sorted vector and then do a binary search, maybe even with linear interpolation to determine the starting point (see SparseNodeArray).

@abigailm How many senders and targets would you typically select, just a ballpark number?

@abigailm

This comment has been minimized.

Show comment
Hide comment
@abigailm

abigailm Sep 30, 2016

Contributor

Another advantage of the GetConnections method is that it allows the weights to be sampled with a lower rate, say every second, which massively reduces the file size on long simulations of large networks. I recommend that this not be a requirement for the current PR, but to open it as a ticket for a later enhancement of the weight_recorder. In the mean time, those who need the down-sampling can still use the GetConnections approach as before.

Contributor

abigailm commented Sep 30, 2016

Another advantage of the GetConnections method is that it allows the weights to be sampled with a lower rate, say every second, which massively reduces the file size on long simulations of large networks. I recommend that this not be a requirement for the current PR, but to open it as a ticket for a later enhancement of the weight_recorder. In the mean time, those who need the down-sampling can still use the GetConnections approach as before.

@abigailm

This comment has been minimized.

Show comment
Hide comment
@abigailm

abigailm Sep 30, 2016

Contributor

@heplesser For example, Kunkel et al. (2011) records weights from ~100s of neurons for ~100s of seconds

Contributor

abigailm commented Sep 30, 2016

@heplesser For example, Kunkel et al. (2011) records weights from ~100s of neurons for ~100s of seconds

@heplesser

@weidel-p Nicely done. Have you done any performance tests, just out of curiosity? I have a few suggestions as inline comments.

Show outdated Hide outdated models/weight_recorder.cpp
if ( !P_.pre_.empty()
&& std::find( P_.pre_.begin(), P_.pre_.end(), e.get_sender_gid() )
== P_.pre_.end() )
return;

This comment has been minimized.

@heplesser

heplesser Sep 30, 2016

Contributor

@weidel-p I would suggest that you combine the pre and post tests. I think that will make the code more readable. Finally, we decided to write out logical operators in new NEST C++ code as in Python:

if ( ( not P_.pre_.empty() and std::find(...) == P_.pre_.end() )
      or ( not P_.post_.empty() and std::find(...) == P_.post_.end() ) ) 
@heplesser

heplesser Sep 30, 2016

Contributor

@weidel-p I would suggest that you combine the pre and post tests. I think that will make the code more readable. Finally, we decided to write out logical operators in new NEST C++ code as in Python:

if ( ( not P_.pre_.empty() and std::find(...) == P_.pre_.end() )
      or ( not P_.post_.empty() and std::find(...) == P_.post_.end() ) ) 
Show outdated Hide outdated models/weight_recorder.cpp
nest::weight_recorder::Parameters_::get( DictionaryDatum& d ) const
{
//( *d )[ "pre" ] = pre_;
//( *d )[ "post" ] = post_;

This comment has been minimized.

@heplesser

heplesser Sep 30, 2016

Contributor

Please remove the commented lines.

@heplesser

heplesser Sep 30, 2016

Contributor

Please remove the commented lines.

Show outdated Hide outdated models/weight_recorder.cpp
//( *d )[ "pre" ] = pre_;
//( *d )[ "post" ] = post_;
( *d )[ "pre" ] = std::vector< long >( pre_.begin(), pre_.end() );
( *d )[ "post" ] = std::vector< long >( post_.begin(), post_.end() );

This comment has been minimized.

@heplesser

heplesser Sep 30, 2016

Contributor

See Parameters_::set() below for comments on pre/post names.

I have a design question here, and maybe @abigailm and @jougs could comment: How large would pre/post be, and how likely would they contain contiguous ranges of GIDs or isolated GIDs? That would influence the question of whether pre/post should be passed as GID vectors or as GIDCollections.

@heplesser

heplesser Sep 30, 2016

Contributor

See Parameters_::set() below for comments on pre/post names.

I have a design question here, and maybe @abigailm and @jougs could comment: How large would pre/post be, and how likely would they contain contiguous ranges of GIDs or isolated GIDs? That would influence the question of whether pre/post should be passed as GID vectors or as GIDCollections.

Show outdated Hide outdated models/weight_recorder.cpp
std::vector< long > tmp =
getValue< std::vector< long > >( d->lookup( "post" ) );
post_ = std::set< long >( tmp.begin(), tmp.end() );
}

This comment has been minimized.

@heplesser

heplesser Sep 30, 2016

Contributor

Please add "pre" and "post" to NEST names. And are pre and post really good names here? In all connect routines we use "source" and "target", so that might be better to use here, too, to minimize user confusion.

See Parameters_::get() for comments on vector vs GidCollection.

@heplesser

heplesser Sep 30, 2016

Contributor

Please add "pre" and "post" to NEST names. And are pre and post really good names here? In all connect routines we use "source" and "target", so that might be better to use here, too, to minimize user confusion.

See Parameters_::get() for comments on vector vs GidCollection.

This comment has been minimized.

@weidel-p

weidel-p Sep 30, 2016

Contributor

As suggested, I changed the names to "source" and "target".

@weidel-p

weidel-p Sep 30, 2016

Contributor

As suggested, I changed the names to "source" and "target".

Show outdated Hide outdated models/weight_recorder.h
struct Parameters_
{
std::set< long > pre_;
std::set< long > post_;

This comment has been minimized.

@heplesser

heplesser Sep 30, 2016

Contributor

I think set is a good first choice, we can later consider if we need something even more efficient. Note that we usually use the alias index for GIDs, I'd suggest using that here as well.

@heplesser

heplesser Sep 30, 2016

Contributor

I think set is a good first choice, we can later consider if we need something even more efficient. Note that we usually use the alias index for GIDs, I'd suggest using that here as well.

This comment has been minimized.

@weidel-p

weidel-p Sep 30, 2016

Contributor

I agree, but actually I had an issue with Travis and std::set. So I picked up your earlier idea and changed these parameters to sorted vector and use a binary search to access elements. Fortunately, these two fields will only be set once (or a few times) in the build phase, so the vector has not to be resorted during the simulation.

Overall, I think this solution is better than a set as the complexity to access elements is also O(log N) and I think a vector needs less memory.

@weidel-p

weidel-p Sep 30, 2016

Contributor

I agree, but actually I had an issue with Travis and std::set. So I picked up your earlier idea and changed these parameters to sorted vector and use a binary search to access elements. Fortunately, these two fields will only be set once (or a few times) in the build phase, so the vector has not to be resorted during the simulation.

Overall, I think this solution is better than a set as the complexity to access elements is also O(log N) and I think a vector needs less memory.

Show outdated Hide outdated models/weight_recorder.cpp
@@ -24,6 +24,7 @@
// C++ includes:
#include <numeric>
#include <typeinfo>

This comment has been minimized.

@heplesser

heplesser Sep 30, 2016

Contributor

Do you need the typeinfo header for anything?

@heplesser

heplesser Sep 30, 2016

Contributor

Do you need the typeinfo header for anything?

@weidel-p

This comment has been minimized.

Show comment
Hide comment
@weidel-p

weidel-p Sep 30, 2016

Contributor

@heplesser and @abigailm: thanks for your comments and help. I tried to implement most suggestions in f6f7f23. I did not run any performance tests, yet.

The idea of having a sampling rate is probably very important for many use-cases and I will try to implement that asap. Another minor change which came up in a discussion with @jakobj is the possibility to record the port of a synapse to identify unique synapses even if there are multapses.
I would prefer to add these two features to this PR before merging.

Contributor

weidel-p commented Sep 30, 2016

@heplesser and @abigailm: thanks for your comments and help. I tried to implement most suggestions in f6f7f23. I did not run any performance tests, yet.

The idea of having a sampling rate is probably very important for many use-cases and I will try to implement that asap. Another minor change which came up in a discussion with @jakobj is the possibility to record the port of a synapse to identify unique synapses even if there are multapses.
I would prefer to add these two features to this PR before merging.

@heplesser

@weidel-p Your changes are fine, also the idea of allowing recording of the port. You should then also allow recording of the rport, in case the target has different input synapses.

I am a bit uncertain about the sampling rate. Weights are sampled only when spikes pass through. If you wanted to record weight only for every n-th spike through a synapse, you would need to keep track of the spike counts for all connections, which would be very demanding. If you record only in short windows, you may miss a lot of synapses.

Show outdated Hide outdated models/weight_recorder.cpp
nest::weight_recorder::Parameters_::get( DictionaryDatum& d ) const
{
( *d )[ nest::names::source ] = sources_;
( *d )[ nest::names::target ] = targets_;

This comment has been minimized.

@heplesser

heplesser Oct 1, 2016

Contributor

Since you are inside namespace nest here, you do not need the nest:: qualifiers. Applies also in set().

@heplesser

heplesser Oct 1, 2016

Contributor

Since you are inside namespace nest here, you do not need the nest:: qualifiers. Applies also in set().

@abigailm

This comment has been minimized.

Show comment
Hide comment
@abigailm

abigailm Oct 1, 2016

Contributor

@heplesser the sampling rate is based on time rather than numbers of spikes. So the simulation is run for time t, we query the synapses as to what was their weight the last time they produced a spike, simulate for time t, query the synapses, etc. No maintenance of spike count required.

Contributor

abigailm commented Oct 1, 2016

@heplesser the sampling rate is based on time rather than numbers of spikes. So the simulation is run for time t, we query the synapses as to what was their weight the last time they produced a spike, simulate for time t, query the synapses, etc. No maintenance of spike count required.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Oct 1, 2016

Contributor

@abigailm That works indeed very nicely with the stop/GetStatus/resume approach, but could it be carried over to continuous recording?

Contributor

heplesser commented Oct 1, 2016

@abigailm That works indeed very nicely with the stop/GetStatus/resume approach, but could it be carried over to continuous recording?

@abigailm

This comment has been minimized.

Show comment
Hide comment
@abigailm

abigailm Oct 1, 2016

Contributor

@heplesser Well I don't know for sure, which is why I would be ok with Philipp submitting this PR as it is and treating that as a separate issue (if he wants to handle it like that). I think it would be compatible, but the mechanism of sending the synaptic information to the weight_recorder would have to be changed, I guess by switching to a polling approach like Multimeter.

Contributor

abigailm commented Oct 1, 2016

@heplesser Well I don't know for sure, which is why I would be ok with Philipp submitting this PR as it is and treating that as a separate issue (if he wants to handle it like that). I think it would be compatible, but the mechanism of sending the synaptic information to the weight_recorder would have to be changed, I guess by switching to a polling approach like Multimeter.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Oct 2, 2016

Contributor

@abigailm @weidel-p I would suggest that we leave the sampling rate out of this PR and first collect some experience with this recorder in practice to see how it performs.

Contributor

heplesser commented Oct 2, 2016

@abigailm @weidel-p I would suggest that we leave the sampling rate out of this PR and first collect some experience with this recorder in practice to see how it performs.

Show outdated Hide outdated models/weight_recorder.cpp
// record time, gid and weight
, device_( *this,
RecordingDevice::SPIKE_DETECTOR,
"gdf",

This comment has been minimized.

@jakobj

jakobj Oct 3, 2016

Contributor

does this mean the file extension from this recorded is also .gdf? maybe it would be better to change that to avoid confusion.

@jakobj

jakobj Oct 3, 2016

Contributor

does this mean the file extension from this recorded is also .gdf? maybe it would be better to change that to avoid confusion.

Show outdated Hide outdated nestkernel/common_synapse_properties.cpp
{
if ( weight_recorders_.size() != 0 )
def< long >( d, "weight_recorder", weight_recorders_[ 0 ]->get_gid() );

This comment has been minimized.

@jakobj

jakobj Oct 3, 2016

Contributor

missing curly braces

@jakobj

jakobj Oct 3, 2016

Contributor

missing curly braces

static_cast< GenericConnectorModel< ConnectionT >* >( cm[ syn_id ] )
->get_common_properties() );
C_[ i ].send( e, t, ConnectorBase::get_t_lastspike(), cp );
ConnectorBase::send_weight_event( cp, e, t );

This comment has been minimized.

@jakobj

jakobj Oct 3, 2016

Contributor

since delivery of spikes is already a bottleneck, I'd really like to see some benchmarks to make sure this does not hit performance too hard. I'll run a weakscaling on JQ later today.

@jakobj

jakobj Oct 3, 2016

Contributor

since delivery of spikes is already a bottleneck, I'd really like to see some benchmarks to make sure this does not hit performance too hard. I'll run a weakscaling on JQ later today.

This comment has been minimized.

@mschmidt87

mschmidt87 Oct 4, 2016

But the weight_events are only communicated locally to the weight_recorder, right? So, at least this should not increase MPI communication.

@mschmidt87

mschmidt87 Oct 4, 2016

But the weight_events are only communicated locally to the weight_recorder, right? So, at least this should not increase MPI communication.

This comment has been minimized.

@jakobj

jakobj Oct 5, 2016

Contributor

yes, you're right. they do not contribute to MPI communication, but adds an additional if for every spike event that is delivered. turns out it's not a performance issue (see below).

@jakobj

jakobj Oct 5, 2016

Contributor

yes, you're right. they do not contribute to MPI communication, but adds an additional if for every spike event that is delivered. turns out it's not a performance issue (see below).

Show outdated Hide outdated nestkernel/recording_device.cpp
if ( P_.withreceivergid_ )
{
receiver =
dynamic_cast< const WeightRecorderEvent* >( &event )->get_receiver_gid();

This comment has been minimized.

@jakobj

jakobj Oct 3, 2016

Contributor

this should be a static_cast

@jakobj

jakobj Oct 3, 2016

Contributor

this should be a static_cast

Show outdated Hide outdated models/weight_recorder.cpp
void
nest::weight_recorder::init_state_( const Node& np )
{
const weight_recorder& wr = dynamic_cast< const weight_recorder& >( np );

This comment has been minimized.

@jakobj

jakobj Oct 3, 2016

Contributor

this should be a static_cast.

@jakobj

jakobj Oct 3, 2016

Contributor

this should be a static_cast.

Show outdated Hide outdated models/weight_recorder.cpp
void
nest::weight_recorder::Parameters_::set( const DictionaryDatum& d )
{

This comment has been minimized.

@jakobj

jakobj Oct 3, 2016

Contributor

you should allow the user to specify the index of the connection (the last entry in every element returned by GetConnections) or record the index as well. otherwise weights from multapses are not distinguishable.

@jakobj

jakobj Oct 3, 2016

Contributor

you should allow the user to specify the index of the connection (the last entry in every element returned by GetConnections) or record the index as well. otherwise weights from multapses are not distinguishable.

Show outdated Hide outdated nestkernel/recording_device.cpp
@@ -290,6 +295,14 @@ nest::RecordingDevice::State_::get( DictionaryDatum& d,
dict, names::weights, std::vector< double >( event_weights_ ) );
}
if ( p.withreceivergid_ )

This comment has been minimized.

@jakobj

jakobj Oct 3, 2016

Contributor

i was wondering, what's the use case of not recording the receiver gid? don't you always need at least the tuple (sender, receiver) to make sense out of the recorded data? or is this just signalling that this is a weight_recorder? in that case, maybe you could use a more explicit variable name?

@jakobj

jakobj Oct 3, 2016

Contributor

i was wondering, what's the use case of not recording the receiver gid? don't you always need at least the tuple (sender, receiver) to make sense out of the recorded data? or is this just signalling that this is a weight_recorder? in that case, maybe you could use a more explicit variable name?

This comment has been minimized.

@weidel-p

weidel-p Oct 4, 2016

Contributor

There was actually a bug in the code. It was meant that only the weight_recorder is using withreceivergid. But actually, the user can set this parameter to every recording device (i.e. the spike_detector), which could lead to undefined behavior as get_receiver_gid() was only implemented in the WeightRecorderEvent. I fixed that in 72c7fae, thanks.

@weidel-p

weidel-p Oct 4, 2016

Contributor

There was actually a bug in the code. It was meant that only the weight_recorder is using withreceivergid. But actually, the user can set this parameter to every recording device (i.e. the spike_detector), which could lead to undefined behavior as get_receiver_gid() was only implemented in the WeightRecorderEvent. I fixed that in 72c7fae, thanks.

@jakobj

This comment has been minimized.

Show comment
Hide comment
@jakobj

jakobj Oct 3, 2016

Contributor

very nice work @weidel-p. i'll let you know as soon as i have same results from benchmarks.

Contributor

jakobj commented Oct 3, 2016

very nice work @weidel-p. i'll let you know as soon as i have same results from benchmarks.

@weidel-p

This comment has been minimized.

Show comment
Hide comment
@weidel-p

weidel-p Oct 4, 2016

Contributor

@heplesser @abigailm: instead of going to a polling approach I added two parameter to the weight_recorder: interval and duration. By setting interval and duration, only events with a timestamp within this interval and duration will be recorded. These parameters are of course optional, and not the perfect solution, but can help to reduce the amount of data.

@jakobj: thanks for your comments. I tried to implement them all.

Contributor

weidel-p commented Oct 4, 2016

@heplesser @abigailm: instead of going to a polling approach I added two parameter to the weight_recorder: interval and duration. By setting interval and duration, only events with a timestamp within this interval and duration will be recorded. These parameters are of course optional, and not the perfect solution, but can help to reduce the amount of data.

@jakobj: thanks for your comments. I tried to implement them all.

@mschmidt87

This comment has been minimized.

Show comment
Hide comment
@mschmidt87

mschmidt87 Oct 4, 2016

I tried the new parameters interval and duration and don't understand how they are supposed to work. From the code, I understand that if I set interval, only spikes that are a multiple of interval, are recorded. I confirmed this by using your example code from the first post and setting interval=16.. Then, only the spike at 15. ms (+1ms delay) being written out, because it is the only multiple integer of 16.
Is that the desired behavior?

If I set duration to a finite value, e.g. 10. ms, then the spikes at 15., 55. and 75. ms are written out.

Am I missing here something? If I understood @abigailm correctly, then she is suggesting to implement a fixed time interval at which the current weight of the recorded synapses are written out.

mschmidt87 commented Oct 4, 2016

I tried the new parameters interval and duration and don't understand how they are supposed to work. From the code, I understand that if I set interval, only spikes that are a multiple of interval, are recorded. I confirmed this by using your example code from the first post and setting interval=16.. Then, only the spike at 15. ms (+1ms delay) being written out, because it is the only multiple integer of 16.
Is that the desired behavior?

If I set duration to a finite value, e.g. 10. ms, then the spikes at 15., 55. and 75. ms are written out.

Am I missing here something? If I understood @abigailm correctly, then she is suggesting to implement a fixed time interval at which the current weight of the recorded synapses are written out.

@weidel-p

This comment has been minimized.

Show comment
Hide comment
@weidel-p

weidel-p Oct 4, 2016

Contributor

@mschmidt87: sorry, I should have explained that a bit more in detail. My idea was to record events only if they have a timestamp which are within a certain duration in a repeating interval. I.e. to record "spikes" if they happen in the first 100ms of each second in the simulation.

To make this example, assume to set interval: 1000ms and duration: 100ms and there are events happening at [50 ms, 700 ms, 1010 ms, 1400 ms] then, only the first and third event are recorded.

If I understood correctly, @abigailm and @heplesser both actually suggested to postpone the topic of sampling. But I think it is very important to have some functionality of down-sampling now (also for my own research), even if it is not the most popular solution. We can still improve the weight_recorder and the sampling method in a new issue after this PR.

Contributor

weidel-p commented Oct 4, 2016

@mschmidt87: sorry, I should have explained that a bit more in detail. My idea was to record events only if they have a timestamp which are within a certain duration in a repeating interval. I.e. to record "spikes" if they happen in the first 100ms of each second in the simulation.

To make this example, assume to set interval: 1000ms and duration: 100ms and there are events happening at [50 ms, 700 ms, 1010 ms, 1400 ms] then, only the first and third event are recorded.

If I understood correctly, @abigailm and @heplesser both actually suggested to postpone the topic of sampling. But I think it is very important to have some functionality of down-sampling now (also for my own research), even if it is not the most popular solution. We can still improve the weight_recorder and the sampling method in a new issue after this PR.

@mschmidt87

This comment has been minimized.

Show comment
Hide comment
@mschmidt87

mschmidt87 Oct 4, 2016

Okay, I see. This sounds like a relatively simple and smart solution without having to change the implementation altogether. Although I am not sure how practicable this might be in the end, because the risk is to miss a lot of events just by chance if interval and duration are not carefully tuned.

In any case, I would suggest to rename the parameters from interval and duration to interval_step and interval_length. In addition, this feature should be documented in the doc string of the weight_recorder.

Okay, I see. This sounds like a relatively simple and smart solution without having to change the implementation altogether. Although I am not sure how practicable this might be in the end, because the risk is to miss a lot of events just by chance if interval and duration are not carefully tuned.

In any case, I would suggest to rename the parameters from interval and duration to interval_step and interval_length. In addition, this feature should be documented in the doc string of the weight_recorder.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Oct 5, 2016

Contributor

@weidel-p Your interval idea is interesting in principle, but I would still prefer to keep it out of this PR for reasons I will explain below. Note that any form of windowing or sub-sampling will be an addition to thw recording itself, so it can easily be added in a later PR (and you can even use it for your own work as is till then).

My main reason for asking you to leave the windowing out is that we should take a little more time to think through approaches, syntax and semantics before committing to a solution. Windowing will probably be useful for many types of recordings, so we should aim for a more general solution. It also needs to be compatible (reasonably) with existing solutions to ensure a consistent user experience across recorders. I opened #505 for suggestions and further discussion on this.

Concerning weight recording, I am also wondering a little whether windowing really is the best approach, since weights are only observable when a spike passes through a synapse. Could it be an idea to simply use random sub-sampling of weights: spikes arriving at the weight recorder are stored only with a fixed, given probability (usually quite small). I think it would depend on the scientific questions investigated whether this would make more or less sense than windowing.

Contributor

heplesser commented Oct 5, 2016

@weidel-p Your interval idea is interesting in principle, but I would still prefer to keep it out of this PR for reasons I will explain below. Note that any form of windowing or sub-sampling will be an addition to thw recording itself, so it can easily be added in a later PR (and you can even use it for your own work as is till then).

My main reason for asking you to leave the windowing out is that we should take a little more time to think through approaches, syntax and semantics before committing to a solution. Windowing will probably be useful for many types of recordings, so we should aim for a more general solution. It also needs to be compatible (reasonably) with existing solutions to ensure a consistent user experience across recorders. I opened #505 for suggestions and further discussion on this.

Concerning weight recording, I am also wondering a little whether windowing really is the best approach, since weights are only observable when a spike passes through a synapse. Could it be an idea to simply use random sub-sampling of weights: spikes arriving at the weight recorder are stored only with a fixed, given probability (usually quite small). I think it would depend on the scientific questions investigated whether this would make more or less sense than windowing.

@jakobj

This comment has been minimized.

Show comment
Hide comment
@jakobj

jakobj Oct 5, 2016

Contributor

here are some results from a weak scaling benchmark on JQ, comparing master and this branch. turns out the additional if in send has basically no influence on performance -- apparently branch prediction works well.
juqueen_weak_scaling_weight_recorder_master
this also means that I am happy with this PR now, so 👍, conditional on @heplesser concerns being addressed.

Contributor

jakobj commented Oct 5, 2016

here are some results from a weak scaling benchmark on JQ, comparing master and this branch. turns out the additional if in send has basically no influence on performance -- apparently branch prediction works well.
juqueen_weak_scaling_weight_recorder_master
this also means that I am happy with this PR now, so 👍, conditional on @heplesser concerns being addressed.

@mschmidt87

This comment has been minimized.

Show comment
Hide comment
@mschmidt87

mschmidt87 Oct 5, 2016

Wouldn't it be interesting to check the performance of actually recording weights vs. no recording? Perhaps not on the supercomputer, but on the cluster for some configurations? Or did you already do that, @weidel-p ? Perhaps there is some performance issue that we don't see from the code itself?

Wouldn't it be interesting to check the performance of actually recording weights vs. no recording? Perhaps not on the supercomputer, but on the cluster for some configurations? Or did you already do that, @weidel-p ? Perhaps there is some performance issue that we don't see from the code itself?

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Oct 5, 2016

Contributor

@mschmidt87 I think the benchmark by @jakobj shows that there is no cost when not recording, and that is the most important thing. Recording will obviously cost, but I don't think we need to benchmark that explicitly at this point.

@weidel-p Could you merge the latest changes from master to solve the merge conflicts that have popped up.

Contributor

heplesser commented Oct 5, 2016

@mschmidt87 I think the benchmark by @jakobj shows that there is no cost when not recording, and that is the most important thing. Recording will obviously cost, but I don't think we need to benchmark that explicitly at this point.

@weidel-p Could you merge the latest changes from master to solve the merge conflicts that have popped up.

@weidel-p

This comment has been minimized.

Show comment
Hide comment
@weidel-p

weidel-p Oct 5, 2016

Contributor

@heplesser: in bc04f15 I removed the sampling parameters and in 522e3fa I merged master. Are all issues addressed now, or did I forget about something?

Contributor

weidel-p commented Oct 5, 2016

@heplesser: in bc04f15 I removed the sampling parameters and in 522e3fa I merged master. Are all issues addressed now, or did I forget about something?

@heplesser

@weidel-p Good work! I added quite a number of comments on implementations (and UI) details that I think can be improved. I will try to have a separate PR on changing the SiblingContainer interface ready very shortly (better method names, move some methods to public interface so "friends" can go).

Show outdated Hide outdated pynest/nest/tests/test_weight_recorder.py
self.addTypeEqualityFunc(type(wr_targets), self.is_subset)
self.assertEqual(wr_targets, targets)
def testMultipses(self):

This comment has been minimized.

@heplesser

heplesser Oct 5, 2016

Contributor

"Multipses" -> "Multapses"

@heplesser

heplesser Oct 5, 2016

Contributor

"Multipses" -> "Multapses"

@nest.check_stack
class WeightRecorderTestCase(unittest.TestCase):

This comment has been minimized.

@heplesser

heplesser Oct 5, 2016

Contributor

Could you add one test that selects by both senders and targets, and one test that checks that rports are recorded properly? Any multisynapse model should work for that.

@heplesser

heplesser Oct 5, 2016

Contributor

Could you add one test that selects by both senders and targets, and one test that checks that rports are recorded properly? Any multisynapse model should work for that.

Show outdated Hide outdated nestkernel/common_synapse_properties.h
/**
* weight recorder
*/
std::vector< Node* > weight_recorders_;

This comment has been minimized.

@heplesser

heplesser Oct 5, 2016

Contributor

This should be private.

I would also suggest a slightly different (and easier) solution here: NEST has a SiblingContainer class, which NodeManager uses to handle nodes that are replicated across threads. You only need a pointer to SiblingContainer* here, which you can obtain from NodeManager::get_thread_siblings(). Class SiblingContainer requires some updates, I will shortly create a separate PR for that---please wait for that.

I would then change the variable name to singluar here, after all, logically it is only a single recorder.

@heplesser

heplesser Oct 5, 2016

Contributor

This should be private.

I would also suggest a slightly different (and easier) solution here: NEST has a SiblingContainer class, which NodeManager uses to handle nodes that are replicated across threads. You only need a pointer to SiblingContainer* here, which you can obtain from NodeManager::get_thread_siblings(). Class SiblingContainer requires some updates, I will shortly create a separate PR for that---please wait for that.

I would then change the variable name to singluar here, after all, logically it is only a single recorder.

Show outdated Hide outdated nestkernel/common_synapse_properties.cpp
@@ -27,6 +27,7 @@
#include "nest_timeconverter.h"
#include "nest_types.h"
#include "node.h"
#include "kernel_manager.h"

This comment has been minimized.

@heplesser

heplesser Oct 5, 2016

Contributor

Includes should be ordered alphabetically.

@heplesser

heplesser Oct 5, 2016

Contributor

Includes should be ordered alphabetically.

Show outdated Hide outdated nestkernel/common_synapse_properties.cpp
@@ -39,6 +40,7 @@ namespace nest
*/
CommonSynapseProperties::CommonSynapseProperties()
: weight_recorders_( 0 )

This comment has been minimized.

@heplesser

heplesser Oct 5, 2016

Contributor

This is an STL vector, so you don't need the 0 as argument, just weight_recorders_() is better. But once this turns into a SiblingContainer* initializing with 0 will be proper.

@heplesser

heplesser Oct 5, 2016

Contributor

This is an STL vector, so you don't need the 0 as argument, just weight_recorders_() is better. But once this turns into a SiblingContainer* initializing with 0 will be proper.

Show outdated Hide outdated models/weight_recorder.cpp
}
}
nest::weight_recorder::Parameters_::Parameters_()

This comment has been minimized.

@heplesser

heplesser Oct 5, 2016

Contributor

Please move Parameters_ code forward so it is in the same location within the file as for other models.

@heplesser

heplesser Oct 5, 2016

Contributor

Please move Parameters_ code forward so it is in the same location within the file as for other models.

Show outdated Hide outdated models/weight_recorder.h
The weight detector can also record weights with full precision
from neurons emitting precisely timed spikes. Set /precise_times to
achieve this.

This comment has been minimized.

@heplesser

heplesser Oct 5, 2016

Contributor

Please document sources and targets parameters.

@heplesser

heplesser Oct 5, 2016

Contributor

Please document sources and targets parameters.

Show outdated Hide outdated models/weight_recorder.cpp
nest::weight_recorder::weight_recorder()
: Node()
// record time, gid, weight and receiver gid

This comment has been minimized.

@heplesser

heplesser Oct 5, 2016

Contributor

Remove comment here, rather add one per line below.

@heplesser

heplesser Oct 5, 2016

Contributor

Remove comment here, rather add one per line below.

Show outdated Hide outdated models/weight_recorder.cpp
: Node()
// record time, gid, weight and receiver gid
, device_( *this,
RecordingDevice::SPIKE_DETECTOR,

This comment has been minimized.

@heplesser

heplesser Oct 5, 2016

Contributor

SPIKE_DETECTOR?

@heplesser

heplesser Oct 5, 2016

Contributor

SPIKE_DETECTOR?

Show outdated Hide outdated models/weight_recorder.cpp
// record time, gid, weight and receiver gid
, device_( *this,
RecordingDevice::SPIKE_DETECTOR,
"csv",

This comment has been minimized.

@heplesser

heplesser Oct 5, 2016

Contributor

"csv" is misleading since we separate by tab, not comma. I would suggest either "tsv" or "dat".

@heplesser

heplesser Oct 5, 2016

Contributor

"csv" is misleading since we separate by tab, not comma. I would suggest either "tsv" or "dat".

This comment has been minimized.

@weidel-p

weidel-p Oct 7, 2016

Contributor

I would like to keep csv and in my opinion it is not misleading. It is very common to have a tab-separated csv file. Every csv reader I've seen so far (i.e. in Python, Excel, LibreOffice, etc) has the option to choose the delimiter. Often this is even done automatically.

The advantage of using a known file extension like 'csv' is that user immediately knows how to open the file.

@weidel-p

weidel-p Oct 7, 2016

Contributor

I would like to keep csv and in my opinion it is not misleading. It is very common to have a tab-separated csv file. Every csv reader I've seen so far (i.e. in Python, Excel, LibreOffice, etc) has the option to choose the delimiter. Often this is even done automatically.

The advantage of using a known file extension like 'csv' is that user immediately knows how to open the file.

@weidel-p

This comment has been minimized.

Show comment
Hide comment
@weidel-p

weidel-p Oct 5, 2016

Contributor

@heplesser thanks for looking over the code again; it was clearly necessary. I will wait for your SiblingContainer PR to address your comments.

Contributor

weidel-p commented Oct 5, 2016

@heplesser thanks for looking over the code again; it was clearly necessary. I will wait for your SiblingContainer PR to address your comments.

Made more of SiblingContainer interface public, moved one method to h…
…eader as inline, removed unused safe-access method.
@weidel-p

This comment has been minimized.

Show comment
Hide comment
@weidel-p

weidel-p Oct 7, 2016

Contributor

@heplesser thanks again for your last comments, I hope I addressed them all.

Contributor

weidel-p commented Oct 7, 2016

@heplesser thanks again for your last comments, I hope I addressed them all.

@heplesser

Except for one place where I would like an additional comment, all is fine from my side and I approve already now with 👍.

@abigailm Are you satified as well?

static_cast< GenericConnectorModel< ConnectionT >* >( cm[ syn_id ] )
->get_common_properties() );
C_[ i ].send( e, t, ConnectorBase::get_t_lastspike(), cp );
ConnectorBase::send_weight_event( cp, e, t );

This comment has been minimized.

@heplesser

heplesser Oct 13, 2016

Contributor

@weidel-p I think it would be good to add a comment here and in the corresponding locations below that the first call to C_[i].send() updates the event e with information on target, weight, etc. That is not obvious from the code.

@heplesser

heplesser Oct 13, 2016

Contributor

@weidel-p I think it would be good to add a comment here and in the corresponding locations below that the first call to C_[i].send() updates the event e with information on target, weight, etc. That is not obvious from the code.

@jakobj

jakobj approved these changes Oct 17, 2016

Looking good 👍

@heplesser heplesser merged commit 37c8cfc into nest:master Oct 17, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@alberto-antonietti

This comment has been minimized.

Show comment
Hide comment
@alberto-antonietti

alberto-antonietti May 24, 2017

Contributor

Hi @weidel-p, I am currently using the current version of weight_recorder. I saw that you mentioned the possibility of having a down-sampling (e.g. saving weights each second, useful for long simulations). Are you (or someone else) working on this or could I try to add this new functionality to the current version?

Thanks!
Alberto

Contributor

alberto-antonietti commented May 24, 2017

Hi @weidel-p, I am currently using the current version of weight_recorder. I saw that you mentioned the possibility of having a down-sampling (e.g. saving weights each second, useful for long simulations). Are you (or someone else) working on this or could I try to add this new functionality to the current version?

Thanks!
Alberto

@weidel-p weidel-p deleted the weidel-p:feature/weight_recorder branch May 27, 2017

@weidel-p weidel-p restored the weidel-p:feature/weight_recorder branch May 27, 2017

@weidel-p

This comment has been minimized.

Show comment
Hide comment
@weidel-p

weidel-p May 27, 2017

Contributor

Hi @alberto-antonietti, if I remember correctly, we decided to leave the down-sampling mechanism out of this PR as this is a mechanism which could be implemented for many devices.
@heplesser created a special issue for this topic (see #505), but far as I know, no-one works directly on this at the moment. So, feel free to implement this functionality, your contribution would be highly appreciated.

Contributor

weidel-p commented May 27, 2017

Hi @alberto-antonietti, if I remember correctly, we decided to leave the down-sampling mechanism out of this PR as this is a mechanism which could be implemented for many devices.
@heplesser created a special issue for this topic (see #505), but far as I know, no-one works directly on this at the moment. So, feel free to implement this functionality, your contribution would be highly appreciated.

@alberto-antonietti

This comment has been minimized.

Show comment
Hide comment
@alberto-antonietti

alberto-antonietti May 30, 2017

Contributor

Thank you for the reply @weidel-p, since no-one is working on this I can try to implement it for the weights_recorder, having in mind that it should be applied also to other devices as discussed in #505.

Contributor

alberto-antonietti commented May 30, 2017

Thank you for the reply @weidel-p, since no-one is working on this I can try to implement it for the weights_recorder, having in mind that it should be applied also to other devices as discussed in #505.

jakobj pushed a commit to jakobj/nest-simulator that referenced this pull request Mar 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment