From 81e129bc52d420a85b7e8f381c865affd951024b Mon Sep 17 00:00:00 2001 From: Carlo Varni <75478407+CarloVarni@users.noreply.github.com> Date: Fri, 4 Oct 2024 15:02:25 +0200 Subject: [PATCH] refactor!: Do not use geometry extent during seeding (#3688) In the seeding we only need the radius range of the space points (and only when the range is computed by the code and not provided by the user). As of now, when filling the grid we waste a lot of time to fill the `Acts::Extent` object (for all the possible `Acts::BinningValue` even if we don't need them all). I have refactored the code so that we do not do this expensive operation. The user will have to do it by them selves or provide a validity search window via JO. No changes expected from this PR --- .../detail/CylindricalSpacePointGrid.hpp | 3 +- .../detail/CylindricalSpacePointGrid.ipp | 5 +--- .../TrackFinding/src/SeedingAlgorithm.cpp | 30 +++++++++++-------- .../src/SeedingAlgorithmHashing.cpp | 29 +++++++++++------- .../UnitTests/Core/Seeding/SeedFinderTest.cpp | 7 +---- 5 files changed, 39 insertions(+), 35 deletions(-) diff --git a/Core/include/Acts/Seeding/detail/CylindricalSpacePointGrid.hpp b/Core/include/Acts/Seeding/detail/CylindricalSpacePointGrid.hpp index b6902ecd35a..3acdc5a4c05 100644 --- a/Core/include/Acts/Seeding/detail/CylindricalSpacePointGrid.hpp +++ b/Core/include/Acts/Seeding/detail/CylindricalSpacePointGrid.hpp @@ -8,7 +8,6 @@ #pragma once -#include "Acts/Geometry/Extent.hpp" #include "Acts/Seeding/BinnedGroup.hpp" #include "Acts/Seeding/SeedFinderConfig.hpp" #include "Acts/Utilities/Grid.hpp" @@ -121,7 +120,7 @@ class CylindricalSpacePointGridCreator { const Acts::SeedFinderOptions& options, Acts::CylindricalSpacePointGrid& grid, external_spacepoint_iterator_t spBegin, - external_spacepoint_iterator_t spEnd, Acts::Extent& rRangeSPExtent); + external_spacepoint_iterator_t spEnd); }; } // namespace Acts diff --git a/Core/include/Acts/Seeding/detail/CylindricalSpacePointGrid.ipp b/Core/include/Acts/Seeding/detail/CylindricalSpacePointGrid.ipp index ea228510042..a80d3105e82 100644 --- a/Core/include/Acts/Seeding/detail/CylindricalSpacePointGrid.ipp +++ b/Core/include/Acts/Seeding/detail/CylindricalSpacePointGrid.ipp @@ -142,7 +142,7 @@ void Acts::CylindricalSpacePointGridCreator::fillGrid( const Acts::SeedFinderOptions& options, Acts::CylindricalSpacePointGrid& grid, external_spacepoint_iterator_t spBegin, - external_spacepoint_iterator_t spEnd, Acts::Extent& rRangeSPExtent) { + external_spacepoint_iterator_t spEnd) { if (!config.isInInternalUnits) { throw std::runtime_error( "SeedFinderConfig not in ACTS internal units in BinnedSPGroup"); @@ -176,9 +176,6 @@ void Acts::CylindricalSpacePointGridCreator::fillGrid( float spY = sp.y(); float spZ = sp.z(); - // store x,y,z values in extent - rRangeSPExtent.extend({spX, spY, spZ}); - // remove SPs according to experiment specific cuts if (!config.spacePointSelector(sp)) { continue; diff --git a/Examples/Algorithms/TrackFinding/src/SeedingAlgorithm.cpp b/Examples/Algorithms/TrackFinding/src/SeedingAlgorithm.cpp index 40111376a9c..b60acc13e85 100644 --- a/Examples/Algorithms/TrackFinding/src/SeedingAlgorithm.cpp +++ b/Examples/Algorithms/TrackFinding/src/SeedingAlgorithm.cpp @@ -11,7 +11,6 @@ #include "Acts/Definitions/Algebra.hpp" #include "Acts/EventData/Seed.hpp" #include "Acts/EventData/SpacePointData.hpp" -#include "Acts/Geometry/Extent.hpp" #include "Acts/Seeding/BinnedGroup.hpp" #include "Acts/Seeding/SeedFilter.hpp" #include "Acts/Utilities/BinningType.hpp" @@ -243,16 +242,28 @@ ActsExamples::ProcessCode ActsExamples::SeedingAlgorithm::execute( using value_type = typename decltype(spContainer)::SpacePointProxyType; using seed_type = Acts::Seed; - // extent used to store r range for middle spacepoint - Acts::Extent rRangeSPExtent; - Acts::CylindricalSpacePointGrid grid = Acts::CylindricalSpacePointGridCreator::createGrid( m_cfg.gridConfig, m_cfg.gridOptions); Acts::CylindricalSpacePointGridCreator::fillGrid( m_cfg.seedFinderConfig, m_cfg.seedFinderOptions, grid, - spContainer.begin(), spContainer.end(), rRangeSPExtent); + spContainer.begin(), spContainer.end()); + + // Compute radius Range + // we rely on the fact the grid is storing the proxies + // with a sorting in the radius + float minRange = std::numeric_limits::max(); + float maxRange = std::numeric_limits::lowest(); + for (const auto& coll : grid) { + if (coll.empty()) { + continue; + } + const auto* firstEl = coll.front(); + const auto* lastEl = coll.back(); + minRange = std::min(firstEl->radius(), minRange); + maxRange = std::max(lastEl->radius(), maxRange); + } std::array, 2ul> navigation; navigation[1ul] = m_cfg.seedFinderConfig.zBinsCustomLooping; @@ -261,15 +272,10 @@ ActsExamples::ProcessCode ActsExamples::SeedingAlgorithm::execute( std::move(grid), *m_bottomBinFinder, *m_topBinFinder, std::move(navigation)); - // safely clamp double to float - float up = Acts::clampValue( - std::floor(rRangeSPExtent.max(Acts::BinningValue::binR) / 2) * 2); - /// variable middle SP radial region of interest const Acts::Range1D rMiddleSPRange( - std::floor(rRangeSPExtent.min(Acts::BinningValue::binR) / 2) * 2 + - m_cfg.seedFinderConfig.deltaRMiddleMinSPRange, - up - m_cfg.seedFinderConfig.deltaRMiddleMaxSPRange); + minRange + m_cfg.seedFinderConfig.deltaRMiddleMinSPRange, + maxRange - m_cfg.seedFinderConfig.deltaRMiddleMaxSPRange); // run the seeding static thread_local std::vector seeds; diff --git a/Examples/Algorithms/TrackFinding/src/SeedingAlgorithmHashing.cpp b/Examples/Algorithms/TrackFinding/src/SeedingAlgorithmHashing.cpp index ec664aef437..84011207f19 100644 --- a/Examples/Algorithms/TrackFinding/src/SeedingAlgorithmHashing.cpp +++ b/Examples/Algorithms/TrackFinding/src/SeedingAlgorithmHashing.cpp @@ -11,7 +11,6 @@ #include "Acts/Definitions/Algebra.hpp" #include "Acts/EventData/Seed.hpp" #include "Acts/EventData/SpacePointData.hpp" -#include "Acts/Geometry/Extent.hpp" #include "Acts/Plugins/Hashing/HashingAlgorithm.hpp" #include "Acts/Plugins/Hashing/HashingTraining.hpp" #include "Acts/Seeding/BinnedGroup.hpp" @@ -269,15 +268,28 @@ ActsExamples::ProcessCode ActsExamples::SeedingAlgorithmHashing::execute( Acts::SpacePointContainer spContainer(spConfig, spOptions, container); - // extent used to store r range for middle spacepoint - Acts::Extent rRangeSPExtent; // construct the seeding tools Acts::CylindricalSpacePointGrid grid = Acts::CylindricalSpacePointGridCreator::createGrid( m_cfg.gridConfig, m_cfg.gridOptions); Acts::CylindricalSpacePointGridCreator::fillGrid( m_cfg.seedFinderConfig, m_cfg.seedFinderOptions, grid, - spContainer.begin(), spContainer.end(), rRangeSPExtent); + spContainer.begin(), spContainer.end()); + + // Compute radius Range + // we rely on the fact the grid is storing the proxies + // with a sorting in the radius + float minRange = std::numeric_limits::max(); + float maxRange = std::numeric_limits::lowest(); + for (const auto& coll : grid) { + if (coll.empty()) { + continue; + } + const auto* firstEl = coll.front(); + const auto* lastEl = coll.back(); + minRange = std::min(firstEl->radius(), minRange); + maxRange = std::max(lastEl->radius(), maxRange); + } std::array, 2ul> navigation; navigation[1ul] = m_cfg.seedFinderConfig.zBinsCustomLooping; @@ -287,15 +299,10 @@ ActsExamples::ProcessCode ActsExamples::SeedingAlgorithmHashing::execute( std::move(grid), *m_bottomBinFinder, *m_topBinFinder, std::move(navigation)); - // safely clamp double to float - float up = Acts::clampValue( - std::floor(rRangeSPExtent.max(Acts::BinningValue::binR) / 2) * 2); - /// variable middle SP radial region of interest const Acts::Range1D rMiddleSPRange( - std::floor(rRangeSPExtent.min(Acts::BinningValue::binR) / 2) * 2 + - m_cfg.seedFinderConfig.deltaRMiddleMinSPRange, - up - m_cfg.seedFinderConfig.deltaRMiddleMaxSPRange); + minRange + m_cfg.seedFinderConfig.deltaRMiddleMinSPRange, + maxRange - m_cfg.seedFinderConfig.deltaRMiddleMaxSPRange); // this creates seeds of proxy, we need to convert it to seed of space // points diff --git a/Tests/UnitTests/Core/Seeding/SeedFinderTest.cpp b/Tests/UnitTests/Core/Seeding/SeedFinderTest.cpp index 4f263e693f7..08bec8a4d92 100644 --- a/Tests/UnitTests/Core/Seeding/SeedFinderTest.cpp +++ b/Tests/UnitTests/Core/Seeding/SeedFinderTest.cpp @@ -10,7 +10,6 @@ #include "Acts/Definitions/Units.hpp" #include "Acts/EventData/Seed.hpp" #include "Acts/EventData/SpacePointContainer.hpp" -#include "Acts/Geometry/Extent.hpp" #include "Acts/Seeding/BinnedGroup.hpp" #include "Acts/Seeding/SeedFilter.hpp" #include "Acts/Seeding/SeedFilterConfig.hpp" @@ -173,9 +172,6 @@ int main(int argc, char** argv) { int numPhiNeighbors = 1; - // extent used to store r range for middle spacepoint - Acts::Extent rRangeSPExtent; - config.useVariableMiddleSPRange = false; const Acts::Range1D rMiddleSPRange; @@ -213,8 +209,7 @@ int main(int argc, char** argv) { Acts::CylindricalSpacePointGridCreator::createGrid(gridConf, gridOpts); Acts::CylindricalSpacePointGridCreator::fillGrid( - config, options, grid, spContainer.begin(), spContainer.end(), - rRangeSPExtent); + config, options, grid, spContainer.begin(), spContainer.end()); auto spGroup = Acts::CylindricalBinnedGroup( std::move(grid), *bottomBinFinder, *topBinFinder);