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.

Sign up for GitHub

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

Jump to bottom

Implemented requires_symmetric flag for synapse models #524

Merged
merged 17 commits into from
Nov 23, 2016
Merged

Implemented requires_symmetric flag for synapse models #524

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
6cd11eb
implemented warning when symmetric flag is not used for two-way conne…
janhahne Oct 18, 2016
0d8f108
found solution for all-to-all connections
janhahne Oct 19, 2016
d523e0d
formatting
janhahne Oct 19, 2016
14de8fa
added required_symmetric to GetStatus
janhahne Oct 20, 2016
cfe0f3e
formatting
janhahne Oct 20, 2016
1f7a3de
removed unused needs_prelim_update
janhahne Oct 31, 2016
d93ab76
added is_scalar function to ConnParameter
janhahne Nov 2, 2016
047c6f8
fixed bug and completed solution
janhahne Nov 2, 2016
7a777ea
added tests for require_symmetric to symmetric_connections unittest
janhahne Nov 2, 2016
71e0755
merged with master
janhahne Nov 2, 2016
ce72b4b
resolved conflict with master
janhahne Nov 2, 2016
bd646a6
merged with master
janhahne Nov 23, 2016
20c53c9
worked on documentation
janhahne Nov 23, 2016
7216fec
renamed symmetric flag to make_symmetric
janhahne Nov 23, 2016
4a3f41d
formatting
janhahne Nov 23, 2016
897e6d1
renamed symmetric flag to make_symmetric
janhahne Nov 23, 2016
111dd31
formatting
janhahne Nov 23, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion models/modelsmodule.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,7 @@ ModelsModule::init( SLIInterpreter* )
kernel()
.model_manager
.register_secondary_connection_model< GapJunction< TargetIdentifierPtrRport > >(
"gap_junction", false );
"gap_junction", /*has_delay=*/false, /*requires_symmetric=*/true );


/* BeginDocumentation
Expand Down
31 changes: 31 additions & 0 deletions nestkernel/conn_builder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,9 +382,19 @@ nest::ConnBuilder::change_connected_synaptic_elements( index sgid,
void
nest::ConnBuilder::connect()
{
if ( kernel().model_manager.connector_requires_symmetric( synapse_model_ )
&& not symmetric_ && not is_symmetric() )
Copy link
Contributor

Choose a reason for hiding this comment

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

@janhahne It seems to me we get a bit of an inflation here: symmetric_, is_symmetric() and supports_symmetric(). Could we at least integrate symmetric_ and is_symmetric()?

Copy link
Contributor Author

@janhahne janhahne Oct 26, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@heplesser I agree that this is a bit unfortunate, but all three of them serve a purpose:

  • symmetric_ is the property of the created connection (is symmetric set to true or false?)
  • supports_symmetric determines if a specific ConnBuilder supports the creation of symmetric connection. This is only implemented for the OnetoOne builder.
  • is_symmetric is needed to allow all-to-all connections without setting symmetric to true. Otherwise the creation of all-to-all connection would not be possible at all for gap-junctions (all-to-all does not support symmetric=true)

Unless we want to prohibit any kind of all-to-all connections for gap-junctions I do not see any way of reducing the number of symmetric parameters. Or is there any solution that I am not thinking of?

Copy link
Contributor

Choose a reason for hiding this comment

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

@janhahne I wonder if we should take a step back and think this through once more. All-to-all is inherently symmetric, provided source and target populations are identical and all synapses are created with the same parameters. Guaranteeing symmetric connections when randomized parameters are used would be very difficult, and for explicit parameter arrays it would require very cumbersome checks for matrix symmetry.

So I would think the best approach might be to prohibit both array and random parameters if the synapse model is derived from gap_junction, and to also require that source and target populations are identical. Then, you get symmetry for free. @jougs Is it possible to compare two GIDCollections for equality?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@heplesser Good points! I think now we found all conditions that need to be checked in order to ensure that an all-to-all connection is symmetric. I would also vote for a solution where we prohibit both array and random parameters, rather than allowing those in general and check them for symmetry case by case.

So I would imagine our solution like this: Change the is_symmetric() function of the all-to-all ConnBuilder to

bool
is_symmetric() const
{
  if( sources != targets )
    return false;

  if( parameters are arrays or randomized )
    return false;

  return true;
}

and adjust the BadProperty-Message to "This synapse model requires symmetric (or suitable uniform all-to-all) connections"

The check in is_symmetric() will only be performed, if the first two conditions of the if-statement are met, ergo only if a synapse model requires symmetric connections and the symmetric flag is set to false

Would you agree on this general plan? In this case we would need to figure out how to implement the two if statements. @jougs could help with this first one, any idea how to check for the second one most efficiently?

Copy link
Contributor

Choose a reason for hiding this comment

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

@janhahne @jougs This looks good, although I think that we could simplify the method to

inline bool
is_symmetric() const
{
  return sources == targets && all_parameters_scalar();
}

The other method would be something like

bool
ConnBuilder::all_parameters_scalar_() const
{
  bool all_scalar = weight_->is_scalar() && delay_->is_scalar();
  for ( ConnParameterMap::const_iterator it = synapse_params_.begin() ; ... ; ++it )
  {
     all_scalar = all_scalar && it->second->is_scalar();
  }
  return all_scalar;
}

We then need in ConnParameter and extra method is_scalar() that returns true only for Scalar{Double,Integer}Parameter. We cannot use existing methods since is_array() returns false for random numbers and number_of_values() returns 0 for scalar and random.

Copy link
Contributor

Choose a reason for hiding this comment

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

I find this still confusing, and I fear anyone reading this in five years time will not feel enlightened. How about renaming symmetric_ to make_symmetric_ and turning the order of checks a little:

if ( kernel().model_manager.connector_requires_symmetric( synapse_model_ )
     and not ( is_symmetric() or make_symmetric_ ) )

That seems more readable to me.

{
throw BadProperty(
"This synapse model requires symmetric (or suitable uniform "
"all-to-all) connections" );
Copy link
Contributor

Choose a reason for hiding this comment

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

"suitable" is unclear. How about "Connections with synapse model {0} can only be created as one-to-one connections with "make_symmetric" set to true or as all-to-all connections with equal source and target populations and default or scalar parameters."?

}

if ( symmetric_ && not supports_symmetric() )
{
throw NotImplemented(
"This connection rule does not support symmetric connections." );
}

if ( pre_synaptic_element_name != "" && post_synaptic_element_name != "" )
{
Expand Down Expand Up @@ -580,6 +590,27 @@ nest::ConnBuilder::set_post_synaptic_element_name( std::string name )
post_synaptic_element_name = name;
}

bool
nest::ConnBuilder::all_parameters_scalar_() const
{
bool all_scalar = true;
if ( weight_ )
{
all_scalar = all_scalar && weight_->is_scalar();
}
if ( delay_ )
{
all_scalar = all_scalar && delay_->is_scalar();
}
for ( ConnParameterMap::const_iterator it = synapse_params_.begin();
it != synapse_params_.end();
++it )
{
all_scalar = all_scalar && it->second->is_scalar();
}
return all_scalar;
}

void
nest::OneToOneBuilder::connect_()
{
Expand Down
14 changes: 14 additions & 0 deletions nestkernel/conn_builder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,8 @@ class ConnBuilder
void set_pre_synaptic_element_name( std::string name );
void set_post_synaptic_element_name( std::string name );

bool all_parameters_scalar_() const;

int change_connected_synaptic_elements( index, index, const int, int );

virtual bool
Expand All @@ -110,6 +112,12 @@ class ConnBuilder
return false;
}

virtual bool
is_symmetric() const
{
return false;
}

protected:
//! Implements the actual connection algorithm
virtual void connect_() = 0;
Expand Down Expand Up @@ -240,6 +248,12 @@ class AllToAllBuilder : public ConnBuilder
{
}

bool
is_symmetric() const
{
return *sources_ == *targets_ && all_parameters_scalar_();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is not always true. If all-to-all has a randomized synapse parameter, it is no longer automatically symmetric. We will still have a backward connection for each forward connection, but their properties will differ.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@heplesser Good catch, I did not think of that. Is there any easy way to determine if all-to-all uses randomized synapse parameters? I am thinking of a solution where this functions first checks if there are any randomized parameters involved (if so throws an BadProperty) and (otherwise) then returns true.

Copy link
Contributor

Choose a reason for hiding this comment

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

See above, array parameters are also problematic because costly to check.


protected:
void connect_();
void sp_connect_();
Expand Down
19 changes: 19 additions & 0 deletions nestkernel/conn_parameter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,12 @@ class ConnParameter
}
virtual bool is_array() const = 0;

virtual bool
is_scalar() const
{
return false;
}

virtual void
reset() const
{
Expand Down Expand Up @@ -148,6 +154,13 @@ class ScalarDoubleParameter : public ConnParameter
{
}

bool
is_scalar() const
{
return true;
}


private:
double value_;
};
Expand Down Expand Up @@ -188,6 +201,12 @@ class ScalarIntegerParameter : public ConnParameter
{
}

bool
is_scalar() const
{
return true;
}

private:
long value_;
};
Expand Down
5 changes: 4 additions & 1 deletion nestkernel/connector_model.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,13 @@ namespace nest

ConnectorModel::ConnectorModel( const std::string name,
bool is_primary,
bool has_delay )
bool has_delay,
bool requires_symmetric )
: name_( name )
, default_delay_needs_check_( true )
, is_primary_( is_primary )
, has_delay_( has_delay )
, requires_symmetric_( requires_symmetric )
{
}

Expand All @@ -41,6 +43,7 @@ ConnectorModel::ConnectorModel( const ConnectorModel& cm,
, default_delay_needs_check_( true )
, is_primary_( cm.is_primary_ )
, has_delay_( cm.has_delay_ )
, requires_symmetric_( cm.requires_symmetric_ )
{
}

Expand Down
25 changes: 20 additions & 5 deletions nestkernel/connector_model.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,10 @@ class ConnectorModel
{

public:
ConnectorModel( const std::string, bool is_primary, bool has_delay );
ConnectorModel( const std::string,
bool is_primary,
bool has_delay,
bool requires_symmetric );
ConnectorModel( const ConnectorModel&, const std::string );
virtual ~ConnectorModel()
{
Expand Down Expand Up @@ -171,13 +174,21 @@ class ConnectorModel
return has_delay_;
}

bool
requires_symmetric() const
{
return requires_symmetric_;
}

protected:
std::string name_;
//! Flag indicating, that the default delay must be checked
bool default_delay_needs_check_;
//! indicates, whether this ConnectorModel belongs to a primary connection
bool is_primary_;
bool has_delay_; //!< indicates, that ConnectorModel has a delay
bool requires_symmetric_;
//!< indicates, that ConnectorModel requires symmetric connections

}; // ConnectorModel

Expand All @@ -196,8 +207,9 @@ class GenericConnectorModel : public ConnectorModel
public:
GenericConnectorModel( const std::string name,
bool is_primary,
bool has_delay )
: ConnectorModel( name, is_primary, has_delay )
bool has_delay,
bool requires_symmetric )
: ConnectorModel( name, is_primary, has_delay, requires_symmetric )
, receptor_type_( 0 )
{
}
Expand Down Expand Up @@ -289,10 +301,13 @@ class GenericSecondaryConnectorModel
typename ConnectionT::EventType* pev_;

public:
GenericSecondaryConnectorModel( const std::string name, bool has_delay )
GenericSecondaryConnectorModel( const std::string name,
bool has_delay,
bool requires_symmetric )
: GenericConnectorModel< ConnectionT >( name,
/*is _primary=*/false,
has_delay )
has_delay,
requires_symmetric )
, pev_( 0 )
{
pev_ = new typename ConnectionT::EventType();
Expand Down
2 changes: 2 additions & 0 deletions nestkernel/connector_model_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,8 @@ GenericConnectorModel< ConnectionT >::get_status( DictionaryDatum& d ) const

( *d )[ names::receptor_type ] = receptor_type_;
( *d )[ "synapsemodel" ] = LiteralDatum( name_ );
( *d )[ "requires_symmetric" ] = requires_symmetric_;
( *d )[ "has_delay" ] = has_delay_;
}

template < typename ConnectionT >
Expand Down
8 changes: 8 additions & 0 deletions nestkernel/model_manager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,14 @@ ModelManager::get_connector_defaults( synindex syn_id ) const
return dict;
}

bool
ModelManager::connector_requires_symmetric( synindex syn_id ) const
{
assert_valid_syn_id( syn_id );

return prototypes_[ 0 ][ syn_id ]->requires_symmetric();
}

void
ModelManager::clear_models_( bool called_from_destructor )
{
Expand Down
12 changes: 10 additions & 2 deletions nestkernel/model_manager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,11 +180,13 @@ class ModelManager : public ManagerInterface
* @return an ID for the synapse prototype.
*/
template < class ConnectionT >
void register_connection_model( const std::string& name );
void register_connection_model( const std::string& name,
bool requires_symmetric = false );

template < class ConnectionT >
void register_secondary_connection_model( const std::string& name,
bool has_delay = true );
bool has_delay = true,
bool requires_symmetric = false );

/**
* @return The model id of a given model name
Expand All @@ -197,6 +199,12 @@ class ModelManager : public ManagerInterface
Model* get_model( index ) const;

DictionaryDatum get_connector_defaults( synindex syn_id ) const;

/**
* Checks, whether synapse type requires symmetric connections
*/
bool connector_requires_symmetric( synindex syn_id ) const;

