From 436c636830ab23f6b320f925e3dc7abaf975a8c9 Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Thu, 7 Nov 2024 16:12:38 +0100 Subject: [PATCH 01/24] fix: Restore Orthogonal seedfinder default constructor (#3827) Uses `getDummyLogger` to restore the default constructor for the orthogonal seed finder. See #3807. --- Core/include/Acts/Seeding/SeedFinderOrthogonal.hpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Core/include/Acts/Seeding/SeedFinderOrthogonal.hpp b/Core/include/Acts/Seeding/SeedFinderOrthogonal.hpp index 594e35b44cc..a4129fad556 100644 --- a/Core/include/Acts/Seeding/SeedFinderOrthogonal.hpp +++ b/Core/include/Acts/Seeding/SeedFinderOrthogonal.hpp @@ -65,7 +65,7 @@ class SeedFinderOrthogonal { */ ~SeedFinderOrthogonal() = default; - SeedFinderOrthogonal() = delete; + SeedFinderOrthogonal() = default; SeedFinderOrthogonal(const SeedFinderOrthogonal &) = delete; SeedFinderOrthogonal &operator=( @@ -254,7 +254,7 @@ class SeedFinderOrthogonal { /** * @brief The logger */ - std::unique_ptr m_logger{nullptr}; + std::unique_ptr m_logger{getDummyLogger().clone()}; }; } // namespace Acts From 76bc018dfc5ecdc76d9f34cb38b4750dbb55fb16 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Thu, 7 Nov 2024 20:44:21 +0100 Subject: [PATCH 02/24] refactor: FPE safe eta/theta conversion (#3788) ## Summary by CodeRabbit - **New Features** - Introduced a new structure for angle conversion traits, enhancing precision and safety for angle calculations. - Added safety checks in angle conversion functions to handle edge cases more gracefully. - **Bug Fixes** - Updated angle conversion functions to prevent floating-point exceptions by implementing bounds checking. - **Tests** - Added new data-driven test cases to validate the robustness of angle conversion functions, ensuring consistent and valid outputs across a range of inputs. --- Core/include/Acts/Utilities/AngleHelpers.hpp | 44 ++++++++++- .../Core/TrackFinding/TrackSelectorTests.cpp | 73 +++++++++---------- .../Core/Utilities/AngleHelpersTests.cpp | 72 +++++++++++++++++- 3 files changed, 148 insertions(+), 41 deletions(-) diff --git a/Core/include/Acts/Utilities/AngleHelpers.hpp b/Core/include/Acts/Utilities/AngleHelpers.hpp index 02feec2323e..8a38aeffc94 100644 --- a/Core/include/Acts/Utilities/AngleHelpers.hpp +++ b/Core/include/Acts/Utilities/AngleHelpers.hpp @@ -9,26 +9,68 @@ #pragma once #include +#include namespace Acts::AngleHelpers { +template +struct EtaThetaConversionTraits {}; + +template <> +struct EtaThetaConversionTraits { + static constexpr float minTheta = 1e-6f; + static constexpr float maxTheta = std::numbers::pi_v - minTheta; + + static constexpr float maxEta = 80.0f; + static constexpr float minEta = -maxEta; +}; + +template <> +struct EtaThetaConversionTraits { + static constexpr double minTheta = 1e-12; + static constexpr double maxTheta = std::numbers::pi - minTheta; + + static constexpr double maxEta = 700.0; + static constexpr double minEta = -maxEta; +}; + /// Calculate the pseudorapidity from the polar angle theta. /// +/// This function aims to be FPE safe and returns infinity for theta values +/// outside the floating point precision range. +/// /// @param theta is the polar angle in radian towards the z-axis. /// /// @return the pseudorapidity towards the z-axis. template Scalar etaFromTheta(Scalar theta) { - return -std::log(std::tan(0.5 * theta)); + if (theta <= EtaThetaConversionTraits::minTheta) { + return std::numeric_limits::infinity(); + } + if (theta >= EtaThetaConversionTraits::maxTheta) { + return -std::numeric_limits::infinity(); + } + + return -std::log(std::tan(theta / 2)); } /// Calculate the polar angle theta from the pseudorapidity. /// +/// This function aims to be FPE safe and returns 0/pi for eta values outside +/// the floating point precision range. +/// /// @param eta is the pseudorapidity towards the z-axis. /// /// @return the polar angle in radian towards the z-axis. template Scalar thetaFromEta(Scalar eta) { + if (eta <= EtaThetaConversionTraits::minEta) { + return std::numbers::pi_v; + } + if (eta >= EtaThetaConversionTraits::maxEta) { + return 0; + } + return 2 * std::atan(std::exp(-eta)); } diff --git a/Tests/UnitTests/Core/TrackFinding/TrackSelectorTests.cpp b/Tests/UnitTests/Core/TrackFinding/TrackSelectorTests.cpp index f149010aef8..af7eef54015 100644 --- a/Tests/UnitTests/Core/TrackFinding/TrackSelectorTests.cpp +++ b/Tests/UnitTests/Core/TrackFinding/TrackSelectorTests.cpp @@ -19,6 +19,7 @@ #include "Acts/Surfaces/PerigeeSurface.hpp" #include "Acts/Surfaces/PlaneSurface.hpp" #include "Acts/TrackFinding/TrackSelector.hpp" +#include "Acts/Utilities/AngleHelpers.hpp" #include @@ -83,10 +84,6 @@ struct MockTrack { TrackStateRange trackStatesReversed() const { return {}; } }; -double thetaFromEta(double eta) { - return 2 * std::atan(std::exp(-eta)); -} - BOOST_AUTO_TEST_SUITE(TrackSelectorTests) std::vector etaValues{-5.0, -4.5, -4.0, -3.5, -3.0, -2.5, -2.0, -1.5, @@ -97,7 +94,7 @@ BOOST_DATA_TEST_CASE(TestSingleBinCase, bdata::make(etaValues), eta) { TrackSelector::EtaBinnedConfig cfgBase; MockTrack baseTrack{}; - baseTrack.m_theta = thetaFromEta(eta); + baseTrack.m_theta = AngleHelpers::thetaFromEta(eta); baseTrack.m_phi = 0.5; baseTrack.m_pt = 0.5; baseTrack.m_loc0 = 0.5; @@ -206,9 +203,9 @@ BOOST_DATA_TEST_CASE(TestSingleBinCase, bdata::make(etaValues), eta) { cfg.cutSets.at(0).etaMin = {-1.0}; TrackSelector selector{cfg}; MockTrack track = baseTrack; - track.m_theta = thetaFromEta(0.5); + track.m_theta = AngleHelpers::thetaFromEta(0.5); BOOST_CHECK(selector.isValidTrack(track)); - track.m_theta = thetaFromEta(-1.1); + track.m_theta = AngleHelpers::thetaFromEta(-1.1); BOOST_CHECK(!selector.isValidTrack(track)); } @@ -218,9 +215,9 @@ BOOST_DATA_TEST_CASE(TestSingleBinCase, bdata::make(etaValues), eta) { cfg.cutSets.at(0).etaMax = {1.0}; TrackSelector selector{cfg}; MockTrack track = baseTrack; - track.m_theta = thetaFromEta(0.5); + track.m_theta = AngleHelpers::thetaFromEta(0.5); BOOST_CHECK(selector.isValidTrack(track)); - track.m_theta = thetaFromEta(1.1); + track.m_theta = AngleHelpers::thetaFromEta(1.1); BOOST_CHECK(!selector.isValidTrack(track)); } @@ -231,11 +228,11 @@ BOOST_DATA_TEST_CASE(TestSingleBinCase, bdata::make(etaValues), eta) { cfg.cutSets.at(0).etaMax = {1.0}; TrackSelector selector{cfg}; MockTrack track = baseTrack; - track.m_theta = thetaFromEta(0.5); + track.m_theta = AngleHelpers::thetaFromEta(0.5); BOOST_CHECK(selector.isValidTrack(track)); - track.m_theta = thetaFromEta(-1.1); + track.m_theta = AngleHelpers::thetaFromEta(-1.1); BOOST_CHECK(!selector.isValidTrack(track)); - track.m_theta = thetaFromEta(1.1); + track.m_theta = AngleHelpers::thetaFromEta(1.1); BOOST_CHECK(!selector.isValidTrack(track)); } @@ -245,14 +242,14 @@ BOOST_DATA_TEST_CASE(TestSingleBinCase, bdata::make(etaValues), eta) { cfg.cutSets.at(0).absEtaMin = {0.2}; TrackSelector selector{cfg}; MockTrack track = baseTrack; - track.m_theta = thetaFromEta(0.5); + track.m_theta = AngleHelpers::thetaFromEta(0.5); BOOST_CHECK(selector.isValidTrack(track)); - track.m_theta = thetaFromEta(-0.5); + track.m_theta = AngleHelpers::thetaFromEta(-0.5); BOOST_CHECK(selector.isValidTrack(track)); - track.m_theta = thetaFromEta(0.1); + track.m_theta = AngleHelpers::thetaFromEta(0.1); BOOST_CHECK(!selector.isValidTrack(track)); - track.m_theta = thetaFromEta(-0.1); + track.m_theta = AngleHelpers::thetaFromEta(-0.1); BOOST_CHECK(!selector.isValidTrack(track)); } @@ -262,14 +259,14 @@ BOOST_DATA_TEST_CASE(TestSingleBinCase, bdata::make(etaValues), eta) { cfg.cutSets.at(0).absEtaMax = {1.0}; TrackSelector selector{cfg}; MockTrack track = baseTrack; - track.m_theta = thetaFromEta(0.5); + track.m_theta = AngleHelpers::thetaFromEta(0.5); BOOST_CHECK(selector.isValidTrack(track)); - track.m_theta = thetaFromEta(-0.5); + track.m_theta = AngleHelpers::thetaFromEta(-0.5); BOOST_CHECK(selector.isValidTrack(track)); - track.m_theta = thetaFromEta(1.1); + track.m_theta = AngleHelpers::thetaFromEta(1.1); BOOST_CHECK(!selector.isValidTrack(track)); - track.m_theta = thetaFromEta(-1.1); + track.m_theta = AngleHelpers::thetaFromEta(-1.1); BOOST_CHECK(!selector.isValidTrack(track)); } @@ -280,19 +277,19 @@ BOOST_DATA_TEST_CASE(TestSingleBinCase, bdata::make(etaValues), eta) { cfg.cutSets.at(0).absEtaMax = {1.0}; TrackSelector selector{cfg}; MockTrack track = baseTrack; - track.m_theta = thetaFromEta(0.5); + track.m_theta = AngleHelpers::thetaFromEta(0.5); BOOST_CHECK(selector.isValidTrack(track)); - track.m_theta = thetaFromEta(-0.5); + track.m_theta = AngleHelpers::thetaFromEta(-0.5); BOOST_CHECK(selector.isValidTrack(track)); - track.m_theta = thetaFromEta(0.1); + track.m_theta = AngleHelpers::thetaFromEta(0.1); BOOST_CHECK(!selector.isValidTrack(track)); - track.m_theta = thetaFromEta(-0.1); + track.m_theta = AngleHelpers::thetaFromEta(-0.1); BOOST_CHECK(!selector.isValidTrack(track)); - track.m_theta = thetaFromEta(1.1); + track.m_theta = AngleHelpers::thetaFromEta(1.1); BOOST_CHECK(!selector.isValidTrack(track)); - track.m_theta = thetaFromEta(-1.1); + track.m_theta = AngleHelpers::thetaFromEta(-1.1); BOOST_CHECK(!selector.isValidTrack(track)); } @@ -373,27 +370,27 @@ BOOST_AUTO_TEST_CASE(TestSingleBinEtaCutByBinEdge) { BOOST_TEST_INFO_SCOPE(selector.config()); MockTrack track{}; - track.m_theta = thetaFromEta(0.0); + track.m_theta = AngleHelpers::thetaFromEta(0.0); BOOST_CHECK(!selector.isValidTrack(track)); - track.m_theta = thetaFromEta(0.5); + track.m_theta = AngleHelpers::thetaFromEta(0.5); BOOST_CHECK(!selector.isValidTrack(track)); // cannot easily check on-edge behavior because of floating point arithmetic // (it won't be exactly 1.0 in selector) - track.m_theta = thetaFromEta(1.01); + track.m_theta = AngleHelpers::thetaFromEta(1.01); BOOST_CHECK(selector.isValidTrack(track)); - track.m_theta = thetaFromEta(1.5); + track.m_theta = AngleHelpers::thetaFromEta(1.5); BOOST_CHECK(selector.isValidTrack(track)); - track.m_theta = thetaFromEta(2.0); + track.m_theta = AngleHelpers::thetaFromEta(2.0); BOOST_CHECK(!selector.isValidTrack(track)); } BOOST_AUTO_TEST_CASE(TestMultiBinCuts) { MockTrack baseTrack{}; - baseTrack.m_theta = thetaFromEta(1.0); + baseTrack.m_theta = AngleHelpers::thetaFromEta(1.0); baseTrack.m_phi = 0.5; baseTrack.m_pt = 0.5; baseTrack.m_loc0 = 0.5; @@ -425,7 +422,7 @@ BOOST_AUTO_TEST_CASE(TestMultiBinCuts) { { // exactly at zero MockTrack track = baseTrack; - track.m_theta = thetaFromEta(0.0); + track.m_theta = AngleHelpers::thetaFromEta(0.0); BOOST_CHECK(selector.isValidTrack(track)); @@ -439,7 +436,7 @@ BOOST_AUTO_TEST_CASE(TestMultiBinCuts) { { // first bin MockTrack track = baseTrack; - track.m_theta = thetaFromEta(1.0); + track.m_theta = AngleHelpers::thetaFromEta(1.0); BOOST_CHECK(selector.isValidTrack(track)); @@ -453,8 +450,8 @@ BOOST_AUTO_TEST_CASE(TestMultiBinCuts) { { // first bin edge MockTrack track = baseTrack; - track.m_theta = - thetaFromEta(2.0 - std::numeric_limits::epsilon()); + track.m_theta = AngleHelpers::thetaFromEta( + 2.0 - std::numeric_limits::epsilon()); BOOST_CHECK(selector.isValidTrack(track)); @@ -468,7 +465,7 @@ BOOST_AUTO_TEST_CASE(TestMultiBinCuts) { { // second bin lower edge MockTrack track = baseTrack; - track.m_theta = thetaFromEta(2.0); + track.m_theta = AngleHelpers::thetaFromEta(2.0); BOOST_CHECK(selector.isValidTrack(track)); @@ -488,7 +485,7 @@ BOOST_AUTO_TEST_CASE(TestMultiBinCuts) { { // second bin MockTrack track = baseTrack; - track.m_theta = thetaFromEta(666.0); + track.m_theta = AngleHelpers::thetaFromEta(10.0); track.*prop = -1.1; BOOST_CHECK(selector.isValidTrack(track)); diff --git a/Tests/UnitTests/Core/Utilities/AngleHelpersTests.cpp b/Tests/UnitTests/Core/Utilities/AngleHelpersTests.cpp index 5898986e6d5..78e1252ad18 100644 --- a/Tests/UnitTests/Core/Utilities/AngleHelpersTests.cpp +++ b/Tests/UnitTests/Core/Utilities/AngleHelpersTests.cpp @@ -6,24 +6,92 @@ // 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 #include "Acts/Tests/CommonHelpers/FloatComparisons.hpp" #include "Acts/Utilities/AngleHelpers.hpp" +#include #include +namespace bd = boost::unit_test::data; + using Acts::AngleHelpers::etaFromTheta; +using Acts::AngleHelpers::EtaThetaConversionTraits; using Acts::AngleHelpers::thetaFromEta; BOOST_AUTO_TEST_SUITE(AngleHelpers) BOOST_AUTO_TEST_CASE(EtaThetaConversion) { CHECK_CLOSE_ABS(0.0, etaFromTheta(std::numbers::pi / 2), 1e-6); - CHECK_CLOSE_ABS(1.0, etaFromTheta(thetaFromEta(1.0)), 1e-6); - CHECK_CLOSE_ABS(1.0, thetaFromEta(etaFromTheta(1.0)), 1e-6); } +BOOST_DATA_TEST_CASE(EtaFromThetaRobustness, bd::xrange(0, 1000, 1), exponent) { + { + // check right + + float thetaRight = exponent < 30 ? std::pow(10.0f, -1.0f * exponent) : 0.0f; + + float etaRight = etaFromTheta(thetaRight); + BOOST_CHECK(!std::isnan(etaRight)); + + // check left + + float thetaLeft = std::numbers::pi_v - thetaRight; + + float etaLeft = etaFromTheta(thetaLeft); + BOOST_CHECK(!std::isnan(etaLeft)); + } + + { + // check right + + double thetaRight = exponent < 300 ? std::pow(10.0, -1.0 * exponent) : 0.0; + + double etaRight = etaFromTheta(thetaRight); + BOOST_CHECK(!std::isnan(etaRight)); + + // check left + + double thetaLeft = std::numbers::pi - thetaRight; + + double etaLeft = etaFromTheta(thetaLeft); + BOOST_CHECK(!std::isnan(etaLeft)); + } +} + +BOOST_DATA_TEST_CASE(ThetaFromEtaRobustness, bd::xrange(1.0, 1000.0, 1.0), + etaRight) { + { + // check right + + float thetaRight = thetaFromEta(etaRight); + BOOST_CHECK(!std::isnan(thetaRight)); + + // check left + + float etaLeft = -etaRight; + + float thetaLeft = thetaFromEta(etaLeft); + BOOST_CHECK(!std::isnan(thetaLeft)); + } + + { + // check right + + double thetaRight = thetaFromEta(etaRight); + BOOST_CHECK(!std::isnan(thetaRight)); + + // check left + + double etaLeft = -etaRight; + + double thetaLeft = thetaFromEta(etaLeft); + BOOST_CHECK(!std::isnan(thetaLeft)); + } +} + BOOST_AUTO_TEST_SUITE_END() From 5b57201dc7552d38473e5affab79cfd7a73eb24f Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Thu, 7 Nov 2024 22:51:15 +0100 Subject: [PATCH 03/24] refactor: rename PortalShell connect outer to fill (#3820) This renames the `connectOuter` method in `PortalShell` to just `fill`, because that is more descriptive of what it actually does. Part of #3502 --- Core/include/Acts/Geometry/PortalShell.hpp | 12 +++++------- Core/src/Geometry/PortalShell.cpp | 2 +- Tests/UnitTests/Core/Geometry/PortalShellTests.cpp | 4 ++-- 3 files changed, 8 insertions(+), 10 deletions(-) diff --git a/Core/include/Acts/Geometry/PortalShell.hpp b/Core/include/Acts/Geometry/PortalShell.hpp index 7405a64ad0a..fe9f606c576 100644 --- a/Core/include/Acts/Geometry/PortalShell.hpp +++ b/Core/include/Acts/Geometry/PortalShell.hpp @@ -34,12 +34,10 @@ class PortalShellBase { /// Virtusl destructor virtual ~PortalShellBase() = default; - /// Connect a volume to the outer side of all portal shells. Which "side" is - /// "outer" depends on the volume type. - /// This method essentially creates a @c TrivialPortalLink on the unconnected - /// side of each portal that is part of the chell + /// Fill the open slots of the shell with a @c TrivialPortalLink + /// to the given @p volume. /// @param volume The volume to connect - virtual void connectOuter(TrackingVolume& volume) = 0; + virtual void fill(TrackingVolume& volume) = 0; /// Get the number of portals in the shell. This number depends on the volume /// type @@ -88,8 +86,8 @@ class CylinderPortalShell : public PortalShellBase { /// @param face The face to set the portal virtual void setPortal(std::shared_ptr portal, Face face) = 0; - /// @copydoc PortalShellBase::connectOuter - void connectOuter(TrackingVolume& volume) override; + /// @copydoc PortalShellBase::fill + void fill(TrackingVolume& volume) override; }; /// Output stream operator for the CylinderPortalShell::Face enum diff --git a/Core/src/Geometry/PortalShell.cpp b/Core/src/Geometry/PortalShell.cpp index b9baf403812..5b2d8e167c8 100644 --- a/Core/src/Geometry/PortalShell.cpp +++ b/Core/src/Geometry/PortalShell.cpp @@ -22,7 +22,7 @@ namespace Acts { -void CylinderPortalShell::connectOuter(TrackingVolume& volume) { +void CylinderPortalShell::fill(TrackingVolume& volume) { for (Face face : {PositiveDisc, NegativeDisc, OuterCylinder, InnerCylinder, NegativePhiPlane, PositivePhiPlane}) { const auto& portalAtFace = portalPtr(face); diff --git a/Tests/UnitTests/Core/Geometry/PortalShellTests.cpp b/Tests/UnitTests/Core/Geometry/PortalShellTests.cpp index 15c5a9c7953..090dc851611 100644 --- a/Tests/UnitTests/Core/Geometry/PortalShellTests.cpp +++ b/Tests/UnitTests/Core/Geometry/PortalShellTests.cpp @@ -720,7 +720,7 @@ BOOST_AUTO_TEST_CASE(NestedStacks) { nullptr); } -BOOST_AUTO_TEST_CASE(ConnectOuter) { +BOOST_AUTO_TEST_CASE(Fill) { auto cyl1 = makeVolume(30_mm, 40_mm, 100_mm); auto cyl2 = makeVolume(0_mm, 50_mm, 110_mm); @@ -736,7 +736,7 @@ BOOST_AUTO_TEST_CASE(ConnectOuter) { BOOST_CHECK_EQUAL( shell.portal(NegativeDisc)->getLink(Direction::OppositeNormal), nullptr); - shell.connectOuter(cyl2); + shell.fill(cyl2); BOOST_CHECK_NE(shell.portal(OuterCylinder)->getLink(Direction::AlongNormal), nullptr); From 2b0cf145bd55a9f106370601efe760ecfbc6c584 Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Fri, 8 Nov 2024 00:05:51 +0100 Subject: [PATCH 04/24] fix: Default label for GraphViz::Node (#3821) --- Core/include/Acts/Utilities/GraphViz.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Core/include/Acts/Utilities/GraphViz.hpp b/Core/include/Acts/Utilities/GraphViz.hpp index 02cf9487d8d..8716fca7d00 100644 --- a/Core/include/Acts/Utilities/GraphViz.hpp +++ b/Core/include/Acts/Utilities/GraphViz.hpp @@ -92,7 +92,7 @@ std::ostream& operator<<(std::ostream& os, const Style& style); struct Node { std::string id; - std::string label; + std::string label = ""; Shape shape = Shape::Ellipse; std::vector