feat!: Estimate parameters on surface also if bottom SP is not on surface #3800
Conversation
|
would be very useful! 👍 |
…y-estimate-params-at-surface
WalkthroughSignificant changes made, yes. In Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
|
@paulgessinger I implemented what we discussed. cc @benjaminhuth |
github-actions
bot
added
Component - Examples
andiwand
changed the title
…y-estimate-params-at-surface
| const GeometryContext& gctx, const MagneticFieldContext& mctx, | ||
| const Surface& surface, const Vector3& sp0, const Vector3& sp1, | ||
| const Vector3& sp2, | ||
| const std::shared_ptr<const MagneticFieldProvider>& bField, |
There was a problem hiding this comment.
You need this as shared to be able to construct the propagator, right?
There was a problem hiding this comment.
Yes the stepper really wants have a shared pointer. I thought it might be good to pass in the shared pointer as a reference so we avoid a copy in case the user also provides a propagator.
We will pay the overhead on the propagator construction on every call if it is constructed by us though. I guess it is not huge but in principle avoidable by factorizing this function in estimate, transform, propagate.
|
Looking at #3800 (comment) what is still not 100% clear to me is if we should really provide such a high level function hiding the potential cost of creating multiple propagator instances which copy shared pointers and needs the creation of a magnetic field cache. @benjaminhuth pointed out that it might be good enough to have the I am sort of in the middle. I think this interface captures what the user whats to do but it does not have 0 cost under the hood if you call it many times. |
andiwand
dismissed stale reviews from paulgessinger and benjaminhuth
via
b682e16
acts-policybot
bot
dismissed
pbutti’s stale review
Invalidated by push of b682e16
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
Examples/Algorithms/TrackFinding/src/TrackParamsEstimationAlgorithm.cpp (1)
104-106: Consider error handling for magnetic field context, you should.Wisdom suggests that explicit validation of magnetic field context before estimation, beneficial it would be. Though Result type handles errors, early validation prevents unnecessary computations.
Add this validation before the estimation, you could:
+ if (!ctx.magFieldContext) { + ACTS_WARNING("Invalid magnetic field context provided"); + continue; + } const auto paramsResult = Acts::estimateTrackParamsFromSeedAtSurface( ctx.geoContext, ctx.magFieldContext, *surface, seed.sp(), m_cfg.magneticField);Tests/UnitTests/Core/Seeding/EstimateTrackParamsFromSeedTest.cpp (1)
Line range hint
177-190: Additional test cases, consider you should.Comprehensive the test matrix is, but strengthen it further we can. Suggest I do:
- Zero magnetic field scenario
- Extreme momentum values test
- Edge case where space points are far apart
Help with implementing these test cases, offer I do.
Additional test cases, shall I generate for you? Open a GitHub issue to track this task, I can.
Core/include/Acts/Seeding/EstimateTrackParamsFromSeed.hpp (1)
149-184: Consider renaming this function, to avoid confusion, you should.Same name, these functions have. Different parameters, but clarity may improve with distinct names.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
Core/include/Acts/Propagator/Propagator.hpp(0 hunks)Core/include/Acts/Seeding/EstimateTrackParamsFromSeed.hpp(4 hunks)Core/src/Seeding/EstimateTrackParamsFromSeed.cpp(2 hunks)Examples/Algorithms/TrackFinding/src/TrackParamsEstimationAlgorithm.cpp(1 hunks)Examples/Algorithms/TrackFindingExaTrkX/src/PrototracksToParameters.cpp(1 hunks)Tests/UnitTests/Core/Seeding/EstimateTrackParamsFromSeedTest.cpp(2 hunks)
💤 Files with no reviewable changes (1)
- Core/include/Acts/Propagator/Propagator.hpp
🔇 Additional comments (13)
Examples/Algorithms/TrackFinding/src/TrackParamsEstimationAlgorithm.cpp (1)
104-106: Hmmmm, approve this change, I do!
Simplified the magnetic field handling has become, and clearer the intention now is. Pass the magnetic field context directly we do, instead of retrieving it separately we must.
Verify the usage of new function across the codebase, we shall:
✅ Verification successful
Correct and proper, this change is, hmmmm!
Clear in the Force, I see that estimateTrackParamsFromSeedAtSurface builds upon the foundation of estimateTrackParamsFromSeed it does. Proper migration path, this change follows. In the new implementation, handle magnetic field context explicitly we do, and surface transformation we perform. No old usages in production code remain, only in core implementation and tests they exist.
A natural evolution of the API this is, where:
- Magnetic field handling, more robust it has become
- Surface transformation, properly integrated it is
- Error handling through Result type, maintained it remains
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining old function calls
rg "estimateTrackParamsFromSeed\(" --type cpp
# Search for new function usage patterns
rg "estimateTrackParamsFromSeedAtSurface\(" --type cpp
Length of output: 1754
Script:
#!/bin/bash
# Check implementation of both functions to understand their relationship
ast-grep --pattern 'FreeVector estimateTrackParamsFromSeed($$$)'
echo "---"
ast-grep --pattern 'Result<BoundVector> estimateTrackParamsFromSeedAtSurface($$$)'
Length of output: 166
Script:
#!/bin/bash
# Search for function implementations with more context
rg "estimateTrackParamsFromSeed" Core/src/Seeding/EstimateTrackParamsFromSeed.cpp -B 2 -A 10
echo "=== New Function Implementation ==="
rg "estimateTrackParamsFromSeedAtSurface" Core/src/Seeding/EstimateTrackParamsFromSeed.cpp -B 2 -A 10
Length of output: 2626
Tests/UnitTests/Core/Seeding/EstimateTrackParamsFromSeedTest.cpp (2)
100-100: Ownership semantics, changed they have. Careful we must be.
Direct field passing instead of std::move, I see. Resource management implications, this has. Yet proper it remains, as shared_ptr reference counting handles ownership well, it does.
177-178: Updated function signature, matches the new way it does.
Hmm, new parameters magCtx and field, added they were. Result type checking, proper error handling it ensures. With the Force strong, this change is.
Yet verify the behavior across edge cases, we should.
✅ Verification successful
Correct the function call is, young padawan
Verified the changes have been, across the entire codebase, hmm. The test file, proper implementation it shows. The new signature with magCtx and field, correctly used it is. Edge cases thoroughly tested they are, with different:
- Momentum values (0.5 GeV, 1.0 GeV)
- Phi angles (20°, -20°)
- Theta angles (80°, 90°, 100°)
- Charges (+1, -1)
Strong with the Force this implementation is. Proper error handling through Result type it has, and comprehensive testing across parameter space it shows. No other instances requiring updates, found they were.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other test cases that might need similar updates
rg "estimateTrackParamsFromSeed\(" --type cpp
Length of output: 783
Script:
#!/bin/bash
# Check the implementation of estimateTrackParamsFromSeedAtSurface
ast-grep --pattern 'estimateTrackParamsFromSeedAtSurface($$$)'
# Check the test file content for context
cat Tests/UnitTests/Core/Seeding/EstimateTrackParamsFromSeedTest.cpp
Length of output: 8950
Examples/Algorithms/TrackFindingExaTrkX/src/PrototracksToParameters.cpp (1)
156-158: Ensure, you must, that error handling remains robust.
Simplifies the code, the new function does. But verify, you should, that essential error checks for the magnetic field are not lost.
Run this script to check for missing error handling:
✅ Verification successful
Robust error handling, I sense. Worry not, we must.
In the file PrototracksToParameters.cpp, proper error handling for magnetic field exists, hmm yes. Before any operation begins, checks for null magnetic field are performed, throwing exceptions they do:
if (m_cfg.magneticField == nullptr) {
throw std::invalid_argument("No magnetic field given");
}Similar error handling patterns, I observe in other track finding algorithms too. A consistent approach throughout the codebase, this is.
The Force is strong with this error handling, young Padawan. Safe to proceed, it is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for error handling related to magnetic field usage.
# Expectation: Error checks for magnetic field context should be present elsewhere in the codebase.
rg 'if.*(magFieldContext|m_cfg\.magneticField).*' -A 5 -B 5
Length of output: 3292
Core/include/Acts/Seeding/EstimateTrackParamsFromSeed.hpp (5)
23-26: Appropriate forward declarations, these are.
28-32: Useful error enumeration, added you have.
34-34: Proper error code function, this is.
65-90: Well-documented new function, added you have.
225-230: Correctly registered error code enumeration, you have.
Core/src/Seeding/EstimateTrackParamsFromSeed.cpp (4)
11-18: Necessary includes, added they are.
Hmm, appropriate the inclusion of required headers is.
25-46: Well-implemented, your custom error category is.
Clear and descriptive, the error messages are.
49-53: Correctly, you construct error codes.
Proper, the use of make_error_code function is.
182-182: Logging macros, verify their proper use you must.
Ensure ACTS_INFO macro is properly defined and usable in this context.
Run the following script to verify the availability of ACTS_INFO macro:
Also applies to: 206-206, 229-229
✅ Verification successful
Properly defined and usable, the ACTS_INFO macro is, young padawan.
In Core/include/Acts/Utilities/Logger.hpp, defined the macro is:
#define ACTS_INFO(x) ACTS_LOG(Acts::Logging::INFO, x)Used extensively throughout the codebase it is, with proper logging header inclusion. Verified its proper definition and usage in the context I have.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for the definition of `ACTS_INFO` macro.
# Command: Find the definition of `ACTS_INFO` in the codebase.
rg '^#define\s+ACTS_INFO' -t c
Length of output: 32
Script:
#!/bin/bash
# Hmm, search for logging macros in header files, we must
rg -g '*.hpp' -g '*.h' 'ACTS_INFO' -B2 -A2
# And in source files too, search we shall
rg -g '*.cpp' 'ACTS_INFO' -B2 -A2
# Look for logging header inclusion patterns, we must
fd -e hpp -e h -e cpp | xargs rg 'include.*logging'
Length of output: 102023
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
Core/include/Acts/Seeding/EstimateTrackParamsFromSeed.hpp (2)
83-89: Consider raw pointer for magnetic field provider, we must.From past discussions, a raw pointer sufficient might be. Unnecessary overhead from shared_ptr, we should avoid.
- const std::shared_ptr<const MagneticFieldProvider>& bField, + const MagneticFieldProvider* bField,
79-80: Clarify propagator documentation, we must.More details about propagator configuration and usage, the documentation needs. Help users understand when propagator is required, it will.
Core/src/Seeding/EstimateTrackParamsFromSeed.cpp (3)
182-183: More specific error messages, provide you should.Generic messages like "The magnetic field failed" and "The propagation failed", helpful they are not. Include more details about the failure cause, you must.
- ACTS_INFO("The magnetic field failed."); + ACTS_INFO("Magnetic field retrieval failed at position " << sp0.transpose()); - ACTS_INFO("The propagation failed."); + ACTS_INFO("Track propagation to surface failed with parameters: origin=" + << origin.transpose() << ", direction=" << direction.transpose());Also applies to: 229-231
199-201: Document the particle hypothesis choice, you should.Why pion hypothesis is chosen, unclear it remains. Add comment explaining this default choice, you must.
- // particle hypothesis does not matter here + // Default to pion hypothesis as it represents a typical particle mass + // and charge for track reconstruction in particle detectors
172-248: Split this function into smaller parts, we should.Too many responsibilities, this function has. Consider extracting these parts:
- Magnetic field retrieval and parameter estimation
- Surface intersection handling
- Propagation logic
Easier to maintain and test, smaller functions are. Hmmmm.
Example structure:
Result<FreeParameters> estimateInitialParameters(/*...*/); Result<BoundVector> handleSurfaceIntersection(/*...*/); Result<BoundVector> propagateToSurface(/*...*/);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
Core/include/Acts/Seeding/EstimateTrackParamsFromSeed.hpp(4 hunks)Core/src/Seeding/EstimateTrackParamsFromSeed.cpp(2 hunks)
🔇 Additional comments (6)
Core/include/Acts/Seeding/EstimateTrackParamsFromSeed.hpp (3)
23-32: Hmm, well-structured these declarations are.
Clear and purposeful, the forward declarations and error enum are. Non-zero values for errors, a wise choice it is.
34-34: Strong with the Force, this error handling is.
Properly integrated with STL error system, it is. A path to robust error handling, this provides.
Also applies to: 222-227
149-180: 🛠️ Refactor suggestion
Verify propagator before use, we should.
When provided, validate the propagator we must. Add a check, like this:
const BasePropagator* propagator = nullptr,
const Acts::Logger& logger = getDummyLogger()) {
+ if (propagator != nullptr) {
+ // Validate propagator configuration
+ if (!propagator->isConfigured()) {
+ return Result<BoundVector>::failure(EstimateTrackParamsFromSeedError::PropagationFailed);
+ }
+ }
// Check the number of provided space pointsCore/src/Seeding/EstimateTrackParamsFromSeed.cpp (3)
25-45: Hmmmm, good error handling, I sense.
Well-structured error category class, you have created. Clear messages and proper inheritance, it shows. The Force is strong with this implementation.
225-225: Fix the logger usage, you must.
Still incorrect, the logger usage is. Direct object it should be, not a function call.
172-177: Verify the usage of this new function across the codebase, we must.
Search for callers of this function and ensure proper parameter passing, we should.
✅ Verification successful
Proper usage of the function across the codebase, I see
Hmm, examined the usage carefully, I have. In balance with the Force, the implementation is:
- Correctly used in test cases with proper parameter passing, it is
- In TrackParamsEstimationAlgorithm and PrototracksToParameters, properly implemented the function calls are
- Multiple overloads in the header file, exist they do, all consistent with the implementation
No disturbance in the Force, I sense. Properly integrated and used throughout the codebase, this function is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of the new function
rg -l "estimateTrackParamsFromSeedAtSurface"
# Check if the propagator parameter is properly handled
ast-grep --pattern 'estimateTrackParamsFromSeedAtSurface($_,$_,$_,$_,$_,$_,$_,$_,nullptr,$_)'
Length of output: 456
Script:
#!/bin/bash
# Check the actual usage in the test file
rg "estimateTrackParamsFromSeedAtSurface" Tests/UnitTests/Core/Seeding/EstimateTrackParamsFromSeedTest.cpp -B 2 -A 2
# Check usage in the algorithms
rg "estimateTrackParamsFromSeedAtSurface" Examples/Algorithms/TrackFinding/src/TrackParamsEstimationAlgorithm.cpp -B 2 -A 2
rg "estimateTrackParamsFromSeedAtSurface" Examples/Algorithms/TrackFindingExaTrkX/src/PrototracksToParameters.cpp -B 2 -A 2
# Check the function declaration in the header
rg "estimateTrackParamsFromSeedAtSurface" Core/include/Acts/Seeding/EstimateTrackParamsFromSeed.hpp -B 2 -A 2
Length of output: 1886
|
|
I think this should rather be achieved by the user by calling the free param version and an extrapolator. #3909 will help with the extrapolation starting from free parameters. The bound version should ultimately be removed as it causes too much confusion. An example of a workflow can be integrated into our estimate params algo in Examples. |
The interface of
suggests that the parameters will be estimated on any surface. The doc is making this clear but we also had multiple cases when this would be useful.
Generally a spacepoint does not need to be on surface. For example for combined strip space points this is not the case.
For this reason I propose to replace this function with
which makes extrapolates the parameters to the
surfaceand the extrapolation is explicit given thepropagatorparameter. The propagator will be default constructed if none is provided (default).Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor