refactor!: Seed Finders create collection of Seed<external_spacepoint_t> instead of Seed<Proxy<external_spacepoint_t>>
#3729
Conversation
github-actions
bot
added
Component - Core
CarloVarni
changed the title
Seed<external_spacepoint_t> instead of Seed<Proxy<external_spacepoint_t>>
|
CarloVarni
force-pushed
the
GridOwnsIt
branch
from
8b162de to
319e1ed
Compare
WalkthroughThe changes in this pull request encompass several modifications across multiple files primarily related to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (17)
Core/include/Acts/Seeding/Neighbour.hpp (1)
82-93: LGTM with refactoring suggestionThe changes consistently apply direct member access throughout the binary search implementation, aligning with the PR objectives.
Consider simplifying the binary search implementation for better maintainability:
- std::size_t start = 0ul; - std::size_t stop = collection.size() - 1; - while (start <= stop) { - std::size_t mid = (start + stop) / 2; - if (collection[mid].radius() == lowerBound) { - itr = collection.begin() + mid; - return; - } else if (collection[mid].radius() > lowerBound) { - if (mid > 0 && collection[mid - 1].radius() < lowerBound) { - itr = collection.begin() + mid; - return; - } - stop = mid - 1; - } else { - if (mid + 1 < collection.size() && - collection[mid + 1].radius() > lowerBound) { - itr = collection.begin() + mid + 1; - return; - } - start = mid + 1; - } - } + auto it = std::lower_bound( + collection.begin(), collection.end(), + lowerBound, + [](const auto& sp, float bound) { return sp.radius() < bound; }); + itr = it;While the comment suggests the custom implementation is faster, the proposed simplification:
- Reduces code complexity
- Uses a well-tested standard library implementation
- Makes the intent clearer
- Is easier to maintain
Consider benchmarking both implementations to validate the performance difference in your specific use case.
Core/include/Acts/EventData/SpacePointContainer.hpp (2)
129-130: Consider documenting performance implications.The change from returning references to returning values for
at()methods aligns with the safer design approach. However, returningProxyTypeby value instead of by reference could impact performance due to copying.Consider adding a comment explaining the rationale for returning by value and any performance considerations.
+ // Returns by value to ensure proxy lifetime safety + // Note: This may involve copying, but is necessary to prevent dangling references ProxyType at(const std::size_t n); ProxyType at(const std::size_t n) const;
Line range hint
1-165: Strong architectural improvement.The removal of stored proxies and shift to value semantics represents a significant architectural improvement:
- Better separation of concerns between container and proxy management
- Elimination of proxy lifetime issues
- More orthogonal design that's easier to reason about
The changes align well with modern C++ best practices and make the codebase more maintainable.
Examples/Algorithms/TrackFinding/src/SeedingOrthogonalAlgorithm.cpp (1)
98-101: Consider adding documentation about the type changeWhile the implementation is clean, it would be helpful to add a brief comment explaining that seeds now directly store space points instead of proxies. This would help future maintainers understand the design choice.
Add a comment like:
+ // Seeds now directly store space points instead of proxies to simplify ownership + // and lifetime management SimSeedContainer seeds = finder.createSeeds(m_cfg.seedFinderOptions, spContainer);Core/include/Acts/Seeding/SeedFilter.hpp (1)
32-40: Consider simplifying the type traits.The concept is well-designed but the nested type traits could be simplified for better readability.
Consider extracting the type manipulation into a type alias:
template <typename collection_t, typename external_t, std::size_t N = 3ul> concept CollectionStoresSeedsToProxied = requires(external_t sp) { typename external_t::ValueType; sp.externalSpacePoint(); + } && requires { + typename std::remove_const_t<std::remove_pointer_t<typename external_t::ValueType>>; } && CollectionStoresSeedsTo<collection_t, - std::remove_const_t<std::remove_pointer_t< - typename external_t::ValueType> >, + typename std::remove_const_t<std::remove_pointer_t< + typename external_t::ValueType>>, N>;Tests/UnitTests/Core/Seeding/SeedFinderTest.cpp (1)
239-247: LGTM with suggestion: Consider reducing output formatting duplication.The direct space point access is correct and aligns with the removal of proxies. However, the output formatting code is repetitive.
Consider extracting the space point printing logic to a helper function:
+void printSpacePoint(const SpacePoint* sp, bool printLayer = false) { + if (printLayer) { + std::cout << sp->layer << " "; + } + std::cout << "(" << sp->x() << ", " << sp->y() << ", " << sp->z() << ") "; +} for (auto& regionVec : seedVector) { for (std::size_t i = 0; i < regionVec.size(); i++) { const seed_type* seed = ®ionVec[i]; const auto* sp = seed->sp()[0]; - std::cout << " (" << sp->x() << ", " << sp->y() << ", " << sp->z() << ") "; + printSpacePoint(sp); sp = seed->sp()[1]; - std::cout << sp->layer << " (" << sp->x() << ", " << sp->y() << ", " << sp->z() << ") "; + printSpacePoint(sp, true); sp = seed->sp()[2]; - std::cout << sp->layer << " (" << sp->x() << ", " << sp->y() << ", " << sp->z() << ") "; + printSpacePoint(sp, true); std::cout << std::endl; } }Core/include/Acts/Seeding/SeedFinder.hpp (1)
116-117: LGTM! Document this breaking change.The removal of the
CollectionStoresSeedsToconcept requirement aligns with the PR objective of moving from proxy-based seeds to direct space point seeds. This is a breaking change that affects the API contract.Consider adding API documentation to:
- Explain why the concept requirement was removed
- Document that collections can now store seeds with direct space points instead of proxies
- Note this as a breaking change for users who might be implementing custom collections
template <typename container_t, Acts::GridBinCollection sp_range_t> - // requires Acts::CollectionStoresSeedsTo<container_t, external_spacepoint_t, - // 3ul> + /// @note The container_t is no longer required to satisfy CollectionStoresSeedsTo + /// concept as seeds now store direct space points instead of proxies. + /// @breaking-api-change Version X.Y.Z: Removed CollectionStoresSeedsTo requirement void createSeedsForGroup(const Acts::SeedFinderOptions& options,Core/include/Acts/Seeding/SeedFinderOrthogonal.hpp (1)
Line range hint
41-195: Consider documenting performance characteristics.The shift from proxy/pointer-based to value-based space point handling is a significant architectural change. Consider:
- Documenting the performance implications in the class documentation
- Adding benchmarks to measure the impact on memory usage and execution time
- Providing migration guidelines for downstream code
Core/include/Acts/Seeding/detail/CylindricalSpacePointGrid.ipp (1)
227-227: Consider adding a type alias for better readability.The sorting implementation looks good and properly uses modern C++ features. Consider adding a type alias for the radius projection function to make the code even more expressive.
+ using RadiusProjection = decltype([](const auto& rb) { return rb.radius(); }); std::ranges::sort(rbin, {}, [](const auto& rb) { return rb.radius(); });Examples/Algorithms/TrackFinding/src/SeedingAlgorithmHashing.cpp (1)
288-291: Add empty check before accessing collection elements.While the changes correctly align with the PR's objective of moving away from proxies to direct space point access, there's a potential issue with accessing
front()andback()without checking if the collection is empty. Although there's an empty check above, it's better to maintain the guard closer to the access point for better readability and maintainability.Consider applying this change:
- const auto& firstEl = coll.front(); - const auto& lastEl = coll.back(); - minRange = std::min(firstEl.radius(), minRange); - maxRange = std::max(lastEl.radius(), maxRange); + if (!coll.empty()) { + const auto& firstEl = coll.front(); + const auto& lastEl = coll.back(); + minRange = std::min(firstEl.radius(), minRange); + maxRange = std::max(lastEl.radius(), maxRange); + }The changes from pointer dereferencing to reference access are approved as they correctly implement the architectural change described in the PR objectives.
Core/include/Acts/Seeding/SeedFilter.ipp (2)
318-335: Consider addingnoexcepttocreateAndStoreSeedsAdding
noexceptto thecreateAndStoreSeedsmethod can provide stronger exception safety guarantees.
323-330: Simplify complex type definitions for readabilityThe use of
std::remove_const_t<std::remove_pointer_t<typename external_spacepoint_t::ValueType>>increases complexity. Introduce a type alias to enhance readability.Apply this refactor:
+ using SpacePointValueType = std::remove_const_t<std::remove_pointer_t< + typename external_spacepoint_t::ValueType>>; - using seed_type = Acts::Seed<std::remove_const_t<std::remove_pointer_t< - typename external_spacepoint_t::ValueType>>, - 3ul>; + using seed_type = Acts::Seed<SpacePointValueType, 3ul>;Core/include/Acts/Seeding/SeedFinderOrthogonal.ipp (1)
738-743: Ineffective Use ofstd::moveon Copied Const ObjectsIn lines 740-743, you are copying each
external_spacepoint_tobjectpfromspacePoints(which is a const reference), and then attempting to movepintointernal_spsusingstd::move(p). Sincepis a copy of a const object, moving it may not have any effect and may not provide the intended performance benefits.Suggested Solution:
To avoid unnecessary copying and moving, iterate over
spacePointsby const reference and push back copies intointernal_sps:-for (external_spacepoint_t p : spacePoints) { +for (const external_spacepoint_t &p : spacePoints) { minRange = std::min(minRange, p.radius()); maxRange = std::max(maxRange, p.radius()); - internal_sps.push_back(std::move(p)); + internal_sps.push_back(p); }If you intend to move the elements for performance reasons, consider changing
spacePointsto be passed by value or non-const reference, ensuring it is safe to move from its elements.Core/include/Acts/Seeding/SeedFinder.ipp (4)
50-51: Remove commented-out code or provide justificationThe lines 50-51 contain commented-out code. It's best practice to avoid leaving commented code in the codebase unless necessary. If this code is needed for future reference, consider adding a comment explaining why it's being retained; otherwise, please remove it to keep the code clean.
368-369: Extract common error calculation into a functionThe error calculations at lines 368-369 and 425-426 are identical. Consider extracting this computation into a dedicated function to enhance readability and avoid duplication.
Proposed refactor:
+// Add a helper function for error calculation +inline float calculateError(float varianceZ1, float varianceZ2, float cotTheta, float varianceR1, float varianceR2, float iDeltaR2) { + return ((varianceZ1 + varianceZ2) + (cotTheta * cotTheta) * (varianceR1 + varianceR2)) * iDeltaR2; +} ... // Replace at lines 368-369 -const float Er = - ((varianceZM + otherSP.varianceZ()) + - (cotTheta * cotTheta) * (varianceRM + otherSP.varianceR())) * - iDeltaR2; +const float Er = calculateError(varianceZM, otherSP.varianceZ(), cotTheta, varianceRM, otherSP.varianceR(), iDeltaR2); ... // Replace at lines 425-426 -const float Er = - ((varianceZM + otherSP.varianceZ()) + - (cotTheta * cotTheta) * (varianceRM + otherSP.varianceR())) * - iDeltaR2; +const float Er = calculateError(varianceZM, otherSP.varianceZ(), cotTheta, varianceRM, otherSP.varianceR(), iDeltaR2);Also applies to: 425-426
419-419: Consolidate repeated experimental cuts checkThe conditional checks at lines 419 and 468 are identical:
if (!m_config.experimentCuts(otherSP.radius(), cotTheta)) { continue; }Consider refactoring this repeated logic into a shared function or moving it outside the conditional blocks if applicable.
Also applies to: 468-468
9-17: Remove duplicate license headersThe lines 9-17 contain a duplicate of the license header already present at the top of the file. Having multiple license headers can be confusing. Please remove the redundant header to keep the file clean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (19)
Core/include/Acts/EventData/Seed.hpp(1 hunks)Core/include/Acts/EventData/Seed.ipp(1 hunks)Core/include/Acts/EventData/SpacePointContainer.hpp(2 hunks)Core/include/Acts/EventData/SpacePointContainer.ipp(2 hunks)Core/include/Acts/Seeding/Neighbour.hpp(2 hunks)Core/include/Acts/Seeding/SeedFilter.hpp(3 hunks)Core/include/Acts/Seeding/SeedFilter.ipp(1 hunks)Core/include/Acts/Seeding/SeedFinder.hpp(1 hunks)Core/include/Acts/Seeding/SeedFinder.ipp(17 hunks)Core/include/Acts/Seeding/SeedFinderOrthogonal.hpp(2 hunks)Core/include/Acts/Seeding/SeedFinderOrthogonal.ipp(3 hunks)Core/include/Acts/Seeding/detail/CylindricalSpacePointGrid.hpp(1 hunks)Core/include/Acts/Seeding/detail/CylindricalSpacePointGrid.ipp(3 hunks)Core/include/Acts/Utilities/KDTree.hpp(1 hunks)Examples/Algorithms/TrackFinding/src/SeedingAlgorithm.cpp(3 hunks)Examples/Algorithms/TrackFinding/src/SeedingAlgorithmHashing.cpp(1 hunks)Examples/Algorithms/TrackFinding/src/SeedingOrthogonalAlgorithm.cpp(1 hunks)Tests/UnitTests/Core/EventData/SpacePointContainerEdmTests.cpp(2 hunks)Tests/UnitTests/Core/Seeding/SeedFinderTest.cpp(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- Core/include/Acts/Utilities/KDTree.hpp
🔇 Additional comments (32)
Core/include/Acts/EventData/Seed.hpp (3)
Line range hint 16-28: LGTM! Template design aligns with PR objectives.
The template design effectively supports the transition from proxy-based to direct space point handling. The static assertion and constructor constraints ensure type safety.
29-30: LGTM! Appropriate use of noexcept for setter methods.
The noexcept specifications are correct as these methods only perform simple float assignments.
32-34: Verify noexcept specifications across implementation.
The noexcept specifications are correct for these accessor methods. However, let's verify consistency with the implementation.
✅ Verification successful
noexcept specifications are correctly implemented and consistent
The implementation in Seed.ipp shows that all methods are properly marked with noexcept specifications matching their declarations. No derived classes were found that could be affected by these specifications.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the implementation in Seed.ipp matches these noexcept declarations
# and check for any derived classes that might be affected.
# Check implementation file
echo "Checking Seed.ipp implementation..."
rg -A 2 "setVertexZ|setQuality|sp\(\)|z\(\)|seedQuality\(\)" "Core/include/Acts/EventData/Seed.ipp"
# Check for derived classes
echo "Checking for derived classes..."
rg -l "class.*:.*public.*Seed\b"
Length of output: 764
Core/include/Acts/EventData/Seed.ipp (5)
19-22: LGTM: Appropriate use of noexcept
The noexcept specification is correctly added as this simple setter cannot throw exceptions.
24-27: LGTM: Appropriate use of noexcept
The noexcept specification is correctly added as this simple setter cannot throw exceptions.
29-32: LGTM: Correctly implements direct space point access
The implementation properly aligns with the PR objectives by returning direct references to external space points instead of proxies. The noexcept specification is appropriate for this non-throwing operation.
34-37: LGTM: Appropriate use of noexcept
The noexcept specification is correctly added as this simple getter cannot throw exceptions.
39-42: LGTM: Appropriate use of noexcept
The noexcept specification is correctly added as this simple getter cannot throw exceptions.
Core/include/Acts/Seeding/Neighbour.hpp (2)
63-63: LGTM: Direct radius access improves code clarity
The change from pointer-based to direct member access aligns with the PR's objective of removing proxy indirection while maintaining the same logical behavior.
70-70: LGTM: Consistent direct access pattern
The change maintains consistency with the previous modification and follows the same direct access pattern.
Core/include/Acts/EventData/SpacePointContainer.hpp (1)
75-75: LGTM! Verify impact on client code.
The removal of reference types from iterator definitions aligns with the PR's objective to move away from proxy-based design. This change promotes safer value semantics over reference semantics for proxies.
Let's verify the impact on client code:
Also applies to: 78-78
✅ Verification successful
Let me gather more information about potential client code usage:
Based on the search results, I can now provide a final response:
The iterator type changes are safe to proceed with.
The codebase analysis shows that:
- Direct iterator usage is limited to internal implementation in
SpacePointContainer.ipp - Client code primarily interacts through high-level container operations (size, indexing)
- No direct iterator manipulation or range-based for loops were found in client code
- The changes align with the container's value semantics design
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any direct usage of iterator types that might be affected by this change
rg -A 2 "SpacePointContainer.*::(const_)?iterator"
Length of output: 1884
Script:
#!/bin/bash
# Search for any usage of SpacePointContainer in client code
rg -A 3 "SpacePointContainer<" --type cpp
# Search for any range-based for loops or direct iterator usage with SpacePointContainer
ast-grep --pattern 'for ($_ : $_spContainer)'
ast-grep --pattern '$_spContainer.begin()'
Length of output: 21921
Examples/Algorithms/TrackFinding/src/SeedingOrthogonalAlgorithm.cpp (2)
99-100: LGTM! Improved ownership semantics
The change from proxy-based seeds to direct space point seeds simplifies the ownership model and eliminates potential lifetime issues with proxies. This aligns well with the PR objectives.
105-105: LGTM! Simplified output handling
The direct move of seeds to output eliminates unnecessary conversion steps. Using std::move is appropriate here as we don't need the seeds after output.
Let's verify that all callers are prepared for this change:
✅ Verification successful
All callers are compatible with the direct move of seeds
The verification shows that all usages of m_outputSeeds across the codebase follow the same pattern of using std::move when writing to the output handle. Key examples:
TruthSeedingAlgorithm.cpp:m_outputSeeds(ctx, std::move(seeds))PrototracksToParameters.cpp:m_outputSeeds(ctx, std::move(seeds))TrackParamsEstimationAlgorithm.cpp:m_outputSeeds(ctx, std::move(outputSeeds))SeedingAlgorithm.cpp:m_outputSeeds(ctx, std::move(seeds))
The change is consistent with the existing codebase pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of m_outputSeeds to ensure they're compatible with the new type
rg -l "m_outputSeeds" | grep -v "SeedingOrthogonalAlgorithm.cpp" | xargs rg -A 5 "m_outputSeeds"
Length of output: 17053
Core/include/Acts/Seeding/SeedFilter.hpp (2)
26-30: LGTM! Well-designed concept for seed storage.
The concept effectively constrains collections to those that can store seeds, using a clean and focused approach.
125-132: LGTM! Well-designed method signature.
The method declaration:
- Uses concept constraints effectively to ensure type safety
- Has clear parameter names and const-qualification
- Aligns with the PR's goal of working with external space points directly
Core/include/Acts/Seeding/detail/CylindricalSpacePointGrid.hpp (1)
34-34: LGTM! Change from pointer to value storage aligns with refactoring goals.
The modification to store space points by value instead of pointers aligns well with the PR's objective to remove proxy usage. This change:
- Improves memory safety by eliminating potential dangling pointer issues
- Provides clearer ownership semantics
- Maintains type safety through the
CylindricalGridElementconcept
Let's verify the consistency of this change across the codebase:
Core/include/Acts/EventData/SpacePointContainer.ipp (2)
39-39: LGTM: Proxy initialization removal aligns with PR objectives.
The removal of proxy-related initialization code is consistent with the PR's goal of eliminating proxy storage in the container.
147-149: Verify impact of returning proxies by value.
The change from returning proxy references to values aligns with removing stored proxies. However, this change requires verification:
- Performance: Ensure that returning proxies by value doesn't introduce significant overhead in performance-critical paths.
- API Compatibility: Verify that all callers are updated to handle proxies by value instead of reference.
Let's verify the callers:
Also applies to: 153-155
✅ Verification successful
Let me analyze the search results to verify the impact of returning proxies by value.
The search results show:
-
The
at()method is used extensively throughout the codebase, but most usages are not related toSpacePointContaineror proxy objects. -
The key usage of
SpacePointContainer::at()appears in test code:
Tests/UnitTests/Core/EventData/SpacePointContainerTest.cpp: auto pTrack = tracks.at(0);Let's verify if there are any other relevant usages by searching specifically for SpacePointContainer:
Based on the search results, I can now verify the impact of returning proxies by value in SpacePointContainer::at(). Let me search for any direct usage of references to these proxies:
Let me search for the actual usage of the proxy objects to verify if they are stored or only used temporarily:
Let me search specifically in the test files that use SpacePointContainer:
Based on the analysis of the codebase:
- The SpacePointContainer's
at()method returns proxies by value, not by reference - The proxy objects are lightweight, containing only a pointer to the container and an index
- The proxy's methods are all const, accessing data through the container
- No code was found storing references to these proxies
- The test files show proxies being used transiently for accessing data
- The iterator implementation uses proxy references internally but manages their lifetime appropriately
The change to return proxies by value is safe and correct.
The change aligns with the goal of removing stored proxies while maintaining the API's functionality. Since proxies are lightweight objects that only hold a pointer and index, returning them by value has minimal performance impact. No code was found that depends on storing proxy references, and the container's iterator implementation properly handles proxy lifetimes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of SpacePointContainer::at() to verify they handle the new return type correctly
# Search for direct method calls
echo "Searching for direct at() method calls..."
rg -A 2 '\.at\s*\('
# Search for auto variable declarations that might store the result
echo "Searching for auto declarations storing the result..."
ast-grep --pattern 'auto $_ = $_.at($$$)'
Length of output: 235020
Script:
#!/bin/bash
# Search for SpacePointContainer usage
rg -A 3 "SpacePointContainer"
Length of output: 84370
Script:
#!/bin/bash
# Search for references to proxy objects
rg -B 2 -A 2 "SpacePointProxy[&<]|ProxyType[&<]"
Length of output: 8333
Script:
#!/bin/bash
# Search for variable declarations that might store proxy objects
rg -B 2 -A 2 "auto .* = .*\.at\(" Tests/UnitTests/Core/EventData/
Length of output: 558
Script:
#!/bin/bash
# Search in SpacePointContainer test files
rg -B 3 -A 3 "\.at\(" Tests/UnitTests/Core/EventData/SpacePointContainer*
Length of output: 86
Tests/UnitTests/Core/EventData/SpacePointContainerEdmTests.cpp (1)
84-84: LGTM: Strengthened container type requirements.
The change from std::ranges::range to std::ranges::random_access_range appropriately enforces stronger type requirements, ensuring the container supports efficient random access operations.
Tests/UnitTests/Core/Seeding/SeedFinderTest.cpp (1)
143-143: LGTM: Type definition change aligns with refactoring goals.
The change from proxy-based to direct space point seeds aligns with the PR objectives of removing proxy indirection.
Let's verify that we're not mixing proxy and direct space point usage:
✅ Verification successful
Based on the comprehensive search results, I can now provide a final response:
No issues found: Type definition change is consistent across codebase
The change from proxy-based to direct space point seeds is consistently implemented. The search results confirm:
- No remaining usages of
Seed<...Proxy...>exist in the codebase - All seed type definitions use direct space points, including test files and core implementations
- Legacy code in
Plugins/Legacy/maintains its own separate implementation which is unaffected
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining proxy usage with seeds
rg --type cpp "Seed<.*Proxy.*>"
Length of output: 33
Script:
#!/bin/bash
# Let's verify the usage patterns more comprehensively
# 1. Check all Seed type usages
rg --type cpp "Seed<" -A 2
# 2. Check for any remaining SpacePointProxy mentions
rg --type cpp "SpacePointProxy"
# 3. Look for seed type definitions
ast-grep --pattern 'using $_seed = Acts::Seed<$_>'
Length of output: 32834
Core/include/Acts/Seeding/SeedFinderOrthogonal.hpp (1)
195-195: LGTM! Consider verifying performance impact.
The change from pointer-based to value-based space point storage aligns with modern C++ practices and improves memory safety. While this provides better ownership semantics and eliminates dangling pointer risks, it may impact performance due to object copying.
Let's verify the consistency of this change across the codebase:
✅ Verification successful
The previous pattern was too specific. Let's broaden the search to understand the context better and verify the change's consistency.
Parameter type change is consistent with internal implementation
The verification shows that the createTree method's parameter type change is correctly implemented in both the header and implementation files. The method consistently uses const std::vector<external_spacepoint_t> &spacePoints as its parameter type. While some internal methods still use pointer vectors for specific operations, this is an implementation detail and doesn't affect the public interface change.
Core/include/Acts/Seeding/SeedFinderOrthogonal.hpp: Declaration matches the new value-based approachCore/include/Acts/Seeding/SeedFinderOrthogonal.ipp: Implementation is consistent with the declaration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other createTree implementations to ensure consistent parameter types
ast-grep --pattern 'createTree(const std::vector<$_> &$_)'
Length of output: 60
Script:
#!/bin/bash
# Let's try a broader search for createTree method declarations and definitions
ast-grep --pattern 'createTree'
# Also search for the specific file to verify its content
rg -l "SeedFinderOrthogonal"
# And look for related test files
fd "Test.*Orthogonal.*\.(cpp|hpp)"
Length of output: 514
Script:
#!/bin/bash
# Let's examine the implementation and usage in the identified files
rg -A 5 "createTree" Core/include/Acts/Seeding/SeedFinderOrthogonal.hpp Core/include/Acts/Seeding/SeedFinderOrthogonal.ipp Examples/Algorithms/TrackFinding/src/SeedingOrthogonalAlgorithm.cpp
# Also check for any other vector<spacepoint> usage patterns
rg "std::vector<.*spacepoint.*>" Core/include/Acts/Seeding/SeedFinderOrthogonal.hpp Core/include/Acts/Seeding/SeedFinderOrthogonal.ipp Examples/Algorithms/TrackFinding/src/SeedingOrthogonalAlgorithm.cpp
Length of output: 3195
Core/include/Acts/Seeding/detail/CylindricalSpacePointGrid.ipp (3)
Line range hint 197-212: LGTM: Improved value semantics for space point handling.
The change from reference to value semantics aligns with the PR objectives and provides better ownership semantics. This ensures each grid cell owns its space points directly instead of holding references.
Line range hint 213-222: LGTM: Efficient space point transfer using move semantics.
Good use of std::move to avoid unnecessary copying when storing space points in the grid. This is particularly beneficial now that we're working with actual space points instead of proxies.
Line range hint 1-245: Verify complete transition from proxy-based implementation.
Let's verify that there are no remaining proxy-based implementations that might conflict with these changes.
✅ Verification successful
Let me gather more specific information about the space point implementation changes.
Let me try another search with corrected paths:
Based on the comprehensive search results, I can now generate the final response:
Transition to value-based space point handling is correctly implemented.
The changes in CylindricalSpacePointGrid.ipp are consistent with the codebase's transition to value-based space point handling:
- The implementation uses direct value storage with
std::vector<external_spacepoint_t> - No proxy usage remains in the seeding implementation files
- The only proxy reference found is in test code (
SpacePointProxyTypeinSeedFinderTest.cpp) - The concept
CylindricalGridElementenforces proper value-based interface requirements
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining proxy usage in space point handling
echo "Checking for remaining proxy usage in space point handling..."
rg -g '!{*.md,*.txt}' -i "proxy.*spacepoint" --type cpp
# Search for potential pointer-based space point handling
echo "Checking for pointer-based space point handling..."
ast-grep --pattern 'SpacePoint*' --lang cpp
Length of output: 2252
Script:
#!/bin/bash
# Check the actual implementation in SpacePointContainer.hpp
echo "Checking SpacePointContainer implementation..."
rg -A 10 "class SpacePointContainer" Core/include/Acts/EventData/SpacePointContainer.hpp
# Check for value-based handling in seeding
echo "Checking value-based handling in seeding..."
rg -g "*/Seeding/*" "SpacePoint" --type cpp
# Look for any remaining proxy usage in seeding
echo "Checking proxy usage in seeding..."
rg -g "*/Seeding/*" "Proxy" --type cpp
Length of output: 1100
Script:
#!/bin/bash
# Check for value-based handling in seeding
echo "Checking value-based handling in seeding..."
rg -g "**/Seeding/**" "SpacePoint" --type cpp
# Look for any remaining proxy usage in seeding
echo "Checking proxy usage in seeding..."
rg -g "**/Seeding/**" "Proxy" --type cpp
# Check specific file implementation
echo "Checking CylindricalSpacePointGrid implementation..."
cat Core/include/Acts/Seeding/detail/CylindricalSpacePointGrid.hpp
Length of output: 56183
Examples/Algorithms/TrackFinding/src/SeedingAlgorithm.cpp (2)
257-260: LGTM: Improved code clarity with references
The change from pointer dereferencing to using references for accessing collection elements is a good improvement. It makes the code more idiomatic C++ while maintaining the same functionality.
278-278: Consider memory management implications of thread_local storage
While using thread_local storage for the seeds container avoids repeated allocations and is thread-safe, there are some considerations:
- The container's memory is never released back to the system, only cleared.
- The container can only grow in capacity, potentially leading to memory bloat over time if large seed collections are processed.
Let's verify the potential memory impact by checking the typical size of seed collections:
Consider these alternatives:
- Add periodic capacity reduction:
if (seeds.capacity() > some_threshold) {
SimSeedContainer tmp;
seeds.swap(tmp); // Reset capacity
}- Or use a non-static container with a pre-allocated reserve based on typical sizes:
SimSeedContainer seeds;
seeds.reserve(expectedSize); // Based on historical data or configurationAlso applies to: 292-292
Core/include/Acts/Seeding/SeedFilter.ipp (2)
310-312: Refactor enhances code reuse and maintainability
The introduction of createAndStoreSeeds encapsulates the seed creation and storage logic, improving code modularity and readability.
333-334: Verify exception safety of pushBackOrInsertAtEnd
Ensure that Acts::detail::pushBackOrInsertAtEnd handles exceptions appropriately, especially if seed's move constructor can throw.
Run the following script to check if Seed's move constructor is marked as noexcept:
✅ Verification successful
Based on the gathered information, I can now provide the final response:
pushBackOrInsertAtEnd is exception-safe for Seed moves
The implementation is safe because:
- The
Seedclass only contains trivially movable members (std::arrayof pointers and two floats) - All member functions including setters are marked
noexcept - The move operation is implicitly
noexceptdue to the trivial members - The
pushBackOrInsertAtEndimplementation uses eitherpush_backorinsertwith perfect forwarding, both of which preserve the move semantics
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if Acts::Seed's move constructor is noexcept
# Search for the move constructor in Seed.hpp and check for noexcept
rg 'Seed\(' --max-count 1 --after-context 5 '**/Seed.hpp' | rg 'noexcept'
Length of output: 125
Script:
#!/bin/bash
# Let's first find the Seed class definition file
fd -e hpp -e h "Seed"
# Then let's look for the move constructor and its implementation
rg "Seed\(.*Seed.*\)" -A 5
Length of output: 7626
Script:
#!/bin/bash
# Let's look at the actual Seed class in Core/include/Acts/EventData/Seed.hpp
cat Core/include/Acts/EventData/Seed.hpp
# Also let's check the implementation of pushBackOrInsertAtEnd
rg -A 5 "pushBackOrInsertAtEnd" Core/include/Acts/
Length of output: 3398
Script:
#!/bin/bash
# Let's check the Seed.ipp implementation for move constructor
cat Core/include/Acts/EventData/Seed.ipp
# Let's also check if there are any other implementations of pushBackOrInsertAtEnd
rg -g '!{SeedFilter.hpp,UtilityFunctions.hpp,PathSeeder.hpp}' "pushBackOrInsertAtEnd"
Length of output: 3508
Core/include/Acts/Seeding/SeedFinderOrthogonal.ipp (2)
723-727: Enforcing Compile-Time Type Safety with static_assert
The static_assert in lines 723-727 ensures that the output_container_t's value type matches seed_t, enforcing type safety at compile time:
static_assert(
std::is_same_v<
typename output_container_t::value_type,
typename SeedFinderOrthogonal<external_spacepoint_t>::seed_t>,
"Output iterator container type must accept seeds.");This is a good practice to catch potential type mismatches early. No action needed.
793-796: Addition of Convenience Overload for createSeeds
The addition of the overload for createSeeds (lines 793-796) that returns a std::vector<seed_t> enhances the usability of the class by providing a straightforward way to obtain seeds without requiring the caller to provide an output container:
std::vector<typename SeedFinderOrthogonal<external_spacepoint_t>::seed_t>
SeedFinderOrthogonal<external_spacepoint_t>::createSeeds(
const Acts::SeedFinderOptions &options,
const input_container_t &spacePoints) const {
std::vector<seed_t> r;
createSeeds(options, spacePoints, r);
return r;
}This improves the interface for users who prefer to receive a vector directly. No action needed.
Core/include/Acts/Seeding/SeedFinder.ipp (2)
203-203: Verify conditional usage of measurement info enums
At lines 203 and 206, the function filterCandidates is called with different measurement info enums based on the configuration:
if (m_config.useDetailedDoubleMeasurementInfo) {
filterCandidates<Acts::DetectorMeasurementInfo::eDetailed>(...);
} else {
filterCandidates<Acts::DetectorMeasurementInfo::eDefault>(...);
}Ensure that the conditional usage correctly impacts the behavior as intended and that both paths are adequately tested.
Also applies to: 206-206
78-79:
Check for potential out-of-bounds access on middleSPs
At line 78, middleSPs is accessed using grid.at(middleSPsIdx). Although there is a check for middleSPs.empty() afterwards, consider verifying that middleSPsIdx is within the valid range to prevent potential exceptions from std::vector::at().
Run the following script to identify all accesses to grid.at() and ensure index safety:
| Acts::ContainerIndexIterator<decltype(spContainer), proxy_t, false>; | ||
| using const_iterator_t = | ||
| Acts::ContainerIndexIterator<decltype(spContainer), const proxy_t&, true>; | ||
| Acts::ContainerIndexIterator<decltype(spContainer), const proxy_t, true>; |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent proxy value semantics found in test file
Found an inconsistency in Tests/UnitTests/Core/EventData/SpacePointContainerEdmTests.cpp where there are two different iterator definitions:
- Line 151-153: Uses value semantics (
proxy_t) - Earlier definition: Uses reference semantics (
proxy_t&)
This inconsistency should be fixed by updating the earlier definition to use value semantics, matching both the PR's objective and the implementation pattern seen in other container types (TrackContainer, SpacePointContainer).
🔗 Analysis chain
Verify consistent proxy value semantics across the codebase.
The change from reference to value semantics in iterator types aligns with the PR's objective of moving away from proxy references. While this change is correct, we should ensure it's consistently applied.
Let me gather more context about the iterator usage patterns to ensure a comprehensive verification.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining proxy reference usage patterns in iterator definitions
# Expected: No occurrences of proxy reference patterns in iterator definitions
# Search for iterator definitions that might still use reference types
rg -A 2 "ContainerIndexIterator.*proxy.*&"
# Search for potential proxy reference patterns in type aliases
rg "using.*iterator.*proxy.*&"
Length of output: 556
Script:
#!/bin/bash
# Search for all ContainerIndexIterator usages to compare patterns
rg -B 2 -A 2 "ContainerIndexIterator"
# Search for proxy_t type definitions to understand the context
ast-grep --pattern 'using proxy_t = $_'
Length of output: 9706
| using seed_t = Seed<std::remove_const_t< | ||
| std::remove_pointer_t<typename external_spacepoint_t::ValueType>>>; |
There was a problem hiding this comment.
💡 Codebase verification
Breaking change in seed type requires updates to example algorithms
The verification reveals that while core seeding components are being updated in this PR (SeedFilter.hpp, SeedFilter.ipp), the example algorithms in Examples/Algorithms/TrackFinding/ still use the old proxy-based approach. These need to be updated to maintain consistency with the new direct space point representation:
Examples/Algorithms/TrackFinding/src/SeedingAlgorithmHashing.cppExamples/Algorithms/TrackFinding/src/SeedingAlgorithm.cpp- Related header files in
Examples/Algorithms/TrackFinding/include/
The change from proxy-based to direct space points is a breaking change that should be propagated throughout the codebase to ensure consistent seed handling.
🔗 Analysis chain
LGTM! Breaking change in seed type representation.
The change from proxy-based to direct space point representation aligns with the PR objectives. This is a breaking change that improves type safety by working with actual values rather than pointers or proxies.
Let's verify the impact of this change on dependent code:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining uses of Proxy with Seed to ensure complete migration
rg "Seed.*Proxy" --type cpp
Length of output: 1148
Script:
#!/bin/bash
# Let's check if these files are being modified in this PR
git diff --name-only HEAD~1 | grep -E "Examples/Algorithms/TrackFinding/"
# Also check the specific changes in SeedingAlgorithm files if they exist in the diff
git diff HEAD~1 Examples/Algorithms/TrackFinding/src/SeedingAlgorithm.cpp Examples/Algorithms/TrackFinding/src/SeedingAlgorithmHashing.cpp Examples/Algorithms/TrackFinding/include/ActsExamples/TrackFinding/SeedingAlgorithm.hpp Examples/Algorithms/TrackFinding/include/ActsExamples/TrackFinding/SeedingAlgorithmHashing.hpp
Length of output: 397
Script:
#!/bin/bash
# Let's check the actual changes in this PR to understand the scope
git diff HEAD~1 --name-status
# And check if there are any other seed-related changes in the codebase
rg "using.*seed_t" --type cpp -A 2
Length of output: 1344
| seed_type seed(*bottom.externalSpacePoint(), *middle.externalSpacePoint(), | ||
| *top.externalSpacePoint()); |
There was a problem hiding this comment.
Ensure externalSpacePoint() does not return null
Before dereferencing, confirm that externalSpacePoint() returns valid pointers to prevent potential null pointer dereferences.
Apply this diff to add assertions:
+ ACTS_ASSERT(bottom.externalSpacePoint() != nullptr);
+ ACTS_ASSERT(middle.externalSpacePoint() != nullptr);
+ ACTS_ASSERT(top.externalSpacePoint() != nullptr);
seed_type seed(*bottom.externalSpacePoint(), *middle.externalSpacePoint(),
*top.externalSpacePoint());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| seed_type seed(*bottom.externalSpacePoint(), *middle.externalSpacePoint(), | |
| *top.externalSpacePoint()); | |
| ACTS_ASSERT(bottom.externalSpacePoint() != nullptr); | |
| ACTS_ASSERT(middle.externalSpacePoint() != nullptr); | |
| ACTS_ASSERT(top.externalSpacePoint() != nullptr); | |
| seed_type seed(*bottom.externalSpacePoint(), *middle.externalSpacePoint(), | |
| *top.externalSpacePoint()); |
| for (const external_spacepoint_t &sp : spacePoints) { | ||
| typename tree_t::coordinate_t point; | ||
|
|
||
| point[DimPhi] = sp->phi(); | ||
| point[DimR] = sp->radius(); | ||
| point[DimZ] = sp->z(); | ||
| point[DimPhi] = sp.phi(); | ||
| point[DimR] = sp.radius(); | ||
| point[DimZ] = sp.z(); | ||
|
|
||
| points.emplace_back(point, sp); | ||
| points.emplace_back(point, &sp); |
There was a problem hiding this comment.
Critical: Potential Dangling Pointers Due to Storing Addresses of Loop Variables
In the createTree function (lines 695-702), you are storing pointers to sp (i.e., &sp) inside the points vector:
points.emplace_back(point, &sp);Since sp is a reference to elements in spacePoints, which is a const reference parameter, there is a risk of dangling pointers if spacePoints gets modified or goes out of scope. Additionally, if any copies or moves occur to spacePoints or its elements elsewhere in the code (as seen in lines 740-743), the pointers may become invalid, leading to undefined behavior.
Suggested Solution:
To prevent potential dangling pointers, consider storing shared pointers or copies of sp instead of raw pointers. This ensures that the objects remain valid as long as they are needed.
-for (const external_spacepoint_t &sp : spacePoints) {
+for (auto &sp : internal_sps) {
typename tree_t::coordinate_t point;
point[DimPhi] = sp.phi();
point[DimR] = sp.radius();
point[DimZ] = sp.z();
- points.emplace_back(point, &sp);
+ points.emplace_back(point, &sp);
}Alternatively, if modifying tree_t is feasible, store the external_spacepoint_t objects directly:
points.emplace_back(point, sp);This may require changes to tree_t to handle objects instead of pointers.
There was a problem hiding this comment.
I agree with what you say, but it would involve a major change in several parts of the ACTS projects. These changes would be out of the scope of this PR and I'd like to follow that up in another PR.
For the time being we can assure that spacePoints does not get modified nor goes out of scope.
Can you open an issue about this and assign it to me?
There was a problem hiding this comment.
@CarloVarni, I've created the issue and assigned it to you. You can find it here: #3814.
🔗 Analysis chain
@CarloVarni, sure, I'll open an issue about this and assign it to you.
Thank you for the output. It seems the backticks in the title and body are being interpreted by the shell, causing the errors. I've corrected the script by escaping the backticks.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Create a new GitHub issue for the potential dangling pointers in createTree function
gh issue create \
--title "Potential Dangling Pointers in `createTree` Function" \
--body "As discussed in [PR #3729](https://github.com/acts-project/acts/pull/3729#discussion_r1829372523), there is a potential issue with storing addresses of loop variables in the `createTree` function, which could lead to dangling pointers if `spacePoints` gets modified or goes out of scope. Addressing this issue would involve major changes in several parts of the ACTS project and is out of scope for PR #3729. This issue is opened to track the work required to resolve this in a future PR.\n\nRequester: @CarloVarni" \
--assignee CarloVarni
Length of output: 916
Script:
#!/bin/bash
# Create a new GitHub issue for the potential dangling pointers in createTree function
gh issue create \
--title 'Potential Dangling Pointers in `createTree` Function' \
--body "As discussed in [PR #3729](https://github.com/acts-project/acts/pull/3729#discussion_r1829372523), there is a potential issue with storing addresses of loop variables in the \`createTree\` function, which could lead to dangling pointers if \`spacePoints\` gets modified or goes out of scope. Addressing this issue would involve major changes in several parts of the ACTS project and is out of scope for PR #3729. This issue is opened to track the work required to resolve this in a future PR.\n\nRequester: @CarloVarni" \
--assignee CarloVarni
Length of output: 675
✏️ Learnings added
Learnt from: CarloVarni
PR: acts-project/acts#3729
File: Core/include/Acts/Seeding/SeedFinderOrthogonal.ipp:695-702
Timestamp: 2024-11-05T13:57:13.315Z
Learning: In `SeedFinderOrthogonal<external_spacepoint_t>::createTree` in `SeedFinderOrthogonal.ipp`, if `spacePoints` does not get modified or go out of scope, and addressing potential dangling pointers would involve major changes that are out of scope for the current PR, it's acceptable to proceed without changes and address the issue in a future PR.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| const float deltaX = otherSP.x() - xM; | ||
| const float deltaY = otherSP.y() - yM; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicate computations of deltaX and deltaY
The calculations of deltaX and deltaY at lines 352-353 and 383-384 are duplicated. Refactoring these into a helper function can improve code maintainability and reduce redundancy.
Here's how you might refactor:
+// Add a helper function to compute deltaX and deltaY
+inline std::pair<float, float> computeDelta(const external_spacepoint_t& sp, float xM, float yM) {
+ return {sp.x() - xM, sp.y() - yM};
+}
...
// Replace at lines 352-353
-const float deltaX = otherSP.x() - xM;
-const float deltaY = otherSP.y() - yM;
+const auto [deltaX, deltaY] = computeDelta(otherSP, xM, yM);
...
// Replace at lines 383-384
-const float deltaX = otherSP.x() - xM;
-const float deltaY = otherSP.y() - yM;
+const auto [deltaX, deltaY] = computeDelta(otherSP, xM, yM);Also applies to: 383-384
As of not the
Acts::SpacePointContainerobject stores a collection of proxies internally. This was necessary due to the fact that seed finders runs on the proxies and return a collection of seeds of proxies. Since the seeds do not take ownership of the proxies (they have pointers to them), the proxies have to remain alive. That was not the case for the orthogonal.This PR changes the behaviour. Now the
Acts::SpacePointContainerobject does not store proxies.In order to make the code work, the seed finders have been modified so that they do not return a collection of seed of proxies, but a collection of seed of external space points.
Summary by CodeRabbit
SeedFilterclass.createAndStoreSeedsmethod to streamline seed creation and storage.SpacePointContainerandSeedFinderclasses by removing pointer usage.SeedingAlgorithmclass to ensure consistency.SpacePointContainertests.