refactor: Use enum class for MaterialUpdateStage (#1205)

This change is motivated by arithmetic between non-class enums and other types that are not the same enum being deprecated in C++20. This mainly affects `NavigationDirection`, but I took the chance to update the `MaterialUpdateStage` enum to make it an enum class.
acts-project · Mar 30, 2022 · 8016339 · 8016339
1 parent f7c32a8
commit 8016339
Show file tree

Hide file tree

Showing 13 changed files with 179 additions and 39 deletions.
diff --git a/Core/CMakeLists.txt b/Core/CMakeLists.txt
@@ -52,6 +52,7 @@ install(
 # target source files are added separately
 add_subdirectory(src/EventData)
 add_subdirectory(src/Digitization)
+add_subdirectory(src/Definitions)
 add_subdirectory(src/Geometry)
 add_subdirectory(src/MagneticField)
 add_subdirectory(src/Material)

diff --git a/Core/include/Acts/Definitions/Common.hpp b/Core/include/Acts/Definitions/Common.hpp
@@ -10,6 +10,7 @@
 
 #include "Acts/Definitions/Algebra.hpp"
 
+#include <iosfwd>
 #include <limits>
 
 namespace Acts {
@@ -36,15 +37,17 @@ static constexpr ActsScalar s_curvilinearProjTolerance = 0.999995;
 enum NavigationDirection : int { backward = -1, forward = 1 };
 
 ///  This is a steering enum to tell which material update stage:
-/// - preUpdate  : update on approach of a surface
-/// - fullUpdate : update when passing a surface
-/// - postUpdate : update when leaving a surface
-enum MaterialUpdateStage : int {
-  preUpdate = -1,
-  fullUpdate = 0,
-  postUpdate = 1
+/// - PreUpdate  : update on approach of a surface
+/// - FullUpdate : update when passing a surface
+/// - PostUpdate : update when leaving a surface
+enum class MaterialUpdateStage : int {
+  PreUpdate = -1,
+  FullUpdate = 0,
+  PostUpdate = 1
 };
 
+std::ostream& operator<<(std::ostream& os, MaterialUpdateStage matUpdate);
+
 /// @enum NoiseUpdateMode to tell how to deal with noise term in covariance
 /// transport
 /// - removeNoise: subtract noise term

diff --git a/Core/include/Acts/Material/ISurfaceMaterial.hpp b/Core/include/Acts/Material/ISurfaceMaterial.hpp
@@ -130,10 +130,15 @@ class ISurfaceMaterial {
 
 inline double ISurfaceMaterial::factor(NavigationDirection pDir,
                                        MaterialUpdateStage mStage) const {
-  if (mStage == Acts::fullUpdate) {
+  if (mStage == Acts::MaterialUpdateStage::FullUpdate) {
     return 1.;
+  } else if (mStage == Acts::MaterialUpdateStage::PreUpdate) {
+    return pDir == NavigationDirection::backward ? m_splitFactor
+                                                 : 1 - m_splitFactor;
+  } else /*if (mStage == Acts::MaterialUpdateStage::PostUpdate)*/ {
+    return pDir == NavigationDirection::forward ? m_splitFactor
+                                                : 1 - m_splitFactor;
   }
-  return (pDir * mStage > 0 ? m_splitFactor : 1. - m_splitFactor);
 }
 
 inline MaterialSlab ISurfaceMaterial::materialSlab(

diff --git a/Core/include/Acts/Material/MaterialCollector.hpp b/Core/include/Acts/Material/MaterialCollector.hpp
@@ -83,13 +83,13 @@ struct MaterialCollector {
             ACTS_VERBOSE("Update on start surface: post-update mode.");
             prepofu =
                 state.navigation.currentSurface->surfaceMaterial()->factor(
-                    state.stepping.navDir, postUpdate);
+                    state.stepping.navDir, MaterialUpdateStage::PostUpdate);
           } else if (state.navigation.targetSurface ==
                      state.navigation.currentSurface) {
             ACTS_VERBOSE("Update on target surface: pre-update mode");
             prepofu =
                 state.navigation.currentSurface->surfaceMaterial()->factor(
-                    state.stepping.navDir, preUpdate);
+                    state.stepping.navDir, MaterialUpdateStage::PreUpdate);
           } else {
             ACTS_VERBOSE("Update while pass through: full mode.");
           }

diff --git a/Core/include/Acts/Propagator/detail/PointwiseMaterialInteraction.hpp b/Core/include/Acts/Propagator/detail/PointwiseMaterialInteraction.hpp
@@ -88,14 +88,15 @@ struct PointwiseMaterialInteraction {
   ///
   /// @return Boolean statement whether the material is valid
   template <typename propagator_state_t>
-  bool evaluateMaterialSlab(const propagator_state_t& state,
-                            MaterialUpdateStage updateStage = fullUpdate) {
+  bool evaluateMaterialSlab(
+      const propagator_state_t& state,
+      MaterialUpdateStage updateStage = MaterialUpdateStage::FullUpdate) {
     // We are at the start surface
     if (surface == state.navigation.startSurface) {
-      updateStage = postUpdate;
+      updateStage = MaterialUpdateStage::PostUpdate;
       // Or is it the target surface ?
     } else if (surface == state.navigation.targetSurface) {
-      updateStage = preUpdate;
+      updateStage = MaterialUpdateStage::PreUpdate;
     }
 
     // Retrieve the material properties

diff --git a/Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp b/Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp
@@ -555,7 +555,8 @@ class CombinatorialKalmanFilter {
         stepper.transportCovarianceToBound(state.stepping, *surface);
 
         // Update state and stepper with pre material effects
-        materialInteractor(surface, state, stepper, preUpdate);
+        materialInteractor(surface, state, stepper,
+                           MaterialUpdateStage::PreUpdate);
 
         // Bind the transported state to the current surface
         auto boundStateRes =
@@ -626,7 +627,8 @@ class CombinatorialKalmanFilter {
                              << result.activeTips.back().first);
         }
         // Update state and stepper with post material effects
-        materialInteractor(surface, state, stepper, postUpdate);
+        materialInteractor(surface, state, stepper,
+                           MaterialUpdateStage::PostUpdate);
       } else if (surface->associatedDetectorElement() != nullptr ||
                  surface->surfaceMaterial() != nullptr) {
         // No splitting on the surface without source links. Set it to one
@@ -703,7 +705,8 @@ class CombinatorialKalmanFilter {
         }
         if (surface->surfaceMaterial() != nullptr) {
           // Update state and stepper with material effects
-          materialInteractor(surface, state, stepper, fullUpdate);
+          materialInteractor(surface, state, stepper,
+                             MaterialUpdateStage::FullUpdate);
         }
       } else {
         // Neither measurement nor material on surface, this branch is still
@@ -972,9 +975,10 @@ class CombinatorialKalmanFilter {
     /// @param updateStage The materal update stage
     ///
     template <typename propagator_state_t, typename stepper_t>
-    void materialInteractor(
-        const Surface* surface, propagator_state_t& state, stepper_t& stepper,
-        const MaterialUpdateStage& updateStage = fullUpdate) const {
+    void materialInteractor(const Surface* surface, propagator_state_t& state,
+                            stepper_t& stepper,
+                            const MaterialUpdateStage& updateStage =
+                                MaterialUpdateStage::FullUpdate) const {
       const auto& logger = state.options.logger;
       // Indicator if having material
       bool hasMaterial = false;

diff --git a/Core/include/Acts/TrackFitting/KalmanFitter.hpp b/Core/include/Acts/TrackFitting/KalmanFitter.hpp
@@ -528,7 +528,8 @@ class KalmanFitter {
         stepper.transportCovarianceToBound(state.stepping, *surface);
 
         // Update state and stepper with pre material effects
-        materialInteractor(surface, state, stepper, preUpdate);
+        materialInteractor(surface, state, stepper,
+                           MaterialUpdateStage::PreUpdate);
 
         // do the kalman update
         auto trackStateProxyRes = detail::kalmanHandleMeasurement(
@@ -559,7 +560,8 @@ class KalmanFitter {
         }
 
         // Update state and stepper with post material effects
-        materialInteractor(surface, state, stepper, postUpdate);
+        materialInteractor(surface, state, stepper,
+                           MaterialUpdateStage::PostUpdate);
         // We count the processed state
         ++result.processedStates;
         // Update the number of holes count only when encoutering a
@@ -596,7 +598,8 @@ class KalmanFitter {
         }
         if (surface->surfaceMaterial() != nullptr) {
           // Update state and stepper with material effects
-          materialInteractor(surface, state, stepper, fullUpdate);
+          materialInteractor(surface, state, stepper,
+                             MaterialUpdateStage::FullUpdate);
         }
       }
       return Result<void>::success();
@@ -636,7 +639,8 @@ class KalmanFitter {
         stepper.transportCovarianceToBound(state.stepping, *surface);
 
         // Update state and stepper with pre material effects
-        materialInteractor(surface, state, stepper, preUpdate);
+        materialInteractor(surface, state, stepper,
+                           MaterialUpdateStage::PreUpdate);
 
         // Bind the transported state to the current surface
         auto res = stepper.boundState(state.stepping, *surface, false);
@@ -708,7 +712,8 @@ class KalmanFitter {
                          trackStateProxy.filteredCovariance(), *surface);
 
           // Update state and stepper with post material effects
-          materialInteractor(surface, state, stepper, postUpdate);
+          materialInteractor(surface, state, stepper,
+                             MaterialUpdateStage::PostUpdate);
         }
       } else if (surface->associatedDetectorElement() != nullptr ||
                  surface->surfaceMaterial() != nullptr) {
@@ -749,9 +754,10 @@ class KalmanFitter {
     /// @param updateStage The materal update stage
     ///
     template <typename propagator_state_t, typename stepper_t>
-    void materialInteractor(
-        const Surface* surface, propagator_state_t& state, stepper_t& stepper,
-        const MaterialUpdateStage& updateStage = fullUpdate) const {
+    void materialInteractor(const Surface* surface, propagator_state_t& state,
+                            stepper_t& stepper,
+                            const MaterialUpdateStage& updateStage =
+                                MaterialUpdateStage::FullUpdate) const {
       const auto& logger = state.options.logger;
       // Indicator if having material
       bool hasMaterial = false;

diff --git a/Core/include/Acts/TrackFitting/detail/GsfActor.hpp b/Core/include/Acts/TrackFitting/detail/GsfActor.hpp
@@ -260,9 +260,11 @@ struct GsfActor {
 
       if (haveMaterial) {
         if (haveMeasurement) {
-          applyMultipleScattering(state, stepper, preUpdate);
+          applyMultipleScattering(state, stepper,
+                                  MaterialUpdateStage::PreUpdate);
         } else {
-          applyMultipleScattering(state, stepper, fullUpdate);
+          applyMultipleScattering(state, stepper,
+                                  MaterialUpdateStage::FullUpdate);
         }
       }
 
@@ -300,7 +302,8 @@ struct GsfActor {
 
       // If we only done preUpdate before, now do postUpdate
       if (haveMaterial && haveMeasurement) {
-        applyMultipleScattering(state, stepper, postUpdate);
+        applyMultipleScattering(state, stepper,
+                                MaterialUpdateStage::PostUpdate);
       }
     }
   }
@@ -341,7 +344,7 @@ struct GsfActor {
     // Evaluate material slab
     auto slab = surface.surfaceMaterial()->materialSlab(
         old_bound.position(state.stepping.geoContext), state.stepping.navDir,
-        MaterialUpdateStage::fullUpdate);
+        MaterialUpdateStage::FullUpdate);
 
     auto pathCorrection =
         surface.pathCorrection(state.stepping.geoContext,
@@ -689,9 +692,10 @@ struct GsfActor {
 
   /// Apply the multipe scattering to the state
   template <typename propagator_state_t, typename stepper_t>
-  void applyMultipleScattering(
-      propagator_state_t& state, const stepper_t& stepper,
-      const MaterialUpdateStage& updateStage = fullUpdate) const {
+  void applyMultipleScattering(propagator_state_t& state,
+                               const stepper_t& stepper,
+                               const MaterialUpdateStage& updateStage =
+                                   MaterialUpdateStage::FullUpdate) const {
     const auto& logger = state.options.logger;
     const auto& surface = *state.navigation.currentSurface;
 

diff --git a/Core/src/Definitions/CMakeLists.txt b/Core/src/Definitions/CMakeLists.txt
@@ -0,0 +1,5 @@
+target_sources(
+  ActsCore
+  PRIVATE
+    Common.cpp
+)
diff --git a/Core/src/Definitions/Common.cpp b/Core/src/Definitions/Common.cpp
@@ -0,0 +1,33 @@
+// This file is part of the Acts project.
+//
+// Copyright (C) 2022 CERN for the benefit of the Acts project
+//
+// This Source Code Form is subject to the terms of the Mozilla Public
+// License, v. 2.0. If a copy of the MPL was not distributed with this
+// file, You can obtain one at http://mozilla.org/MPL/2.0/.
+
+#include "Acts/Definitions/Common.hpp"
+
+#include <ostream>
+
+namespace Acts {
+
+std::ostream& operator<<(std::ostream& os, MaterialUpdateStage matUpdate) {
+  switch (matUpdate) {
+    case MaterialUpdateStage::PreUpdate:
+      os << "PreUpdate (-1)";
+      break;
+    case MaterialUpdateStage::PostUpdate:
+      os << "PostUpdate (1)";
+      break;
+    case MaterialUpdateStage::FullUpdate:
+      os << "FullUpdate (0)";
+      break;
+    default:
+      assert(false && "Invalid material update stage (shouldn't happen)");
+      std::abort();
+  }
+  return os;
+}
+
+}  // namespace Acts
diff --git a/Tests/UnitTests/Core/Material/CMakeLists.txt b/Tests/UnitTests/Core/Material/CMakeLists.txt
@@ -15,3 +15,4 @@ add_unittest(ProtoSurfaceMaterial ProtoSurfaceMaterialTests.cpp)
 add_unittest(ProtoVolumeMaterial ProtoVolumeMaterialTests.cpp)
 add_unittest(SurfaceMaterialMapper SurfaceMaterialMapperTests.cpp)
 add_unittest(VolumeMaterialMapper VolumeMaterialMapperTests.cpp)
+add_unittest(ISurfaceMaterial ISurfaceMaterialTests.cpp)
diff --git a/Tests/UnitTests/Core/Material/HomogeneousSurfaceMaterialTests.cpp b/Tests/UnitTests/Core/Material/HomogeneousSurfaceMaterialTests.cpp
@@ -86,9 +86,9 @@ BOOST_AUTO_TEST_CASE(HomogeneousSurfaceMaterial_access_test) {
   NavigationDirection fDir = forward;
   NavigationDirection bDir = backward;
 
-  MaterialUpdateStage pre = preUpdate;
-  MaterialUpdateStage full = fullUpdate;
-  MaterialUpdateStage post = postUpdate;
+  MaterialUpdateStage pre = MaterialUpdateStage::PreUpdate;
+  MaterialUpdateStage full = MaterialUpdateStage::FullUpdate;
+  MaterialUpdateStage post = MaterialUpdateStage::PostUpdate;
 
   // (a) Forward factor material test
   BOOST_CHECK_EQUAL(hsmfwd.factor(fDir, full), 1.);

diff --git a/Tests/UnitTests/Core/Material/ISurfaceMaterialTests.cpp b/Tests/UnitTests/Core/Material/ISurfaceMaterialTests.cpp
@@ -0,0 +1,77 @@
+// This file is part of the Acts project.
+//
+// Copyright (C) 2022 CERN for the benefit of the Acts project
+//
+// This Source Code Form is subject to the terms of the Mozilla Public
+// License, v. 2.0. If a copy of the MPL was not distributed with this
+// file, You can obtain one at http://mozilla.org/MPL/2.0/.
+
+#include <boost/test/unit_test.hpp>
+
+#include "Acts/Definitions/Algebra.hpp"
+#include "Acts/Material/ISurfaceMaterial.hpp"
+
+#include <climits>
+
+namespace Acts {
+
+namespace Test {
+
+class SurfaceMaterialStub : public ISurfaceMaterial {
+  using ISurfaceMaterial::ISurfaceMaterial;
+
+  ISurfaceMaterial& operator*=(double /*scale*/) override { return *this; };
+
+  const MaterialSlab& materialSlab(const Vector2& /*lp*/) const override {
+    return m_fullMaterial;
+  }
+
+  const MaterialSlab& materialSlab(const Vector3& /*gp*/) const override {
+    return m_fullMaterial;
+  }
+
+  const MaterialSlab& materialSlab(size_t /*bin0*/,
+                                   size_t /*bin1*/) const override {
+    return m_fullMaterial;
+  }
+
+  std::ostream& toStream(std::ostream& sl) const override {
+    sl << "SurfaceMaterialStub";
+    return sl;
+  };
+
+  MaterialSlab m_fullMaterial{};
+};
+
+/// Test the constructors
+BOOST_AUTO_TEST_CASE(ISurfaceMaterial_factor_test) {
+  double splitFactor = 42.0;
+  SurfaceMaterialStub stub{splitFactor};
+
+  BOOST_CHECK_EQUAL(stub.factor(NavigationDirection::forward,
+                                MaterialUpdateStage::FullUpdate),
+                    1.0);
+
+  BOOST_CHECK_EQUAL(stub.factor(NavigationDirection::backward,
+                                MaterialUpdateStage::FullUpdate),
+                    1.0);
+
+  BOOST_CHECK_EQUAL(stub.factor(NavigationDirection::forward,
+                                MaterialUpdateStage::PostUpdate),
+                    splitFactor);
+
+  BOOST_CHECK_EQUAL(stub.factor(NavigationDirection::backward,
+                                MaterialUpdateStage::PreUpdate),
+                    splitFactor);
+
+  BOOST_CHECK_EQUAL(
+      stub.factor(NavigationDirection::forward, MaterialUpdateStage::PreUpdate),
+      1 - splitFactor);
+
+  BOOST_CHECK_EQUAL(stub.factor(NavigationDirection::backward,
+                                MaterialUpdateStage::PostUpdate),
+                    1 - splitFactor);
+}
+
+}  // namespace Test
+}  // namespace Acts