From 8489084f547718c015529406fdaf894121fd27e7 Mon Sep 17 00:00:00 2001 From: Andreas Salzburger Date: Fri, 31 Aug 2018 13:56:31 +0200 Subject: [PATCH 1/5] fix step estimation --- Core/include/Acts/Extrapolator/Navigator.hpp | 44 ++++++++++++------- .../Propagator/detail/ConstrainedStep.hpp | 17 ++++--- .../detail/StandardAbortConditions.hpp | 1 - .../Extrapolator/MaterialCollectionTests.cpp | 6 +-- Tests/Integration/PropagationTestBase.hpp | 2 + Tests/Integration/PropagationTestHelper.hpp | 7 ++- Tests/Integration/PropagationTests.cpp | 2 +- 7 files changed, 50 insertions(+), 29 deletions(-) diff --git a/Core/include/Acts/Extrapolator/Navigator.hpp b/Core/include/Acts/Extrapolator/Navigator.hpp index cc4d7c4fea8..1670f4b0e2e 100644 --- a/Core/include/Acts/Extrapolator/Navigator.hpp +++ b/Core/include/Acts/Extrapolator/Navigator.hpp @@ -448,9 +448,11 @@ struct Navigator // set the iterator state.navigation.navSurfaceIter = state.navigation.navSurfaces.begin(); - // update the navigation step size before you return + // Update the navigation step size before you return + // That's a new navigation stream, thus release the former step updateStep(state, - state.navigation.navSurfaceIter->intersection.pathLength); + state.navigation.navSurfaceIter->intersection.pathLength, + true); return true; } return false; @@ -665,9 +667,11 @@ struct Navigator return std::string("Starting from boundary surface."); }); } else { - // update the navigation step size before you return + // Update the navigation step size before you return + // That's a new navigation stream, release the prior navigation updateStep(state, - state.navigation.navBoundaryIter->intersection.pathLength); + state.navigation.navBoundaryIter->intersection.pathLength, + true); return true; } } else { @@ -767,8 +771,9 @@ struct Navigator lastBoundary = boundarySurface; continue; } else { - // update the navigation step size - updateStep(state, boundaryIntersect.intersection.pathLength); + // Update the navigation step size + // This is a new navigation stream, release the former first + updateStep(state, boundaryIntersect.intersection.pathLength, true); return true; } } else { @@ -843,7 +848,8 @@ struct Navigator [&] { return std::string("Stepping towards first layer."); }); // update the navigation step size before you return updateStep(state, - state.navigation.navLayerIter->intersection.pathLength); + state.navigation.navLayerIter->intersection.pathLength, + true); return true; } else { debugLog(state, [&] { @@ -882,6 +888,7 @@ struct Navigator }); // the step size will be set to the aborter step size double abortStep = state.stepping.stepSize.value(cstep::aborter); + // Do not release the aborter step size here updateStep(state, abortStep); state.navigation.navigationBreak = true; return true; @@ -995,8 +1002,8 @@ struct Navigator }); ++state.navigation.navLayerIter; } else { - // update the navigation step size - updateStep(state, layerIntersect.intersection.pathLength); + // update the navigation step size, release the former first + updateStep(state, layerIntersect.intersection.pathLength, true); return true; } } @@ -1009,7 +1016,7 @@ struct Navigator return std::string( "Done in final volume, release stepSize & proceed to target."); }); - // the step size will be set to the aborter step size + // the step size will be set to the aborter step size, do not release double abortStep = state.stepping.stepSize.value(cstep::aborter); updateStep(state, abortStep); state.navigation.navigationBreak = true; @@ -1071,8 +1078,10 @@ struct Navigator // set the iterator state.navigation.navSurfaceIter = state.navigation.navSurfaces.begin(); // update the navigation step size before you return to the stepper + // This is a new navigation stream, release the former step size first updateStep(state, - state.navigation.navSurfaceIter->intersection.pathLength); + state.navigation.navSurfaceIter->intersection.pathLength, + true); return true; } state.navigation.navSurfaceIter = state.navigation.navSurfaces.end(); @@ -1174,7 +1183,8 @@ struct Navigator ++state.navigation.navSurfaceIter; continue; } else { - updateStep(state, surfaceDistance); + /// Releae the former step size + updateStep(state, surfaceDistance, true); return true; } } @@ -1200,23 +1210,25 @@ struct Navigator /// /// @param[in,out] state The state object for the step length /// @param[in] step the step size + /// @param[in] release flat steers if the step is released first template void - updateStep(propagator_state_t& state, double step) const + updateStep(propagator_state_t& state, double step, bool release = false) const { /// If we have an initial step and are configured to modify it if (state.stepping.pathAccumulated == 0. && initialStepFactor != 1.) { debugLog(state, [&] { - return std::string("Initial step modification detected."); + return std::string("Initial navigation step modifiction applied."); }); step *= initialStepFactor; } // update the step - state.stepping.stepSize.update(step, cstep::actor); + state.stepping.stepSize.update(step, cstep::actor, release); debugLog(state, [&] { std::stringstream dstream; - dstream << "Navigation stepSize updated to "; + std::string releaseFlag = release ? "released and " : ""; + dstream << "Navigation stepSize " << releaseFlag << "updated to "; dstream << state.stepping.stepSize.toString(); return dstream.str(); }); diff --git a/Core/include/Acts/Propagator/detail/ConstrainedStep.hpp b/Core/include/Acts/Propagator/detail/ConstrainedStep.hpp index 6c13799d17d..4ab5532d3f7 100644 --- a/Core/include/Acts/Propagator/detail/ConstrainedStep.hpp +++ b/Core/include/Acts/Propagator/detail/ConstrainedStep.hpp @@ -38,17 +38,20 @@ namespace detail { /// The Navigation direction NavigationDirection direction = forward; - /// update the step size of a certain type - /// - for accuracy and navigation that can go either way - /// - for aborters it can only get (direction)*smaller + /// Update the step size of a certain type + /// + /// Only navigation and target abortion step size + /// updates may change the sign due to overstepping + /// /// @param value is the new value to be updated /// @param type is the constraint type void - update(const double& value, Type type) + update(const double& value, Type type, bool releaseStep = false) { - if (type != aborter || (direction * values[type] > direction * value)) { - values[type] = value; - } + if (releaseStep) release(type); + // The check the current value and set it if appropriate + double cValue = values[type]; + values[type] = cValue * cValue < value * value ? cValue : value; } /// release a certain constraint value diff --git a/Core/include/Acts/Propagator/detail/StandardAbortConditions.hpp b/Core/include/Acts/Propagator/detail/StandardAbortConditions.hpp index bb6d42405f3..335a574696a 100644 --- a/Core/include/Acts/Propagator/detail/StandardAbortConditions.hpp +++ b/Core/include/Acts/Propagator/detail/StandardAbortConditions.hpp @@ -160,7 +160,6 @@ namespace detail { return true; } // calculate the distance to the surface - // @todo: add corrector const double tolerance = state.options.targetTolerance; const auto iestimate = state.navigation.targetSurface->intersectionEstimate( diff --git a/Tests/Core/Extrapolator/MaterialCollectionTests.cpp b/Tests/Core/Extrapolator/MaterialCollectionTests.cpp index 86c2ad527c5..6a9626c2617 100644 --- a/Tests/Core/Extrapolator/MaterialCollectionTests.cpp +++ b/Tests/Core/Extrapolator/MaterialCollectionTests.cpp @@ -70,8 +70,8 @@ namespace Test { StraightLinePropagator slpropagator(std::move(slstepper), std::move(navigatorSL)); - const int ntests = 10; // test with 0.4 lower boundary - 952; - const int skip = 0; // test with 0.4 lower boundary - 951; + const int ntests = 100; + const int skip = 0; bool debugModeFwd = false; bool debugModeBwd = false; bool debugModeFwdStep = false; @@ -425,7 +425,7 @@ namespace Test { charge, index) { - runTest(epropagator, pT, phi, theta, charge, index); + // runTest(epropagator, pT, phi, theta, charge, index); runTest(slpropagator, pT, phi, theta, charge, index); } diff --git a/Tests/Integration/PropagationTestBase.hpp b/Tests/Integration/PropagationTestBase.hpp index e4573b6ca3d..a234129997e 100644 --- a/Tests/Integration/PropagationTestBase.hpp +++ b/Tests/Integration/PropagationTestBase.hpp @@ -332,6 +332,7 @@ BOOST_DATA_TEST_CASE( BOOST_CHECK(e_at_disc.first.isApprox(a_at_disc.first, 1 * units::_um)); BOOST_CHECK(e_at_disc.first.isApprox(w_at_disc.first, 1 * units::_um)); } + /// test consistency of propagators to a line BOOST_DATA_TEST_CASE( propagation_to_line_, @@ -432,6 +433,7 @@ BOOST_DATA_TEST_CASE( BOOST_CHECK(e_at_line.first.isApprox(a_at_line.first, 1 * units::_um)); BOOST_CHECK(e_at_line.first.isApprox(w_at_line.first, 1 * units::_um)); } + /// test correct covariance transport for curvilinear parameters /// this test only works within the /// s_curvilinearProjTolerance (in: Definitions.hpp) diff --git a/Tests/Integration/PropagationTestHelper.hpp b/Tests/Integration/PropagationTestHelper.hpp index f5588741be3..2db81426a27 100644 --- a/Tests/Integration/PropagationTestHelper.hpp +++ b/Tests/Integration/PropagationTestHelper.hpp @@ -365,7 +365,12 @@ namespace IntegrationTest { Surface_type endSurface(seTransform, nullptr); // Increase the path limit - to be safe hitting the surface options.pathLimit *= 2; - options.maxStepSize *= 0.9; + + if (debug) { + std::cout << ">>> Path limit for this propgation is set to: " + << options.pathLimit << std::endl; + } + const auto result = propagator.propagate(start, endSurface, options); const auto& tp = result.endParameters; // check the result for nullptr diff --git a/Tests/Integration/PropagationTests.cpp b/Tests/Integration/PropagationTests.cpp index 082cbde288a..1d196ec4056 100644 --- a/Tests/Integration/PropagationTests.cpp +++ b/Tests/Integration/PropagationTests.cpp @@ -52,7 +52,7 @@ namespace IntegrationTest { = PropagatorWrapper>; // number of tests - const int ntests = 1; + const int ntests = 100; const int skip = 0; const bool covtpr = true; const bool debug = false; From 921df6d1f4392ca606ba284704cbd7585d2295ec Mon Sep 17 00:00:00 2001 From: Andreas Salzburger Date: Fri, 31 Aug 2018 14:01:23 +0200 Subject: [PATCH 2/5] switching material collection in bfield on --- Tests/Core/Extrapolator/MaterialCollectionTests.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/Core/Extrapolator/MaterialCollectionTests.cpp b/Tests/Core/Extrapolator/MaterialCollectionTests.cpp index 6a9626c2617..9c5b18ed949 100644 --- a/Tests/Core/Extrapolator/MaterialCollectionTests.cpp +++ b/Tests/Core/Extrapolator/MaterialCollectionTests.cpp @@ -425,7 +425,7 @@ namespace Test { charge, index) { - // runTest(epropagator, pT, phi, theta, charge, index); + runTest(epropagator, pT, phi, theta, charge, index); runTest(slpropagator, pT, phi, theta, charge, index); } From 06cb3c45df0c7d7d6c37d535bc1ef759c15ca52b Mon Sep 17 00:00:00 2001 From: Andreas Salzburger Date: Fri, 31 Aug 2018 14:36:05 +0200 Subject: [PATCH 3/5] fixing ConstrainedStep unit test --- Tests/Core/Propagator/ConstrainedStepTests.cpp | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/Tests/Core/Propagator/ConstrainedStepTests.cpp b/Tests/Core/Propagator/ConstrainedStepTests.cpp index 7bafe3305dd..5d403876f6e 100644 --- a/Tests/Core/Propagator/ConstrainedStepTests.cpp +++ b/Tests/Core/Propagator/ConstrainedStepTests.cpp @@ -53,8 +53,12 @@ namespace Test { // now we update the actor to smaller stepSize_p.update(0.05, cstep::actor); BOOST_CHECK(stepSize_p == 0.05); - // we increase the actor and accuracy is smaller again - stepSize_p.update(0.15, cstep::actor); + // we increase the actor, but do not release the step size + stepSize_p.update(0.15, cstep::actor, false); + BOOST_CHECK(stepSize_p == 0.05); + // we increase the actor, but now DO release the step size + // it falls back to the accuracy + stepSize_p.update(0.15, cstep::actor, true); BOOST_CHECK(stepSize_p == 0.1); // now set two and update them @@ -86,8 +90,11 @@ namespace Test { // now we update the actor to smaller stepSize_n.update(-0.05, cstep::actor); BOOST_CHECK(stepSize_n == -0.05); - // we increase the actor and accuracy is smaller again - stepSize_n.update(-0.15, cstep::actor); + // we increase the actor and accuracy is smaller again, without reset + stepSize_n.update(-0.15, cstep::actor, false); + BOOST_CHECK(stepSize_n == -0.05); + // we increase the actor and accuracy is smaller again, with reset + stepSize_n.update(-0.15, cstep::actor, true); BOOST_CHECK(stepSize_n == -0.1); // now set two and update them From d4ab9d6d34958f92d4f6502f6138c62007a1cd5f Mon Sep 17 00:00:00 2001 From: Andreas Salzburger Date: Fri, 31 Aug 2018 14:42:01 +0200 Subject: [PATCH 4/5] flat -> flag --- Core/include/Acts/Extrapolator/Navigator.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Core/include/Acts/Extrapolator/Navigator.hpp b/Core/include/Acts/Extrapolator/Navigator.hpp index 1670f4b0e2e..c9e63d2e488 100644 --- a/Core/include/Acts/Extrapolator/Navigator.hpp +++ b/Core/include/Acts/Extrapolator/Navigator.hpp @@ -1210,7 +1210,7 @@ struct Navigator /// /// @param[in,out] state The state object for the step length /// @param[in] step the step size - /// @param[in] release flat steers if the step is released first + /// @param[in] release flag steers if the step is released first template void updateStep(propagator_state_t& state, double step, bool release = false) const From 6f27cde78fb5afd5d4c7c49e0f5ee8281c727cf0 Mon Sep 17 00:00:00 2001 From: Andreas Salzburger Date: Fri, 31 Aug 2018 15:25:58 +0200 Subject: [PATCH 5/5] make clang-tidy happy --- Core/include/Acts/Propagator/detail/ConstrainedStep.hpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Core/include/Acts/Propagator/detail/ConstrainedStep.hpp b/Core/include/Acts/Propagator/detail/ConstrainedStep.hpp index 4ab5532d3f7..929c00d05e7 100644 --- a/Core/include/Acts/Propagator/detail/ConstrainedStep.hpp +++ b/Core/include/Acts/Propagator/detail/ConstrainedStep.hpp @@ -48,7 +48,9 @@ namespace detail { void update(const double& value, Type type, bool releaseStep = false) { - if (releaseStep) release(type); + if (releaseStep) { + release(type); + } // The check the current value and set it if appropriate double cValue = values[type]; values[type] = cValue * cValue < value * value ? cValue : value;