Merge pull request #3536 from med-ayssar/3532

Fix issue: #3532: Disconnect issue with CompressedSpikes:ON/Off

Author: heplesser

libnestutil/nest_types.h

@@ -92,7 +92,7 @@ constexpr uint8_t NUM_BITS_PROCESSED_FLAG = 1U;
 constexpr uint8_t NUM_BITS_MARKER_SPIKE_DATA = 2U;
 constexpr uint8_t NUM_BITS_LAG = 14U;
 constexpr uint8_t NUM_BITS_DELAY = 21U;
-constexpr uint8_t NUM_BITS_NODE_ID = 62U;
+constexpr uint8_t NUM_BITS_NODE_ID = 61U;
 
 // Maximally allowed values for bitfields
 

models/eprop_synapse.cpp

@@ -108,7 +108,6 @@ Connector< eprop_synapse< TargetIdentifierPtrRport > >::disable_connection( cons
 {
   assert( not C_[ lcid ].is_disabled() );
   C_[ lcid ].disable();
-  C_[ lcid ].delete_optimizer();
 }
 
 template <>
@@ -117,7 +116,6 @@ Connector< eprop_synapse< TargetIdentifierIndex > >::disable_connection( const s
 {
   assert( not C_[ lcid ].is_disabled() );
   C_[ lcid ].disable();
-  C_[ lcid ].delete_optimizer();
 }
 
 
@@ -136,7 +134,10 @@ Connector< eprop_synapse< TargetIdentifierIndex > >::~Connector()
 {
   for ( auto& c : C_ )
   {
-    c.delete_optimizer();
+    if ( not c.is_disabled() )
+    {
+      c.delete_optimizer();
+    }
   }
   C_.clear();
 }

models/eprop_synapse_bsshslm_2020.cpp

@@ -113,7 +113,6 @@ Connector< eprop_synapse_bsshslm_2020< TargetIdentifierPtrRport > >::disable_con
 {
   assert( not C_[ lcid ].is_disabled() );
   C_[ lcid ].disable();
-  C_[ lcid ].delete_optimizer();
 }
 
 template <>
@@ -122,7 +121,6 @@ Connector< eprop_synapse_bsshslm_2020< TargetIdentifierIndex > >::disable_connec
 {
   assert( not C_[ lcid ].is_disabled() );
   C_[ lcid ].disable();
-  C_[ lcid ].delete_optimizer();
 }
 
 

nestkernel/connection_manager.cpp

@@ -55,6 +55,7 @@
 #include "kernel_manager.h"
 #include "mpi_manager_impl.h"
 #include "nest_names.h"
+#include "nest_types.h"
 #include "node.h"
 #include "sonata_connector.h"
 #include "stopwatch_impl.h"
@@ -976,23 +977,25 @@ nest::ConnectionManager::find_connection( const size_t tid,
   const size_t snode_id,
   const size_t tnode_id )
 {
-  // lcid will hold the position of the /first/ connection from node
-  // snode_id to any local node, or be invalid
-  size_t lcid = source_table_.find_first_source( tid, syn_id, snode_id );
-  if ( lcid == invalid_index )
+  if ( use_compressed_spikes_ )
   {
-    return invalid_index;
-  }
+    const size_t source_index = source_table_.find_first_source( tid, syn_id, snode_id );
+    if ( source_index == invalid_index )
+    {
+      return invalid_index;
+    }
+
+    // lcid will hold the position of the /first/ enabled connection from node
+    // snode_id to node tnode_id, or be invalid
+    const size_t lcid = connections_[ tid ][ syn_id ]->find_first_target( tid, source_index, tnode_id );
 
-  // lcid will hold the position of the /first/ connection from node
-  // snode_id to node tnode_id, or be invalid
-  lcid = connections_[ tid ][ syn_id ]->find_first_target( tid, lcid, tnode_id );
-  if ( lcid != invalid_index )
-  {
     return lcid;
   }
-
-  return lcid;
+  else
+  {
+    return connections_[ tid ][ syn_id ]->find_enabled_connection( tid, syn_id, snode_id, tnode_id, source_table_ );
+  }
+  return invalid_index;
 }
 
 void
@@ -1003,7 +1006,7 @@ nest::ConnectionManager::disconnect( const size_t tid,
 {
   assert( syn_id != invalid_synindex );
 
-  const size_t lcid = find_connection( tid, syn_id, snode_id, tnode_id );
+  const auto lcid = find_connection( tid, syn_id, snode_id, tnode_id );
 
   if ( lcid == invalid_index ) // this function should only be called
                                // with a valid connection
@@ -1450,7 +1453,7 @@ nest::ConnectionManager::sort_connections( const size_t tid )
         connections_[ tid ][ syn_id ]->sort_connections( source_table_.get_thread_local_sources( tid )[ syn_id ] );
       }
     }
-    remove_disabled_connections( tid );
+    remove_disabled_connections_( tid );
   }
 }
 
