From ba521200fce09c3a32e11b41f4f237fe27c6aaf2 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Thu, 29 Aug 2024 20:46:11 +0200 Subject: [PATCH] refactor: Remove target volume estimation from `Navigator` (#3242) This is https://github.com/acts-project/acts/pull/3217 after it was reverted with https://github.com/acts-project/acts/pull/3241. There were a couple of failures monitored in Athena which could not be fixed right away. This is an attempt to remove (IMO) unnecessary complexity from the Navigator. We have a target volume estimation which runs a couple of times to guess the target volume based on an intersection with the target surface. IMO if we know the target surface we should also know the target volume and the user is responsible for passing this information. --- Core/include/Acts/Propagator/Navigator.hpp | 155 +----------------- .../Core/Propagator/NavigatorTests.cpp | 48 +++--- 2 files changed, 26 insertions(+), 177 deletions(-) diff --git a/Core/include/Acts/Propagator/Navigator.hpp b/Core/include/Acts/Propagator/Navigator.hpp index 007e0b7d19a..99f6a436341 100644 --- a/Core/include/Acts/Propagator/Navigator.hpp +++ b/Core/include/Acts/Propagator/Navigator.hpp @@ -168,10 +168,6 @@ class Navigator { const Layer* currentLayer = nullptr; /// Navigation state - external state: the current surface const Surface* currentSurface = nullptr; - /// Navigation state: the target volume - const TrackingVolume* targetVolume = nullptr; - /// Navigation state: the target layer - const Layer* targetLayer = nullptr; /// Navigation state: the target surface const Surface* targetSurface = nullptr; @@ -348,16 +344,7 @@ class Navigator { if (state.navigation.startLayer != nullptr) { ACTS_VERBOSE(volInfo(state) << "Start layer to be resolved."); // We provide the layer to the resolve surface method in this case - bool startResolved = resolveSurfaces(state, stepper); - if (!startResolved && - state.navigation.startLayer == state.navigation.targetLayer) { - ACTS_VERBOSE(volInfo(state) - << "Start is target layer and we have no surface " - "candidates. Nothing left to do."); - // set the navigation break - state.navigation.navigationBreak = true; - stepper.releaseStepSize(state.stepping, ConstrainedStep::actor); - } + resolveSurfaces(state, stepper); } } @@ -386,13 +373,6 @@ class Navigator { // Navigator pre step always resets the current surface state.navigation.currentSurface = nullptr; - // Initialize the target and target volume - if (state.navigation.targetSurface != nullptr && - state.navigation.targetVolume == nullptr) { - // Find out about the target as much as you can - initializeTarget(state, stepper); - } - auto tryTargetNextSurface = [&]() { // Try targeting the surfaces - then layers - then boundaries @@ -576,19 +556,6 @@ class Navigator { state.navigation.navigationStage = Stage::boundaryTarget; ACTS_VERBOSE(volInfo(state) << "Staying focussed on boundary."); } - } else if (state.navigation.currentVolume == - state.navigation.targetVolume) { - if (state.navigation.targetSurface == nullptr) { - ACTS_WARNING(volInfo(state) - << "No further navigation action, proceed to " - "target. This is very likely an error"); - } else { - ACTS_VERBOSE(volInfo(state) - << "No further navigation action, proceed to target."); - } - // Set navigation break and release the navigation step size - state.navigation.navigationBreak = true; - stepper.releaseStepSize(state.stepping, ConstrainedStep::actor); } else { ACTS_VERBOSE(volInfo(state) << "Status could not be determined - good luck."); @@ -752,12 +719,6 @@ class Navigator { } else { ACTS_VERBOSE(volInfo(state) << "Last surface on layer reached, and no layer."); - if (state.navigation.currentVolume == state.navigation.targetVolume) { - ACTS_VERBOSE(volInfo(state) - << "This is the target volume, stop navigation."); - state.navigation.navigationBreak = true; - stepper.releaseStepSize(state.stepping, ConstrainedStep::actor); - } } } @@ -835,30 +796,8 @@ class Navigator { ++state.navigation.navLayerIndex; } - // Re-initialize target at last layer, only in case it is the target volume - // This avoids a wrong target volume estimation - if (state.navigation.currentVolume == state.navigation.targetVolume) { - initializeTarget(state, stepper); - } - // Screen output - if (logger().doPrint(Logging::VERBOSE)) { - std::ostringstream os; - os << "Last layer"; - if (state.navigation.currentVolume == state.navigation.targetVolume) { - os << " (final volume) done, proceed to target."; - } else { - os << " done, target volume boundary."; - } - logger().log(Logging::VERBOSE, os.str()); - } + ACTS_VERBOSE(volInfo(state) << "Last layer done, target volume boundary."); - // Set the navigation break if necessary - if (state.navigation.currentVolume == state.navigation.targetVolume) { - ACTS_VERBOSE(volInfo(state) - << "This is the target volume, stop navigation."); - state.navigation.navigationBreak = true; - stepper.releaseStepSize(state.stepping, ConstrainedStep::actor); - } return false; } @@ -900,19 +839,8 @@ class Navigator { "stopping navigation."); 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, ConstrainedStep::actor); - return true; } - // Re-initialize target at volume boundary - initializeTarget(state, stepper); - // Helper function to find boundaries auto findBoundaries = [&]() -> bool { // The navigation options @@ -1005,85 +933,6 @@ class Navigator { return false; } - /// @brief Navigation (re-)initialisation for the target - /// - /// @note This is only called a few times every propagation/extrapolation - /// - /// As a straight line estimate can lead you to the wrong destination - /// Volume, this will be called at: - /// - initialization - /// - attempted volume switch - /// Target finding by association will not be done again - /// - /// @tparam propagator_state_t The state type of the propagator - /// @tparam stepper_t The type of stepper used for the propagation - /// - /// @param [in,out] state is the propagation state object - /// @param [in] stepper Stepper in use - /// - /// boolean return triggers exit to stepper - template - void initializeTarget(propagator_state_t& state, - const stepper_t& stepper) const { - if (state.navigation.targetVolume != nullptr && - state.stepping.pathAccumulated == 0.) { - ACTS_VERBOSE(volInfo(state) - << "Re-initialzing cancelled as it is the first step."); - return; - } - // Fast Navigation initialization for target: - if (state.navigation.targetSurface != nullptr && - state.navigation.targetSurface->associatedLayer() && - state.navigation.targetVolume == nullptr) { - ACTS_VERBOSE(volInfo(state) - << "Fast target initialization through association."); - ACTS_VERBOSE(volInfo(state) - << "Target surface set to " - << state.navigation.targetSurface->geometryId()); - state.navigation.targetLayer = - state.navigation.targetSurface->associatedLayer(); - state.navigation.targetVolume = - state.navigation.targetLayer->trackingVolume(); - } else if (state.navigation.targetSurface != nullptr) { - // screen output that you do a re-initialization - if (state.navigation.targetVolume != nullptr) { - ACTS_VERBOSE(volInfo(state) - << "Re-initialization of target volume triggered."); - } - // Slow navigation initialization for target: - // target volume and layer search through global search - auto targetIntersection = - state.navigation.targetSurface - ->intersect( - state.geoContext, stepper.position(state.stepping), - state.options.direction * stepper.direction(state.stepping), - BoundaryTolerance::Infinite(), state.options.surfaceTolerance) - .closest(); - if (targetIntersection.isValid()) { - ACTS_VERBOSE(volInfo(state) - << "Target estimate position (" - << targetIntersection.position().x() << ", " - << targetIntersection.position().y() << ", " - << targetIntersection.position().z() << ")"); - /// get the target volume from the intersection - auto tPosition = targetIntersection.position(); - state.navigation.targetVolume = - m_cfg.trackingGeometry->lowestTrackingVolume(state.geoContext, - tPosition); - state.navigation.targetLayer = - state.navigation.targetVolume != nullptr - ? state.navigation.targetVolume->associatedLayer( - state.geoContext, tPosition) - : nullptr; - if (state.navigation.targetVolume != nullptr) { - ACTS_VERBOSE(volInfo(state) - << "Target volume estimated : " - << state.navigation.targetVolume->volumeName()); - } - } - } - } - /// @brief Resolve the surfaces of this layer /// /// @tparam propagator_state_t The state type of the propagator diff --git a/Tests/UnitTests/Core/Propagator/NavigatorTests.cpp b/Tests/UnitTests/Core/Propagator/NavigatorTests.cpp index 8bf56b6855b..1e4afdf3b6c 100644 --- a/Tests/UnitTests/Core/Propagator/NavigatorTests.cpp +++ b/Tests/UnitTests/Core/Propagator/NavigatorTests.cpp @@ -291,18 +291,18 @@ bool testNavigatorStateVectors(Navigator::State& state, std::size_t navSurf, /// @param [in] targetVol Target volume /// @param [in] targetLay Target layer /// @param [in] targetSurf Target surface -bool testNavigatorStatePointers( - Navigator::State& state, const TrackingVolume* worldVol, - const TrackingVolume* startVol, const Layer* startLay, - const Surface* startSurf, const Surface* currSurf, - const TrackingVolume* currVol, const TrackingVolume* targetVol, - const Layer* targetLay, const Surface* targetSurf) { - return ( - (state.worldVolume == worldVol) && (state.startVolume == startVol) && - (state.startLayer == startLay) && (state.startSurface == startSurf) && - (state.currentSurface == currSurf) && (state.currentVolume == currVol) && - (state.targetVolume == targetVol) && (state.targetLayer == targetLay) && - (state.targetSurface == targetSurf)); +bool testNavigatorStatePointers(Navigator::State& state, + const TrackingVolume* worldVol, + const TrackingVolume* startVol, + const Layer* startLay, const Surface* startSurf, + const Surface* currSurf, + const TrackingVolume* currVol, + const Surface* targetSurf) { + return ((state.worldVolume == worldVol) && (state.startVolume == startVol) && + (state.startLayer == startLay) && (state.startSurface == startSurf) && + (state.currentSurface == currSurf) && + (state.currentVolume == currVol) && + (state.targetSurface == targetSurf)); } // the surface cache & the creation of the geometry @@ -344,7 +344,7 @@ BOOST_AUTO_TEST_CASE(Navigator_status_methods) { BOOST_CHECK(testNavigatorStateVectors(state.navigation, 0u, 0u, 0u)); BOOST_CHECK(testNavigatorStatePointers(state.navigation, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, - nullptr, nullptr, nullptr)); + nullptr)); } ACTS_INFO(" b) Run with geometry but without resolving"); @@ -360,7 +360,7 @@ BOOST_AUTO_TEST_CASE(Navigator_status_methods) { BOOST_CHECK(testNavigatorStateVectors(state.navigation, 0u, 0u, 0u)); BOOST_CHECK(testNavigatorStatePointers(state.navigation, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, - nullptr, nullptr, nullptr)); + nullptr)); } ACTS_INFO( @@ -381,7 +381,7 @@ BOOST_AUTO_TEST_CASE(Navigator_status_methods) { BOOST_CHECK(testNavigatorStateVectors(state.navigation, 0u, 0u, 0u)); BOOST_CHECK(testNavigatorStatePointers(state.navigation, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, - nullptr, nullptr, nullptr)); + nullptr)); ACTS_INFO(" ii) Because of no target surface"); state.navigation.targetReached = false; @@ -390,7 +390,7 @@ BOOST_AUTO_TEST_CASE(Navigator_status_methods) { BOOST_CHECK(testNavigatorStateVectors(state.navigation, 0u, 0u, 0u)); BOOST_CHECK(testNavigatorStatePointers(state.navigation, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, - nullptr, nullptr, nullptr)); + nullptr)); ACTS_INFO(" iii) Because the target surface is reached"); const Surface* startSurf = tGeometry->getBeamline(); state.stepping.pos4.segment<3>(Acts::ePos0) = @@ -399,9 +399,9 @@ BOOST_AUTO_TEST_CASE(Navigator_status_methods) { state.navigation.targetSurface = targetSurf; navigator.postStep(state, stepper); BOOST_CHECK(testNavigatorStateVectors(state.navigation, 0u, 0u, 0u)); - BOOST_CHECK(testNavigatorStatePointers( - state.navigation, nullptr, nullptr, nullptr, nullptr, targetSurf, - nullptr, nullptr, nullptr, targetSurf)); + BOOST_CHECK(testNavigatorStatePointers(state.navigation, nullptr, nullptr, + nullptr, nullptr, targetSurf, + nullptr, targetSurf)); ACTS_INFO("(2) Test the initialisation"); ACTS_INFO(" a) Initialise without additional information"); @@ -416,16 +416,16 @@ BOOST_AUTO_TEST_CASE(Navigator_status_methods) { BOOST_CHECK(testNavigatorStateVectors(state.navigation, 0u, 0u, 0u)); BOOST_CHECK(testNavigatorStatePointers(state.navigation, worldVol, startVol, startLay, nullptr, nullptr, startVol, - nullptr, nullptr, nullptr)); + nullptr)); ACTS_INFO(" b) Initialise having a start surface"); state.navigation = Navigator::State(); state.navigation.startSurface = startSurf; navigator.initialize(state, stepper); BOOST_CHECK(testNavigatorStateVectors(state.navigation, 0u, 0u, 0u)); - BOOST_CHECK(testNavigatorStatePointers( - state.navigation, worldVol, startVol, startLay, startSurf, startSurf, - startVol, nullptr, nullptr, nullptr)); + BOOST_CHECK(testNavigatorStatePointers(state.navigation, worldVol, startVol, + startLay, startSurf, startSurf, + startVol, nullptr)); ACTS_INFO(" c) Initialise having a start volume"); state.navigation = Navigator::State(); @@ -434,7 +434,7 @@ BOOST_AUTO_TEST_CASE(Navigator_status_methods) { BOOST_CHECK(testNavigatorStateVectors(state.navigation, 0u, 0u, 0u)); BOOST_CHECK(testNavigatorStatePointers(state.navigation, worldVol, startVol, startLay, nullptr, nullptr, startVol, - nullptr, nullptr, nullptr)); + nullptr)); } }