feat: Add Propagator::propagateToSurface with FreeVector as starting parameters
#3909
Conversation
WalkthroughA new method, 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 (
|
|
andiwand
changed the title
Propagator::propagateToSurface for FreeVector as startPropagator::propagateToSurface with FreeVector as starting parameters
![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 (2)
Core/include/Acts/Propagator/Propagator.ipp (1)
389-402: Documentation improvements needed, they are!Missing documentation about the expected structure of the FreeVector input, I see. Add Doxygen comments explaining the vector layout and required dimensions, you must.
Add this documentation above the method:
+/** + * @brief Propagate using free parameters to a target surface + * + * @param start Free parameters vector with structure: + * - [0-3]: 4D position (x,y,z,t) + * - [4-6]: 3D direction + * - [7]: q/p (charge divided by momentum) + * @param target The target surface to propagate to + * @param options The propagation options + * + * @note The particle hypothesis (pion) used in the conversion is not meaningful + * as propagation occurs without interactions + */Core/include/Acts/Propagator/Propagator.hpp (1)
Line range hint
52-78: Strong with the Force, this design pattern isTemplate method pattern, well utilized it is. Abstraction for both bound and free vector propagation, elegantly unified it has become. Extensible architecture, maintained it is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
Core/include/Acts/Propagator/Propagator.hpp(2 hunks)Core/include/Acts/Propagator/Propagator.ipp(2 hunks)
🔇 Additional comments (5)
Core/include/Acts/Propagator/Propagator.ipp (3)
9-9: Approve these includes, I do!
Necessary for the new functionality, these headers are. Well-placed and following conventions, they remain.
Also applies to: 16-16
394-396: Wise choice in documentation, this is!
Clear explanation about pion hypothesis being arbitrary, you provide. Good practice in code documentation, this represents.
401-401: Elegant reuse of existing method, this is!
Wise decision to delegate to existing implementation, you made. Clean and maintainable, the code remains.
Core/include/Acts/Propagator/Propagator.hpp (2)
52-61: Wise addition to the interface, this is!
Clear documentation about limitations, you have provided. No covariance transport and meaningless particle hypothesis, well documented they are. Interface consistency with bound parameters version, maintained it is.
76-78: Verify the implementation in .ipp file, we must
Correctly declared the override is, but implementation in Propagator.ipp file, verify we must.
✅ Verification successful
Found the implementation, I have. Correct and complete, it is.
In the Propagator.ipp file, the implementation exists at line 482:
template <typename derived_t>
Acts::Result<Acts::BoundTrackParameters>
Acts::detail::BasePropagatorHelper<derived_t>::propagateToSurface(
const FreeVector& start, const Surface& target,
const Options& options) const {
// Convert free parameters to curvilinear parameters...
CurvilinearTrackParameters startBound(
start.segment<4>(Acts::eFreePos0), start.segment<3>(Acts::eFreeDir0),
start[Acts::eFreeQOverP], std::nullopt, ParticleHypothesis::pion());
return propagateToSurface(startBound, target, options);
}Properly implemented the method is, converting free parameters to curvilinear parameters it does, before propagating to the surface. Hmmmm, yes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the implementation exists in the .ipp file
# Search for the implementation
rg -A 5 "propagateToSurface.*FreeVector.*const Surface" Core/include/Acts/Propagator/Propagator.ipp
Length of output: 101
Script:
#!/bin/bash
# Search for any implementation of propagateToSurface in .ipp files
rg -l "propagateToSurface" --type cpp Core/include/Acts/Propagator/
# Check if the .ipp file exists
fd "Propagator.ipp" Core/include/Acts/Propagator/
# Try broader search for implementation
rg -A 5 "Result.*propagateToSurface.*FreeVector" Core/include/Acts/Propagator/
Length of output: 337
Script:
#!/bin/bash
# Let's examine the content of Propagator.ipp directly
cat Core/include/Acts/Propagator/Propagator.ipp
# Also check the header file for any inline implementations
cat Core/include/Acts/Propagator/Propagator.hpp
Length of output: 33899
Similar to the existing
propagateToSurfacemethod for bound parameters, this adds another one forFreeVectorinputs.Summary by CodeRabbit
New Features
FreeVectorto a target surface, expanding the range of input types.Documentation