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

Numerical imprecision stdp #924

Merged
merged 6 commits into from Apr 11, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 3 additions & 4 deletions models/stdp_connection.h
Expand Up @@ -250,10 +250,9 @@ STDPConnection< targetidentifierT >::send( Event& e,
{
minus_dt = t_lastspike - ( start->t_ + dendritic_delay );
++start;
if ( minus_dt == 0 )
{
continue;
}
// get_history() should make sure that
// start->t_ > t_lastspike - dendritic_delay, i.e. minus_dt < 0
assert( minus_dt < -1.0 * kernel().connection_manager.get_stdp_eps() );
weight_ = facilitate_( weight_, Kplus_ * std::exp( minus_dt / tau_plus_ ) );
}

Expand Down
7 changes: 3 additions & 4 deletions models/stdp_connection_hom.h
Expand Up @@ -302,10 +302,9 @@ STDPConnectionHom< targetidentifierT >::send( Event& e,
{
minus_dt = t_lastspike - ( start->t_ + dendritic_delay );
++start;
if ( minus_dt == 0 )
{
continue;
}
// get_history() should make sure that
// start->t_ > t_lastspike - dendritic_delay, i.e. minus_dt < 0
assert( minus_dt < -1.0 * kernel().connection_manager.get_stdp_eps() );
weight_ =
facilitate_( weight_, Kplus_ * std::exp( minus_dt / cp.tau_plus_ ), cp );
}
Expand Down
11 changes: 7 additions & 4 deletions models/stdp_dopa_connection.h
Expand Up @@ -452,7 +452,8 @@ STDPDopaConnection< targetidentifierT >::process_dopa_spikes_(
// process dopa spikes in (t0, t1]
// propagate weight from t0 to t1
if ( ( dopa_spikes.size() > dopa_spikes_idx_ + 1 )
&& ( dopa_spikes[ dopa_spikes_idx_ + 1 ].spike_time_ <= t1 ) )
&& ( t1 - dopa_spikes[ dopa_spikes_idx_ + 1 ].spike_time_ > -1.0
* kernel().connection_manager.get_stdp_eps() ) )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you try to get the -1.0 on the same line as kernel()....? I think that would be good for legibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am afraid the coding style guidelines do not permit this.

{
// there is at least 1 dopa spike in (t0, t1]
// propagate weight up to first dopa spike and update dopamine trace
Expand All @@ -468,7 +469,8 @@ STDPDopaConnection< targetidentifierT >::process_dopa_spikes_(
// process remaining dopa spikes in (t0, t1]
double cd;
while ( ( dopa_spikes.size() > dopa_spikes_idx_ + 1 )
&& ( dopa_spikes[ dopa_spikes_idx_ + 1 ].spike_time_ <= t1 ) )
&& ( t1 - dopa_spikes[ dopa_spikes_idx_ + 1 ].spike_time_ > -1.0
* kernel().connection_manager.get_stdp_eps() ) )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you try to get the -1.0 on the same line as kernel()....? I think that would be good for legibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, not possible due to coding style guidelines.

{
// propagate weight up to next dopa spike and update dopamine trace
// weight and dopamine trace n are at time of last dopa spike td but
Expand Down Expand Up @@ -565,8 +567,9 @@ STDPDopaConnection< targetidentifierT >::send( Event& e,
process_dopa_spikes_( dopa_spikes, t0, start->t_ + dendritic_delay, cp );
t0 = start->t_ + dendritic_delay;
minus_dt = t_last_update_ - t0;
if ( start->t_ < t_spike ) // only depression if pre- and postsyn. spike
// occur at the same time
// facilitate only in case of post- after presyn. spike
// skip facilitation if pre- and postsyn. spike occur at the same time
if ( t_spike - start->t_ > kernel().connection_manager.get_stdp_eps() )
{
facilitate_( Kplus_ * std::exp( minus_dt / cp.tau_plus_ ), cp );
}
Expand Down
7 changes: 3 additions & 4 deletions models/stdp_pl_connection_hom.h
Expand Up @@ -257,10 +257,9 @@ STDPPLConnectionHom< targetidentifierT >::send( Event& e,
{
minus_dt = t_lastspike - ( start->t_ + dendritic_delay );
start++;
if ( minus_dt == 0 )
{
continue;
}
// get_history() should make sure that
// start->t_ > t_lastspike - dendritic_delay, i.e. minus_dt < 0
assert( minus_dt < -1.0 * kernel().connection_manager.get_stdp_eps() );
weight_ = facilitate_(
weight_, Kplus_ * std::exp( minus_dt * cp.tau_plus_inv_ ), cp );
}
Expand Down
8 changes: 3 additions & 5 deletions models/stdp_triplet_connection.h
Expand Up @@ -259,11 +259,9 @@ STDPTripletConnection< targetidentifierT >::send( Event& e,
// Pfister et al, 2006
double ky = start->triplet_Kminus_ - 1.0;
++start;
if ( minus_dt == 0 )
{
continue;
}

// get_history() should make sure that
// start->t_ > t_lastspike - dendritic_delay, i.e. minus_dt < 0
assert( minus_dt < -1.0 * kernel().connection_manager.get_stdp_eps() );
weight_ =
facilitate_( weight_, Kplus_ * std::exp( minus_dt / tau_plus_ ), ky );
}
Expand Down
7 changes: 3 additions & 4 deletions models/vogels_sprekeler_connection.h
Expand Up @@ -215,10 +215,9 @@ VogelsSprekelerConnection< targetidentifierT >::send( Event& e,
{
minus_dt = t_lastspike - ( start->t_ + dendritic_delay );
++start;
if ( minus_dt == 0 )
{
continue;
}
// get_history() should make sure that
// start->t_ > t_lastspike - dendritic_delay, i.e. minus_dt < 0
assert( minus_dt < -1.0 * kernel().connection_manager.get_stdp_eps() );
weight_ = facilitate_( weight_, Kplus_ * std::exp( minus_dt / tau_ ) );
}

Expand Down
19 changes: 14 additions & 5 deletions nestkernel/archiving_node.cpp
Expand Up @@ -29,6 +29,9 @@

#include "archiving_node.h"

// Includes from nestkernel:
#include "kernel_manager.h"

// Includes from sli:
#include "dictutils.h"

Expand Down Expand Up @@ -81,7 +84,9 @@ Archiving_Node::register_stdp_connection( double t_first_read )
// For details see bug #218. MH 08-04-22

for ( std::deque< histentry >::iterator runner = history_.begin();
runner != history_.end() && runner->t_ <= t_first_read;
runner != history_.end()
&& ( t_first_read - runner->t_ > -1.0
* kernel().connection_manager.get_stdp_eps() );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you try to get the -1.0 on the same line as kernel()....? I think that would be good for legibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, not possible due to coding style guidelines.

++runner )
{
( runner->access_counter_ )++;
Expand All @@ -100,7 +105,7 @@ nest::Archiving_Node::get_K_value( double t )
int i = history_.size() - 1;
while ( i >= 0 )
{
if ( t > history_[ i ].t_ )
if ( t - history_[ i ].t_ > kernel().connection_manager.get_stdp_eps() )
{
return ( history_[ i ].Kminus_
* std::exp( ( history_[ i ].t_ - t ) * tau_minus_inv_ ) );
Expand All @@ -126,7 +131,7 @@ nest::Archiving_Node::get_K_values( double t,
int i = history_.size() - 1;
while ( i >= 0 )
{
if ( t > history_[ i ].t_ )
if ( t - history_[ i ].t_ > kernel().connection_manager.get_stdp_eps() )
{
triplet_K_value = ( history_[ i ].triplet_Kminus_
* std::exp( ( history_[ i ].t_ - t ) * tau_minus_triplet_inv_ ) );
Expand Down Expand Up @@ -159,12 +164,16 @@ nest::Archiving_Node::get_history( double t1,
else
{
std::deque< histentry >::iterator runner = history_.begin();
while ( ( runner != history_.end() ) && ( runner->t_ <= t1 ) )
while ( ( runner != history_.end() )
&& ( t1 - runner->t_ > -1.0
* kernel().connection_manager.get_stdp_eps() ) )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you try to get the -1.0 on the same line as kernel()....? I think that would be good for legibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, not possible due to coding style guidelines.

{
++runner;
}
*start = runner;
while ( ( runner != history_.end() ) && ( runner->t_ <= t2 ) )
while ( ( runner != history_.end() )
&& ( t2 - runner->t_ > -1.0
* kernel().connection_manager.get_stdp_eps() ) )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you try to get the -1.0 on the same line as kernel()....? I think that would be good for legibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, not possible due to coding style guidelines.

{
( runner->access_counter_ )++;
++runner;
Expand Down
31 changes: 31 additions & 0 deletions nestkernel/connection_manager.cpp
Expand Up @@ -28,6 +28,8 @@
// C++ includes:
#include <cassert>
#include <cmath>
#include <iomanip>
#include <limits>
#include <set>
#include <vector>

Expand Down Expand Up @@ -77,6 +79,7 @@ nest::ConnectionManager::ConnectionManager()
, initial_connector_capacity_( CONFIG_CONNECTOR_CUTOFF )
, large_connector_limit_( CONFIG_CONNECTOR_CUTOFF * 2 )
, large_connector_growth_factor_( 1.5 )
, stdp_eps_( 1.0e-6 )
{
}

Expand Down Expand Up @@ -1450,3 +1453,31 @@ nest::ConnectionManager::get_targets( const std::vector< index >& sources,
}
}
}

void
nest::ConnectionManager::set_stdp_eps( const double stdp_eps )
{
if ( not( stdp_eps < Time::get_resolution().get_ms() ) )
{
throw KernelException(
"The epsilon used for spike-time comparison in STDP must be less "
"than the simulation resolution." );
}
else if ( stdp_eps < 0 )
{
throw KernelException(
"The epsilon used for spike-time comparison in STDP must not be "
"negative." );
}
else
{
stdp_eps_ = stdp_eps;

std::ostringstream os;
os << "Epsilon for spike-time comparison in STDP was set to "
<< std::setprecision( std::numeric_limits< long double >::digits10 )
<< stdp_eps_ << ".";

LOG( M_INFO, "ConnectionManager::set_stdp_eps", os.str() );
}
}
14 changes: 14 additions & 0 deletions nestkernel/connection_manager.h
Expand Up @@ -313,6 +313,10 @@ class ConnectionManager : public ManagerInterface
*/
double get_large_connector_growth_factor() const;

double get_stdp_eps() const;

void set_stdp_eps( const double stdp_eps );

private:
/**
* Update delay extrema to current values.
Expand Down Expand Up @@ -419,6 +423,10 @@ class ConnectionManager : public ManagerInterface

//! Capacity growth factor to use beyond the limit
double large_connector_growth_factor_;

//! Maximum distance between (double) spike times in STDP that is
//! still considered 0. See issue #894
double stdp_eps_;
};

inline DictionaryDatum&
Expand Down Expand Up @@ -457,6 +465,12 @@ ConnectionManager::get_large_connector_growth_factor() const
return large_connector_growth_factor_;
}

inline double
ConnectionManager::get_stdp_eps() const
{
return stdp_eps_;
}

} // namespace nest

#endif /* CONNECTION_MANAGER_H */
23 changes: 23 additions & 0 deletions nestkernel/nestmodule.cpp
Expand Up @@ -1699,6 +1699,26 @@ NestModule::DisableStructuralPlasticity_Function::execute(
i->EStack.pop();
}

/**
* Set epsilon that is used for comparing spike times in STDP.
* Spike times in STDP synapses are currently represented as double
* values. The epsilon defines the maximum distance between spike
* times that is still considered 0.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a Note: See issue ...?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, good point. Done.

*
* Note: See issue #894
*/
void
NestModule::SetStdpEps_dFunction::execute( SLIInterpreter* i ) const
{
i->assert_stack_load( 1 );
const double stdp_eps = getValue< double >( i->OStack.top() );

kernel().connection_manager.set_stdp_eps( stdp_eps );

i->OStack.pop();
i->EStack.pop();
}

void
NestModule::init( SLIInterpreter* i )
{
Expand Down Expand Up @@ -1794,6 +1814,9 @@ NestModule::init( SLIInterpreter* i )
"GetStructuralPlasticityStatus", &getstructuralplasticitystatus_function );
i->createcommand( "Disconnect", &disconnect_i_i_lfunction );
i->createcommand( "Disconnect_g_g_D_D", &disconnect_g_g_D_Dfunction );

i->createcommand( "SetStdpEps", &setstdpeps_dfunction );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have a GetStdpEps function, too? Or maybe "hide" this in the kernel status dict after all?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't think that we want a GetStdpEps. As this is a temporary fix, I would not like to make the variable visible in the kernel dict. I have added an info message to SetStdpEps though, which states the new value of stdp_eps and I have added two checks.


// Add connection rules
kernel().connection_manager.register_conn_builder< OneToOneBuilder >(
"one_to_one" );
Expand Down
6 changes: 6 additions & 0 deletions nestkernel/nestmodule.h
Expand Up @@ -441,6 +441,12 @@ class NestModule : public SLIModule
void execute( SLIInterpreter* ) const;
} disablestructuralplasticity_function;

class SetStdpEps_dFunction : public SLIFunction
{
public:
void execute( SLIInterpreter* ) const;
} setstdpeps_dfunction;

//@}
};

Expand Down