From a18fcd0cfa2330eab7ec02e8b355a34c42e9f916 Mon Sep 17 00:00:00 2001 From: Carlo Varni <75478407+CarloVarni@users.noreply.github.com> Date: Tue, 5 Nov 2024 15:58:15 +0100 Subject: [PATCH 01/17] feat: Logger passed to Gbts and Orthogonal seeders (#3807) Passing logger also to the GBTS and Orthogonal seeders ## Summary by CodeRabbit - **New Features** - Enhanced logging capabilities across various classes, including `GbtsTrackingFilter`, `SeedFinderGbts`, and `SeedFinderOrthogonal`, allowing for better tracking and debugging. - Introduced logger parameters in constructors for `SeedFinderGbts`, `SeedFinderOrthogonal`, and `SeedingOrthogonalAlgorithm`, improving configurability. - **Bug Fixes** - Improved error handling and logging for negative covariance values in `GbtsTrackingFilter`. - Adjusted warning messages in `GbtsSeedingAlgorithm` for better clarity. - **Refactor** - Updated member variable types to use smart pointers for better memory management in `SeedFinderGbts` and `SeedFinderOrthogonal`. - Streamlined initialization processes in `SeedingOrthogonalAlgorithm` and `GbtsSeedingAlgorithm` to enhance code clarity and efficiency. - Renamed logger instances for consistency across `SeedFilter` and `SeedFinder`. --- .../Acts/Seeding/GbtsTrackingFilter.hpp | 19 +++++++++++--- Core/include/Acts/Seeding/SeedFilter.hpp | 2 +- Core/include/Acts/Seeding/SeedFinder.hpp | 2 +- Core/include/Acts/Seeding/SeedFinderGbts.hpp | 17 +++++++----- Core/include/Acts/Seeding/SeedFinderGbts.ipp | 26 +++++++++---------- .../Acts/Seeding/SeedFinderOrthogonal.hpp | 25 +++++++++++++++--- .../Acts/Seeding/SeedFinderOrthogonal.ipp | 12 +++++++-- .../SeedingOrthogonalAlgorithm.hpp | 2 +- .../TrackFinding/src/GbtsSeedingAlgorithm.cpp | 22 ++++++++-------- .../src/SeedingOrthogonalAlgorithm.cpp | 9 ++++--- 10 files changed, 89 insertions(+), 47 deletions(-) diff --git a/Core/include/Acts/Seeding/GbtsTrackingFilter.hpp b/Core/include/Acts/Seeding/GbtsTrackingFilter.hpp index 86640eb946b..c2111482947 100644 --- a/Core/include/Acts/Seeding/GbtsTrackingFilter.hpp +++ b/Core/include/Acts/Seeding/GbtsTrackingFilter.hpp @@ -10,6 +10,7 @@ // TODO: update to C++17 style #include "Acts/Seeding/GbtsDataStorage.hpp" //includes geo which has trigindetsilayer, may move this to trigbase +#include "Acts/Utilities/Logger.hpp" #include #include @@ -112,8 +113,11 @@ template class GbtsTrackingFilter { public: GbtsTrackingFilter(const std::vector& g, - std::vector>& sb) - : m_geo(g), m_segStore(sb) {} + std::vector>& sb, + std::unique_ptr logger = + Acts::getDefaultLogger("Filter", + Acts::Logging::Level::INFO)) + : m_geo(g), m_segStore(sb), m_logger(std::move(logger)) {} void followTrack(Acts::GbtsEdge* pS, GbtsEdgeState& output) { @@ -233,11 +237,15 @@ class GbtsTrackingFilter { const float add_hit = 14.0; if (ts.m_Cx[2][2] < 0.0 || ts.m_Cx[1][1] < 0.0 || ts.m_Cx[0][0] < 0.0) { - std::cout << "Negative cov_x" << std::endl; + ACTS_WARNING("Negative covariance detected in X components: " + << "cov[2][2]=" << ts.m_Cx[2][2] << " cov[1][1]=" + << ts.m_Cx[1][1] << " cov[0][0]=" << ts.m_Cx[0][0]); } if (ts.m_Cy[1][1] < 0.0 || ts.m_Cy[0][0] < 0.0) { - std::cout << "Negative cov_y" << std::endl; + ACTS_WARNING("Negative covariance detected in Y components: " + << "cov[1][1]=" << ts.m_Cy[1][1] + << " cov[0][0]=" << ts.m_Cy[0][0]); } // add ms. @@ -380,4 +388,7 @@ class GbtsTrackingFilter { GbtsEdgeState m_stateStore[MAX_EDGE_STATE]; int m_globalStateCounter{0}; + + const Acts::Logger& logger() const { return *m_logger; } + std::unique_ptr m_logger{nullptr}; }; diff --git a/Core/include/Acts/Seeding/SeedFilter.hpp b/Core/include/Acts/Seeding/SeedFilter.hpp index 94ceb17ae87..d6e1479a9cd 100644 --- a/Core/include/Acts/Seeding/SeedFilter.hpp +++ b/Core/include/Acts/Seeding/SeedFilter.hpp @@ -107,7 +107,7 @@ class SeedFilter final { const SeedFilterConfig m_cfg; std::unique_ptr m_logger = - Acts::getDefaultLogger("SeedFilter", Logging::Level::INFO); + Acts::getDefaultLogger("Filter", Logging::Level::INFO); const IExperimentCuts* m_experimentCuts; }; } // namespace Acts diff --git a/Core/include/Acts/Seeding/SeedFinder.hpp b/Core/include/Acts/Seeding/SeedFinder.hpp index 3e2e7ae8bad..35a101015a5 100644 --- a/Core/include/Acts/Seeding/SeedFinder.hpp +++ b/Core/include/Acts/Seeding/SeedFinder.hpp @@ -91,7 +91,7 @@ class SeedFinder { /// @param logger the ACTS logger SeedFinder(const Acts::SeedFinderConfig& config, std::unique_ptr logger = - getDefaultLogger("SeedFinder", Logging::Level::INFO)); + getDefaultLogger("Finder", Logging::Level::INFO)); SeedFinder(SeedFinder&&) noexcept = default; SeedFinder& operator=(SeedFinder #include @@ -46,14 +46,15 @@ class SeedFinderGbts { static constexpr std::size_t NDims = 3; using seed_t = Seed; - // using internal_sp_t = InternalSpacePoint; - // using tree_t = KDTree; // constructors SeedFinderGbts(const SeedFinderGbtsConfig &config, - const GbtsGeometry &gbtsgeo); + const GbtsGeometry &gbtsgeo, + std::unique_ptr logger = + Acts::getDefaultLogger("Finder", + Acts::Logging::Level::INFO)); - ~SeedFinderGbts(); //!!! is it dangerous not to use default? got def in ipp + ~SeedFinderGbts() = default; SeedFinderGbts() = default; SeedFinderGbts(const SeedFinderGbts &) = delete; SeedFinderGbts &operator=( @@ -85,10 +86,14 @@ class SeedFinderGbts { const Acts::GbtsGeometry &gbtsgeo); // needs to be member of class so can accessed by all member functions - GbtsDataStorage *m_storage; + std::unique_ptr> m_storage{nullptr}; // for create seeds: std::vector> m_triplets; + + const Acts::Logger &logger() const { return *m_logger; } + std::unique_ptr m_logger = + Acts::getDefaultLogger("Finder", Acts::Logging::Level::INFO); }; } // namespace Acts diff --git a/Core/include/Acts/Seeding/SeedFinderGbts.ipp b/Core/include/Acts/Seeding/SeedFinderGbts.ipp index 347ca8e1e13..ab557d86977 100644 --- a/Core/include/Acts/Seeding/SeedFinderGbts.ipp +++ b/Core/include/Acts/Seeding/SeedFinderGbts.ipp @@ -34,22 +34,18 @@ namespace Acts { template SeedFinderGbts::SeedFinderGbts( const SeedFinderGbtsConfig& config, - const GbtsGeometry& gbtsGeo) - : m_config(config) { - m_storage = new GbtsDataStorage(gbtsGeo); -} - -template -SeedFinderGbts::~SeedFinderGbts() { - delete m_storage; - - m_storage = nullptr; -} + const GbtsGeometry& gbtsGeo, + std::unique_ptr logger) + : m_config(config), + m_storage( + std::make_unique>(gbtsGeo)), + m_logger(std::move(logger)) {} // define loadspace points function template void SeedFinderGbts::loadSpacePoints( const std::vector>& gbtsSPvect) { + ACTS_VERBOSE("Loading space points"); for (const auto& gbtssp : gbtsSPvect) { bool is_Pixel = gbtssp.isPixel(); if (!is_Pixel) { @@ -70,6 +66,7 @@ void SeedFinderGbts::runGbts_TrackFinder( std::vector>& vTracks, const Acts::RoiDescriptor& roi, const Acts::GbtsGeometry& gbtsGeo) { + ACTS_VERBOSE("Running GBTS Track Finder"); const float min_z0 = roi.zedMinus(); const float max_z0 = roi.zedPlus(); const float cut_zMinU = min_z0 + m_config.maxOuterRadius * roi.dzdrMinus(); @@ -328,6 +325,7 @@ void SeedFinderGbts::runGbts_TrackFinder( m_storage->getConnectingNodes(vNodes); if (vNodes.empty()) { + ACTS_VERBOSE("No nodes"); return; } @@ -506,8 +504,9 @@ void SeedFinderGbts::runGbts_TrackFinder( // backtracking - GbtsTrackingFilter tFilter(m_config.m_layerGeometry, - edgeStorage); + GbtsTrackingFilter tFilter( + m_config.m_layerGeometry, edgeStorage, + logger().cloneWithSuffix("GbtsFilter")); for (auto pS : vSeeds) { if (pS->m_level == -1) { @@ -651,6 +650,7 @@ void SeedFinderGbts::createSeeds( const Acts::RoiDescriptor& roi, const Acts::GbtsGeometry& gbtsGeo, output_container_t& out_cont) { + ACTS_VERBOSE("Creating seeds"); std::vector> vTracks; // make empty vector diff --git a/Core/include/Acts/Seeding/SeedFinderOrthogonal.hpp b/Core/include/Acts/Seeding/SeedFinderOrthogonal.hpp index 2221bff6626..594e35b44cc 100644 --- a/Core/include/Acts/Seeding/SeedFinderOrthogonal.hpp +++ b/Core/include/Acts/Seeding/SeedFinderOrthogonal.hpp @@ -13,6 +13,7 @@ #include "Acts/Seeding/SeedFinderConfig.hpp" #include "Acts/Seeding/SeedFinderOrthogonalConfig.hpp" #include "Acts/Utilities/KDTree.hpp" +#include "Acts/Utilities/Logger.hpp" #include #include @@ -53,20 +54,26 @@ class SeedFinderOrthogonal { * @brief Construct a new orthogonal seed finder. * * @param config The configuration parameters for this seed finder. + * @param logger The ACTS logger. */ SeedFinderOrthogonal( - const Acts::SeedFinderOrthogonalConfig &config); - + const Acts::SeedFinderOrthogonalConfig &config, + std::unique_ptr logger = + Acts::getDefaultLogger("Finder", Logging::Level::INFO)); /** * @brief Destroy the orthogonal seed finder object. */ ~SeedFinderOrthogonal() = default; - SeedFinderOrthogonal() = default; + SeedFinderOrthogonal() = delete; SeedFinderOrthogonal(const SeedFinderOrthogonal &) = delete; SeedFinderOrthogonal &operator=( - const SeedFinderOrthogonal &) = default; + const SeedFinderOrthogonal &) = delete; + SeedFinderOrthogonal( + SeedFinderOrthogonal &&) noexcept = default; + SeedFinderOrthogonal &operator=( + SeedFinderOrthogonal &&) noexcept = default; /** * @brief Perform seed finding, appending seeds to a container. @@ -238,6 +245,16 @@ class SeedFinderOrthogonal { * @brief The configuration for the seeding algorithm. */ Acts::SeedFinderOrthogonalConfig m_config; + + /** + * @brief Get the logger. + */ + const Logger &logger() const { return *m_logger; } + + /** + * @brief The logger + */ + std::unique_ptr m_logger{nullptr}; }; } // namespace Acts diff --git a/Core/include/Acts/Seeding/SeedFinderOrthogonal.ipp b/Core/include/Acts/Seeding/SeedFinderOrthogonal.ipp index 056b6cc6831..a725d68e92d 100644 --- a/Core/include/Acts/Seeding/SeedFinderOrthogonal.ipp +++ b/Core/include/Acts/Seeding/SeedFinderOrthogonal.ipp @@ -244,8 +244,9 @@ bool SeedFinderOrthogonal::validTuple( template SeedFinderOrthogonal::SeedFinderOrthogonal( - const SeedFinderOrthogonalConfig &config) - : m_config(config) { + const SeedFinderOrthogonalConfig &config, + std::unique_ptr logger) + : m_config(config), m_logger(std::move(logger)) { if (!config.isInInternalUnits) { throw std::runtime_error( "SeedFinderOrthogonalConfig not in ACTS internal units in " @@ -687,6 +688,7 @@ auto SeedFinderOrthogonal::createTree( const std::vector &spacePoints) const -> tree_t { std::vector points; + points.reserve(spacePoints.size()); /* * For every input point, we create a coordinate-pointer pair, which we then @@ -703,6 +705,8 @@ auto SeedFinderOrthogonal::createTree( points.emplace_back(point, sp); } + ACTS_VERBOSE("Created k-d tree populated with " << points.size() + << " space points"); return tree_t(std::move(points)); } @@ -711,6 +715,7 @@ template void SeedFinderOrthogonal::createSeeds( const Acts::SeedFinderOptions &options, const input_container_t &spacePoints, output_container_t &out_cont) const { + ACTS_VERBOSE("Creating seeds with Orthogonal strategy"); if (!options.isInInternalUnits) { throw std::runtime_error( "SeedFinderOptions not in ACTS internal units in " @@ -734,6 +739,7 @@ void SeedFinderOrthogonal::createSeeds( * take each external spacepoint, allocate a corresponding internal space * point, and save it in a vector. */ + ACTS_VERBOSE("Running on " << spacePoints.size() << " input space points"); Acts::Extent rRangeSPExtent; std::vector internal_sps; internal_sps.reserve(spacePoints.size()); @@ -746,6 +752,8 @@ void SeedFinderOrthogonal::createSeeds( rRangeSPExtent.extend({p.x(), p.y(), p.z()}); internal_sps.push_back(&p); } + ACTS_VERBOSE(rRangeSPExtent); + // variable middle SP radial region of interest const Acts::Range1D rMiddleSPRange( std::floor(rRangeSPExtent.min(Acts::BinningValue::binR) / 2) * 2 + diff --git a/Examples/Algorithms/TrackFinding/include/ActsExamples/TrackFinding/SeedingOrthogonalAlgorithm.hpp b/Examples/Algorithms/TrackFinding/include/ActsExamples/TrackFinding/SeedingOrthogonalAlgorithm.hpp index 527860ee556..963c179476b 100644 --- a/Examples/Algorithms/TrackFinding/include/ActsExamples/TrackFinding/SeedingOrthogonalAlgorithm.hpp +++ b/Examples/Algorithms/TrackFinding/include/ActsExamples/TrackFinding/SeedingOrthogonalAlgorithm.hpp @@ -70,7 +70,7 @@ class SeedingOrthogonalAlgorithm final : public IAlgorithm { private: Config m_cfg; - Acts::SeedFinderOrthogonal m_finder; + std::unique_ptr> m_finder{nullptr}; std::vector>> m_inputSpacePoints{}; diff --git a/Examples/Algorithms/TrackFinding/src/GbtsSeedingAlgorithm.cpp b/Examples/Algorithms/TrackFinding/src/GbtsSeedingAlgorithm.cpp index 385a3db7ec1..48f1490946e 100644 --- a/Examples/Algorithms/TrackFinding/src/GbtsSeedingAlgorithm.cpp +++ b/Examples/Algorithms/TrackFinding/src/GbtsSeedingAlgorithm.cpp @@ -88,20 +88,20 @@ ActsExamples::ProcessCode ActsExamples::GbtsSeedingAlgorithm::execute( MakeGbtsSpacePoints(ctx, m_cfg.ActsGbtsMap); for (auto sp : GbtsSpacePoints) { - ACTS_DEBUG("Gbts space points: " << " Gbts_id: " << sp.gbtsID << " z: " - << sp.SP->z() << " r: " << sp.SP->r() - << " ACTS volume: " - << sp.SP->sourceLinks() - .front() - .get() - .geometryId() - .volume() - << "\n"); + ACTS_DEBUG("Gbts space points: Gbts_id: " + << sp.gbtsID << " z: " << sp.SP->z() << " r: " << sp.SP->r() + << " ACTS volume: " + << sp.SP->sourceLinks() + .front() + .get() + .geometryId() + .volume()); } // this is now calling on a core algorithm - Acts::SeedFinderGbts finder(m_cfg.seedFinderConfig, - *m_gbtsGeo); + Acts::SeedFinderGbts finder( + m_cfg.seedFinderConfig, *m_gbtsGeo, + logger().cloneWithSuffix("GbtdFinder")); // output of function needed for seed diff --git a/Examples/Algorithms/TrackFinding/src/SeedingOrthogonalAlgorithm.cpp b/Examples/Algorithms/TrackFinding/src/SeedingOrthogonalAlgorithm.cpp index dcfd995f4e9..6a1673fe6f6 100644 --- a/Examples/Algorithms/TrackFinding/src/SeedingOrthogonalAlgorithm.cpp +++ b/Examples/Algorithms/TrackFinding/src/SeedingOrthogonalAlgorithm.cpp @@ -67,9 +67,11 @@ ActsExamples::SeedingOrthogonalAlgorithm::SeedingOrthogonalAlgorithm( // construct seed filter m_cfg.seedFinderConfig.seedFilter = - std::make_unique>(m_cfg.seedFilterConfig); + std::make_unique>( + m_cfg.seedFilterConfig, logger().cloneWithSuffix("Filter")); - m_finder = Acts::SeedFinderOrthogonal(m_cfg.seedFinderConfig); + m_finder = std::make_unique>( + m_cfg.seedFinderConfig, logger().cloneWithSuffix("Finder")); } ActsExamples::ProcessCode ActsExamples::SeedingOrthogonalAlgorithm::execute( @@ -95,9 +97,8 @@ ActsExamples::ProcessCode ActsExamples::SeedingOrthogonalAlgorithm::execute( ACTS_INFO("About to process " << spContainer.size() << " space points ..."); - Acts::SeedFinderOrthogonal finder(m_cfg.seedFinderConfig); std::vector> seeds = - finder.createSeeds(m_cfg.seedFinderOptions, spContainer); + m_finder->createSeeds(m_cfg.seedFinderOptions, spContainer); ACTS_INFO("Created " << seeds.size() << " track seeds from " << spacePoints.size() << " space points"); From 8c1f1553996609a94ea0147aa7f682c1e188bd7d Mon Sep 17 00:00:00 2001 From: Tim Adye Date: Tue, 5 Nov 2024 16:13:50 +0000 Subject: [PATCH 02/17] feat: ProxyAccessor constructor can now be constexpr (#3757) Since C++20 supports `constexpr std::string`, we can make `ProxyAccessorBase(const std::string& _key)` constructor `constexpr`. --- Core/include/Acts/EventData/ProxyAccessor.hpp | 3 ++- .../UnitTests/Core/EventData/TrackContainerComplianceTests.cpp | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/Core/include/Acts/EventData/ProxyAccessor.hpp b/Core/include/Acts/EventData/ProxyAccessor.hpp index 7a9ebe3eff0..429c0ef670b 100644 --- a/Core/include/Acts/EventData/ProxyAccessor.hpp +++ b/Core/include/Acts/EventData/ProxyAccessor.hpp @@ -74,7 +74,8 @@ struct ProxyAccessorBase { /// Create the accessor from a string key /// @param _key the key - ProxyAccessorBase(const std::string& _key) : key{hashString(_key)} {} + constexpr ProxyAccessorBase(const std::string& _key) + : key{hashString(_key)} {} /// Access the stored key on the proxy given as an argument. Mutable version /// @tparam proxy_t the type of the proxy diff --git a/Tests/UnitTests/Core/EventData/TrackContainerComplianceTests.cpp b/Tests/UnitTests/Core/EventData/TrackContainerComplianceTests.cpp index 636b30bff54..731f9e01a47 100644 --- a/Tests/UnitTests/Core/EventData/TrackContainerComplianceTests.cpp +++ b/Tests/UnitTests/Core/EventData/TrackContainerComplianceTests.cpp @@ -75,6 +75,7 @@ ACTS_DOES_NOT_COMPILE_SUITE_BEGIN(BuildFromConstRef) (void)t; ConstProxyAccessor caccNMeasuements("nMeasurements"); + static_cast(caccNMeasuements); // suppressed unused warning ACTS_DOES_NOT_COMPILE_BEGIN(ConstAccessorMutate) caccNMeasuements(t) = 66; ACTS_DOES_NOT_COMPILE_END() From 436c636830ab23f6b320f925e3dc7abaf975a8c9 Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Thu, 7 Nov 2024 16:12:38 +0100 Subject: [PATCH 03/17] 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 04/17] 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 05/17] 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 06/17] 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