From 33845a7c8aae425d613e2f7658ff7d0d160bcda7 Mon Sep 17 00:00:00 2001 From: andiwand Date: Mon, 30 Oct 2023 21:15:23 +0100 Subject: [PATCH 1/2] remove target estimation from navigator --- Core/include/Acts/Propagator/Navigator.hpp | 99 +------------------ Core/src/Geometry/TrackingVolume.cpp | 3 +- .../Propagator/MaterialCollectionTests.cpp | 31 ++++-- 3 files changed, 26 insertions(+), 107 deletions(-) diff --git a/Core/include/Acts/Propagator/Navigator.hpp b/Core/include/Acts/Propagator/Navigator.hpp index 815b414d186..e5113366c66 100644 --- a/Core/include/Acts/Propagator/Navigator.hpp +++ b/Core/include/Acts/Propagator/Navigator.hpp @@ -51,8 +51,6 @@ struct NavigationOptions { /// object to check against: at end const object_t* endObject = nullptr; - /// Target surface to exclude - const Surface* targetSurface = nullptr; /// External surface identifier for which the boundary check is ignored std::vector externalSurfaces = {}; @@ -401,11 +399,6 @@ class Navigator { // Call the navigation helper prior to actual navigation ACTS_VERBOSE(volInfo(state) << "Entering navigator::preStep."); - // Initialize the target and target volume - if (state.navigation.targetSurface and not state.navigation.targetVolume) { - // Find out about the target as much as you can - initializeTarget(state, stepper); - } // Try targeting the surfaces - then layers - then boundaries if (state.navigation.navigationStage <= Stage::surfaceTarget and targetSurfaces(state, stepper)) { @@ -564,6 +557,9 @@ class Navigator { state.navigation.navigationBreak = true; stepper.releaseStepSize(state.stepping); } else { + // Set navigation break and release the navigation step size + state.navigation.navigationBreak = true; + stepper.releaseStepSize(state.stepping); ACTS_VERBOSE(volInfo(state) << "Status could not be determined - good luck."); } @@ -872,11 +868,6 @@ 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; @@ -942,9 +933,6 @@ class Navigator { return true; } - // Re-initialize target at volume boundary - initializeTarget(state, stepper); - // Helper function to find boundaries auto findBoundaries = [&]() -> bool { // The navigation options @@ -1036,85 +1024,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 and - 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 && - state.navigation.targetSurface->associatedLayer() && - !state.navigation.targetVolume) { - 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) { - // screen output that you do a re-initialization - if (state.navigation.targetVolume) { - 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), - false, state.options.targetTolerance) - .closest(); - if (targetIntersection) { - 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 - ? state.navigation.targetVolume->associatedLayer( - state.geoContext, tPosition) - : nullptr; - if (state.navigation.targetVolume) { - 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 @@ -1225,8 +1134,6 @@ class Navigator { navOpts.resolveMaterial = m_cfg.resolveMaterial; navOpts.resolvePassive = m_cfg.resolvePassive; navOpts.startObject = startLayer; - // Set also the target surface - navOpts.targetSurface = state.navigation.targetSurface; navOpts.pathLimit = stepper.getStepSize(state.stepping, ConstrainedStep::aborter); navOpts.overstepLimit = stepper.overstepLimit(state.stepping); diff --git a/Core/src/Geometry/TrackingVolume.cpp b/Core/src/Geometry/TrackingVolume.cpp index 0e9ff902d50..8e604758e6f 100644 --- a/Core/src/Geometry/TrackingVolume.cpp +++ b/Core/src/Geometry/TrackingVolume.cpp @@ -611,8 +611,7 @@ Acts::TrackingVolume::compatibleLayers( auto path = atIntersection.pathLength(); bool withinLimit = std::abs(path) <= std::abs(options.pathLimit); // Intersection is ok - take it (move to surface on approach) - if (atIntersection && - (atIntersection.object() != options.targetSurface) && withinLimit) { + if (atIntersection && withinLimit) { // create a layer intersection lIntersections.push_back(LayerIntersection( atIntersection.intersection(), tLayer, atIntersection.object(), diff --git a/Tests/UnitTests/Core/Propagator/MaterialCollectionTests.cpp b/Tests/UnitTests/Core/Propagator/MaterialCollectionTests.cpp index 4d772235e07..120f79d2cf9 100644 --- a/Tests/UnitTests/Core/Propagator/MaterialCollectionTests.cpp +++ b/Tests/UnitTests/Core/Propagator/MaterialCollectionTests.cpp @@ -31,6 +31,7 @@ #include "Acts/Surfaces/Surface.hpp" #include "Acts/Tests/CommonHelpers/CylindricalTrackingGeometry.hpp" #include "Acts/Tests/CommonHelpers/FloatComparisons.hpp" +#include "Acts/Utilities/Logger.hpp" #include #include @@ -63,7 +64,8 @@ auto tGeometry = cGeometry(); // create a navigator for this tracking geometry Navigator navigatorES({tGeometry}); -Navigator navigatorSL({tGeometry}); +Navigator navigatorSL({tGeometry}, + getDefaultLogger("nav", Logging::Level::VERBOSE)); using BField = ConstantBField; using EigenStepper = Acts::EigenStepper<>; @@ -76,13 +78,15 @@ EigenStepper estepper(bField); EigenPropagator epropagator(std::move(estepper), std::move(navigatorES)); StraightLineStepper slstepper; -StraightLinePropagator slpropagator(slstepper, std::move(navigatorSL)); -const int ntests = 500; +StraightLinePropagator slpropagator(slstepper, std::move(navigatorSL), + getDefaultLogger("prop", + Logging::Level::VERBOSE)); +const int ntests = 1; const int skip = 0; -bool debugModeFwd = false; -bool debugModeBwd = false; -bool debugModeFwdStep = false; -bool debugModeBwdStep = false; +bool debugModeFwd = true; +bool debugModeBwd = true; +bool debugModeFwdStep = true; +bool debugModeBwdStep = true; /// the actual test nethod that runs the test /// can be used with several propagator types @@ -226,7 +230,7 @@ void runTest(const propagator_t& prop, double pT, double phi, double theta, fwdMaterial.materialInteractions.size()); CHECK_CLOSE_REL(bwdMaterial.materialInX0, fwdMaterial.materialInX0, 1e-3); - CHECK_CLOSE_REL(bwdMaterial.materialInL0, bwdMaterial.materialInL0, 1e-3); + CHECK_CLOSE_REL(bwdMaterial.materialInL0, fwdMaterial.materialInL0, 1e-3); // stepping from one surface to the next // now go from surface to surface and check @@ -274,6 +278,8 @@ void runTest(const propagator_t& prop, double pT, double phi, double theta, fwdStepStepMaterialInX0 += fwdStepMaterial.materialInX0; fwdStepStepMaterialInL0 += fwdStepMaterial.materialInL0; + std::cout << fwdStepMaterial.materialInX0 << std::endl; + if (fwdStep.endParameters.has_value()) { // make sure the parameters do not run out of scope stepParameters.push_back(*fwdStep.endParameters); @@ -347,6 +353,8 @@ void runTest(const propagator_t& prop, double pT, double phi, double theta, bwdStepStepMaterialInX0 += bwdStepMaterial.materialInX0; bwdStepStepMaterialInL0 += bwdStepMaterial.materialInL0; + std::cout << bwdStepMaterial.materialInX0 << std::endl; + if (bwdStep.endParameters.has_value()) { // make sure the parameters do not run out of scope stepParameters.push_back(*bwdStep.endParameters); @@ -370,6 +378,11 @@ void runTest(const propagator_t& prop, double pT, double phi, double theta, bwdStepStepMaterialInX0 += bwdStepMaterial.materialInX0; bwdStepStepMaterialInL0 += bwdStepMaterial.materialInL0; + std::cout << fwdStepMaterialInX0 << " " << fwdStepStepMaterialInX0 + << std::endl; + std::cout << bwdStepMaterialInX0 << " " << bwdStepStepMaterialInX0 + << std::endl; + // forward-forward step compatibility test CHECK_CLOSE_REL(bwdStepStepMaterialInX0, bwdStepMaterialInX0, 1e-3); CHECK_CLOSE_REL(bwdStepStepMaterialInL0, bwdStepMaterialInL0, 1e-3); @@ -411,7 +424,7 @@ BOOST_DATA_TEST_CASE( bdata::distribution = std::uniform_int_distribution<>(0, 100))) ^ bdata::xrange(ntests), pT, phi, theta, charge, time, index) { - runTest(epropagator, pT, phi, theta, charge, time, index); + // runTest(epropagator, pT, phi, theta, charge, time, index); runTest(slpropagator, pT, phi, theta, charge, time, index); } From 0b81d0750791d58afac24c90a4593a6337284cac Mon Sep 17 00:00:00 2001 From: andiwand Date: Mon, 30 Oct 2023 21:19:33 +0100 Subject: [PATCH 2/2] restore --- .../Propagator/MaterialCollectionTests.cpp | 31 ++++++------------- 1 file changed, 9 insertions(+), 22 deletions(-) diff --git a/Tests/UnitTests/Core/Propagator/MaterialCollectionTests.cpp b/Tests/UnitTests/Core/Propagator/MaterialCollectionTests.cpp index 120f79d2cf9..4d772235e07 100644 --- a/Tests/UnitTests/Core/Propagator/MaterialCollectionTests.cpp +++ b/Tests/UnitTests/Core/Propagator/MaterialCollectionTests.cpp @@ -31,7 +31,6 @@ #include "Acts/Surfaces/Surface.hpp" #include "Acts/Tests/CommonHelpers/CylindricalTrackingGeometry.hpp" #include "Acts/Tests/CommonHelpers/FloatComparisons.hpp" -#include "Acts/Utilities/Logger.hpp" #include #include @@ -64,8 +63,7 @@ auto tGeometry = cGeometry(); // create a navigator for this tracking geometry Navigator navigatorES({tGeometry}); -Navigator navigatorSL({tGeometry}, - getDefaultLogger("nav", Logging::Level::VERBOSE)); +Navigator navigatorSL({tGeometry}); using BField = ConstantBField; using EigenStepper = Acts::EigenStepper<>; @@ -78,15 +76,13 @@ EigenStepper estepper(bField); EigenPropagator epropagator(std::move(estepper), std::move(navigatorES)); StraightLineStepper slstepper; -StraightLinePropagator slpropagator(slstepper, std::move(navigatorSL), - getDefaultLogger("prop", - Logging::Level::VERBOSE)); -const int ntests = 1; +StraightLinePropagator slpropagator(slstepper, std::move(navigatorSL)); +const int ntests = 500; const int skip = 0; -bool debugModeFwd = true; -bool debugModeBwd = true; -bool debugModeFwdStep = true; -bool debugModeBwdStep = true; +bool debugModeFwd = false; +bool debugModeBwd = false; +bool debugModeFwdStep = false; +bool debugModeBwdStep = false; /// the actual test nethod that runs the test /// can be used with several propagator types @@ -230,7 +226,7 @@ void runTest(const propagator_t& prop, double pT, double phi, double theta, fwdMaterial.materialInteractions.size()); CHECK_CLOSE_REL(bwdMaterial.materialInX0, fwdMaterial.materialInX0, 1e-3); - CHECK_CLOSE_REL(bwdMaterial.materialInL0, fwdMaterial.materialInL0, 1e-3); + CHECK_CLOSE_REL(bwdMaterial.materialInL0, bwdMaterial.materialInL0, 1e-3); // stepping from one surface to the next // now go from surface to surface and check @@ -278,8 +274,6 @@ void runTest(const propagator_t& prop, double pT, double phi, double theta, fwdStepStepMaterialInX0 += fwdStepMaterial.materialInX0; fwdStepStepMaterialInL0 += fwdStepMaterial.materialInL0; - std::cout << fwdStepMaterial.materialInX0 << std::endl; - if (fwdStep.endParameters.has_value()) { // make sure the parameters do not run out of scope stepParameters.push_back(*fwdStep.endParameters); @@ -353,8 +347,6 @@ void runTest(const propagator_t& prop, double pT, double phi, double theta, bwdStepStepMaterialInX0 += bwdStepMaterial.materialInX0; bwdStepStepMaterialInL0 += bwdStepMaterial.materialInL0; - std::cout << bwdStepMaterial.materialInX0 << std::endl; - if (bwdStep.endParameters.has_value()) { // make sure the parameters do not run out of scope stepParameters.push_back(*bwdStep.endParameters); @@ -378,11 +370,6 @@ void runTest(const propagator_t& prop, double pT, double phi, double theta, bwdStepStepMaterialInX0 += bwdStepMaterial.materialInX0; bwdStepStepMaterialInL0 += bwdStepMaterial.materialInL0; - std::cout << fwdStepMaterialInX0 << " " << fwdStepStepMaterialInX0 - << std::endl; - std::cout << bwdStepMaterialInX0 << " " << bwdStepStepMaterialInX0 - << std::endl; - // forward-forward step compatibility test CHECK_CLOSE_REL(bwdStepStepMaterialInX0, bwdStepMaterialInX0, 1e-3); CHECK_CLOSE_REL(bwdStepStepMaterialInL0, bwdStepMaterialInL0, 1e-3); @@ -424,7 +411,7 @@ BOOST_DATA_TEST_CASE( bdata::distribution = std::uniform_int_distribution<>(0, 100))) ^ bdata::xrange(ntests), pT, phi, theta, charge, time, index) { - // runTest(epropagator, pT, phi, theta, charge, time, index); + runTest(epropagator, pT, phi, theta, charge, time, index); runTest(slpropagator, pT, phi, theta, charge, time, index); }