chore: Remove explicit (bin0/bin1) access to MaterialSlab (acts-proje…

…ct#3028) This PR removes the explicit: `materialSlab(std::size_t bin0, std::size_t bin1)` access from the `ISurfaceMaterial` base class (and the derived classes), because it only makes sense for `BinnedSurfaceMaterial `, and not for other surface material versions. In all cases this was used, it was clear that you had `BinnedSurfaceMaterial` in hand, so you could directly access the `fullMaterial()` matrix.
andiwand · Mar 15, 2024 · 4cb1e5b · 4cb1e5b
1 parent 6d1cf04
commit 4cb1e5b
Show file tree

Hide file tree

Showing 11 changed files with 36 additions and 79 deletions.
diff --git a/Core/include/Acts/Material/BinnedSurfaceMaterial.hpp b/Core/include/Acts/Material/BinnedSurfaceMaterial.hpp
@@ -100,10 +100,6 @@ class BinnedSurfaceMaterial : public ISurfaceMaterial {
   /// @copydoc ISurfaceMaterial::materialSlab(const Vector3&) const
   const MaterialSlab& materialSlab(const Vector3& gp) const final;
 
-  /// @copydoc ISurfaceMaterial::materialSlab(std::size_t, std::size_t) const
-  const MaterialSlab& materialSlab(std::size_t bin0,
-                                   std::size_t bin1) const final;
-
   /// Output Method for std::ostream, to be overloaded by child classes
   std::ostream& toStream(std::ostream& sl) const final;
 
@@ -123,8 +119,4 @@ inline const MaterialSlabMatrix& BinnedSurfaceMaterial::fullMaterial() const {
   return m_fullMaterial;
 }
 
-inline const MaterialSlab& BinnedSurfaceMaterial::materialSlab(
-    std::size_t bin0, std::size_t bin1) const {
-  return m_fullMaterial[bin1][bin0];
-}
 }  // namespace Acts
diff --git a/Core/include/Acts/Material/GridSurfaceMaterial.hpp b/Core/include/Acts/Material/GridSurfaceMaterial.hpp
@@ -140,18 +140,6 @@ class GridSurfaceMaterialT : public ISurfaceMaterial {
                                                            m_globalCasts));
   }
 
-  /// @copydoc ISurfaceMaterial::materialSlab(std::size_t bin0, std::size_t bin1) const
-  ///
-  /// The ISurface material class expects bins in [ 0 - n ] x [ 0, m ], we will
-  /// convert into this format, but it is gonna be slow
-  const MaterialSlab& materialSlab(std::size_t bin0,
-                                   std::size_t bin1) const final {
-    // Access via bin0 and bin1
-    Acts::Vector2 lposition =
-        GridAccessHelpers::toLocal(m_grid, bin0, bin1, m_localAccessors[0u]);
-    return materialSlab(lposition);
-  }
-
   /// Scale operator
   ///
   /// @param scale is the scale factor applied

