feat: Add/activate reverse filter cov inflation for KF and GSF #3996
Conversation
WalkthroughEnhancements to track fitting algorithms, introduced in this pull request, across multiple files, they are. New parameter Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
github-actions
bot
added
Component - Core
![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
🧹 Nitpick comments (5)
Core/include/Acts/TrackFitting/GaussianSumFitter.hpp (1)
332-335: Initialize inflatedParams, you do. Gravely important, correct reference surface is.
Check you must that 'particleHypothesis()' aligns well with your scaling changes. Otherwise, confusion it can cause.Core/include/Acts/TrackFitting/GsfOptions.hpp (1)
115-116: New scaling factor introduced, powerful it can be.
Default of 1.0 is safe, yes. But beware if left unchanged, no effect it has. Document well, you must.Examples/Algorithms/TrackFitting/include/ActsExamples/TrackFitting/TrackFitterFunction.hpp (1)
69-69: Document the new parameter, you must!Missing documentation for
reverseFilteringCovarianceScalingparameter, I sense. Add documentation to explain its purpose and impact on the covariance inflation during reverse filtering, you should.Examples/Algorithms/TrackFitting/src/KalmanFitterFunction.cpp (1)
79-79: Initialize in constructor, you should!Missing member initialization in constructor, I sense. Add it to the constructor's initialization list, you must, to ensure proper initialization.
KalmanFitterFunctionImpl(Fitter&& f, DirectFitter&& df, const Acts::TrackingGeometry& trkGeo) : fitter(std::move(f)), directFitter(std::move(df)), + reverseFilteringCovarianceScaling(1.0), slSurfaceAccessor{trkGeo} {}Examples/Algorithms/TrackFitting/src/GsfFitterFunction.cpp (1)
Line range hint
90-236: Wise architectural decision this is, hmmmm.Flexibility in reverse filtering covariance scaling, this feature brings. Good separation of concerns it maintains - from configuration through implementation. For track fitting quality, important this is.
Consider documenting the impact of different scaling values on the fitting quality, we should. Help future Padawans in their journey, it will.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
CI/physmon/workflows/physmon_trackfitting_kf.py(1 hunks)Core/include/Acts/TrackFitting/GaussianSumFitter.hpp(1 hunks)Core/include/Acts/TrackFitting/GsfOptions.hpp(1 hunks)Examples/Algorithms/TrackFitting/include/ActsExamples/TrackFitting/TrackFitterFunction.hpp(2 hunks)Examples/Algorithms/TrackFitting/src/GsfFitterFunction.cpp(4 hunks)Examples/Algorithms/TrackFitting/src/KalmanFitterFunction.cpp(4 hunks)Examples/Python/python/acts/examples/reconstruction.py(3 hunks)Examples/Python/src/TrackFitting.cpp(2 hunks)Examples/Scripts/Python/truth_tracking_kalman.py(2 hunks)
🔇 Additional comments (13)
Core/include/Acts/TrackFitting/GaussianSumFitter.hpp (3)
324-326: A wise container, this is.
Well chosen, the creation of 'inflatedParamVector' is. Maintains clarity, it does.
327-330: Multiply covariances, you do.
Right it is, to scale by 'reverseFilteringCovarianceScaling'. Validate no overflow or numeric instability occurs, one must.
340-340: Brace yourself, do.
Building the propagator state with 'inflatedParams' is cunning. Test thoroughly the resulting backward fit, to ensure correctness, you should.
CI/physmon/workflows/physmon_trackfitting_kf.py (1)
27-28: Infinite threshold and great scaling, hmmm?
Wise to override at runtime, this design is. Confirm no unexpected side effects for large scaling, you must.
Examples/Scripts/Python/truth_tracking_kalman.py (2)
21-21: Default value of 1, set.
Neutral it is—makes sense for stable baseline. Good parsing and validations, ensure.
126-126: Apply the force of scaling, you do.
Large expansions or contractions possible. Balanced with thresholds, they should be.
Examples/Python/python/acts/examples/reconstruction.py (2)
1292-1292: Approve the addition of reverseFilteringCovarianceScaling to Kalman configuration.
Wise choice, the default value of 1.0 is. To the kalman options, properly flows the parameter does.
Also applies to: 1306-1306
1367-1367: Verify the significant difference in default scaling values between KF and GSF.
Hmm, curious this is. For KF, default of 1.0 we have, yet for GSF, 100.0 we choose. Explain this significant difference, can you? Verify that intentional this disparity is, we must.
Examples/Algorithms/TrackFitting/include/ActsExamples/TrackFitting/TrackFitterFunction.hpp (1)
93-94: Approve these changes, I do!
Well documented and properly placed, the new parameter is. Clear explanation of its purpose in covariance inflation, you have provided.
Also applies to: 102-102
Examples/Python/src/TrackFitting.cpp (1)
56-67: Strong with the Force, these Python bindings are!
Consistent with C++ interface and following Python binding best practices, these changes are. Named arguments for better Python API, you have used wisely.
Also applies to: 112-122
Examples/Algorithms/TrackFitting/src/KalmanFitterFunction.cpp (1)
117-118: Approve this implementation, I do!
Properly integrated with KF options, the new scaling parameter is. Correct initialization and usage throughout the code, you have achieved.
Also applies to: 197-198
Examples/Algorithms/TrackFitting/src/GsfFitterFunction.cpp (2)
119-120: Correct this implementation is, approve it I do.
Clean and proper, the setting of reverseFilteringCovarianceScaling in gsfOptions is. Follows the pattern of other options, it does.
201-201: Approve this change I do, but verify its usage we must.
Correctly implemented the parameter passing is, but verify we should that all callers of makeGsfFitterFunction updated they are.
Also applies to: 235-236
✅ Verification successful
Verified the changes are, all callers updated they have been.
In the codebase, a single caller of makeGsfFitterFunction exists in Examples/Python/src/TrackFitting.cpp, and correctly passes all parameters including the new reverseFilteringCovarianceScaling parameter it does. The declaration in the header file and implementation in GsfFitterFunction.cpp, aligned perfectly they are.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all calls to makeGsfFitterFunction
rg "makeGsfFitterFunction\(" --type cpp -A 5
Length of output: 2317
Script:
#!/bin/bash
# Search for the declaration and implementation to see if they match
ast-grep --pattern 'makeGsfFitterFunction($$$)'
Length of output: 49
Script:
#!/bin/bash
# Check the actual call sites in more detail
rg "makeGsfFitterFunction\(" --type cpp -l | xargs cat
Length of output: 22149
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Examples/Scripts/Python/truth_tracking_kalman_refitting.py (1)
21-21: Document the ways of the Force, we should.Add docstring explanation for this parameter, you must. Help future Padawans understand its purpose and effects, it will.
reverseFilteringMomThreshold=0 * u.GeV, - reverseFilteringCovarianceScaling=1.0, + reverseFilteringCovarianceScaling=1.0, # Scale factor for covariance inflation during reverse filtering s: acts.examples.Sequencer = None,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Examples/Scripts/Python/truth_tracking_kalman_refitting.py(2 hunks)
🔇 Additional comments (2)
Examples/Scripts/Python/truth_tracking_kalman_refitting.py (2)
21-21: Wise addition to the function parameters, this is.
Default value of 1.0, maintain original behavior it does. Positioned well after momentum threshold parameter, it is. Hmmmm.
36-36: Verify the usage of this parameter in the Kalman fitter, we must.
Correctly added to options dictionary, it is. But confirm its proper handling in the fitter implementation, we should.
✅ Verification successful
Properly used throughout the codebase, this parameter is, hmmmm.
Clear evidence I see:
- In KalmanFitterFunction implementation, present and properly passed, it is (
Examples/Algorithms/TrackFitting/src/KalmanFitterFunction.cpp) - Default value of 1.0, wisely set it has (
Examples/Algorithms/TrackFitting/include/ActsExamples/TrackFitting/TrackFitterFunction.hpp) - Through Python bindings, correctly flows it does (
Examples/Python/src/TrackFitting.cpp)
Properly integrated and used throughout the fitting chain, this parameter is. Trust in the Force and the implementation, we can.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the parameter usage in the Kalman fitter implementation
ast-grep --pattern 'reverseFilteringCovarianceScaling'
# Check the fitter function implementation
rg -A 5 'makeKalmanFitterFunction'
Length of output: 6095
|
looks like GSF refit is back to life but KF refit is still broken @benjaminhuth
The other changes look like they are compatible with the usual noise level. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CI/physmon/workflows/physmon_trackrefitting_kf.py(1 hunks)Examples/Scripts/Python/truth_tracking_gsf_refitting.py(2 hunks)Examples/Scripts/Python/truth_tracking_kalman_refitting.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Examples/Scripts/Python/truth_tracking_kalman_refitting.py
- Examples/Scripts/Python/truth_tracking_gsf_refitting.py
🔇 Additional comments (1)
CI/physmon/workflows/physmon_trackrefitting_kf.py (1)
27-27: Verify the implications of infinite momentum threshold, we must.
Hmmmm. Set to infinity, this threshold is. Wisdom, it requires, to understand why no filtering of momentum we seek. Performance implications, this might have.
Run this script to search for momentum threshold usage, we shall:
✅ Verification successful
Let me search deeper into the Force to understand the implementation details of this reverse filtering mechanism.
Hmmmm. Failed, our first attempt did. Try again with proper file type definitions, we must. The Force guides us to search deeper.
Safe, the infinite momentum threshold is, young Padawan.
Through the Force, clear it has become:
- When momentum threshold is set high (or infinite), reverse filtering is activated
- When threshold is low (0.1 GeV), reverse filtering is not used
- Setting to infinity ensures reverse filtering always runs, as no track can exceed it
- Part of a deliberate strategy for covariance inflation in Kalman Filter, this is
The test files reveal:
// Track of 1 GeV with a threshold set at 10 GeV, reversed filtering should be used
test(10._GeV, false, true, false);
Wisdom from the ancient texts shows this parameter controls when to apply reverse filtering, not a performance limitation it is. A design choice to always enable reverse filtering with covariance scaling, this represents.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for momentum threshold usage in related files
rg -A 5 "reverseFilteringMomThreshold"
Length of output: 9187
Script:
#!/bin/bash
# Search for implementation details of the Kalman filter's reverse filtering
ast-grep --pattern 'class $_ {
$$$
reverseFilteringMomThreshold
$$$
}'
# Also search for any documentation or comments about reverse filtering
rg -B 2 -A 2 "reverse.*filter" --type cpp --type hpp
Length of output: 161
Script:
#!/bin/bash
# Search for implementation details of the Kalman filter's reverse filtering
rg -B 3 -A 3 "reverse.*filter" --type-add 'hpp:*.hpp' --type-add 'cpp:*.cpp'
# Look for default values and usage patterns
rg "reverseFilteringMomThreshold" --type python -C 3
Length of output: 14963
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Core/include/Acts/TrackFitting/KalmanFitter.hpp (1)
487-489: Improve membership check approach, we must.Check membership with 'rangeContainsValue' for each track state, expensive can be. A set or map-based approach, consider we should; more efficient it would be. Overwriting previous outlier flags, watchful we must remain, lest inconsistent states arise.
|
KF looks good now 👍 do we expect GSF refit also to change? |
The weird thing is that we see hash changes in the GSF refit trackstates, but not in the GSF refit tracksummary... very weird |
|
I am also glad to see this change as well but why are some the inflation factors set to 1.0 ? |
|
@benjaminhuth I fear we write out KF tracks in the GSF refitting 🙈 acts/Examples/Scripts/Python/truth_tracking_gsf_refitting.py Lines 76 to 94 in 32d4bc1 can you change these two cases here? |
|
puh thats not good... I'll do that in a separate PR to see the physmon changes... |
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
Examples/Python/src/TrackFitting.cpp (1)
Line range hint
1-150: Balanced and harmonious, the architecture remains!Through Python bindings, the Force flows smoothly. Consistent naming and proper separation, maintained they are. But remember, young Padawan:
- Test coverage for different scaling values, essential it is
- Documentation for these parameters, helpful it would be
- Performance impact of scaling operations, monitor we should
Examples/Algorithms/TrackFitting/include/ActsExamples/TrackFitting/RefittingAlgorithm.hpp (1)
36-37: Document the purpose and impact of initialVarInflation, you must.Hmmmm, clear documentation for future padawans, essential it is. Explain what each element in the array represents and how it affects the covariance matrix, we should.
Add this documentation, you should:
/// Inflate initial covariance. + /// @brief Array of scaling factors for the diagonal elements of the initial covariance matrix + /// @details Elements correspond to [d0, z0, phi, theta, q/p, t] bound parameters + /// Values > 1 increase uncertainty, values < 1 decrease uncertainty std::array<double, 6> initialVarInflation = {1., 1., 1., 1., 1., 1.};Examples/Algorithms/TrackFitting/src/RefittingAlgorithm.cpp (1)
87-91: Add bounds checking for inflation values, we should.Protect against negative or extreme inflation values, wise it would be. Prevent numerical instabilities in the fitting process, this will.
Consider this implementation, you might:
if (initialParams.covariance()) { + // Ensure inflation values are positive and within reasonable bounds + for (const auto& inflation : m_cfg.initialVarInflation) { + if (inflation <= 0.0 || inflation > 1000.0) { + ACTS_WARNING("Invalid inflation value: " << inflation + << ". Must be in range (0, 1000]"); + return ProcessCode::ABORT; + } + } for (auto i = 0ul; i < m_cfg.initialVarInflation.size(); ++i) { (*initialParams.covariance())(i, i) *= m_cfg.initialVarInflation.at(i); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Examples/Algorithms/TrackFitting/include/ActsExamples/TrackFitting/RefittingAlgorithm.hpp(1 hunks)Examples/Algorithms/TrackFitting/src/KalmanFitterFunction.cpp(4 hunks)Examples/Algorithms/TrackFitting/src/RefittingAlgorithm.cpp(1 hunks)Examples/Python/src/TrackFitting.cpp(3 hunks)Examples/Scripts/Python/truth_tracking_gsf_refitting.py(3 hunks)Examples/Scripts/Python/truth_tracking_kalman_refitting.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Examples/Scripts/Python/truth_tracking_kalman_refitting.py
- Examples/Algorithms/TrackFitting/src/KalmanFitterFunction.cpp
- Examples/Scripts/Python/truth_tracking_gsf_refitting.py
🔇 Additional comments (4)
Examples/Python/src/TrackFitting.cpp (3)
44-44: Approve this change, I do!
Wise addition of initialVarInflation parameter, it is. Follows the path of the Force, this change does.
56-67: Strong with the Force, these changes are!
Properly integrated, the new reverseFilteringCovarianceScaling parameter is. Through Python bindings, flows the Force does. Clear and well-documented, the path is.
112-122: Balance to the Force, this change brings!
Correctly implemented, the reverseFilteringCovarianceScaling parameter is. But verify its effects across the galaxy, we must.
✅ Verification successful
Hmmmm... Clear patterns in the Force, I sense!
Observed throughout the codebase, a consistent default value of 1.0 for reverseFilteringCovarianceScaling is. Only in CI monitoring workflows, a different value of 100.0 appears, which for testing purposes, natural it is.
The implementation:
- In Core's GsfOptions.hpp, the default value of 1.0 established is
- In Python bindings and example scripts, this default preserved is
- In CI monitoring, a higher value for specific test cases used is
Balanced and harmonious, this implementation appears. No disturbance in the Force, I detect.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for GSF refit usage and configuration patterns
# Search for GSF refit configuration
rg -A 5 "makeGsfFitterFunction.*reverseFilteringCovarianceScaling"
# Search for any hardcoded scaling values
rg "reverseFilteringCovarianceScaling.*=.*[0-9]"
Length of output: 1177
Examples/Algorithms/TrackFitting/src/RefittingAlgorithm.cpp (1)
87-91: Approve the covariance inflation implementation, I do. Yet verify its effects, we must.
Correctly implemented, the covariance inflation is. Guards against invalid covariance, it does.
Run these tests, we should:
For KF, this wires the existing flags through the examples framework.
For the GSF, this adds this flag and activates it in the examples framework.
--- END COMMIT MESSAGE ---
Any further description goes here, @-mentions are ok here!
feat,fix,refactor,docs,choreandbuildtypes.Summary by CodeRabbit
New Features
reverseFilteringCovarianceScalingto multiple functions for enhanced configuration options in tracking and fitting algorithms.initialVarInflationparameter to theRefittingAlgorithmfor improved covariance handling.Bug Fixes
Documentation