Extended comments on and some code improvements for conn builders

Author: heplesser

nestkernel/conn_builder.cpp

@@ -789,13 +789,6 @@ nest::ThirdOutBuilder::ThirdOutBuilder( const NodeCollectionPTR third,
 {
 }
 
-void
-nest::ThirdOutBuilder::connect()
-{
-  assert( false ); // should never be called
-}
-
-
 nest::ThirdBernoulliWithPoolBuilder::ThirdBernoulliWithPoolBuilder( const NodeCollectionPTR third,
   const NodeCollectionPTR targets,
   ThirdInBuilder* third_in,
@@ -855,6 +848,9 @@ nest::ThirdBernoulliWithPoolBuilder::ThirdBernoulliWithPoolBuilder( const NodeCo
 
   if ( not random_pool_ )
   {
+    // Tell every target neuron its position in the target node collection.
+    // This is necessary to assign the right block pool to it.
+    //
     // We cannot do this parallel with targets->local_begin() since we need to
     // count over all elements of the node collection which might be a complex
     // composition of slices with non-trivial mapping between elements and vps.
@@ -879,6 +875,9 @@ nest::ThirdBernoulliWithPoolBuilder::~ThirdBernoulliWithPoolBuilder()
 
     if ( not random_pool_ )
     {
+      // Reset tmp_nc_index in target nodes in case a node has never been a target.
+      // We do not want non-invalid values to persist beyond the lifetime of this builder.
+      //
       // Here we can work in parallel since we just reset to invalid_index
       for ( auto tgt_it = targets_->thread_local_begin(); tgt_it != targets_->end(); ++tgt_it )
       {

nestkernel/conn_builder.h

@@ -65,7 +65,7 @@ class ThirdOutBuilder;
  * the connect interface. Derived classes implement the connect
  * method.
  *
- * @note Naming classes *Builder to avoid name confusion with Connector classes.
+ * @note This class is also the base class for all components of a TripartiteConnBuilder.
  */
 class BipartiteConnBuilder
 {
@@ -76,6 +76,16 @@ class BipartiteConnBuilder
   //! Delete synapses with or without structural plasticity
   virtual void disconnect();
 
+  /**
+   * Create new bipartite builder.
+   *
+   * @param sources Source population to connect from
+   * @param targets Target population to connect to
+   * @param third_out `nullptr` if pure bipartite connection, pointer to \class ThirdOutBuilder object if this builder
+   * creates the primary connection of a tripartite connectivity
+   * @param conn_spec Connection specification (if part of tripartite, spec for the specific part)
+   * @param syn_specs Collection of synapse specifications (usually single element, several for collocated synapses)
+   */
   BipartiteConnBuilder( NodeCollectionPTR sources,
     NodeCollectionPTR targets,
     ThirdOutBuilder* third_out,
@@ -105,8 +115,6 @@ class BipartiteConnBuilder
 
   void set_synaptic_element_names( const std::string& pre_name, const std::string& post_name );
 
-  bool all_parameters_scalar_() const;
-
   /**
    * Updates the number of connected synaptic elements in the target and the source.
    *
@@ -118,12 +126,14 @@ class BipartiteConnBuilder
    */
   bool change_connected_synaptic_elements( size_t snode_id, size_t tnode_id, const size_t tid, int update );
 
+  //! Return true if rule allows creation of symmetric connectivity
   virtual bool
   supports_symmetric() const
   {
     return false;
   }
 
+  //! Return true if rule automatically creates symmetric connectivity
   virtual bool
   is_symmetric() const
   {
@@ -153,6 +163,8 @@ class BipartiteConnBuilder
   //! Implements the actual connection algorithm
   virtual void connect_() = 0;
 
+  bool all_parameters_scalar_() const;
+
   virtual void
   sp_connect_()
   {
@@ -204,17 +216,17 @@ class BipartiteConnBuilder
    */
   bool loop_over_targets_() const;
 
-  NodeCollectionPTR sources_;
-  NodeCollectionPTR targets_;
+  NodeCollectionPTR sources_; //!< Population to connect from
+  NodeCollectionPTR targets_; //!< Population to connect to
 
-  ThirdOutBuilder* third_out_; //!< to be triggered when primary connection is created
+  ThirdOutBuilder* third_out_; //!< To be triggered when primary connection is created
 
   bool allow_autapses_;
   bool allow_multapses_;
   bool make_symmetric_;
   bool creates_symmetric_connections_;
 
-  //! buffer for exceptions raised in threads
+  //! Buffer for exceptions raised in threads
   std::vector< std::shared_ptr< WrappedThreadException > > exceptions_raised_;
 
   // Name of the pre synaptic and postsynaptic elements for this connection builder
@@ -223,12 +235,18 @@ class BipartiteConnBuilder
 
   bool use_structural_plasticity_;
 
-  //! pointers to connection parameters specified as arrays
+  //! Pointers to connection parameters specified as arrays
   std::vector< ConnParameter* > parameters_requiring_skipping_;
 
   std::vector< size_t > synapse_model_id_;
 
-  //! dictionaries to pass to connect function, one per thread for every syn_spec
+  /**
+   * Dictionaries to pass to connect function, one per thread for every syn_spec
+   *
+   * Outer dim: syn_spec, inner dim: thread
+   *
+   * @note Each thread can independently modify its dictionary to pass parameters on
+   */
   std::vector< std::vector< DictionaryDatum > > param_dicts_;
 
 private:
@@ -300,20 +318,47 @@ class BipartiteConnBuilder
  * This builder creates the actual connections from primary sources to third-factor nodes
  * based on the source-third lists generated by the third-out builder.
  *
+ * The `ThirdOutBuilder::third_connect()` method calls `register_connection()`
+ * for each source node to third-factor node connection that needs to be created to store this
+ * information in the `ThirdIn` builder.
+ *
+ * The `connect_()` method of this class needs to be called after all primary connections
+ * and third-factor to target connections have been created. It then exchanges information
+ * about required source-third connections with the other MPI ranks and creates required
+ * connections locally.
+ *
  * The class is final because there is no freedom of choice of connection rule at this stage.
  */
 class ThirdInBuilder final : public BipartiteConnBuilder
 {
 public:
-  ThirdInBuilder( NodeCollectionPTR,
-    NodeCollectionPTR,
-    const DictionaryDatum&, // only for compatibility with BipartiteConnBuilder
-    const std::vector< DictionaryDatum >& );
+  /**
+   * Create ThirdInBuilder
+   *
+   * @param sources Source population of primary connection
+   * @param third Third-factor population
+   * @param third_conn_spec is ignored by this builder but required to make base class happy
+   * @param syn_specs Collection of synapse specification for connection from primary source to third factor
+   *
+   * @todo Once DictionaryDatums are gone, see if we can remove `third_conn_spec` and just pass empty conn spec
+   * container to base-class constructor, since \class ThirdInBuilder has no connection rule properties to set.
+   */
+  ThirdInBuilder( NodeCollectionPTR sources,
+    NodeCollectionPTR third,
+    const DictionaryDatum& third_conn_spec,
+    const std::vector< DictionaryDatum >& syn_specs );
   ~ThirdInBuilder();
 
-  void register_connection( size_t primary_source_id, size_t primary_target_id );
+  /**
+   * Register required source node to third-factor node connection.
+   *
+   * @param primary_source_id GID of source node to connect from
+   * @param third_node_id GID of target node to connect to
+   */
+  void register_connection( size_t primary_source_id, size_t third_node_id );
 
 private:
+  //! Exchange required connection info via MPI and create needed connections locally
   void connect_() override;
 
   /**
@@ -336,15 +381,16 @@ class ThirdInBuilder final : public BipartiteConnBuilder
     {
     }
 
-    size_t source_gid;
-    size_t third_gid;
-    size_t third_rank;
+    size_t source_gid; //!< GID of source node to connect from
+    size_t third_gid;  //!< GID of third-factor node to connect to
+    size_t third_rank; //!< Rank of third-factor node (stored locally as it is needed multiple times)
   };
 
-  //! source-thirdparty GID pairs to be communicated; one per thread
+  //! Container for register source-third connections, one per thread via pointer for collision free operation in
+  //! thread-local storage
   std::vector< BlockVector< SourceThirdInfo_ >* > source_third_gids_;
 
-  //! number of source-third pairs to send. Outer dimension writing thread, inner dimension rank to send to
+  //! Number of source-third pairs to send. Outer dimension is writing thread, inner dimension MPI rank to send to
   std::vector< std::vector< size_t >* > source_third_counts_;
 };
 
@@ -361,14 +407,33 @@ class ThirdInBuilder final : public BipartiteConnBuilder
 class ThirdOutBuilder : public BipartiteConnBuilder
 {
 public:
-  ThirdOutBuilder( NodeCollectionPTR,
-    NodeCollectionPTR,
-    ThirdInBuilder*,
-    const DictionaryDatum&, // only for compatibility with BipartiteConnBuilder
-    const std::vector< DictionaryDatum >& );
+  /**
+   * Create ThirdOutBuilder.
+   *
+   * @param third Third-factor population
+   * @param targets Target population of primary connection
+   * @param third_in ThirdInBuilder which will create source-third connections later
+   * @param third_conn_spec Specification for third-factor connectivity
+   * @param syn_specs Collection of synapse specifications for third-target connections
+   */
+  ThirdOutBuilder( const NodeCollectionPTR third,
+    const NodeCollectionPTR targets,
+    ThirdInBuilder* third_in,
+    const DictionaryDatum& third_conn_spec,
+    const std::vector< DictionaryDatum >& syn_specs );
 
-  void connect() override;
+  void
+  connect() override final
+  {
+    assert( false );
+  } //!< only call third_connect() on ThirdOutBuilder
 
+  /**
+   * Create third-factor connection for given primary connection.
+   *
+   * @param source_gid GID of source of primary connection
+   * @param target Target node of primary connection
+   */
   virtual void third_connect( size_t source_gid, Node& target ) = 0;
 
 protected:
@@ -392,7 +457,7 @@ class ConnBuilder
    * @param sources Source population for primary connection
    * @param targets Target population for primary connection
    * @param conn_spec Connection specification dictionary for tripartite bernoulli rule
-   * @param syn_spec Dictionary of synapse specification
+   * @param syn_spec Collection of dictionaries with synapse specifications
    */
   ConnBuilder( const std::string& primary_rule,
     NodeCollectionPTR sources,
@@ -410,7 +475,8 @@ class ConnBuilder
    * @param third Third-party population
    * @param conn_spec Connection specification dictionary for tripartite bernoulli rule
    * @param syn_specs Dictionary of synapse specifications for the three connections that may be created. Allowed keys
-   * are `"primary"`, `"third_in"`, `"third_out"`
+   * are `"primary"`, `"third_in"`, `"third_out"`, and for each of these the value must be a collection of dictionaries
+   * with synapse specifications as for bipartite connectivity.
    */
   ConnBuilder( const std::string& primary_rule,
     const std::string& third_rule,
@@ -430,20 +496,23 @@ class ConnBuilder
   void disconnect();
 
 private:
-  // order of declarations based on dependencies
+  // Order of declarations based on dependencies, do not change.
   ThirdInBuilder* third_in_builder_;
   ThirdOutBuilder* third_out_builder_;
   BipartiteConnBuilder* primary_builder_;
 };
 
-
+/**
+ * Build third-factor connectivity based on Bernoulli trials, selecting third factor nodes from a fixed pool per target
+ * node.
+ */
 class ThirdBernoulliWithPoolBuilder : public ThirdOutBuilder
 {
 public:
   ThirdBernoulliWithPoolBuilder( NodeCollectionPTR,
     NodeCollectionPTR,
-    ThirdInBuilder* third_in,
-    const DictionaryDatum&, // only for compatibility with BCB
+    ThirdInBuilder*,
+    const DictionaryDatum&,
     const std::vector< DictionaryDatum >& );
   ~ThirdBernoulliWithPoolBuilder();
 
@@ -454,16 +523,39 @@ class ThirdBernoulliWithPoolBuilder : public ThirdOutBuilder
   connect_() override
   {
     assert( false );
-  }
+  } //!< only call third_connect()
+
+  /**
+   * For block pool, return index of first pool element for given target node.
+   *
+   * @param targe_index
+   */
   size_t get_first_pool_index_( const size_t target_index ) const;
 
-  double p_;
-  bool random_pool_;
-  size_t pool_size_;
-  size_t targets_per_third_;
+  double p_;                 //!< probability of creating a third-factor connection
+  bool random_pool_;         //!< random or block pool?
+  size_t pool_size_;         //!< number of nodes per pool
+  size_t targets_per_third_; //!< number of target nodes per third-factor node
 
+  /**
+   * Type for single pool of third-factor nodes
+   *
+   * @todo Could probably be BlockVector, but currently some problem with back_inserter when sampling pool.
+   */
   typedef std::vector< NodeIDTriple > PoolType_;
+
+  //! Type mapping target GID to pool for this target
   typedef std::map< size_t, PoolType_ > TgtPoolMap_;
+
+  /**
+   * Thread-specific pools of third-factor nodes.
+   *
+   * Each thread maintains a map from target node IDs to the third-factor node pool for that target node.
+   * Since each target lives on exactly one thread, there will be no overlap. For each node, the pool is
+   * created when a third-factor connection needs to be made to that node for the first time.
+   * The pools are deleted when the ConnBuilder is destroyed at the end of the connect call.
+   * We store a pointer instead of the map itself to ensure that the map is in thread-local memory.
+   */
   std::vector< TgtPoolMap_* > pools_; // outer: threads
 };
 

nestkernel/node.h

@@ -954,14 +954,16 @@ class Node
   DeprecationWarning deprecation_warning;
 
   /**
-   * This is only to be used to get the index in the NC to the ThirdOutBuilder
+   * Set index in node collection; required by ThirdOutBuilder.
    */
   void set_tmp_nc_index( size_t index );
 
   /**
-   * Return index in NC and invalidate entry to avoid multiple reads. Only to be called by ThirdOutBuilder
+   * Return and invalidate index in node collection; required by ThirdOutBuilder.
+   *
+   * @note Not const since it invalidates index in node object.
    */
-  size_t get_tmp_nc_index() const;
+  size_t get_tmp_nc_index();
 
 
 private:
@@ -1048,6 +1050,8 @@ class Node
    * @note This is only here so that the primary connection builder can inform the ThirdOutBuilder
    * about the index of the target neuron in the targets node collection. This is required for block-based
    * builders.
+   *
+   * @note Set by set_tmp_nc_index() and invalidated by get_tmp_nc_index().
    */
   size_t tmp_nc_index_;
 };
@@ -1196,11 +1200,14 @@ Node::set_tmp_nc_index( size_t index )
 }
 
 inline size_t
-Node::get_tmp_nc_index() const
+Node::get_tmp_nc_index()
 {
   assert( tmp_nc_index_ != invalid_index );
 
-  return tmp_nc_index_;
+  const auto index = tmp_nc_index_;
+  tmp_nc_index_ = invalid_index;
+
+  return index;
 }