@@ -1700,8 +1703,10 @@ nest::ConnectionManager::compress_secondary_send_buffer_pos( const size_t tid )
 }
 
 void
-nest::ConnectionManager::remove_disabled_connections( const size_t tid )
+nest::ConnectionManager::remove_disabled_connections_( const size_t tid )
 {
+  assert( use_compressed_spikes_ );
+
   std::vector< ConnectorBase* >& connectors = connections_[ tid ];
 
   for ( synindex syn_id = 0; syn_id < connectors.size(); ++syn_id )

nestkernel/connection_manager.h

@@ -201,6 +201,11 @@ class ConnectionManager : public ManagerInterface
     const DictionaryDatum& third_connectivity,
     const std::map< Name, std::vector< DictionaryDatum > >& synapse_specs );
 
+  /**
+   * Find first non-disabled thread-local connection of given synapse type with given source and target node.
+   *
+   * @returns Local connection id (lcid) or `invalid_index`
+   */
   size_t find_connection( const size_t tid, const synindex syn_id, const size_t snode_id, const size_t tnode_id );
 
   void disconnect( const size_t tid, const synindex syn_id, const size_t snode_id, const size_t tnode_id );
@@ -388,11 +393,6 @@ class ConnectionManager : public ManagerInterface
    */
   void sort_connections( const size_t tid );
 
-  /**
-   * Removes disabled connections (of structural plasticity)
-   */
-  void remove_disabled_connections( const size_t tid );
-
   /**
    * Returns true if connection information needs to be
    * communicated. False otherwise.
@@ -496,6 +496,11 @@ class ConnectionManager : public ManagerInterface
     const size_t tnode_id,
     std::vector< size_t >& sources );
 
+  /**
+   * Removes disabled connections (of structural plasticity)
+   */
+  void remove_disabled_connections_( const size_t tid );
+
   /**
    * Splits a TokenArray of node IDs to two vectors containing node IDs of neurons and
    * node IDs of devices.

nestkernel/connector_base.h

@@ -44,6 +44,7 @@
 #include "nest_names.h"
 #include "node.h"
 #include "source.h"
+#include "source_table.h"
 #include "spikecounter.h"
 
 // Includes from sli:
@@ -192,14 +193,16 @@ class ConnectorBase
   virtual size_t find_first_target( const size_t tid, const size_t start_lcid, const size_t target_node_id ) const = 0;
 
   /**
-   * Return lcid of first connection where the node ID of the target
-   * matches target_node_id; consider only the connections with lcids
-   * given in matching_lcids. If there is no match, the function returns
-   * invalid_index.
+   * Return lcid of first connection matching source and target node id and that
+   * is not disabled.
+   *
+   * Intended for use with unsorted (uncompressed) connections.
    */
-  virtual size_t find_matching_target( const size_t tid,
-    const std::vector< size_t >& matching_lcids,
-    const size_t target_node_id ) const = 0;
+  virtual size_t find_enabled_connection( const size_t tid,
+    const size_t syn_id,
+    const size_t source_node_id,
+    const size_t target_node_id,
+    const SourceTable& source_table ) const = 0;
 
   /**
    * Disable the transfer of events through the connection at position
@@ -466,6 +469,10 @@ class Connector : public ConnectorBase
   size_t
   find_first_target( const size_t tid, const size_t start_lcid, const size_t target_node_id ) const override
   {
+    // TODO: Once #3544 is merged, activate this assertion. It is currently
+    //       commented out to avoid circular inclusions.
+    // assert( kernel().connection_manager.use_compressed_spikes() );
+
     size_t lcid = start_lcid;
     while ( true )
     {
@@ -484,15 +491,18 @@ class Connector : public ConnectorBase
   }
 
   size_t
-  find_matching_target( const size_t tid,
-    const std::vector< size_t >& matching_lcids,
-    const size_t target_node_id ) const override
+  find_enabled_connection( const size_t tid,
+    const size_t syn_id,
+    const size_t source_node_id,
+    const size_t target_node_id,
+    const SourceTable& source_table ) const override
   {
-    for ( size_t i = 0; i < matching_lcids.size(); ++i )
+    for ( size_t lcid = 0; lcid < C_.size(); ++lcid )
     {
-      if ( C_[ matching_lcids[ i ] ].get_target( tid )->get_node_id() == target_node_id )
+      if ( source_table.get_node_id( tid, syn_id, lcid ) == source_node_id
+        and C_[ lcid ].get_target( tid )->get_node_id() == target_node_id and not C_[ lcid ].is_disabled() )
       {
-        return matching_lcids[ i ];
+        return lcid;
       }
     }
 

nestkernel/source.h

@@ -43,17 +43,13 @@ class Source
   uint64_t node_id_ : NUM_BITS_NODE_ID; //!< node ID of source
   bool processed_ : 1;                  //!< whether this target has already been moved
                                         //!< to the MPI buffer
-  bool primary_ : 1;
+  bool primary_ : 1;                    //!< source of primary connection
+  bool disabled_ : 1;                   //!< connection has been disabled
 
 public:
   Source();
   explicit Source( const uint64_t node_id, const bool primary );
 
-  /**
-   * Sets node_id_ to the specified value.
-   */
-  void set_node_id( const uint64_t node_id );
-
   /**
    * Returns this Source's node ID.
    */
