From 0c57839b5b469b4ab585fe6a8254f06bb086ce7b Mon Sep 17 00:00:00 2001 From: Carlo Varni <75478407+CarloVarni@users.noreply.github.com> Date: Wed, 30 Oct 2024 22:39:14 +0100 Subject: [PATCH] refactor: Improve log in seeding (#3792) Adding some `VERBOSE` statements in the seeding, from Grid creation and Filling to seed finder and filter. --- Core/include/Acts/Seeding/SeedFilter.hpp | 14 ++++++++- Core/include/Acts/Seeding/SeedFilter.ipp | 20 +++++++++++-- Core/include/Acts/Seeding/SeedFinder.hpp | 15 ++++++++-- Core/include/Acts/Seeding/SeedFinder.ipp | 21 ++++++++++++-- .../detail/CylindricalSpacePointGrid.hpp | 10 +++++-- .../detail/CylindricalSpacePointGrid.ipp | 29 ++++++++++++++----- .../TrackFinding/src/SeedingAlgorithm.cpp | 15 ++++------ .../UnitTests/Core/Seeding/SeedFinderTest.cpp | 4 +-- 8 files changed, 100 insertions(+), 28 deletions(-) diff --git a/Core/include/Acts/Seeding/SeedFilter.hpp b/Core/include/Acts/Seeding/SeedFilter.hpp index 3c8720c42f0..94ceb17ae87 100644 --- a/Core/include/Acts/Seeding/SeedFilter.hpp +++ b/Core/include/Acts/Seeding/SeedFilter.hpp @@ -13,6 +13,7 @@ #include "Acts/Seeding/CandidatesForMiddleSp.hpp" #include "Acts/Seeding/IExperimentCuts.hpp" #include "Acts/Seeding/SeedFilterConfig.hpp" +#include "Acts/Utilities/Logger.hpp" #include #include @@ -37,8 +38,15 @@ struct SeedFilterState { template class SeedFilter final { public: - SeedFilter(SeedFilterConfig config, + SeedFilter(const SeedFilterConfig& config, IExperimentCuts* expCuts = nullptr); + SeedFilter(const SeedFilterConfig& config, + std::unique_ptr logger, + IExperimentCuts* expCuts = nullptr); + SeedFilter(const SeedFilter&) = delete; + SeedFilter& operator=(const SeedFilter&) = delete; + SeedFilter(SeedFilter&&) noexcept = default; + SeedFilter& operator=(SeedFilter&&) noexcept = default; SeedFilter() = delete; ~SeedFilter() = default; @@ -95,7 +103,11 @@ class SeedFilter final { } private: + const Logger& logger() const { return *m_logger; } + const SeedFilterConfig m_cfg; + std::unique_ptr m_logger = + Acts::getDefaultLogger("SeedFilter", Logging::Level::INFO); const IExperimentCuts* m_experimentCuts; }; } // namespace Acts diff --git a/Core/include/Acts/Seeding/SeedFilter.ipp b/Core/include/Acts/Seeding/SeedFilter.ipp index 5e54789b714..66498916a7e 100644 --- a/Core/include/Acts/Seeding/SeedFilter.ipp +++ b/Core/include/Acts/Seeding/SeedFilter.ipp @@ -16,7 +16,7 @@ namespace Acts { // constructor template SeedFilter::SeedFilter( - SeedFilterConfig config, + const SeedFilterConfig& config, IExperimentCuts* expCuts /* = 0*/) : m_cfg(config), m_experimentCuts(expCuts) { if (!config.isInInternalUnits) { @@ -24,6 +24,18 @@ SeedFilter::SeedFilter( "SeedFilterConfig not in ACTS internal units in SeedFilter"); } } + +template +SeedFilter::SeedFilter( + const SeedFilterConfig& config, std::unique_ptr logger, + IExperimentCuts* expCuts /* = 0*/) + : m_cfg(config), m_logger(std::move(logger)), m_experimentCuts(expCuts) { + if (!config.isInInternalUnits) { + throw std::runtime_error( + "SeedFilterConfig not in ACTS internal units in SeedFilter"); + } +} + // function to filter seeds based on all seeds with same bottom- and // middle-spacepoint. // return vector must contain weight of each seed @@ -295,10 +307,14 @@ void SeedFilter::filterSeeds_1SpFixed( seed.setVertexZ(zOrigin); seed.setQuality(bestSeedQuality); + ACTS_VERBOSE("Adding seed: [b=" << bottom->index() << ", m=" + << medium->index() << ", t=" << top->index() + << "], quality=" << bestSeedQuality + << ", vertexZ=" << zOrigin); Acts::detail::pushBackOrInsertAtEnd(outputCollection, std::move(seed)); - ++numTotalSeeds; } + ACTS_VERBOSE("Identified " << numTotalSeeds << " seeds"); } } // namespace Acts diff --git a/Core/include/Acts/Seeding/SeedFinder.hpp b/Core/include/Acts/Seeding/SeedFinder.hpp index cfe9908574c..cb5e7b06ff3 100644 --- a/Core/include/Acts/Seeding/SeedFinder.hpp +++ b/Core/include/Acts/Seeding/SeedFinder.hpp @@ -18,6 +18,7 @@ #include "Acts/Seeding/SeedFinderUtils.hpp" #include "Acts/Seeding/SpacePointGrid.hpp" #include "Acts/Seeding/detail/UtilityFunctions.hpp" +#include "Acts/Utilities/Logger.hpp" #include #include @@ -87,7 +88,14 @@ class SeedFinder { /// The only constructor. Requires a config object. /// @param config the configuration for the SeedFinder - SeedFinder(const Acts::SeedFinderConfig& config); + /// @param logger the ACTS logger + SeedFinder(const Acts::SeedFinderConfig& config, + std::unique_ptr logger = + getDefaultLogger("SeedFinder", Logging::Level::INFO)); + SeedFinder(SeedFinder&&) noexcept = + default; + SeedFinder& operator=(SeedFinder&&) noexcept = default; ~SeedFinder() = default; /** @name Disallow default instantiation, copy, assignment */ //@{ @@ -95,7 +103,7 @@ class SeedFinder { SeedFinder(const SeedFinder&) = delete; SeedFinder& operator=( - const SeedFinder&) = default; + const SeedFinder&) = delete; //@} /// Create all seeds from the space points in the three iterators. @@ -172,7 +180,10 @@ class SeedFinder { SeedingState& state) const; private: + const Logger& logger() const { return *m_logger; } + Acts::SeedFinderConfig m_config; + std::unique_ptr m_logger{nullptr}; }; } // namespace Acts diff --git a/Core/include/Acts/Seeding/SeedFinder.ipp b/Core/include/Acts/Seeding/SeedFinder.ipp index 45b95ec05d5..e6ccb9f589f 100644 --- a/Core/include/Acts/Seeding/SeedFinder.ipp +++ b/Core/include/Acts/Seeding/SeedFinder.ipp @@ -15,8 +15,9 @@ namespace Acts { template SeedFinder::SeedFinder( - const Acts::SeedFinderConfig& config) - : m_config(config) { + const Acts::SeedFinderConfig& config, + std::unique_ptr logger) + : m_config(config), m_logger(std::move(logger)) { if (!config.isInInternalUnits) { throw std::runtime_error( "SeedFinderConfig not in ACTS internal units in SeedFinder"); @@ -110,6 +111,12 @@ void SeedFinder::createSeedsForGroup( // same z-bin auto [minRadiusRangeForMiddle, maxRadiusRangeForMiddle] = retrieveRadiusRangeForMiddle(*middleSPs.front(), rMiddleSPRange); + ACTS_VERBOSE("Current global bin: " << middleSPsIdx << ", z value of " + << middleSPs.front()->z()); + ACTS_VERBOSE("Validity range (radius) for the middle space point is [" + << minRadiusRangeForMiddle << ", " << maxRadiusRangeForMiddle + << "]"); + for (const external_spacepoint_t* spM : middleSPs) { const float rM = spM->radius(); @@ -136,6 +143,7 @@ void SeedFinder::createSeedsForGroup( // no top SP found -> try next spM if (state.compatTopSP.empty()) { + ACTS_VERBOSE("No compatible Tops, moving to next middle candidate"); continue; } @@ -157,6 +165,10 @@ void SeedFinder::createSeedsForGroup( seedFilterState.rMaxSeedConf = seedConfRange.rMaxSeedConf; // continue if number of top SPs is smaller than minimum if (state.compatTopSP.size() < seedFilterState.nTopSeedConf) { + ACTS_VERBOSE( + "Number of top SPs is " + << state.compatTopSP.size() + << " and is smaller than minimum, moving to next middle candidate"); continue; } } @@ -170,9 +182,14 @@ void SeedFinder::createSeedsForGroup( // no bottom SP found -> try next spM if (state.compatBottomSP.empty()) { + ACTS_VERBOSE("No compatible Bottoms, moving to next middle candidate"); continue; } + ACTS_VERBOSE("Candidates: " << state.compatBottomSP.size() + << " bottoms and " << state.compatTopSP.size() + << " tops for middle candidate indexed " + << spM->index()); // filter candidates if (m_config.useDetailedDoubleMeasurementInfo) { filterCandidates( diff --git a/Core/include/Acts/Seeding/detail/CylindricalSpacePointGrid.hpp b/Core/include/Acts/Seeding/detail/CylindricalSpacePointGrid.hpp index 4410f1c125b..a2a214fa39b 100644 --- a/Core/include/Acts/Seeding/detail/CylindricalSpacePointGrid.hpp +++ b/Core/include/Acts/Seeding/detail/CylindricalSpacePointGrid.hpp @@ -11,6 +11,7 @@ #include "Acts/Seeding/BinnedGroup.hpp" #include "Acts/Seeding/SeedFinderConfig.hpp" #include "Acts/Utilities/Grid.hpp" +#include "Acts/Utilities/Logger.hpp" #include #include @@ -158,7 +159,8 @@ class CylindricalSpacePointGridCreator { template static Acts::CylindricalSpacePointGrid createGrid( const Acts::CylindricalSpacePointGridConfig& _config, - const Acts::CylindricalSpacePointGridOptions& _options); + const Acts::CylindricalSpacePointGridOptions& _options, + const Acts::Logger& logger = Acts::getDummyLogger()); template @@ -167,7 +169,8 @@ class CylindricalSpacePointGridCreator { const Acts::SeedFinderOptions& options, Acts::CylindricalSpacePointGrid& grid, external_spacepoint_iterator_t spBegin, - external_spacepoint_iterator_t spEnd); + external_spacepoint_iterator_t spEnd, + const Acts::Logger& logger = Acts::getDummyLogger()); template requires std::ranges::range && @@ -177,7 +180,8 @@ class CylindricalSpacePointGridCreator { const Acts::SeedFinderConfig& config, const Acts::SeedFinderOptions& options, Acts::CylindricalSpacePointGrid& grid, - const external_collection_t& collection); + const external_collection_t& collection, + const Acts::Logger& logger = Acts::getDummyLogger()); }; } // namespace Acts diff --git a/Core/include/Acts/Seeding/detail/CylindricalSpacePointGrid.ipp b/Core/include/Acts/Seeding/detail/CylindricalSpacePointGrid.ipp index 523d470eae5..b99cdbdfaf9 100644 --- a/Core/include/Acts/Seeding/detail/CylindricalSpacePointGrid.ipp +++ b/Core/include/Acts/Seeding/detail/CylindricalSpacePointGrid.ipp @@ -12,7 +12,8 @@ template Acts::CylindricalSpacePointGrid Acts::CylindricalSpacePointGridCreator::createGrid( const Acts::CylindricalSpacePointGridConfig& config, - const Acts::CylindricalSpacePointGridOptions& options) { + const Acts::CylindricalSpacePointGridOptions& options, + const Acts::Logger& logger) { if (!config.isInInternalUnits) { throw std::runtime_error( "CylindricalSpacePointGridConfig not in ACTS internal units in " @@ -30,6 +31,9 @@ Acts::CylindricalSpacePointGridCreator::createGrid( // for no magnetic field, create 100 phi-bins if (options.bFieldInZ == 0) { phiBins = 100; + ACTS_VERBOSE( + "B-Field is 0 (z-coordinate), setting the number of bins in phi to " + << phiBins); } else { // calculate circle intersections of helix and max detector radius float minHelixRadius = @@ -39,7 +43,7 @@ Acts::CylindricalSpacePointGridCreator::createGrid( // = pT[MeV] / (300 *Bz[kT]) // sanity check: if yOuter takes the square root of a negative number - if (minHelixRadius < config.rMax / 2) { + if (minHelixRadius < config.rMax * 0.5) { throw std::domain_error( "The value of minHelixRadius cannot be smaller than rMax / 2. Please " "check the configuration of bFieldInZ and minPt"); @@ -141,6 +145,12 @@ Acts::CylindricalSpacePointGridCreator::createGrid( Axis zAxis(std::move(zValues)); Axis rAxis(std::move(rValues)); + + ACTS_VERBOSE("Defining Grid:"); + ACTS_VERBOSE("- Phi Axis: " << phiAxis); + ACTS_VERBOSE("- Z axis : " << zAxis); + ACTS_VERBOSE("- R axis : " << rAxis); + return Acts::CylindricalSpacePointGrid( std::make_tuple(std::move(phiAxis), std::move(zAxis), std::move(rAxis))); } @@ -152,7 +162,7 @@ void Acts::CylindricalSpacePointGridCreator::fillGrid( const Acts::SeedFinderOptions& options, Acts::CylindricalSpacePointGrid& grid, external_spacepoint_iterator_t spBegin, - external_spacepoint_iterator_t spEnd) { + external_spacepoint_iterator_t spEnd, const Acts::Logger& logger) { if (!config.isInInternalUnits) { throw std::runtime_error( "SeedFinderConfig not in ACTS internal units in BinnedSPGroup"); @@ -180,9 +190,10 @@ void Acts::CylindricalSpacePointGridCreator::fillGrid( std::vector rBinsIndex; rBinsIndex.reserve(grid.size()); + ACTS_VERBOSE("Fetching " << std::distance(spBegin, spEnd) + << " space points to the grid"); std::size_t counter = 0ul; - for (external_spacepoint_iterator_t it = spBegin; it != spEnd; - it++, ++counter) { + for (external_spacepoint_iterator_t it = spBegin; it != spEnd; ++it) { const external_spacepoint_t& sp = *it; // remove SPs according to experiment specific cuts @@ -199,6 +210,7 @@ void Acts::CylindricalSpacePointGridCreator::fillGrid( std::size_t globIndex = grid.globalBinFromPosition(position); auto& rbin = grid.at(globIndex); rbin.push_back(&sp); + ++counter; // keep track of the bins we modify so that we can later sort the SPs in // those bins only @@ -213,6 +225,9 @@ void Acts::CylindricalSpacePointGridCreator::fillGrid( auto& rbin = grid.atPosition(binIndex); std::ranges::sort(rbin, {}, [](const auto& rb) { return rb->radius(); }); } + + ACTS_VERBOSE( + "Number of space points inserted (within grid range): " << counter); } template @@ -223,8 +238,8 @@ void Acts::CylindricalSpacePointGridCreator::fillGrid( const Acts::SeedFinderConfig& config, const Acts::SeedFinderOptions& options, Acts::CylindricalSpacePointGrid& grid, - const external_collection_t& collection) { + const external_collection_t& collection, const Acts::Logger& logger) { Acts::CylindricalSpacePointGridCreator::fillGrid( config, options, grid, std::ranges::begin(collection), - std::ranges::end(collection)); + std::ranges::end(collection), logger); } diff --git a/Examples/Algorithms/TrackFinding/src/SeedingAlgorithm.cpp b/Examples/Algorithms/TrackFinding/src/SeedingAlgorithm.cpp index ed5b11e4df2..2cea4b7b505 100644 --- a/Examples/Algorithms/TrackFinding/src/SeedingAlgorithm.cpp +++ b/Examples/Algorithms/TrackFinding/src/SeedingAlgorithm.cpp @@ -45,9 +45,8 @@ ActsExamples::SeedingAlgorithm::SeedingAlgorithm( // internal units m_cfg.seedFilterConfig = m_cfg.seedFilterConfig.toInternalUnits(); m_cfg.seedFinderConfig.seedFilter = - std::make_shared>( - m_cfg.seedFilterConfig); - + std::make_unique>( + m_cfg.seedFilterConfig, logger().cloneWithSuffix("SeedFilter")); m_cfg.seedFinderConfig = m_cfg.seedFinderConfig.toInternalUnits().calculateDerivedQuantities(); m_cfg.seedFinderOptions = @@ -195,13 +194,10 @@ ActsExamples::SeedingAlgorithm::SeedingAlgorithm( m_topBinFinder = std::make_unique>( m_cfg.numPhiNeighbors, m_cfg.zBinNeighborsTop, 0); - m_cfg.seedFinderConfig.seedFilter = - std::make_unique>( - m_cfg.seedFilterConfig); m_seedFinder = Acts::SeedFinder>( - m_cfg.seedFinderConfig); + m_cfg.seedFinderConfig, logger().cloneWithSuffix("SeedFinder")); } ActsExamples::ProcessCode ActsExamples::SeedingAlgorithm::execute( @@ -244,10 +240,11 @@ ActsExamples::ProcessCode ActsExamples::SeedingAlgorithm::execute( Acts::CylindricalSpacePointGrid grid = Acts::CylindricalSpacePointGridCreator::createGrid( - m_cfg.gridConfig, m_cfg.gridOptions); + m_cfg.gridConfig, m_cfg.gridOptions, logger()); Acts::CylindricalSpacePointGridCreator::fillGrid( - m_cfg.seedFinderConfig, m_cfg.seedFinderOptions, grid, spContainer); + m_cfg.seedFinderConfig, m_cfg.seedFinderOptions, grid, spContainer, + logger()); // Compute radius Range // we rely on the fact the grid is storing the proxies diff --git a/Tests/UnitTests/Core/Seeding/SeedFinderTest.cpp b/Tests/UnitTests/Core/Seeding/SeedFinderTest.cpp index 9d89c1c96c6..8368bdbc3f2 100644 --- a/Tests/UnitTests/Core/Seeding/SeedFinderTest.cpp +++ b/Tests/UnitTests/Core/Seeding/SeedFinderTest.cpp @@ -186,8 +186,8 @@ int main(int argc, char** argv) { Acts::SeedFilterConfig sfconf; Acts::ATLASCuts atlasCuts = Acts::ATLASCuts(); - config.seedFilter = std::make_unique>( - Acts::SeedFilter(sfconf, &atlasCuts)); + config.seedFilter = + std::make_unique>(sfconf, &atlasCuts); Acts::SeedFinder> a; // test creation of unconfigured finder a = Acts::SeedFinder>(