fix: Handle cell duplication with an error (acts-project#4745)

This makes clusterization (both 1D and 2D) throw an exception when duplicate cells are given as input.

Additionally, the `Connect2d::operator()` is a bit smarter in returning `ConnectResult::eNoConnStop` as quickly as possible (it previously waited unnecessarily until going past a full column). This could provide some performance improvement in cases where your cells are heavily "column-like".

Fixes acts-project#4736

Author: DJDuque

Core/include/Acts/Clusterization/Clusterization.hpp

@@ -109,7 +109,8 @@ concept CanReserve = requires(Cluster cluster, std::size_t n) {
 enum class ConnectResult {
   eNoConn,      // No connections, keep looking
   eNoConnStop,  // No connections, stop looking
-  eConn         // Found connection
+  eConn,        // Found connection
+  eDuplicate    // Found duplicate cell, throw an exception
 };
 
 // Default connection type for 2-D grids: 4- or 8-cell connectivity
@@ -160,6 +161,7 @@ struct DefaultConnect<Cell, 2> : public Connect2D<Cell> {
 /// @param [in] data collection of quantities for clusterization
 /// @param [in] cells the cell collection to be labeled
 /// @param [in] connect the connection type (see DefaultConnect)
+/// @throws std::invalid_argument if the input contains duplicate cells.
 template <typename CellCollection, std::size_t GridDim = 2,
           typename Connect =
               DefaultConnect<typename CellCollection::value_type, GridDim>>
@@ -191,6 +193,8 @@ ClusterCollection createClusters(CellCollection& cells,
 /// @brief createClusters
 /// Alternative convenience function which runs both labelClusters and
 /// createClusters.
+///
+/// @throws std::invalid_argument if the input contains duplicate cells.
 template <typename CellCollection, typename ClusterCollection,
           std::size_t GridDim = 2,
           typename Connect =

Core/include/Acts/Clusterization/Clusterization.ipp

@@ -89,6 +89,10 @@ Connections<GridDim> getConnections(std::size_t idx, std::vector<Cell>& cells,
     std::size_t idx2 = idx - i - 1;
     ConnectResult cr = connect(cells[idx], cells[idx2]);
 
+    if (cr == ConnectResult::eDuplicate) {
+      throw std::invalid_argument(
+          "Clusterization: input contains duplicate cells");
+    }
     if (cr == ConnectResult::eNoConnStop) {
       break;
     }
@@ -153,31 +157,47 @@ template <typename Cell>
            Acts::Ccl::HasRetrievableRowInfo<Cell>)
 ConnectResult Connect2D<Cell>::operator()(const Cell& ref,
                                           const Cell& iter) const {
-  int deltaRow = std::abs(getCellRow(ref) - getCellRow(iter));
-  int deltaCol = std::abs(getCellColumn(ref) - getCellColumn(iter));
-  // Iteration is column-wise, so if too far in column, can
-  // safely stop
-  if (deltaCol > 1) {
-    return ConnectResult::eNoConnStop;
-  }
-  // For same reason, if too far in row we know the pixel is not
-  // connected, but need to keep iterating
-  if (deltaRow > 1) {
-    return ConnectResult::eNoConn;
-  }
-  // Decide whether or not cluster is connected based on 4- or
-  // 8-connectivity
-  if ((deltaRow + deltaCol) <= (conn8 ? 2 : 1)) {
-    return ConnectResult::eConn;
+  int deltaRow = getCellRow(iter) - getCellRow(ref);
+  int deltaCol = getCellColumn(iter) - getCellColumn(ref);
+  assert((deltaCol < 0 || (deltaCol == 0 && deltaRow <= 0)) &&
+         "Not iterating backwards");
+
+  switch (deltaCol) {
+    case 0:
+      if (deltaRow == 0) {
+        return ConnectResult::eDuplicate;
+      } else if (deltaRow == -1) {
+        return ConnectResult::eConn;
+      } else {
+        return ConnectResult::eNoConn;
+      }
+    case -1:
+      if (deltaRow > static_cast<int>(conn8)) {
+        return ConnectResult::eNoConn;
+      } else if (deltaRow < -static_cast<int>(conn8)) {
+        return ConnectResult::eNoConnStop;
+      } else {
+        return ConnectResult::eConn;
+      }
+    default:
+      return ConnectResult::eNoConnStop;
   }
-  return ConnectResult::eNoConn;
 }
 
 template <Acts::Ccl::HasRetrievableColumnInfo Cell>
 ConnectResult Connect1D<Cell>::operator()(const Cell& ref,
                                           const Cell& iter) const {
-  int deltaCol = std::abs(getCellColumn(ref) - getCellColumn(iter));
-  return deltaCol == 1 ? ConnectResult::eConn : ConnectResult::eNoConnStop;
+  int deltaCol = getCellColumn(iter) - getCellColumn(ref);
+  assert((deltaCol <= 0) && "Not iterating backwards");
+
+  switch (deltaCol) {
+    case 0:
+      return ConnectResult::eDuplicate;
+    case -1:
+      return ConnectResult::eConn;
+    default:
+      return ConnectResult::eNoConnStop;
+  }
 }
 
 template <std::size_t GridDim>

Tests/UnitTests/Core/Clusterization/ClusterizationTests1D.cpp

@@ -129,6 +129,22 @@ BOOST_AUTO_TEST_CASE(Grid_1D_rand) {
   }
 }
 
+BOOST_AUTO_TEST_CASE(Grid_1D_duplicate_cells) {
+  using Cell = Cell1D;
+  using CellC = std::vector<Cell>;
+  using Cluster = Cluster1D;
+  using ClusterC = std::vector<Cluster>;
+
+  CellC cells = {Cell(42), Cell(42)};
+  ClusterC clusters;
+
+  Ccl::ClusteringData data;
+
+  BOOST_CHECK_THROW(
+      (Ccl::createClusters<CellC, ClusterC, 1>(data, cells, clusters)),
+      std::invalid_argument);
+}
+
 BOOST_AUTO_TEST_SUITE_END()
 
 }  // namespace ActsTests

Tests/UnitTests/Core/Clusterization/ClusterizationTests2D.cpp

@@ -248,6 +248,23 @@ BOOST_AUTO_TEST_CASE(Grid_2D_rand) {
     }
   }
 }
+
+BOOST_AUTO_TEST_CASE(Grid_2D_duplicate_cells) {
+  using Cell = Cell2D;
+  using CellC = std::vector<Cell>;
+  using Cluster = Cluster2D;
+  using ClusterC = std::vector<Cluster>;
+
+  CellC cells = {Cell(10, 20), Cell(10, 20)};
+  ClusterC clusters;
+
+  Ccl::ClusteringData data;
+
+  BOOST_CHECK_THROW(
+      (Ccl::createClusters<CellC, ClusterC>(data, cells, clusters)),
+      std::invalid_argument);
+}
+
 BOOST_AUTO_TEST_SUITE_END()
 
 }  // namespace ActsTests