@@ -91,24 +87,19 @@ inline Source::Source()
   : node_id_( 0 )
   , processed_( false )
   , primary_( true )
+  , disabled_( false )
 {
 }
 
 inline Source::Source( const uint64_t node_id, const bool is_primary )
   : node_id_( node_id )
   , processed_( false )
   , primary_( is_primary )
+  , disabled_( false )
 {
   assert( node_id <= MAX_NODE_ID );
 }
 
-inline void
-Source::set_node_id( const uint64_t node_id )
-{
-  assert( node_id <= MAX_NODE_ID );
-  node_id_ = node_id;
-}
-
 inline uint64_t
 Source::get_node_id() const
 {
@@ -142,13 +133,13 @@ Source::is_primary() const
 inline void
 Source::disable()
 {
-  node_id_ = DISABLED_NODE_ID;
+  disabled_ = true;
 }
 
 inline bool
 Source::is_disabled() const
 {
-  return node_id_ == DISABLED_NODE_ID;
+  return disabled_;
 }
 
 inline bool

nestkernel/source_table.cpp

@@ -162,9 +162,41 @@ nest::SourceTable::get_node_id( const size_t tid, const synindex syn_id, const s
   return sources_[ tid ][ syn_id ][ lcid ].get_node_id();
 }
 
+size_t
+nest::SourceTable::find_first_source( const size_t tid, const synindex syn_id, const size_t snode_id ) const
+{
+  const auto source_begin = sources_[ tid ][ syn_id ].begin();
+  const auto source_end = sources_[ tid ][ syn_id ].end();
+
+  auto first_source_match = source_begin;
+  if ( kernel().connection_manager.use_compressed_spikes() )
+  {
+    // Binary search for first entry matching snode_id; is_primary is ignored
+    const Source requested_source { snode_id, /* is_primary */ true };
+    first_source_match = std::lower_bound( source_begin, source_end, requested_source );
+  }
+
+  // Linear search for first non-disabled connection
+  const auto first_enabled = std::find_if( first_source_match,
+    source_end,
+    [ &snode_id ]( const Source& src ) { return src.get_node_id() == snode_id and not src.is_disabled(); } );
+  if ( first_enabled != source_end )
+  {
+    // lcid is iterator difference
+    return first_enabled - source_begin;
+  }
+  else
+  {
+    // no enabled entry with this snode ID found
+    return invalid_index;
+  }
+}
+
 size_t
 nest::SourceTable::remove_disabled_sources( const size_t tid, const synindex syn_id )
 {
+  assert( kernel().connection_manager.use_compressed_spikes() );
+
   if ( sources_[ tid ].size() <= syn_id )
   {
     return invalid_index; // no source table entry for this synapse model

nestkernel/source_table.h

@@ -137,7 +137,6 @@ class SourceTable
    */
   static const size_t min_deleted_elements_ = 1000000;
 
-
   /**
    * Returns whether this Source object should be considered when
    * constructing MPI buffers for communicating connections.
@@ -297,8 +296,12 @@ class SourceTable
     std::map< size_t, size_t >& buffer_pos_of_source_node_id_syn_id_ );
 
   /**
-   * Finds the first entry in sources_ at the given thread id and
-   * synapse type that is equal to snode_id.
+   * Finds the first non-disabled entry in sources_ at the given thread id and synapse type that has sender equal to
+   * snode_id.
+   *
+   * @returns local connection id (lcid) or `invalid_index`
+   *
+   * @note For compressed spikes, it uses binary search, otherwise linear search.
    */
   size_t find_first_source( const size_t tid, const synindex syn_id, const size_t snode_id ) const;
 
@@ -470,30 +473,6 @@ SourceTable::no_targets_to_process( const size_t tid )
   current_positions_[ tid ].lcid = -1;
 }
 
-inline size_t
-SourceTable::find_first_source( const size_t tid, const synindex syn_id, const size_t snode_id ) const
-{
-  // binary search in sorted sources
-  const BlockVector< Source >::const_iterator begin = sources_[ tid ][ syn_id ].begin();
-  const BlockVector< Source >::const_iterator end = sources_[ tid ][ syn_id ].end();
-  BlockVector< Source >::const_iterator it = std::lower_bound( begin, end, Source( snode_id, true ) );
-
-  // source found by binary search could be disabled, iterate through
-  // sources until a valid one is found
-  while ( it != end )
-  {
-    if ( it->get_node_id() == snode_id and not it->is_disabled() )
-    {
-      const size_t lcid = it - begin;
-      return lcid;
-    }
-    ++it;
-  }
-
-  // no enabled entry with this snode ID found
-  return invalid_index;
-}
-
 inline void
 SourceTable::disable_connection( const size_t tid, const synindex syn_id, const size_t lcid )
 {