refactor: Release step constraint after stepper in propagation (#2596)

This avoids cases where an actor/aborter sets a step constrained that will never be overwritten again. I also generalize the `release` function and rename the `set` function in the stepper. blocked by - #2599
acts-project · Nov 21, 2023 · 37a2f9e · 37a2f9e
1 parent e574e8a
commit 37a2f9e
Show file tree

Hide file tree

Showing 19 changed files with 67 additions and 64 deletions.
diff --git a/CI/physmon/reference/performance_gsf.root b/CI/physmon/reference/performance_gsf.root
diff --git a/Core/include/Acts/Propagator/AtlasStepper.hpp b/Core/include/Acts/Propagator/AtlasStepper.hpp
@@ -422,15 +422,14 @@ class AtlasStepper {
     detail::updateSingleStepSize<AtlasStepper>(state, oIntersection, release);
   }
 
-  /// Set Step size - explicitly with a double
+  /// Update step size - explicitly with a double
   ///
   /// @param [in,out] state The stepping state (thread-local cache)
   /// @param [in] stepSize The step size value
   /// @param [in] stype The step size type to be set
   /// @param release [in] Do we release the step size?
-  void setStepSize(State& state, double stepSize,
-                   ConstrainedStep::Type stype = ConstrainedStep::actor,
-                   bool release = true) const {
+  void updateStepSize(State& state, double stepSize,
+                      ConstrainedStep::Type stype, bool release = true) const {
     state.previousStepSize = state.stepSize.value();
     state.stepSize.update(stepSize, stype, release);
   }
@@ -446,8 +445,9 @@ class AtlasStepper {
   /// Release the Step size
   ///
   /// @param [in,out] state The stepping state (thread-local cache)
-  void releaseStepSize(State& state) const {
-    state.stepSize.release(ConstrainedStep::actor);
+  /// @param [in] stype The step size type to be released
+  void releaseStepSize(State& state, ConstrainedStep::Type stype) const {
+    state.stepSize.release(stype);
   }
 
   /// Output the Step Size - single component

diff --git a/Core/include/Acts/Propagator/DirectNavigator.hpp b/Core/include/Acts/Propagator/DirectNavigator.hpp
@@ -331,7 +331,6 @@ class DirectNavigator {
             state.navigation.navSurfaces.end()) {
           ACTS_VERBOSE("Next surface candidate is  "
                        << (*state.navigation.navSurfaceIter)->geometryId());
-          stepper.releaseStepSize(state.stepping);
         }
       } else if (surfaceStatus == Intersection3D::Status::reachable) {
         ACTS_VERBOSE("Next surface reachable at distance  "

diff --git a/Core/include/Acts/Propagator/EigenStepper.hpp b/Core/include/Acts/Propagator/EigenStepper.hpp
@@ -272,15 +272,14 @@ class EigenStepper {
     detail::updateSingleStepSize<EigenStepper>(state, oIntersection, release);
   }
 
-  /// Set Step size - explicitly with a double
+  /// Update step size - explicitly with a double
   ///
   /// @param state [in,out] The stepping state (thread-local cache)
   /// @param stepSize [in] The step size value
   /// @param stype [in] The step size type to be set
   /// @param release [in] Do we release the step size?
-  void setStepSize(State& state, double stepSize,
-                   ConstrainedStep::Type stype = ConstrainedStep::actor,
-                   bool release = true) const {
+  void updateStepSize(State& state, double stepSize,
+                      ConstrainedStep::Type stype, bool release = true) const {
     state.previousStepSize = state.stepSize.value();
     state.stepSize.update(stepSize, stype, release);
   }
@@ -296,8 +295,9 @@ class EigenStepper {
   /// Release the Step size
   ///
   /// @param state [in,out] The stepping state (thread-local cache)
-  void releaseStepSize(State& state) const {
-    state.stepSize.release(ConstrainedStep::actor);
+  /// @param [in] stype The step size type to be released
+  void releaseStepSize(State& state, ConstrainedStep::Type stype) const {
+    state.stepSize.release(stype);
   }
 
   /// Output the Step Size - single component

diff --git a/Core/include/Acts/Propagator/MultiEigenStepperLoop.hpp b/Core/include/Acts/Propagator/MultiEigenStepperLoop.hpp
@@ -675,17 +675,16 @@ class MultiEigenStepperLoop
     }
   }
 
-  /// Set Step size - explicitly with a double
+  /// Update step size - explicitly with a double
   ///
   /// @param state [in,out] The stepping state (thread-local cache)
   /// @param stepSize [in] The step size value
   /// @param stype [in] The step size type to be set
   /// @param release [in] Do we release the step size?
-  void setStepSize(State& state, double stepSize,
-                   ConstrainedStep::Type stype = ConstrainedStep::actor,
-                   bool release = true) const {
+  void updateStepSize(State& state, double stepSize,
+                      ConstrainedStep::Type stype, bool release = true) const {
     for (auto& component : state.components) {
-      SingleStepper::setStepSize(component.state, stepSize, stype, release);
+      SingleStepper::updateStepSize(component.state, stepSize, stype, release);
     }
   }
 
@@ -708,9 +707,10 @@ class MultiEigenStepperLoop
   /// Release the step-size for all components
   ///
   /// @param state [in,out] The stepping state (thread-local cache)
-  void releaseStepSize(State& state) const {
+  /// @param [in] stype The step size type to be released
+  void releaseStepSize(State& state, ConstrainedStep::Type stype) const {
     for (auto& component : state.components) {
-      SingleStepper::releaseStepSize(component.state);
+      SingleStepper::releaseStepSize(component.state, stype);
     }
   }
 

diff --git a/Core/include/Acts/Propagator/Navigator.hpp b/Core/include/Acts/Propagator/Navigator.hpp
@@ -420,7 +420,7 @@ class Navigator {
                    << "No further navigation action, proceed to target.");
       // Set navigation break and release the navigation step size
       state.navigation.navigationBreak = true;
-      stepper.releaseStepSize(state.stepping);
+      stepper.releaseStepSize(state.stepping, ConstrainedStep::actor);
     }
 
     // Navigator target always resets the current surface
@@ -536,7 +536,6 @@ class Navigator {
               << "No more volume to progress to, stopping navigation.");
           // Navigation break & release navigation stepping
           state.navigation.navigationBreak = true;
-          stepper.releaseStepSize(state.stepping);
           return;
         } else {
           ACTS_VERBOSE(volInfo(state) << "Volume updated.");
@@ -562,7 +561,6 @@ class Navigator {
       }
       // Set navigation break and release the navigation step size
       state.navigation.navigationBreak = true;
-      stepper.releaseStepSize(state.stepping);
     } else {
       ACTS_VERBOSE(volInfo(state)
                    << "Status could not be determined - good luck.");
@@ -649,7 +647,7 @@ class Navigator {
                      << "Start is target layer, nothing left to do.");
         // set the navigation break
         state.navigation.navigationBreak = true;
-        stepper.releaseStepSize(state.stepping);
+        stepper.releaseStepSize(state.stepping, ConstrainedStep::actor);
       }
       return startResolved;
     }
@@ -934,15 +932,15 @@ class Navigator {
       ACTS_VERBOSE(volInfo(state)
                    << "No sufficient information to resolve boundary, "
                       "stopping navigation.");
-      stepper.releaseStepSize(state.stepping);
+      stepper.releaseStepSize(state.stepping, ConstrainedStep::actor);
       return false;
     } else if (state.navigation.currentVolume ==
                state.navigation.targetVolume) {
       ACTS_VERBOSE(volInfo(state)
                    << "In target volume: no need to resolve boundary, "
                       "stopping navigation.");
       state.navigation.navigationBreak = true;
-      stepper.releaseStepSize(state.stepping);
+      stepper.releaseStepSize(state.stepping, ConstrainedStep::actor);
       return true;
     }
 
@@ -1275,7 +1273,7 @@ class Navigator {
     // Screen output - no layer candidates found
     ACTS_VERBOSE(volInfo(state) << "No compatible layer candidates found.");
     // Release the step size
-    stepper.releaseStepSize(state.stepping);
+    stepper.releaseStepSize(state.stepping, ConstrainedStep::actor);
     return false;
   }
 

diff --git a/Core/include/Acts/Propagator/Propagator.ipp b/Core/include/Acts/Propagator/Propagator.ipp
@@ -7,6 +7,7 @@
 // file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 #include "Acts/EventData/TrackParametersConcept.hpp"
+#include "Acts/Propagator/ConstrainedStep.hpp"
 #include "Acts/Propagator/PropagatorError.hpp"
 #include "Acts/Propagator/detail/LoopProtection.hpp"
 
@@ -58,6 +59,9 @@ auto Acts::Propagator<S, N>::propagate_impl(propagator_state_t& state,
         // pass error to caller
         return res.error();
       }
+      // release actor and aborter constrains after step was performed
+      m_stepper.releaseStepSize(state.stepping, ConstrainedStep::actor);
+      m_stepper.releaseStepSize(state.stepping, ConstrainedStep::aborter);
       // Post-stepping:
       // navigator post step call - action list - aborter list
       state.stage = PropagatorStage::postStep;

diff --git a/Core/include/Acts/Propagator/StandardAborters.hpp b/Core/include/Acts/Propagator/StandardAborters.hpp
@@ -58,8 +58,8 @@ struct PathLimitReached {
       navigator.targetReached(state.navigation, true);
       return true;
     }
-    stepper.setStepSize(state.stepping, distance, ConstrainedStep::aborter,
-                        false);
+    stepper.updateStepSize(state.stepping, distance, ConstrainedStep::aborter,
+                           false);
     ACTS_VERBOSE("PathLimit aborter | "
                  << "Target stepSize (path limit) updated to "
                  << stepper.outputStepSize(state.stepping));
@@ -126,8 +126,8 @@ struct SurfaceReached {
       if (intersection &&
           detail::checkIntersection(intersection.intersection(), pLimit, oLimit,
                                     tolerance, logger)) {
-        stepper.setStepSize(state.stepping, intersection.pathLength(),
-                            ConstrainedStep::aborter, false);
+        stepper.updateStepSize(state.stepping, intersection.pathLength(),
+                               ConstrainedStep::aborter, false);
         ACTS_VERBOSE(
             "SurfaceReached aborter | "
             "Target stepSize (surface) updated to "

diff --git a/Core/include/Acts/Propagator/StepperConcept.hpp b/Core/include/Acts/Propagator/StepperConcept.hpp
@@ -54,7 +54,7 @@ METHOD_TRAIT(covariance_transport_curvilinear_t,
              transportCovarianceToCurvilinear);
 METHOD_TRAIT(step_t, step);
 METHOD_TRAIT(update_surface_status_t, updateSurfaceStatus);
-METHOD_TRAIT(set_step_size_t, setStepSize);
+METHOD_TRAIT(update_step_size_t, updateStepSize);
 METHOD_TRAIT(get_step_size_t, getStepSize);
 METHOD_TRAIT(release_step_size_t, releaseStepSize);
 METHOD_TRAIT(output_step_size_t, outputStepSize);
@@ -126,11 +126,11 @@ constexpr bool MultiStepperStateConcept= require<
         static_assert(covariance_transport_exists, "covarianceTransport method not found");
         constexpr static bool update_surface_exists = has_method<const S, Intersection3D::Status, update_surface_status_t, state&, const Surface&, std::uint8_t, Direction, const BoundaryCheck&, ActsScalar, const Logger&>;
         static_assert(update_surface_exists, "updateSurfaceStatus method not found");
-        constexpr static bool set_step_size_exists = has_method<const S, void, set_step_size_t, state&, double, ConstrainedStep::Type, bool>;
-        static_assert(set_step_size_exists, "setStepSize method not found");
+        constexpr static bool update_step_size_exists = has_method<const S, void, update_step_size_t, state&, double, ConstrainedStep::Type, bool>;
+        static_assert(update_step_size_exists, "updateStepSize method not found");
         constexpr static bool get_step_size_exists = has_method<const S, double, get_step_size_t, const state &, ConstrainedStep::Type>;
         static_assert(get_step_size_exists, "getStepSize method not found");
-        constexpr static bool release_step_size_exists = has_method<const S, void, release_step_size_t, state&>;
+        constexpr static bool release_step_size_exists = has_method<const S, void, release_step_size_t, state&, ConstrainedStep::Type>;
         static_assert(release_step_size_exists, "releaseStepSize method not found");
         constexpr static bool output_step_size_exists = has_method<const S, std::string, output_step_size_t, const state&>;
         static_assert(output_step_size_exists, "outputStepSize method not found");
@@ -151,7 +151,7 @@ constexpr bool MultiStepperStateConcept= require<
                                               curvilinear_state_method_exists,
                                               covariance_transport_exists,
                                               update_surface_exists,
-                                              set_step_size_exists,
+                                              update_step_size_exists,
                                               release_step_size_exists,
                                               output_step_size_exists>;
 

diff --git a/Core/include/Acts/Propagator/StraightLineStepper.hpp b/Core/include/Acts/Propagator/StraightLineStepper.hpp
@@ -263,15 +263,15 @@ class StraightLineStepper {
                                                       release);
   }
 
-  /// Set Step size - explicitly with a double
+  /// Update step size - explicitly with a double
   ///
   /// @param state [in,out] The stepping state (thread-local cache)
   /// @param stepSize [in] The step size value
   /// @param stype [in] The step size type to be set
   /// @param release [in] Do we release the step size?
-  void setStepSize(State& state, double stepSize,
-                   ConstrainedStep::Type stype = ConstrainedStep::actor,
-                   bool release = true) const {
+  void updateStepSize(State& state, double stepSize,
+                      ConstrainedStep::Type stype = ConstrainedStep::actor,
+                      bool release = true) const {
     state.previousStepSize = state.stepSize.value();
     state.stepSize.update(stepSize, stype, release);
   }
@@ -286,9 +286,10 @@ class StraightLineStepper {
 
   /// Release the Step size
   ///
-  /// @param state [in,out] The stepping state (thread-local cache)
-  void releaseStepSize(State& state) const {
-    state.stepSize.release(ConstrainedStep::actor);
+  /// @param [in,out] state The stepping state (thread-local cache)
+  /// @param [in] stype The step size type to be released
+  void releaseStepSize(State& state, ConstrainedStep::Type stype) const {
+    state.stepSize.release(stype);
   }
 
   /// Output the Step Size - single component

diff --git a/Core/include/Acts/Propagator/detail/SteppingHelper.hpp b/Core/include/Acts/Propagator/detail/SteppingHelper.hpp
@@ -59,7 +59,8 @@ Acts::Intersection3D::Status updateSingleSurfaceStatus(
       detail::checkIntersection(sIntersection.intersection(), pLimit, oLimit,
                                 surfaceTolerance, logger)) {
     ACTS_VERBOSE("Surface is reachable");
-    stepper.setStepSize(state, sIntersection.pathLength());
+    stepper.updateStepSize(state, sIntersection.pathLength(),
+                           ConstrainedStep::actor);
     return Intersection3D::Status::reachable;
   }
 

diff --git a/Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp b/Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp
@@ -395,7 +395,7 @@ class CombinatorialKalmanFilter {
       if (!result.filtered &&
           filterTargetReached(state, stepper, navigator, logger())) {
         navigator.navigationBreak(state.navigation, true);
-        stepper.releaseStepSize(state.stepping);
+        stepper.releaseStepSize(state.stepping, ConstrainedStep::actor);
       }
 
       // Update:

diff --git a/Examples/Python/tests/root_file_hashes.txt b/Examples/Python/tests/root_file_hashes.txt
@@ -50,21 +50,21 @@ test_digitization_example_input[smeared]__measurements.root: 2e4fd9d3e6244e53486
 test_digitization_example_input[geometric]__particles.root: 8549ba6e20338004ab8ba299fc65e1ee5071985b46df8f77f887cb6fef56a8ec
 test_digitization_example_input[geometric]__measurements.root: 20128357c9238926d628d18b3b91a0a73947b2b5682762bf5d8fd94342faf976
 test_ckf_tracks_example[generic-full_seeding]__trackstates_ckf.root: 5ddcc228073553ba14205897451951ea475541643bb677e7718bd3369b484bcd
-test_ckf_tracks_example[generic-full_seeding]__tracksummary_ckf.root: 6ccdd72caf4d645debea57fc823d5ceedb74c70b9832b3a906ae4a06ac8fe796
+test_ckf_tracks_example[generic-full_seeding]__tracksummary_ckf.root: afa6eaffe366533d343c78685015a10f71f8f49b318431878a1bb49098bcbfc4
 test_ckf_tracks_example[generic-full_seeding]__performance_seeding_trees.root: 0e0676ffafdb27112fbda50d1cf627859fa745760f98073261dcf6db3f2f991e
 test_ckf_tracks_example[generic-truth_estimated]__trackstates_ckf.root: 10ecbc093239670b0a116fad5a25caf941f2a401a03892a6853652d20ee98a67
 test_ckf_tracks_example[generic-truth_estimated]__tracksummary_ckf.root: 068c236fd9f52801eb39e72498c7ec37857523a617d77b5987db4bfb736c6f88
 test_ckf_tracks_example[generic-truth_estimated]__performance_seeding.root: 1facb05c066221f6361b61f015cdf0918e94d9f3fce2269ec7b6a4dffeb2bc7e
 test_ckf_tracks_example[generic-truth_smeared]__trackstates_ckf.root: 4c081f8d984b7a9eb680ddc870d03ada2f3bbbb6fb0e41877720e449b82b87d8
 test_ckf_tracks_example[generic-truth_smeared]__tracksummary_ckf.root: 3ac06b38c59d673099dd4474320423932c84eea47b226fad887ed1c2c83d18c1
 test_ckf_tracks_example[odd-full_seeding]__trackstates_ckf.root: f8511a029465842e39894bbb6054bc330747e0948e686787aa9f90fea2e59102
-test_ckf_tracks_example[odd-full_seeding]__tracksummary_ckf.root: 24eea0bc0c0c2a790fbf8038e0bd84c62325bccc9c9bc580f66c27e7a35654c3
+test_ckf_tracks_example[odd-full_seeding]__tracksummary_ckf.root: fd2e99f7fef639aeb55991a55d31310f3a2e173c04d81010e4f60aba850be68d
 test_ckf_tracks_example[odd-full_seeding]__performance_seeding_trees.root: 43c58577aafe07645e5660c4f43904efadf91d8cda45c5c04c248bbe0f59814f
 test_ckf_tracks_example[odd-truth_estimated]__trackstates_ckf.root: f244ea1201fedaec0619d595945d33d976fc1e09e8d218e16ea0737f35debe0f
-test_ckf_tracks_example[odd-truth_estimated]__tracksummary_ckf.root: f7b683c6c0c8a57dd61bd3ba2965bf5f689a6eb9c2e8c8e8e13a7907ea7411a3
+test_ckf_tracks_example[odd-truth_estimated]__tracksummary_ckf.root: 91ce6097ba491dd83529e60a08fb97459f2e221af553545c600db209358d299a
 test_ckf_tracks_example[odd-truth_estimated]__performance_seeding.root: 1a36b7017e59f1c08602ef3c2cb0483c51df248f112e3780c66594110719c575
 test_ckf_tracks_example[odd-truth_smeared]__trackstates_ckf.root: cee2ef40dbc341861a9abac8e4d2503240c5522d9fef6edf30fa163129f17cef
-test_ckf_tracks_example[odd-truth_smeared]__tracksummary_ckf.root: 6a8e6ef7c5a2b17b58d3c551e951a4cfb894c795e5c91df8b4499b5688020ee1
+test_ckf_tracks_example[odd-truth_smeared]__tracksummary_ckf.root: 1ec3257d7eccedd73bde17f3554e2bce1f4402a169b6003b3908d25833decd40
 test_vertex_fitting_reading[Truth-False-100]__performance_vertexing.root: 76ef6084d758dfdfc0151ddec2170e12d73394424e3dac4ffe46f0f339ec8293
 test_vertex_fitting_reading[Iterative-False-100]__performance_vertexing.root: 60372210c830a04f95ceb78c6c68a9b0de217746ff59e8e73053750c837b57eb
 test_vertex_fitting_reading[Iterative-True-100]__performance_vertexing.root: e34f217d524a5051dbb04a811d3407df3ebe2cc4bb7f54f6bda0847dbd7b52c3

diff --git a/Fatras/include/ActsFatras/Kernel/detail/SimulationActor.hpp b/Fatras/include/ActsFatras/Kernel/detail/SimulationActor.hpp
@@ -148,8 +148,8 @@ struct SimulationActor {
       const auto stepSize = properTimeDiff *
                             result.particle.absoluteMomentum() /
                             result.particle.mass();
-      stepper.setStepSize(state.stepping, stepSize,
-                          Acts::ConstrainedStep::user);
+      stepper.updateStepSize(state.stepping, stepSize,
+                             Acts::ConstrainedStep::user);
     }
 
     // arm the point-like interaction limits in the first step

diff --git a/Tests/UnitTests/Core/Propagator/AtlasStepperTests.cpp b/Tests/UnitTests/Core/Propagator/AtlasStepperTests.cpp
@@ -561,11 +561,11 @@ BOOST_AUTO_TEST_CASE(StepSize) {
   // TODO figure out why this fails and what it should be
   // BOOST_CHECK_EQUAL(stepper.overstepLimit(state), tolerance);
 
-  stepper.setStepSize(state, -5_cm);
+  stepper.updateStepSize(state, -5_cm, ConstrainedStep::actor);
   BOOST_CHECK_EQUAL(state.previousStepSize, stepSize);
   BOOST_CHECK_EQUAL(state.stepSize.value(), -5_cm);
 
-  stepper.releaseStepSize(state);
+  stepper.releaseStepSize(state, ConstrainedStep::actor);
   BOOST_CHECK_EQUAL(state.stepSize.value(), stepSize);
 }
 

diff --git a/Tests/UnitTests/Core/Propagator/EigenStepperTests.cpp b/Tests/UnitTests/Core/Propagator/EigenStepperTests.cpp
@@ -267,11 +267,11 @@ BOOST_AUTO_TEST_CASE(eigen_stepper_test) {
   // Step size modifies
   const std::string originalStepSize = esState.stepSize.toString();
 
-  es.setStepSize(esState, -1337.);
+  es.updateStepSize(esState, -1337., ConstrainedStep::actor);
   BOOST_CHECK_EQUAL(esState.previousStepSize, stepSize);
   BOOST_CHECK_EQUAL(esState.stepSize.value(), -1337.);
 
-  es.releaseStepSize(esState);
+  es.releaseStepSize(esState, ConstrainedStep::actor);
   BOOST_CHECK_EQUAL(esState.stepSize.value(), stepSize);
   BOOST_CHECK_EQUAL(es.outputStepSize(esState), originalStepSize);
 

diff --git a/Tests/UnitTests/Core/Propagator/NavigatorTests.cpp b/Tests/UnitTests/Core/Propagator/NavigatorTests.cpp
@@ -159,9 +159,9 @@ struct PropagatorState {
       detail::updateSingleStepSize<Stepper>(state, oIntersection, release);
     }
 
-    void setStepSize(State& state, double stepSize,
-                     ConstrainedStep::Type stype = ConstrainedStep::actor,
-                     bool release = true) const {
+    void updateStepSize(State& state, double stepSize,
+                        ConstrainedStep::Type stype,
+                        bool release = true) const {
       state.previousStepSize = state.stepSize.value();
       state.stepSize.update(stepSize, stype, release);
     }
@@ -170,8 +170,8 @@ struct PropagatorState {
       return state.stepSize.value(stype);
     }
 
-    void releaseStepSize(State& state) const {
-      state.stepSize.release(ConstrainedStep::actor);
+    void releaseStepSize(State& state, ConstrainedStep::Type stype) const {
+      state.stepSize.release(stype);
     }
 
     std::string outputStepSize(const State& state) const {