diff --git a/Core/include/Acts/Material/HomogeneousSurfaceMaterial.hpp b/Core/include/Acts/Material/HomogeneousSurfaceMaterial.hpp
@@ -80,12 +80,6 @@ class HomogeneousSurfaceMaterial : public ISurfaceMaterial {
   /// @note the input parameter is ignored
   const MaterialSlab& materialSlab(const Vector3& gp) const final;
 
-  /// @copydoc ISurfaceMaterial::materialSlab(std::size_t, std::size_t) const
-  ///
-  /// @note the input parameters are ignored
-  const MaterialSlab& materialSlab(std::size_t bin0,
-                                   std::size_t bin1) const final;
-
   /// The inherited methods - for MaterialSlab access
   using ISurfaceMaterial::materialSlab;
 
@@ -112,11 +106,6 @@ inline const MaterialSlab& HomogeneousSurfaceMaterial::materialSlab(
   return (m_fullMaterial);
 }
 
-inline const MaterialSlab& HomogeneousSurfaceMaterial::materialSlab(
-    std::size_t /*ib0*/, std::size_t /*ib1*/) const {
-  return (m_fullMaterial);
-}
-
 inline bool HomogeneousSurfaceMaterial::operator==(
     const HomogeneousSurfaceMaterial& hsm) const {
   return (m_fullMaterial == hsm.m_fullMaterial);

diff --git a/Core/include/Acts/Material/ISurfaceMaterial.hpp b/Core/include/Acts/Material/ISurfaceMaterial.hpp
@@ -70,13 +70,6 @@ class ISurfaceMaterial {
   /// @return const MaterialSlab
   virtual const MaterialSlab& materialSlab(const Vector3& gp) const = 0;
 
-  /// Direct access via bins to the MaterialSlab
-  ///
-  /// @param bin0 is the material bin in dimension 0
-  /// @param bin1 is the material bin in dimension 1
-  virtual const MaterialSlab& materialSlab(std::size_t bin0,
-                                           std::size_t bin1) const = 0;
-
   /// Update pre factor
   ///
   /// @param pDir is the positive direction through the surface

diff --git a/Core/include/Acts/Material/ProtoSurfaceMaterial.hpp b/Core/include/Acts/Material/ProtoSurfaceMaterial.hpp
@@ -91,14 +91,6 @@ class ProtoSurfaceMaterialT : public ISurfaceMaterial {
     return (m_materialSlab);
   }
 
-  /// Direct access via bins to the MaterialSlab
-  ///
-  /// @return will return dummy material
-  const MaterialSlab& materialSlab(std::size_t /*unused*/,
-                                   std::size_t /*unused*/) const final {
-    return (m_materialSlab);
-  }
-
   /// Output Method for std::ostream, to be overloaded by child classes
   ///
   /// @param sl is the output stream

diff --git a/Core/src/Material/SurfaceMaterialMapper.cpp b/Core/src/Material/SurfaceMaterialMapper.cpp
@@ -425,9 +425,14 @@ void Acts::SurfaceMaterialMapper::mapInteraction(
   for (auto tmapBin : touchedMapBins) {
     std::vector<std::array<std::size_t, 3>> trackBins = {tmapBin.second};
     if (m_cfg.computeVariance) {
-      tmapBin.first->trackVariance(
-          trackBins, touchedMaterialBin[tmapBin.first]->materialSlab(
-                         trackBins[0][0], trackBins[0][1]));
+      // This only makes sense for the binned material
+      auto binnedMaterial = dynamic_cast<const BinnedSurfaceMaterial*>(
+          touchedMaterialBin[tmapBin.first].get());
+      if (binnedMaterial != nullptr) {
+        tmapBin.first->trackVariance(
+            trackBins,
+            binnedMaterial->fullMaterial()[trackBins[0][1]][trackBins[0][0]]);
+      }
     }
     tmapBin.first->trackAverage(trackBins);
   }
@@ -498,11 +503,15 @@ void Acts::SurfaceMaterialMapper::mapSurfaceInteraction(
   for (auto tmapBin : touchedMapBins) {
     std::vector<std::array<std::size_t, 3>> trackBins = {tmapBin.second};
     if (m_cfg.computeVariance) {
-      tmapBin.first->trackVariance(
-          trackBins,
-          touchedMaterialBin[tmapBin.first]->materialSlab(trackBins[0][0],
-                                                          trackBins[0][1]),
-          true);
+      // This only makes sense for the binned material
+      auto binnedMaterial = dynamic_cast<const BinnedSurfaceMaterial*>(
+          touchedMaterialBin[tmapBin.first].get());
+      if (binnedMaterial != nullptr) {
+        tmapBin.first->trackVariance(
+            trackBins,
+            binnedMaterial->fullMaterial()[trackBins[0][1]][trackBins[0][0]],
+            true);
+      }
     }
     // No need to do an extra pass for untouched surfaces they would have been
     // added to the material interaction in the initial mapping

diff --git a/Examples/Io/Root/src/RootMaterialWriter.cpp b/Examples/Io/Root/src/RootMaterialWriter.cpp
@@ -8,6 +8,7 @@
 
 #include "ActsExamples/Io/Root/RootMaterialWriter.hpp"
 
+#include "Acts/Definitions/Algebra.hpp"
 #include "Acts/Geometry/ApproachDescriptor.hpp"
 #include "Acts/Geometry/BoundarySurfaceT.hpp"
 #include "Acts/Geometry/Layer.hpp"
@@ -24,6 +25,7 @@
 #include "Acts/Utilities/BinUtility.hpp"
 #include "Acts/Utilities/BinnedArray.hpp"
 #include "Acts/Utilities/BinningData.hpp"
+#include "Acts/Utilities/Enumerate.hpp"
 #include "Acts/Utilities/Logger.hpp"
 #include <Acts/Geometry/GeometryIdentifier.hpp>
 #include <Acts/Material/BinnedSurfaceMaterial.hpp>
@@ -156,11 +158,10 @@ void ActsExamples::RootMaterialWriter::writeMaterial(
                          -0.5, bins0 - 0.5, bins1, -0.5, bins1 - 0.5);
 
     // loop over the material and fill
-    for (std::size_t b0 = 0; b0 < bins0; ++b0) {
-      for (std::size_t b1 = 0; b1 < bins1; ++b1) {
-        // get the material for the bin
-        auto& mat = sMaterial->materialSlab(b0, b1);
-        if (mat) {
+    if (bsm != nullptr) {
+      const auto& materialMatrix = bsm->fullMaterial();
+      for (auto [b1, materialVector] : Acts::enumerate(materialMatrix)) {
+        for (auto [b0, mat] : Acts::enumerate(materialVector)) {
           t->SetBinContent(b0 + 1, b1 + 1, mat.thickness());
           x0->SetBinContent(b0 + 1, b1 + 1, mat.material().X0());
           l0->SetBinContent(b0 + 1, b1 + 1, mat.material().L0());
@@ -169,6 +170,15 @@ void ActsExamples::RootMaterialWriter::writeMaterial(
           rho->SetBinContent(b0 + 1, b1 + 1, mat.material().massDensity());
         }
       }
+    } else if (bins1 == 1 && bins0 == 1) {
+      // homogeneous surface
+      auto mat = sMaterial->materialSlab(Acts::Vector3{0, 0, 0});
+      t->SetBinContent(1, 1, mat.thickness());
+      x0->SetBinContent(1, 1, mat.material().X0());
+      l0->SetBinContent(1, 1, mat.material().L0());
+      A->SetBinContent(1, 1, mat.material().Ar());
+      Z->SetBinContent(1, 1, mat.material().Z());
+      rho->SetBinContent(1, 1, mat.material().massDensity());
     }
     t->Write();
     x0->Write();
@@ -343,6 +353,8 @@ void ActsExamples::RootMaterialWriter::collectMaterial(
 
   // If confined layers exist, loop over them and collect the layer material
   if (tVolume.confinedLayers() != nullptr) {
+    ACTS_VERBOSE("Collecting material for " << tVolume.volumeName()
+                                            << " layers");
     for (auto& lay : tVolume.confinedLayers()->arrayObjects()) {
       collectMaterial(*lay, detMatMap);
     }

diff --git a/Plugins/Json/src/MaterialJsonConverter.cpp b/Plugins/Json/src/MaterialJsonConverter.cpp
@@ -120,7 +120,7 @@ void Acts::to_json(nlohmann::json& j, const surfaceMaterialPointer& material) {
     jMaterial[Acts::jsonKey().maptype] = mapType;
     // Material has been mapped
     jMaterial[Acts::jsonKey().mapkey] = true;
-    nlohmann::json jmat(hsMaterial->materialSlab(0, 0));
+    nlohmann::json jmat(hsMaterial->materialSlab(Acts::Vector3(0., 0., 0.)));
     jMaterial[Acts::jsonKey().datakey] = nlohmann::json::array({
         nlohmann::json::array({
             jmat,

diff --git a/Tests/UnitTests/Core/Material/GridSurfaceMaterialTests.cpp b/Tests/UnitTests/Core/Material/GridSurfaceMaterialTests.cpp
@@ -82,13 +82,6 @@ BOOST_AUTO_TEST_CASE(GridIndexedMaterial1D) {
   BOOST_CHECK_EQUAL(ml3.material().X0(), 11.);
   BOOST_CHECK_EQUAL(ml4.material().X0(), 21.);
 
-  // Direct access with bin0 and bin1
-  const Acts::MaterialSlab& mb0 = ism.materialSlab(3u, 0);  // should be vacuum
-  BOOST_CHECK_EQUAL(mb0.material().X0(), 11.);
-
-  const Acts::MaterialSlab& mb1 = ism.materialSlab(4u, 0);  // should be vacuum
-  BOOST_CHECK_EQUAL(mb1.material().X0(), 21.);
-
   // Now scale it - and access again
   ism *= 2.;
   const Acts::MaterialSlab& sml0 = ism.materialSlab(l0);
@@ -154,10 +147,6 @@ BOOST_AUTO_TEST_CASE(GridIndexedMaterial2D) {
                    -9.5);  // should be material 3, same phi but different z
   const Acts::MaterialSlab& mg3 = ism.materialSlab(g3);
   BOOST_CHECK_EQUAL(mg3.material().X0(), 21.);
-
-  // Direct access with bin0 and bin1
-  const Acts::MaterialSlab& mb0 = ism.materialSlab(0u, 2u);  // should be vacuum
-  BOOST_CHECK(!mb0.material());                              // vacuum
 }
 
 BOOST_AUTO_TEST_SUITE_END()
diff --git a/Tests/UnitTests/Core/Material/HomogeneousSurfaceMaterialTests.cpp b/Tests/UnitTests/Core/Material/HomogeneousSurfaceMaterialTests.cpp
@@ -57,7 +57,7 @@ BOOST_AUTO_TEST_CASE(HomogeneousSurfaceMaterial_scaling_test) {
   HomogeneousSurfaceMaterial hsm(mat, 1.);
   hsm *= 0.5;
 
-  auto matBin = hsm.materialSlab(0, 0);
+  auto matBin = hsm.materialSlab(Vector3(0., 0., 0.));
 
   BOOST_CHECK_EQUAL(matBin, matHalf);
   BOOST_CHECK_NE(matBin, mat);
@@ -79,12 +79,10 @@ BOOST_AUTO_TEST_CASE(HomogeneousSurfaceMaterial_access_test) {
 
   auto mat2d = hsmfwd.materialSlab(Vector2{0., 0.});
   auto mat3d = hsmfwd.materialSlab(Vector3{0., 0., 0.});
-  auto matbin = hsmfwd.materialSlab(0, 0);
 
   // Test equality of the copy
   BOOST_CHECK_EQUAL(mat, mat2d);
   BOOST_CHECK_EQUAL(mat, mat3d);
-  BOOST_CHECK_EQUAL(mat, matbin);
 
   Direction fDir = Direction::Forward;
   Direction bDir = Direction::Backward;

diff --git a/Tests/UnitTests/Core/Material/ISurfaceMaterialTests.cpp b/Tests/UnitTests/Core/Material/ISurfaceMaterialTests.cpp
@@ -34,11 +34,6 @@ class SurfaceMaterialStub : public ISurfaceMaterial {
     return m_fullMaterial;
   }
 
-  const MaterialSlab& materialSlab(std::size_t /*bin0*/,
-                                   std::size_t /*bin1*/) const override {
-    return m_fullMaterial;
-  }
-
   std::ostream& toStream(std::ostream& sl) const override {
     sl << "SurfaceMaterialStub";
     return sl;