From 00c897a20d2ef62ca41803e4033248bbf902832f Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Thu, 17 Oct 2024 16:02:37 +0200 Subject: [PATCH 01/10] feat: Validate track parameters --- .../EventData/GenericBoundTrackParameters.hpp | 4 +- .../EventData/GenericFreeTrackParameters.hpp | 8 +++- .../Acts/EventData/TrackParameterHelpers.hpp | 34 ++++++++++++++ Core/src/EventData/CMakeLists.txt | 1 + Core/src/EventData/TrackParameterHelpers.cpp | 44 +++++++++++++++++++ 5 files changed, 89 insertions(+), 2 deletions(-) create mode 100644 Core/include/Acts/EventData/TrackParameterHelpers.hpp create mode 100644 Core/src/EventData/TrackParameterHelpers.cpp diff --git a/Core/include/Acts/EventData/GenericBoundTrackParameters.hpp b/Core/include/Acts/EventData/GenericBoundTrackParameters.hpp index 27b411a99cc..e101e33f07a 100644 --- a/Core/include/Acts/EventData/GenericBoundTrackParameters.hpp +++ b/Core/include/Acts/EventData/GenericBoundTrackParameters.hpp @@ -9,6 +9,7 @@ #pragma once #include "Acts/Definitions/Tolerance.hpp" +#include "Acts/EventData/TrackParameterHelpers.hpp" #include "Acts/EventData/TransformationHelpers.hpp" #include "Acts/EventData/detail/PrintParameters.hpp" #include "Acts/Surfaces/Surface.hpp" @@ -61,7 +62,8 @@ class GenericBoundTrackParameters { m_cov(std::move(cov)), m_surface(std::move(surface)), m_particleHypothesis(std::move(particleHypothesis)) { - assert(m_surface); + assert(isBoundVectorValid(m_params) && "Invalid bound parameters"); + assert(m_surface != nullptr && "Reference surface must not be null"); normalizePhiTheta(); } diff --git a/Core/include/Acts/EventData/GenericFreeTrackParameters.hpp b/Core/include/Acts/EventData/GenericFreeTrackParameters.hpp index 1e7846318ef..ff039c8b70a 100644 --- a/Core/include/Acts/EventData/GenericFreeTrackParameters.hpp +++ b/Core/include/Acts/EventData/GenericFreeTrackParameters.hpp @@ -53,7 +53,10 @@ class GenericFreeTrackParameters { ParticleHypothesis particleHypothesis) : m_params(params), m_cov(std::move(cov)), - m_particleHypothesis(std::move(particleHypothesis)) {} + m_particleHypothesis(std::move(particleHypothesis)) { + assert(isFreeVectorValid(m_params) && + "Invalid free track parameters vector"); + } /// Construct from four-position, angles, absolute momentum, and charge. /// @@ -78,6 +81,9 @@ class GenericFreeTrackParameters { m_params[eFreeDir1] = dir[eMom1]; m_params[eFreeDir2] = dir[eMom2]; m_params[eFreeQOverP] = qOverP; + + assert(isFreeVectorValid(m_params) && + "Invalid free track parameters vector"); } /// Converts a free track parameter with a different hypothesis. diff --git a/Core/include/Acts/EventData/TrackParameterHelpers.hpp b/Core/include/Acts/EventData/TrackParameterHelpers.hpp new file mode 100644 index 00000000000..5f0d9cb5cc6 --- /dev/null +++ b/Core/include/Acts/EventData/TrackParameterHelpers.hpp @@ -0,0 +1,34 @@ +// This file is part of the ACTS project. +// +// Copyright (C) 2016 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 https://mozilla.org/MPL/2.0/. + +#pragma once + +#include "Acts/Definitions/TrackParametrization.hpp" + +#include +#include + +namespace Acts { + +/// Check if a bound vector is valid +/// +/// @param v The bound vector to check +/// @param epsilon The epsilon to use for the checks +/// +/// @return True if the bound vector is valid +bool isBoundVectorValid(const BoundVector& v, double epsilon = 1e-6); + +/// Check if a free vector is valid +/// +/// @param v The free vector to check +/// @param epsilon The epsilon to use for the checks +/// +/// @return True if the free vector is valid +bool isFreeVectorValid(const FreeVector& v, double epsilon = 1e-6); + +} // namespace Acts diff --git a/Core/src/EventData/CMakeLists.txt b/Core/src/EventData/CMakeLists.txt index 08a69711378..c425d533649 100644 --- a/Core/src/EventData/CMakeLists.txt +++ b/Core/src/EventData/CMakeLists.txt @@ -8,4 +8,5 @@ target_sources( TrackStatePropMask.cpp VectorMultiTrajectory.cpp VectorTrackContainer.cpp + TrackParameterHelpers.cpp ) diff --git a/Core/src/EventData/TrackParameterHelpers.cpp b/Core/src/EventData/TrackParameterHelpers.cpp new file mode 100644 index 00000000000..a94a8701e18 --- /dev/null +++ b/Core/src/EventData/TrackParameterHelpers.cpp @@ -0,0 +1,44 @@ +// This file is part of the ACTS project. +// +// Copyright (C) 2016 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 https://mozilla.org/MPL/2.0/. + +#include "Acts/EventData/TrackParameterHelpers.hpp" + +bool Acts::isBoundVectorValid(const BoundVector& v, double epsilon) { + auto pi = std::numbers::pi_v; + + bool loc0Valid = std::isfinite(v[eBoundLoc0]); + bool loc1Valid = std::isfinite(v[eBoundLoc1]); + bool phiValid = std::isfinite(v[eBoundPhi]) && + (v[eBoundPhi] + epsilon >= -pi) && + (v[eBoundPhi] - epsilon < pi); + bool thetaValid = std::isfinite(v[eBoundTheta]) && + (v[eBoundTheta] + epsilon >= 0) && + (v[eBoundTheta] - epsilon <= pi); + bool qOverPValid = std::isfinite(v[eBoundQOverP]) && + (std::abs(v[eBoundQOverP]) + epsilon > 0); + bool timeValid = std::isfinite(v[eBoundTime]); + + return loc0Valid && loc1Valid && phiValid && thetaValid && qOverPValid && + timeValid; +} + +bool Acts::isFreeVectorValid(const FreeVector& v, double epsilon) { + bool pos0Valid = std::isfinite(v[eFreePos0]); + bool pos1Valid = std::isfinite(v[eFreePos1]); + bool pos2Valid = std::isfinite(v[eFreePos2]); + bool dir0Valid = std::isfinite(v[eFreeDir0]); + bool dir1Valid = std::isfinite(v[eFreeDir1]); + bool dir2Valid = std::isfinite(v[eFreeDir2]); + bool dirValid = (v.segment<3>(eFreeDir0).norm() + epsilon > 0); + bool qOverPValid = + std::isfinite(v[eFreeQOverP]) && (std::abs(v[eFreeQOverP]) + epsilon > 0); + bool timeValid = std::isfinite(v[eFreeTime]); + + return pos0Valid && pos1Valid && pos2Valid && dir0Valid && dir1Valid && + dir2Valid && dirValid && qOverPValid && timeValid; +} From 9675442557c4a6d95367c6aeeb02594c7f3e9b12 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Thu, 17 Oct 2024 16:34:47 +0200 Subject: [PATCH 02/10] fix silly --- Core/include/Acts/EventData/GenericFreeTrackParameters.hpp | 1 + 1 file changed, 1 insertion(+) diff --git a/Core/include/Acts/EventData/GenericFreeTrackParameters.hpp b/Core/include/Acts/EventData/GenericFreeTrackParameters.hpp index ff039c8b70a..12af57bb4ee 100644 --- a/Core/include/Acts/EventData/GenericFreeTrackParameters.hpp +++ b/Core/include/Acts/EventData/GenericFreeTrackParameters.hpp @@ -11,6 +11,7 @@ #include "Acts/Definitions/Algebra.hpp" #include "Acts/Definitions/Common.hpp" #include "Acts/Definitions/TrackParametrization.hpp" +#include "Acts/EventData/TrackParameterHelpers.hpp" #include "Acts/EventData/TrackParametersConcept.hpp" #include "Acts/EventData/detail/PrintParameters.hpp" #include "Acts/Utilities/MathHelpers.hpp" From 0a02484e43c7683ca69cd49eb12ddf8151fc47f8 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Fri, 18 Oct 2024 09:43:10 +0200 Subject: [PATCH 03/10] fix multi stepper tests --- .../Acts/EventData/TrackParameterHelpers.hpp | 11 ++++- Core/src/EventData/TrackParameterHelpers.cpp | 2 +- .../Core/Propagator/MultiStepperTests.cpp | 45 ++++++++++++------- 3 files changed, 39 insertions(+), 19 deletions(-) diff --git a/Core/include/Acts/EventData/TrackParameterHelpers.hpp b/Core/include/Acts/EventData/TrackParameterHelpers.hpp index 5f0d9cb5cc6..b2a03f9981d 100644 --- a/Core/include/Acts/EventData/TrackParameterHelpers.hpp +++ b/Core/include/Acts/EventData/TrackParameterHelpers.hpp @@ -15,7 +15,11 @@ namespace Acts { -/// Check if a bound vector is valid +/// Check if a bound vector is valid. This checks the following: +/// - All values are finite +/// - The phi value is in the range [-pi, pi) +/// - The theta value is in the range [0, pi] +/// - The q/p value is non-zero /// /// @param v The bound vector to check /// @param epsilon The epsilon to use for the checks @@ -23,7 +27,10 @@ namespace Acts { /// @return True if the bound vector is valid bool isBoundVectorValid(const BoundVector& v, double epsilon = 1e-6); -/// Check if a free vector is valid +/// Check if a free vector is valid. This checks the following: +/// - All values are finite +/// - Direction is normalized +/// - The q/p value is non-zero /// /// @param v The free vector to check /// @param epsilon The epsilon to use for the checks diff --git a/Core/src/EventData/TrackParameterHelpers.cpp b/Core/src/EventData/TrackParameterHelpers.cpp index a94a8701e18..1399280796b 100644 --- a/Core/src/EventData/TrackParameterHelpers.cpp +++ b/Core/src/EventData/TrackParameterHelpers.cpp @@ -9,7 +9,7 @@ #include "Acts/EventData/TrackParameterHelpers.hpp" bool Acts::isBoundVectorValid(const BoundVector& v, double epsilon) { - auto pi = std::numbers::pi_v; + constexpr auto pi = std::numbers::pi_v; bool loc0Valid = std::isfinite(v[eBoundLoc0]); bool loc1Valid = std::isfinite(v[eBoundLoc1]); diff --git a/Tests/UnitTests/Core/Propagator/MultiStepperTests.cpp b/Tests/UnitTests/Core/Propagator/MultiStepperTests.cpp index b9842b55420..e7edefb1f03 100644 --- a/Tests/UnitTests/Core/Propagator/MultiStepperTests.cpp +++ b/Tests/UnitTests/Core/Propagator/MultiStepperTests.cpp @@ -39,6 +39,7 @@ #include #include #include +#include #include #include #include @@ -124,9 +125,30 @@ auto makeDefaultBoundPars(bool cov = true, std::size_t n = 4, return c; }; + // note that we are using the default random device + std::mt19937 gen; + std::uniform_real_distribution<> locDis(-10.0, 10.0); + std::uniform_real_distribution<> phiDis(-M_PI, M_PI); + std::uniform_real_distribution<> thetaDis(0, M_PI); + std::uniform_real_distribution<> qOverPDis(-10.0, 10.0); + std::uniform_real_distribution<> timeDis(0.0, 100.0); + for (auto i = 0ul; i < n; ++i) { - cmps.push_back({1. / n, ext_pars ? *ext_pars : BoundVector::Random(), - cov ? Opt{make_random_sym_matrix()} : Opt{}}); + BoundVector params = BoundVector::Zero(); + + if (ext_pars) { + params = *ext_pars; + } else { + params[eBoundLoc0] = locDis(gen); + params[eBoundLoc1] = locDis(gen); + params[eBoundPhi] = phiDis(gen); + params[eBoundTheta] = thetaDis(gen); + params[eBoundQOverP] = qOverPDis(gen); + params[eBoundTime] = timeDis(gen); + } + + cmps.push_back( + {1. / n, params, cov ? Opt{make_random_sym_matrix()} : Opt{}}); } auto surface = Acts::CurvilinearSurface(Vector3::Zero(), Vector3{1., 0., 0.}) @@ -430,7 +452,8 @@ void test_multi_stepper_surface_status_update() { std::vector>> cmps(2, {0.5, BoundVector::Zero(), std::nullopt}); std::get(cmps[0])[eBoundTheta] = M_PI_2; - std::get(cmps[1])[eBoundTheta] = -M_PI_2; + std::get(cmps[1])[eBoundPhi] = M_PI; + std::get(cmps[1])[eBoundTheta] = M_PI_2; std::get(cmps[0])[eBoundQOverP] = 1.0; std::get(cmps[1])[eBoundQOverP] = 1.0; @@ -541,7 +564,8 @@ void test_component_bound_state() { std::vector>> cmps(2, {0.5, BoundVector::Zero(), std::nullopt}); std::get(cmps[0])[eBoundTheta] = M_PI_2; - std::get(cmps[1])[eBoundTheta] = -M_PI_2; + std::get(cmps[1])[eBoundPhi] = M_PI; + std::get(cmps[1])[eBoundTheta] = M_PI_2; std::get(cmps[0])[eBoundQOverP] = 1.0; std::get(cmps[1])[eBoundQOverP] = 1.0; @@ -703,18 +727,7 @@ void test_single_component_interface_function() { using MultiState = typename multi_stepper_t::State; using MultiStepper = multi_stepper_t; - std::vector>> - cmps; - for (int i = 0; i < 4; ++i) { - cmps.push_back({0.25, BoundVector::Random(), BoundSquareMatrix::Random()}); - } - - auto surface = - Acts::CurvilinearSurface(Vector3::Zero(), Vector3::Ones().normalized()) - .planeSurface(); - - MultiComponentBoundTrackParameters multi_pars(surface, cmps, - particleHypothesis); + MultiComponentBoundTrackParameters multi_pars = makeDefaultBoundPars(true, 4); MultiState multi_state(geoCtx, magCtx, defaultBField, multi_pars, defaultStepSize); From 01994077815bc73d18bb1f7a71dc6516965ecb24 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sat, 19 Oct 2024 19:29:15 +0200 Subject: [PATCH 04/10] fix another broken bound param --- .../Plugins/Json/TrackParametersJsonConverterTests.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/UnitTests/Plugins/Json/TrackParametersJsonConverterTests.cpp b/Tests/UnitTests/Plugins/Json/TrackParametersJsonConverterTests.cpp index 566464a1889..dfad89f3f59 100644 --- a/Tests/UnitTests/Plugins/Json/TrackParametersJsonConverterTests.cpp +++ b/Tests/UnitTests/Plugins/Json/TrackParametersJsonConverterTests.cpp @@ -73,7 +73,7 @@ BOOST_AUTO_TEST_CASE(TrackParametersJsonIO) { ctpRead.referenceSurface().bounds()); // Bound track parameters conversion - Acts::BoundVector boundPosition{1., 2., 3., 4., 5., 6.}; + Acts::BoundVector boundPosition{1., 2., 0, 0, 5., 6.}; Acts::BoundTrackParameters btp(surface, boundPosition, boundCov, particle); nlohmann::json btpJson = btp; From ad20b327164a27548a2dd9e383957ac5ecc49ecf Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Mon, 21 Oct 2024 13:45:08 +0200 Subject: [PATCH 05/10] tests and fixes --- .../EventData/GenericBoundTrackParameters.hpp | 3 -- .../EventData/GenericFreeTrackParameters.hpp | 11 +--- .../Acts/EventData/TrackParameterHelpers.hpp | 23 +++++---- .../Acts/EventData/TrackParametersConcept.hpp | 3 +- Core/src/EventData/TrackParameterHelpers.cpp | 10 ++-- Tests/UnitTests/Core/EventData/CMakeLists.txt | 1 + .../EventData/TrackParameterHelpersTests.cpp | 51 +++++++++++++++++++ 7 files changed, 74 insertions(+), 28 deletions(-) create mode 100644 Tests/UnitTests/Core/EventData/TrackParameterHelpersTests.cpp diff --git a/Core/include/Acts/EventData/GenericBoundTrackParameters.hpp b/Core/include/Acts/EventData/GenericBoundTrackParameters.hpp index eecb197f464..9f790634f3c 100644 --- a/Core/include/Acts/EventData/GenericBoundTrackParameters.hpp +++ b/Core/include/Acts/EventData/GenericBoundTrackParameters.hpp @@ -9,7 +9,6 @@ #pragma once #include "Acts/Definitions/Tolerance.hpp" -#include "Acts/EventData/TrackParameterHelpers.hpp" #include "Acts/EventData/TransformationHelpers.hpp" #include "Acts/EventData/detail/PrintParameters.hpp" #include "Acts/Surfaces/Surface.hpp" @@ -19,7 +18,6 @@ #include #include #include -#include namespace Acts { @@ -62,7 +60,6 @@ class GenericBoundTrackParameters { m_cov(std::move(cov)), m_surface(std::move(surface)), m_particleHypothesis(std::move(particleHypothesis)) { - assert(isBoundVectorValid(m_params) && "Invalid bound parameters"); assert(m_surface != nullptr && "Reference surface must not be null"); normalizePhiTheta(); } diff --git a/Core/include/Acts/EventData/GenericFreeTrackParameters.hpp b/Core/include/Acts/EventData/GenericFreeTrackParameters.hpp index 53d2e7ab0df..014634cada8 100644 --- a/Core/include/Acts/EventData/GenericFreeTrackParameters.hpp +++ b/Core/include/Acts/EventData/GenericFreeTrackParameters.hpp @@ -11,7 +11,6 @@ #include "Acts/Definitions/Algebra.hpp" #include "Acts/Definitions/Common.hpp" #include "Acts/Definitions/TrackParametrization.hpp" -#include "Acts/EventData/TrackParameterHelpers.hpp" #include "Acts/EventData/TrackParametersConcept.hpp" #include "Acts/EventData/TransformationHelpers.hpp" #include "Acts/EventData/detail/PrintParameters.hpp" @@ -19,10 +18,8 @@ #include "Acts/Utilities/UnitVectors.hpp" #include "Acts/Utilities/VectorHelpers.hpp" -#include #include #include -#include namespace Acts { @@ -56,10 +53,7 @@ class GenericFreeTrackParameters { ParticleHypothesis particleHypothesis) : m_params(params), m_cov(std::move(cov)), - m_particleHypothesis(std::move(particleHypothesis)) { - assert(isFreeVectorValid(m_params) && - "Invalid free track parameters vector"); - } + m_particleHypothesis(std::move(particleHypothesis)) {} /// Construct from four-position, direction, absolute momentum, and charge. /// @@ -107,9 +101,6 @@ class GenericFreeTrackParameters { m_params[eFreeDir1] = dir[eMom1]; m_params[eFreeDir2] = dir[eMom2]; m_params[eFreeQOverP] = qOverP; - - assert(isFreeVectorValid(m_params) && - "Invalid free track parameters vector"); } /// Converts a free track parameter with a different hypothesis. diff --git a/Core/include/Acts/EventData/TrackParameterHelpers.hpp b/Core/include/Acts/EventData/TrackParameterHelpers.hpp index 3ffc3882f8b..1bfa0574df8 100644 --- a/Core/include/Acts/EventData/TrackParameterHelpers.hpp +++ b/Core/include/Acts/EventData/TrackParameterHelpers.hpp @@ -8,13 +8,9 @@ #pragma once -#include "Acts/Definitions/Algebra.hpp" #include "Acts/Definitions/TrackParametrization.hpp" #include "Acts/Utilities/detail/periodic.hpp" -#include -#include - namespace Acts { /// Check if a bound vector is valid. This checks the following: @@ -52,8 +48,20 @@ inline BoundVector normalizeBoundParameters(const BoundVector& boundParams) { return result; } +/// Add bound parameters and take care of angle periodicity for phi and theta. +/// This is intended for small differences only i.e. KF updates. +/// +/// @param lhs The left hand side bound parameters +/// @param rhs The right hand side bound parameters +/// +/// @return The sum of the bound parameters +inline BoundVector addBoundParameters(const BoundVector& lhs, + const BoundVector& rhs) { + return normalizeBoundParameters(lhs + rhs); +} + /// Subtract bound parameters and take care of angle periodicity for phi and -/// theta. +/// theta. This is intended for small differences only i.e. KF updates. /// /// @param lhs The left hand side bound parameters /// @param rhs The right hand side bound parameters @@ -61,10 +69,7 @@ inline BoundVector normalizeBoundParameters(const BoundVector& boundParams) { /// @return The difference of the bound parameters inline BoundVector subtractBoundParameters(const BoundVector& lhs, const BoundVector& rhs) { - BoundVector result = lhs - rhs; - result[eBoundPhi] = - detail::difference_periodic(lhs[eBoundPhi], rhs[eBoundPhi], 2 * M_PI); - return result; + return normalizeBoundParameters(lhs - rhs); } } // namespace Acts diff --git a/Core/include/Acts/EventData/TrackParametersConcept.hpp b/Core/include/Acts/EventData/TrackParametersConcept.hpp index b39255e5ef7..5e4982545a4 100644 --- a/Core/include/Acts/EventData/TrackParametersConcept.hpp +++ b/Core/include/Acts/EventData/TrackParametersConcept.hpp @@ -12,9 +12,7 @@ #include "Acts/Definitions/TrackParametrization.hpp" #include "Acts/Geometry/GeometryContext.hpp" -#include #include -#include namespace Acts { @@ -68,4 +66,5 @@ concept BoundTrackParametersConcept = { p.position(c) } -> std::same_as; }; }; + } // namespace Acts diff --git a/Core/src/EventData/TrackParameterHelpers.cpp b/Core/src/EventData/TrackParameterHelpers.cpp index 1399280796b..69c8cf7aafe 100644 --- a/Core/src/EventData/TrackParameterHelpers.cpp +++ b/Core/src/EventData/TrackParameterHelpers.cpp @@ -8,6 +8,8 @@ #include "Acts/EventData/TrackParameterHelpers.hpp" +#include + bool Acts::isBoundVectorValid(const BoundVector& v, double epsilon) { constexpr auto pi = std::numbers::pi_v; @@ -20,7 +22,7 @@ bool Acts::isBoundVectorValid(const BoundVector& v, double epsilon) { (v[eBoundTheta] + epsilon >= 0) && (v[eBoundTheta] - epsilon <= pi); bool qOverPValid = std::isfinite(v[eBoundQOverP]) && - (std::abs(v[eBoundQOverP]) + epsilon > 0); + (std::abs(v[eBoundQOverP]) - epsilon >= 0); bool timeValid = std::isfinite(v[eBoundTime]); return loc0Valid && loc1Valid && phiValid && thetaValid && qOverPValid && @@ -34,9 +36,9 @@ bool Acts::isFreeVectorValid(const FreeVector& v, double epsilon) { bool dir0Valid = std::isfinite(v[eFreeDir0]); bool dir1Valid = std::isfinite(v[eFreeDir1]); bool dir2Valid = std::isfinite(v[eFreeDir2]); - bool dirValid = (v.segment<3>(eFreeDir0).norm() + epsilon > 0); - bool qOverPValid = - std::isfinite(v[eFreeQOverP]) && (std::abs(v[eFreeQOverP]) + epsilon > 0); + bool dirValid = (std::abs(v.segment<3>(eFreeDir0).norm() - 1) - epsilon <= 0); + bool qOverPValid = std::isfinite(v[eFreeQOverP]) && + (std::abs(v[eFreeQOverP]) - epsilon >= 0); bool timeValid = std::isfinite(v[eFreeTime]); return pos0Valid && pos1Valid && pos2Valid && dir0Valid && dir1Valid && diff --git a/Tests/UnitTests/Core/EventData/CMakeLists.txt b/Tests/UnitTests/Core/EventData/CMakeLists.txt index 61b5079da8e..1efda4cff29 100644 --- a/Tests/UnitTests/Core/EventData/CMakeLists.txt +++ b/Tests/UnitTests/Core/EventData/CMakeLists.txt @@ -16,5 +16,6 @@ add_unittest(MultiTrajectoryHelpers MultiTrajectoryHelpersTests.cpp) add_unittest(SubspaceHelpers SubspaceHelpersTests.cpp) add_unittest(SeedEdm SeedEdmTests.cpp) add_unittest(SpacePointContainerEdm SpacePointContainerEdmTests.cpp) +add_unittest(TrackParameterHelpers TrackParameterHelpersTests.cpp) add_non_compile_test(MultiTrajectory TrackContainerComplianceTests.cpp) diff --git a/Tests/UnitTests/Core/EventData/TrackParameterHelpersTests.cpp b/Tests/UnitTests/Core/EventData/TrackParameterHelpersTests.cpp new file mode 100644 index 00000000000..cc2a5c10573 --- /dev/null +++ b/Tests/UnitTests/Core/EventData/TrackParameterHelpersTests.cpp @@ -0,0 +1,51 @@ +// This file is part of the ACTS project. +// +// Copyright (C) 2016 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 https://mozilla.org/MPL/2.0/. + +#include + +#include "Acts/Definitions/TrackParametrization.hpp" +#include "Acts/EventData/TrackParameterHelpers.hpp" +#include "Acts/Tests/CommonHelpers/FloatComparisons.hpp" + +BOOST_AUTO_TEST_SUITE(TrackParameterHelpers) + +BOOST_AUTO_TEST_CASE(isBoundVectorValid) { + BOOST_CHECK(!Acts::isBoundVectorValid({1, 2, 3, 4, 5, 6})); + BOOST_CHECK(Acts::isBoundVectorValid({1, 2, 1, 1, 5, 6})); +} + +BOOST_AUTO_TEST_CASE(isFreeVectorValid) { + BOOST_CHECK(!Acts::isFreeVectorValid({1, 2, 3, 4, 5, 6, 7, 8})); + BOOST_CHECK(Acts::isFreeVectorValid({1, 2, 3, 4, 1, 0, 0, 8})); +} + +BOOST_AUTO_TEST_CASE(normalizeBoundParameters) { + CHECK_CLOSE_OR_SMALL(Acts::normalizeBoundParameters({1, 2, 3, 4, 5, 6}), + Acts::BoundVector(1, 2, -0.141593, 2.28319, 5, 6), 1e-3, + 1e-3); +} + +BOOST_AUTO_TEST_CASE(addBoundParameters) { + CHECK_CLOSE_OR_SMALL( + Acts::addBoundParameters({1, 2, 3, 4, 5, 6}, {0, 0, 0, 0, 0, 0}), + Acts::normalizeBoundParameters({1, 2, 3, 4, 5, 6}), 1e-3, 1e-3); + CHECK_CLOSE_OR_SMALL( + Acts::addBoundParameters({1, 2, 3, 4, 5, 6}, {0, 0, 1, 1, 0, 0}), + Acts::normalizeBoundParameters({1, 2, 4, 5, 5, 6}), 1e-3, 1e-3); +} + +BOOST_AUTO_TEST_CASE(subtractBoundParameters) { + CHECK_CLOSE_OR_SMALL( + Acts::subtractBoundParameters({1, 2, 3, 4, 5, 6}, {1, 2, 3, 4, 5, 6}), + Acts::BoundVector(0, 0, 0, 0, 0, 0), 1e-3, 1e-3); + CHECK_CLOSE_OR_SMALL( + Acts::subtractBoundParameters({1, 2, 4, 5, 5, 6}, {0, 0, 1, 1, 0, 0}), + Acts::normalizeBoundParameters({1, 2, 3, 4, 5, 6}), 1e-3, 1e-3); +} + +BOOST_AUTO_TEST_SUITE_END() From 11f64beecd66ba0653c4a178c97eaafd820fcc6c Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Mon, 21 Oct 2024 13:48:25 +0200 Subject: [PATCH 06/10] includes; move again from pole --- .../Plugins/Json/TrackParametersJsonConverterTests.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Tests/UnitTests/Plugins/Json/TrackParametersJsonConverterTests.cpp b/Tests/UnitTests/Plugins/Json/TrackParametersJsonConverterTests.cpp index dfad89f3f59..8876580b44e 100644 --- a/Tests/UnitTests/Plugins/Json/TrackParametersJsonConverterTests.cpp +++ b/Tests/UnitTests/Plugins/Json/TrackParametersJsonConverterTests.cpp @@ -8,12 +8,12 @@ #include +#include "Acts/EventData/ParticleHypothesis.hpp" +#include "Acts/EventData/TrackParameters.hpp" #include "Acts/Plugins/Json/TrackParametersJsonConverter.hpp" #include "Acts/Surfaces/PlaneSurface.hpp" #include "Acts/Surfaces/RectangleBounds.hpp" -#include "Acts/Tests/CommonHelpers/FloatComparisons.hpp" -#include #include #include @@ -73,7 +73,7 @@ BOOST_AUTO_TEST_CASE(TrackParametersJsonIO) { ctpRead.referenceSurface().bounds()); // Bound track parameters conversion - Acts::BoundVector boundPosition{1., 2., 0, 0, 5., 6.}; + Acts::BoundVector boundPosition{1., 2., 1.0, 1.0, 5., 6.}; Acts::BoundTrackParameters btp(surface, boundPosition, boundCov, particle); nlohmann::json btpJson = btp; From fd1606109676da10d87832f8cfa3d7197af62f38 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Mon, 21 Oct 2024 13:59:10 +0200 Subject: [PATCH 07/10] maxAbsEta --- .../Acts/EventData/TrackParameterHelpers.hpp | 8 ++++++-- Core/src/EventData/TrackParameterHelpers.cpp | 18 ++++++++++++++---- 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/Core/include/Acts/EventData/TrackParameterHelpers.hpp b/Core/include/Acts/EventData/TrackParameterHelpers.hpp index 1bfa0574df8..84234db0295 100644 --- a/Core/include/Acts/EventData/TrackParameterHelpers.hpp +++ b/Core/include/Acts/EventData/TrackParameterHelpers.hpp @@ -21,9 +21,11 @@ namespace Acts { /// /// @param v The bound vector to check /// @param epsilon The epsilon to use for the checks +/// @param maxAbsEta The maximum allowed eta value /// /// @return True if the bound vector is valid -bool isBoundVectorValid(const BoundVector& v, double epsilon = 1e-6); +bool isBoundVectorValid(const BoundVector& v, double epsilon = 1e-6, + double maxAbsEta = 6.); /// Check if a free vector is valid. This checks the following: /// - All values are finite @@ -32,9 +34,11 @@ bool isBoundVectorValid(const BoundVector& v, double epsilon = 1e-6); /// /// @param v The free vector to check /// @param epsilon The epsilon to use for the checks +/// @param maxAbsEta The maximum allowed eta value /// /// @return True if the free vector is valid -bool isFreeVectorValid(const FreeVector& v, double epsilon = 1e-6); +bool isFreeVectorValid(const FreeVector& v, double epsilon = 1e-6, + double maxAbsEta = 6.); /// Normalize the bound parameter angles /// diff --git a/Core/src/EventData/TrackParameterHelpers.cpp b/Core/src/EventData/TrackParameterHelpers.cpp index 69c8cf7aafe..e882dbce27f 100644 --- a/Core/src/EventData/TrackParameterHelpers.cpp +++ b/Core/src/EventData/TrackParameterHelpers.cpp @@ -8,9 +8,12 @@ #include "Acts/EventData/TrackParameterHelpers.hpp" +#include "Acts/Utilities/VectorHelpers.hpp" + #include -bool Acts::isBoundVectorValid(const BoundVector& v, double epsilon) { +bool Acts::isBoundVectorValid(const BoundVector& v, double epsilon, + double maxAbsEta) { constexpr auto pi = std::numbers::pi_v; bool loc0Valid = std::isfinite(v[eBoundLoc0]); @@ -25,11 +28,15 @@ bool Acts::isBoundVectorValid(const BoundVector& v, double epsilon) { (std::abs(v[eBoundQOverP]) - epsilon >= 0); bool timeValid = std::isfinite(v[eBoundTime]); + double eta = -std::log(std::tan(v[eBoundTheta] / 2)); + bool etaValid = std::isfinite(eta) && (std::abs(eta) - epsilon <= maxAbsEta); + return loc0Valid && loc1Valid && phiValid && thetaValid && qOverPValid && - timeValid; + timeValid && etaValid; } -bool Acts::isFreeVectorValid(const FreeVector& v, double epsilon) { +bool Acts::isFreeVectorValid(const FreeVector& v, double epsilon, + double maxAbsEta) { bool pos0Valid = std::isfinite(v[eFreePos0]); bool pos1Valid = std::isfinite(v[eFreePos1]); bool pos2Valid = std::isfinite(v[eFreePos2]); @@ -41,6 +48,9 @@ bool Acts::isFreeVectorValid(const FreeVector& v, double epsilon) { (std::abs(v[eFreeQOverP]) - epsilon >= 0); bool timeValid = std::isfinite(v[eFreeTime]); + double eta = VectorHelpers::eta(v.segment<3>(eFreeDir0)); + bool etaValid = std::isfinite(eta) && (std::abs(eta) - epsilon <= maxAbsEta); + return pos0Valid && pos1Valid && pos2Valid && dir0Valid && dir1Valid && - dir2Valid && dirValid && qOverPValid && timeValid; + dir2Valid && dirValid && qOverPValid && timeValid && etaValid; } From bb83ef7206714ae1cea4bb0dc5293078eb03966c Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Mon, 21 Oct 2024 14:45:48 +0200 Subject: [PATCH 08/10] revert some changes --- .../Acts/EventData/TrackParameterHelpers.hpp | 19 +++++-------------- .../EventData/TrackParameterHelpersTests.cpp | 18 ------------------ 2 files changed, 5 insertions(+), 32 deletions(-) diff --git a/Core/include/Acts/EventData/TrackParameterHelpers.hpp b/Core/include/Acts/EventData/TrackParameterHelpers.hpp index 84234db0295..2de51ae688a 100644 --- a/Core/include/Acts/EventData/TrackParameterHelpers.hpp +++ b/Core/include/Acts/EventData/TrackParameterHelpers.hpp @@ -52,20 +52,8 @@ inline BoundVector normalizeBoundParameters(const BoundVector& boundParams) { return result; } -/// Add bound parameters and take care of angle periodicity for phi and theta. -/// This is intended for small differences only i.e. KF updates. -/// -/// @param lhs The left hand side bound parameters -/// @param rhs The right hand side bound parameters -/// -/// @return The sum of the bound parameters -inline BoundVector addBoundParameters(const BoundVector& lhs, - const BoundVector& rhs) { - return normalizeBoundParameters(lhs + rhs); -} - /// Subtract bound parameters and take care of angle periodicity for phi and -/// theta. This is intended for small differences only i.e. KF updates. +/// theta. /// /// @param lhs The left hand side bound parameters /// @param rhs The right hand side bound parameters @@ -73,7 +61,10 @@ inline BoundVector addBoundParameters(const BoundVector& lhs, /// @return The difference of the bound parameters inline BoundVector subtractBoundParameters(const BoundVector& lhs, const BoundVector& rhs) { - return normalizeBoundParameters(lhs - rhs); + BoundVector result = lhs - rhs; + result[eBoundPhi] = + detail::difference_periodic(lhs[eBoundPhi], rhs[eBoundPhi], 2 * M_PI); + return result; } } // namespace Acts diff --git a/Tests/UnitTests/Core/EventData/TrackParameterHelpersTests.cpp b/Tests/UnitTests/Core/EventData/TrackParameterHelpersTests.cpp index cc2a5c10573..3167d8e20e5 100644 --- a/Tests/UnitTests/Core/EventData/TrackParameterHelpersTests.cpp +++ b/Tests/UnitTests/Core/EventData/TrackParameterHelpersTests.cpp @@ -30,22 +30,4 @@ BOOST_AUTO_TEST_CASE(normalizeBoundParameters) { 1e-3); } -BOOST_AUTO_TEST_CASE(addBoundParameters) { - CHECK_CLOSE_OR_SMALL( - Acts::addBoundParameters({1, 2, 3, 4, 5, 6}, {0, 0, 0, 0, 0, 0}), - Acts::normalizeBoundParameters({1, 2, 3, 4, 5, 6}), 1e-3, 1e-3); - CHECK_CLOSE_OR_SMALL( - Acts::addBoundParameters({1, 2, 3, 4, 5, 6}, {0, 0, 1, 1, 0, 0}), - Acts::normalizeBoundParameters({1, 2, 4, 5, 5, 6}), 1e-3, 1e-3); -} - -BOOST_AUTO_TEST_CASE(subtractBoundParameters) { - CHECK_CLOSE_OR_SMALL( - Acts::subtractBoundParameters({1, 2, 3, 4, 5, 6}, {1, 2, 3, 4, 5, 6}), - Acts::BoundVector(0, 0, 0, 0, 0, 0), 1e-3, 1e-3); - CHECK_CLOSE_OR_SMALL( - Acts::subtractBoundParameters({1, 2, 4, 5, 5, 6}, {0, 0, 1, 1, 0, 0}), - Acts::normalizeBoundParameters({1, 2, 3, 4, 5, 6}), 1e-3, 1e-3); -} - BOOST_AUTO_TEST_SUITE_END() From c9a259e60f5e014ad3d545f8b587008f83aebf1e Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Mon, 21 Oct 2024 15:47:35 +0200 Subject: [PATCH 09/10] revert unit test --- .../Plugins/Json/TrackParametersJsonConverterTests.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/UnitTests/Plugins/Json/TrackParametersJsonConverterTests.cpp b/Tests/UnitTests/Plugins/Json/TrackParametersJsonConverterTests.cpp index 8876580b44e..5c0882cb12c 100644 --- a/Tests/UnitTests/Plugins/Json/TrackParametersJsonConverterTests.cpp +++ b/Tests/UnitTests/Plugins/Json/TrackParametersJsonConverterTests.cpp @@ -73,7 +73,7 @@ BOOST_AUTO_TEST_CASE(TrackParametersJsonIO) { ctpRead.referenceSurface().bounds()); // Bound track parameters conversion - Acts::BoundVector boundPosition{1., 2., 1.0, 1.0, 5., 6.}; + Acts::BoundVector boundPosition{1., 2., 3., 4., 5., 6.}; Acts::BoundTrackParameters btp(surface, boundPosition, boundCov, particle); nlohmann::json btpJson = btp; From c92b75f3fd2edd8f7f11e90f7eb0dd0ae1704146 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Wed, 23 Oct 2024 09:38:23 +0200 Subject: [PATCH 10/10] optionally validate angle range --- .../EventData/GenericBoundTrackParameters.hpp | 4 ++++ .../EventData/GenericFreeTrackParameters.hpp | 9 ++++++++- .../Acts/EventData/TrackParameterHelpers.hpp | 9 +++++---- Core/src/EventData/TrackParameterHelpers.cpp | 19 +++++++++++-------- .../EventData/TrackParameterHelpersTests.cpp | 4 ++-- 5 files changed, 30 insertions(+), 15 deletions(-) diff --git a/Core/include/Acts/EventData/GenericBoundTrackParameters.hpp b/Core/include/Acts/EventData/GenericBoundTrackParameters.hpp index 9f790634f3c..38f297764b1 100644 --- a/Core/include/Acts/EventData/GenericBoundTrackParameters.hpp +++ b/Core/include/Acts/EventData/GenericBoundTrackParameters.hpp @@ -9,6 +9,7 @@ #pragma once #include "Acts/Definitions/Tolerance.hpp" +#include "Acts/EventData/TrackParameterHelpers.hpp" #include "Acts/EventData/TransformationHelpers.hpp" #include "Acts/EventData/detail/PrintParameters.hpp" #include "Acts/Surfaces/Surface.hpp" @@ -60,6 +61,9 @@ class GenericBoundTrackParameters { m_cov(std::move(cov)), m_surface(std::move(surface)), m_particleHypothesis(std::move(particleHypothesis)) { + // TODO set `validateAngleRange` to `true` after fixing caller code + assert(isBoundVectorValid(m_params, false) && + "Invalid bound parameters vector"); assert(m_surface != nullptr && "Reference surface must not be null"); normalizePhiTheta(); } diff --git a/Core/include/Acts/EventData/GenericFreeTrackParameters.hpp b/Core/include/Acts/EventData/GenericFreeTrackParameters.hpp index 014634cada8..7a2b4b46522 100644 --- a/Core/include/Acts/EventData/GenericFreeTrackParameters.hpp +++ b/Core/include/Acts/EventData/GenericFreeTrackParameters.hpp @@ -11,6 +11,7 @@ #include "Acts/Definitions/Algebra.hpp" #include "Acts/Definitions/Common.hpp" #include "Acts/Definitions/TrackParametrization.hpp" +#include "Acts/EventData/TrackParameterHelpers.hpp" #include "Acts/EventData/TrackParametersConcept.hpp" #include "Acts/EventData/TransformationHelpers.hpp" #include "Acts/EventData/detail/PrintParameters.hpp" @@ -53,7 +54,9 @@ class GenericFreeTrackParameters { ParticleHypothesis particleHypothesis) : m_params(params), m_cov(std::move(cov)), - m_particleHypothesis(std::move(particleHypothesis)) {} + m_particleHypothesis(std::move(particleHypothesis)) { + assert(isFreeVectorValid(m_params) && "Invalid free parameters vector"); + } /// Construct from four-position, direction, absolute momentum, and charge. /// @@ -76,6 +79,8 @@ class GenericFreeTrackParameters { m_params[eFreeDir1] = dir[eMom1]; m_params[eFreeDir2] = dir[eMom2]; m_params[eFreeQOverP] = qOverP; + + assert(isFreeVectorValid(m_params) && "Invalid free parameters vector"); } /// Construct from four-position, angles, absolute momentum, and charge. @@ -101,6 +106,8 @@ class GenericFreeTrackParameters { m_params[eFreeDir1] = dir[eMom1]; m_params[eFreeDir2] = dir[eMom2]; m_params[eFreeQOverP] = qOverP; + + assert(isFreeVectorValid(m_params) && "Invalid free parameters vector"); } /// Converts a free track parameter with a different hypothesis. diff --git a/Core/include/Acts/EventData/TrackParameterHelpers.hpp b/Core/include/Acts/EventData/TrackParameterHelpers.hpp index 2de51ae688a..fd7148ab36a 100644 --- a/Core/include/Acts/EventData/TrackParameterHelpers.hpp +++ b/Core/include/Acts/EventData/TrackParameterHelpers.hpp @@ -15,17 +15,18 @@ namespace Acts { /// Check if a bound vector is valid. This checks the following: /// - All values are finite -/// - The phi value is in the range [-pi, pi) -/// - The theta value is in the range [0, pi] +/// - (optionally) The phi value is in the range [-pi, pi) +/// - (optionally) The theta value is in the range [0, pi] /// - The q/p value is non-zero /// /// @param v The bound vector to check +/// @param validateAngleRange If true, the phi and theta values are range checked /// @param epsilon The epsilon to use for the checks /// @param maxAbsEta The maximum allowed eta value /// /// @return True if the bound vector is valid -bool isBoundVectorValid(const BoundVector& v, double epsilon = 1e-6, - double maxAbsEta = 6.); +bool isBoundVectorValid(const BoundVector& v, bool validateAngleRange, + double epsilon = 1e-6, double maxAbsEta = 6.); /// Check if a free vector is valid. This checks the following: /// - All values are finite diff --git a/Core/src/EventData/TrackParameterHelpers.cpp b/Core/src/EventData/TrackParameterHelpers.cpp index e882dbce27f..d580366d3f7 100644 --- a/Core/src/EventData/TrackParameterHelpers.cpp +++ b/Core/src/EventData/TrackParameterHelpers.cpp @@ -12,22 +12,25 @@ #include -bool Acts::isBoundVectorValid(const BoundVector& v, double epsilon, - double maxAbsEta) { +bool Acts::isBoundVectorValid(const BoundVector& v, bool validateAngleRange, + double epsilon, double maxAbsEta) { constexpr auto pi = std::numbers::pi_v; bool loc0Valid = std::isfinite(v[eBoundLoc0]); bool loc1Valid = std::isfinite(v[eBoundLoc1]); - bool phiValid = std::isfinite(v[eBoundPhi]) && - (v[eBoundPhi] + epsilon >= -pi) && - (v[eBoundPhi] - epsilon < pi); - bool thetaValid = std::isfinite(v[eBoundTheta]) && - (v[eBoundTheta] + epsilon >= 0) && - (v[eBoundTheta] - epsilon <= pi); + bool phiValid = std::isfinite(v[eBoundPhi]); + bool thetaValid = std::isfinite(v[eBoundTheta]); bool qOverPValid = std::isfinite(v[eBoundQOverP]) && (std::abs(v[eBoundQOverP]) - epsilon >= 0); bool timeValid = std::isfinite(v[eBoundTime]); + if (validateAngleRange) { + phiValid = phiValid && (v[eBoundPhi] + epsilon >= -pi) && + (v[eBoundPhi] - epsilon < pi); + thetaValid = thetaValid && (v[eBoundTheta] + epsilon >= 0) && + (v[eBoundTheta] - epsilon <= pi); + } + double eta = -std::log(std::tan(v[eBoundTheta] / 2)); bool etaValid = std::isfinite(eta) && (std::abs(eta) - epsilon <= maxAbsEta); diff --git a/Tests/UnitTests/Core/EventData/TrackParameterHelpersTests.cpp b/Tests/UnitTests/Core/EventData/TrackParameterHelpersTests.cpp index 3167d8e20e5..4520593ec37 100644 --- a/Tests/UnitTests/Core/EventData/TrackParameterHelpersTests.cpp +++ b/Tests/UnitTests/Core/EventData/TrackParameterHelpersTests.cpp @@ -15,8 +15,8 @@ BOOST_AUTO_TEST_SUITE(TrackParameterHelpers) BOOST_AUTO_TEST_CASE(isBoundVectorValid) { - BOOST_CHECK(!Acts::isBoundVectorValid({1, 2, 3, 4, 5, 6})); - BOOST_CHECK(Acts::isBoundVectorValid({1, 2, 1, 1, 5, 6})); + BOOST_CHECK(!Acts::isBoundVectorValid({1, 2, 3, 4, 5, 6}, true)); + BOOST_CHECK(Acts::isBoundVectorValid({1, 2, 1, 1, 5, 6}, true)); } BOOST_AUTO_TEST_CASE(isFreeVectorValid) {