chore: Follow-up after SurfaceArray changes in #5186

Core/include/Acts/Geometry/LayerCreator.hpp

@@ -220,14 +220,6 @@ class LayerCreator {
   void associateSurfacesToLayer(Layer& layer) const;
 
  private:
-  /// Validates that all the sensitive surfaces are actually accessible through
-  /// the binning
-  ///
-  /// @param gctx Geometry context to work with
-  /// @param sArray @c SurfaceArray instance to check
-  bool checkBinning(const GeometryContext& gctx,
-                    const SurfaceArray& sArray) const;
-
   /// configuration object
   Config m_cfg;
 

Core/include/Acts/Surfaces/SurfaceArray.hpp

@@ -68,6 +68,7 @@ class SurfaceArray {
     /// Performs lookup at global bin and returns bin content as const reference
     /// @param bin Global lookup bin
     /// @return A vector of surfaces at given bin
+    [[deprecated("Use at(gridIndices, neighborDistance) instead")]]
     virtual const std::vector<const Surface*>& lookup(
         std::size_t bin) const = 0;
 
@@ -221,7 +222,9 @@ class SurfaceArray {
     /// @param bin Global lookup bin
     /// @return A vector of surfaces at given bin
     const std::vector<const Surface*>& lookup(std::size_t bin) const override {
-      return const_cast<const ISurfaceGridLookup*>(m_impl.get())->lookup(bin);
+      ACTS_PUSH_IGNORE_DEPRECATED()
+      return m_impl->lookup(bin);
+      ACTS_POP_IGNORE_DEPRECATED()
     }
 
     /// Performs a lookup at @c pos, but returns neighbors as well
@@ -440,6 +443,7 @@ class SurfaceArray {
   /// @param position the lookup position
   /// @param direction the lookup direction
   /// @return const reference to surface vector contained in bin at that position
+  [[deprecated("Use at with GeometryContext instead")]]
   const std::vector<const Surface*>& at(const Vector3& position,
                                         const Vector3& direction) const {
     ACTS_PUSH_IGNORE_DEPRECATED()
@@ -472,9 +476,11 @@ class SurfaceArray {
   /// Get all surfaces in bin given by global bin index.
   /// @param bin the global bin index
   /// @return const reference to surface vector contained in bin
+  [[deprecated("Use at(gridIndices, neighborDistance) instead")]]
   const std::vector<const Surface*>& at(std::size_t bin) const {
-    return const_cast<const ISurfaceGridLookup*>(m_gridLookup.get())
-        ->lookup(bin);
+    ACTS_PUSH_IGNORE_DEPRECATED()
+    return m_gridLookup->lookup(bin);
+    ACTS_POP_IGNORE_DEPRECATED()
   }
 
   /// Get all surfaces in bin at @p pos and its neighbors

Core/src/Geometry/LayerCreator.cpp

@@ -20,6 +20,7 @@
 #include "Acts/Surfaces/RadialBounds.hpp"
 #include "Acts/Surfaces/RectangleBounds.hpp"
 #include "Acts/Surfaces/Surface.hpp"
+#include "Acts/Utilities/Diagnostics.hpp"
 
 #include <algorithm>
 #include <iterator>
@@ -103,8 +104,6 @@ MutableLayerPtr LayerCreator::cylinderLayer(
     sArray = m_cfg.surfaceArrayCreator->surfaceArrayOnCylinder(
         gctx, std::move(surfaces), binsPhi, binsZ, protoLayer, fullTransform,
         maxNeighborDistance);
-
-    checkBinning(gctx, *sArray);
   }
 
   // create the layer and push it back
@@ -183,8 +182,6 @@ MutableLayerPtr LayerCreator::cylinderLayer(
     sArray = m_cfg.surfaceArrayCreator->surfaceArrayOnCylinder(
         gctx, std::move(surfaces), bTypePhi, bTypeZ, protoLayer, fullTransform,
         maxNeighborDistance);
-
-    checkBinning(gctx, *sArray);
   }
 
   // create the layer and push it back
@@ -253,8 +250,6 @@ MutableLayerPtr LayerCreator::discLayer(
     sArray = m_cfg.surfaceArrayCreator->surfaceArrayOnDisc(
         gctx, std::move(surfaces), binsR, binsPhi, protoLayer, fullTransform,
         maxNeighborDistance);
-
-    checkBinning(gctx, *sArray);
   }
 
   // create the share disc bounds
@@ -325,8 +320,6 @@ MutableLayerPtr LayerCreator::discLayer(
     sArray = m_cfg.surfaceArrayCreator->surfaceArrayOnDisc(
         gctx, std::move(surfaces), bTypeR, bTypePhi, protoLayer, fullTransform,
         maxNeighborDistance);
-
-    checkBinning(gctx, *sArray);
   }
 
   // create the shared disc bounds
@@ -418,8 +411,6 @@ MutableLayerPtr LayerCreator::planeLayer(
     sArray = m_cfg.surfaceArrayCreator->surfaceArrayOnPlane(
         gctx, std::move(surfaces), bins1, bins2, aDir, protoLayer,
         fullTransform, maxNeighborDistance);
-
-    checkBinning(gctx, *sArray);
   }
 
   // create the layer and push it back
@@ -450,73 +441,4 @@ void LayerCreator::associateSurfacesToLayer(Layer& layer) const {
   }
 }
 
-bool LayerCreator::checkBinning(const GeometryContext& gctx,
-                                const SurfaceArray& sArray) const {
-  // do consistency check: can we access all sensitive surfaces
-  // through the binning? If not, surfaces get lost and the binning does not
-  // work
-
-  ACTS_VERBOSE("Performing consistency check");
-
-  std::vector<const Surface*> surfaces = sArray.surfaces();
-  std::set<const Surface*> sensitiveSurfaces(surfaces.begin(), surfaces.end());
-  std::set<const Surface*> accessibleSurfaces;
-  std::size_t nEmptyBins = 0;
-  std::size_t nBinsChecked = 0;
-
-  // iterate over all bins
-  std::size_t size = sArray.size();
-  for (std::size_t b = 0; b < size; ++b) {
-    std::vector<const Surface*> binContent = sArray.at(b);
-    // we don't check under/overflow bins
-    if (!sArray.isValidBin(b)) {
-      continue;
-    }
-    for (const auto& srf : binContent) {
-      accessibleSurfaces.insert(srf);
-    }
-    if (binContent.empty()) {
-      nEmptyBins++;
-    }
-    nBinsChecked++;
-  }
-
-  std::vector<const Surface*> diff;
-  std::set_difference(sensitiveSurfaces.begin(), sensitiveSurfaces.end(),
-                      accessibleSurfaces.begin(), accessibleSurfaces.end(),
-                      std::inserter(diff, diff.begin()));
-
-  ACTS_VERBOSE(" - Checked " << nBinsChecked << " valid bins");
-
-  if (nEmptyBins > 0) {
-    ACTS_VERBOSE(" -- Not all bins point to surface. " << nEmptyBins
-                                                       << " empty");
-  } else {
-    ACTS_VERBOSE(" -- All bins point to a surface");
-  }
-
-  if (!diff.empty()) {
-    ACTS_ERROR(
-        " -- Not all sensitive surfaces are accessible through binning. "
-        "sensitive: "
-        << sensitiveSurfaces.size()
-        << "    accessible: " << accessibleSurfaces.size());
-
-    // print all inaccessibles
-    ACTS_ERROR(" -- Inaccessible surfaces: ");
-    for (const auto& srf : diff) {
-      // have to choose AxisDirection here
-      Vector3 ctr = srf->referencePosition(gctx, AxisDirection::AxisR);
-      ACTS_ERROR(" Surface(x=" << ctr.x() << ", y=" << ctr.y()
-                               << ", z=" << ctr.z() << ", r=" << perp(ctr)
-                               << ", phi=" << phi(ctr) << ")");
-    }
-
-  } else {
-    ACTS_VERBOSE(" -- All sensitive surfaces are accessible through binning.");
-  }
-
-  return diff.empty();
-}
-
 }  // namespace Acts

Core/src/Navigation/SurfaceArrayNavigationPolicy.cpp

@@ -15,76 +15,6 @@
 #include "Acts/Navigation/NavigationStream.hpp"
 
 namespace Acts {
-namespace {
-bool checkBinning(const GeometryContext& gctx, const SurfaceArray& sArray,
-                  const Logger& logger) {
-  // do consistency check: can we access all sensitive surfaces
-  // through the binning? If not, surfaces get lost and the binning does not
-  // work
-
-  ACTS_VERBOSE("Performing consistency check");
-
-  std::vector<const Surface*> surfaces = sArray.surfaces();
-  std::set<const Surface*> sensitiveSurfaces(surfaces.begin(), surfaces.end());
-  std::set<const Surface*> accessibleSurfaces;
-  std::size_t nEmptyBins = 0;
-  std::size_t nBinsChecked = 0;
-
-  // iterate over all bins
-  std::size_t size = sArray.size();
-  for (std::size_t b = 0; b < size; ++b) {
-    std::vector<const Surface*> binContent = sArray.at(b);
-    // we don't check under/overflow bins
-    if (!sArray.isValidBin(b)) {
-      continue;
-    }
-    for (const auto& srf : binContent) {
-      accessibleSurfaces.insert(srf);
-    }
-    if (binContent.empty()) {
-      nEmptyBins++;
-    }
-    nBinsChecked++;
-  }
-
-  std::vector<const Surface*> diff;
-  std::set_difference(sensitiveSurfaces.begin(), sensitiveSurfaces.end(),
-                      accessibleSurfaces.begin(), accessibleSurfaces.end(),
-                      std::inserter(diff, diff.begin()));
-
-  ACTS_VERBOSE(" - Checked " << nBinsChecked << " valid bins");
-
-  if (nEmptyBins > 0) {
-    ACTS_VERBOSE(" -- Not all bins point to surface. " << nEmptyBins
-                                                       << " empty");
-  } else {
-    ACTS_VERBOSE(" -- All bins point to a surface");
-  }
-
-  if (!diff.empty()) {
-    ACTS_ERROR(
-        " -- Not all sensitive surfaces are accessible through binning. "
-        "sensitive: "
-        << sensitiveSurfaces.size()
-        << "    accessible: " << accessibleSurfaces.size());
-
-    // print all inaccessibles
-    ACTS_ERROR(" -- Inaccessible surfaces: ");
-    for (const auto& srf : diff) {
-      // have to choose AxisDirection here
-      Vector3 ctr = srf->referencePosition(gctx, AxisDirection::AxisR);
-      ACTS_ERROR(" Surface(x=" << ctr.x() << ", y=" << ctr.y() << ", z="
-                               << ctr.z() << ", r=" << VectorHelpers::perp(ctr)
-                               << ", phi=" << VectorHelpers::phi(ctr) << ")");
-    }
-
-  } else {
-    ACTS_VERBOSE(" -- All sensitive surfaces are accessible through binning.");
-  }
-
-  return diff.empty();
-}
-}  // namespace
 
 SurfaceArrayNavigationPolicy::SurfaceArrayNavigationPolicy(
     const GeometryContext& gctx, const TrackingVolume& volume,
@@ -188,8 +118,6 @@ SurfaceArrayNavigationPolicy::SurfaceArrayNavigationPolicy(
     ACTS_ERROR("Failed to create surface array");
     throw std::runtime_error("Failed to create surface array");
   }
-
-  checkBinning(gctx, *m_surfaceArray, logger);
 }
 
 void SurfaceArrayNavigationPolicy::initializeCandidates(

Tests/UnitTests/Core/Geometry/LayerCreatorTests.cpp

@@ -25,6 +25,7 @@
 #include "Acts/Surfaces/Surface.hpp"
 #include "Acts/Surfaces/SurfaceArray.hpp"
 #include "Acts/Utilities/BinningType.hpp"
+#include "Acts/Utilities/Diagnostics.hpp"
 #include "Acts/Utilities/IAxis.hpp"
 #include "Acts/Utilities/Logger.hpp"
 #include "ActsTests/CommonHelpers/FloatComparisons.hpp"
@@ -99,19 +100,16 @@ struct LayerCreatorFixture {
         cfg, getDefaultLogger("LayerCreator", Logging::VERBOSE));
   }
 
-  template <typename... Args>
-  bool checkBinning(Args&&... args) {
-    return p_LC->checkBinning(std::forward<Args>(args)...);
-  }
-
   bool checkBinContentSize(const SurfaceArray* sArray, std::size_t n) {
     std::size_t nBins = sArray->size();
     bool result = true;
     for (std::size_t i = 0; i < nBins; ++i) {
       if (!sArray->isValidBin(i)) {
         continue;
       }
-      std::vector<const Surface*> binContent = sArray->at(i);
+      ACTS_PUSH_IGNORE_DEPRECATED()
+      const std::vector<const Surface*>& binContent = sArray->at(i);
+      ACTS_POP_IGNORE_DEPRECATED()
       BOOST_TEST_INFO("Bin: " << i);
       BOOST_CHECK_EQUAL(binContent.size(), n);
       result = result && binContent.size() == n;
@@ -257,7 +255,6 @@ BOOST_FIXTURE_TEST_CASE(LayerCreator_createCylinderLayer, LayerCreatorFixture) {
   const CylinderBounds* bounds = &layer->bounds();
   CHECK_CLOSE_REL(bounds->get(CylinderBounds::eR), (rMax + rMin) / 2., 1e-3);
   CHECK_CLOSE_REL(bounds->get(CylinderBounds::eHalfLengthZ), 14 + envZ, 1e-3);
-  BOOST_CHECK(checkBinning(tgContext, *layer->surfaceArray()));
   auto axes = layer->surfaceArray()->getAxes();
   BOOST_CHECK_EQUAL(axes.at(0)->getNBins(), 30u);
   BOOST_CHECK_EQUAL(axes.at(1)->getNBins(), 7u);
@@ -277,7 +274,6 @@ BOOST_FIXTURE_TEST_CASE(LayerCreator_createCylinderLayer, LayerCreatorFixture) {
   bounds = &layer->bounds();
   CHECK_CLOSE_REL(bounds->get(CylinderBounds::eR), (rMax + rMin) / 2., 1e-3);
   CHECK_CLOSE_REL(bounds->get(CylinderBounds::eHalfLengthZ), 14 + envZ, 1e-3);
-  BOOST_CHECK(checkBinning(tgContext, *layer->surfaceArray()));
   axes = layer->surfaceArray()->getAxes();
   BOOST_CHECK_EQUAL(axes.at(0)->getNBins(), 30u);
   BOOST_CHECK_EQUAL(axes.at(1)->getNBins(), 7u);
@@ -292,9 +288,6 @@ BOOST_FIXTURE_TEST_CASE(LayerCreator_createCylinderLayer, LayerCreatorFixture) {
   bounds = &layer->bounds();
   CHECK_CLOSE_REL(bounds->get(CylinderBounds::eR), (rMax + rMin) / 2., 1e-3);
   CHECK_CLOSE_REL(bounds->get(CylinderBounds::eHalfLengthZ), 14 + envZ, 1e-3);
-  // this succeeds despite sub-optimal binning
-  // since we now have multientry bins
-  BOOST_CHECK(checkBinning(tgContext, *layer->surfaceArray()));
   axes = layer->surfaceArray()->getAxes();
   BOOST_CHECK_EQUAL(axes.at(0)->getNBins(), 13u);
   BOOST_CHECK_EQUAL(axes.at(1)->getNBins(), 3u);
@@ -314,11 +307,6 @@ BOOST_FIXTURE_TEST_CASE(LayerCreator_createCylinderLayer, LayerCreatorFixture) {
   CHECK_CLOSE_REL(bounds->get(CylinderBounds::eR), 10.5, 1e-3);
   CHECK_CLOSE_REL(bounds->get(CylinderBounds::eHalfLengthZ), 25, 1e-3);
 
-  // this should fail, b/c it's a completely inconvenient binning
-  // but it succeeds despite sub-optimal binning
-  // since we now have multientry bins
-  BOOST_CHECK(checkBinning(tgContext, *layer->surfaceArray()));
-
   axes = layer->surfaceArray()->getAxes();
   BOOST_CHECK_EQUAL(axes.at(0)->getNBins(), 30u);
   BOOST_CHECK_EQUAL(axes.at(1)->getNBins(), 7u);
@@ -348,7 +336,6 @@ BOOST_FIXTURE_TEST_CASE(LayerCreator_createDiscLayer, LayerCreatorFixture) {
       dynamic_cast<const RadialBounds*>(&layer->bounds());
   CHECK_CLOSE_REL(bounds->rMin(), 5, 1e-3);
   CHECK_CLOSE_REL(bounds->rMax(), 25, 1e-3);
-  BOOST_CHECK(checkBinning(tgContext, *layer->surfaceArray()));
   auto axes = layer->surfaceArray()->getAxes();
   BOOST_CHECK_EQUAL(axes.at(0)->getNBins(), 3u);
   BOOST_CHECK_EQUAL(axes.at(1)->getNBins(), 30u);
@@ -377,7 +364,6 @@ BOOST_FIXTURE_TEST_CASE(LayerCreator_createDiscLayer, LayerCreatorFixture) {
   bounds = dynamic_cast<const RadialBounds*>(&layer->bounds());
   CHECK_CLOSE_REL(bounds->rMin(), rMin - envMinR, 1e-3);
   CHECK_CLOSE_REL(bounds->rMax(), rMax + envMaxR, 1e-3);
-  BOOST_CHECK(checkBinning(tgContext, *layer->surfaceArray()));
   axes = layer->surfaceArray()->getAxes();
   BOOST_CHECK_EQUAL(axes.at(0)->getNBins(), nBinsR);
   BOOST_CHECK_EQUAL(axes.at(1)->getNBins(), nBinsPhi);
@@ -399,7 +385,6 @@ BOOST_FIXTURE_TEST_CASE(LayerCreator_createDiscLayer, LayerCreatorFixture) {
   bounds = dynamic_cast<const RadialBounds*>(&layer->bounds());
   CHECK_CLOSE_REL(bounds->rMin(), rMin - envMinR, 1e-3);
   CHECK_CLOSE_REL(bounds->rMax(), rMax + envMaxR, 1e-3);
-  BOOST_CHECK(checkBinning(tgContext, *layer->surfaceArray()));
   axes = layer->surfaceArray()->getAxes();
   BOOST_CHECK_EQUAL(axes.at(0)->getNBins(), nBinsR);
   BOOST_CHECK_EQUAL(axes.at(1)->getNBins(), nBinsPhi);
@@ -444,16 +429,16 @@ BOOST_FIXTURE_TEST_CASE(LayerCreator_barrelStagger, LayerCreatorFixture) {
     // std::endl;
 
     Vector3 ctr = A->referencePosition(tgContext, AxisDirection::AxisR);
-    auto binContent = layer->surfaceArray()->at(ctr, ctr.normalized());
+    ACTS_PUSH_IGNORE_DEPRECATED()
+    const std::vector<const Surface*>& binContent =
+        layer->surfaceArray()->at(ctr, ctr.normalized());
+    ACTS_POP_IGNORE_DEPRECATED()
     BOOST_CHECK_EQUAL(binContent.size(), 2u);
     std::set<const Surface*> act(binContent.begin(), binContent.end());
 
     std::set<const Surface*> exp({A, B});
     BOOST_CHECK(std::ranges::includes(act, exp));
   }
-
-  // checkBinning should also report everything is fine
-  checkBinning(tgContext, *layer->surfaceArray());
 }
 
 BOOST_AUTO_TEST_SUITE_END()