void set_connector_defaults( synindex syn_id, const DictionaryDatum& d );

/**
Expand Down
19 changes: 12 additions & 7 deletions nestkernel/model_manager_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,16 +80,20 @@ ModelManager::register_preconf_node_model( const Name& name,

template < class ConnectionT >
void
ModelManager::register_connection_model( const std::string& name )
ModelManager::register_connection_model( const std::string& name,
bool requires_symmetric )
{
ConnectorModel* cf = new GenericConnectorModel< ConnectionT >(
name, /*is_primary=*/true, /*has_delay=*/true );
name, /*is_primary=*/true, /*has_delay=*/true, requires_symmetric );
register_connection_model_( cf );

if ( not ends_with( name, "_hpc" ) )
{
cf = new GenericConnectorModel< ConnectionLabel< ConnectionT > >(
name + "_lbl", /*is_primary=*/true, /*has_delay=*/true );
name + "_lbl",
/*is_primary=*/true,
/*has_delay=*/true,
requires_symmetric );
register_connection_model_( cf );
}
}
Expand All @@ -100,10 +104,11 @@ ModelManager::register_connection_model( const std::string& name )
template < class ConnectionT >
void
ModelManager::register_secondary_connection_model( const std::string& name,
bool has_delay )
bool has_delay,
bool requires_symmetric )
{
ConnectorModel* cm =
new GenericSecondaryConnectorModel< ConnectionT >( name, has_delay );
ConnectorModel* cm = new GenericSecondaryConnectorModel< ConnectionT >(
name, has_delay, requires_symmetric );

synindex synid = register_connection_model_( cm );

Expand All @@ -119,7 +124,7 @@ ModelManager::register_secondary_connection_model( const std::string& name,

// create labeled secondary event connection model
cm = new GenericSecondaryConnectorModel< ConnectionLabel< ConnectionT > >(
name + "_lbl", has_delay );
name + "_lbl", has_delay, requires_symmetric );

synid = register_connection_model_( cm );

Expand Down
1 change: 0 additions & 1 deletion nestkernel/nest_names.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,6 @@ const Name N_channels( "N_channels" );
const Name n_events( "n_events" );
const Name n_proc( "n_proc" );
const Name n_receptors( "n_receptors" );
const Name needs_prelim_update( "needs_prelim_update" );
const Name neuron( "neuron" );
const Name node_uses_wfr( "node_uses_wfr" );
const Name noise( "noise" );
Expand Down
11 changes: 5 additions & 6 deletions nestkernel/nest_names.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,12 +266,11 @@ extern const Name N_channels; //!< Specific to correlomatrix_detector
extern const Name n_events; //!< Recorder parameter
extern const Name
n_proc; //!< Number of component processes of ppd_sup_/gamma_sup_generator
extern const Name n_receptors; //!< number of receptor ports
extern const Name needs_prelim_update; //!< Node parameter
extern const Name neuron; //!< Node type
extern const Name node_uses_wfr; //!< Node parameter
extern const Name noise; //!< Specific to iaf_chs_2008 neuron
extern const Name ns; //!< Number of release sites (property arrays)
extern const Name n_receptors; //!< number of receptor ports
extern const Name neuron; //!< Node type
extern const Name node_uses_wfr; //!< Node parameter
extern const Name noise; //!< Specific to iaf_chs_2008 neuron
extern const Name ns; //!< Number of release sites (property arrays)

extern const Name offset; //!< Miscellaneous parameters
extern const Name offsets; //!< Recorder parameter
Expand Down
Loading