feat: Add chi2 cut option for outliers to KF fitter function #4059
Conversation
WalkthroughA new Changes
Possibly related PRs
Suggested Reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (80)
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 - Examples
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Examples/Algorithms/TrackFitting/src/KalmanFitterFunction.cpp (1)
70-78: Consider using existing Acts::TrackHelpers::checkPredictedChi2, hmm.In TrackHelpers.hpp, a similar function exists already. Duplicate code, this is. Use the existing utility, we should.
-struct SimpleOutlierFinder { - double chi2Cut = std::numeric_limits<double>::infinity(); - - bool isOutlier( - Acts::VectorMultiTrajectory::ConstTrackStateProxy trackState) const { - double chi2 = Acts::calculatePredictedChi2(trackState); - return chi2 > chi2Cut; - } -}; +using SimpleOutlierFinder = Acts::TrackHelpers::OutlierFinder;Examples/Python/python/acts/examples/reconstruction.py (1)
1307-1307: Hmmmm, wise addition this is!A new parameter
chi2Cutwith default value of infinity, added you have. No outliers rejected by default, this ensures. Backward compatibility maintained, this does.But caution, young padawan! Document the parameter in the function's docstring, you should. Help future Jedi understand its purpose, this will.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Examples/Algorithms/TrackFitting/include/ActsExamples/TrackFitting/TrackFitterFunction.hpp(1 hunks)Examples/Algorithms/TrackFitting/src/KalmanFitterFunction.cpp(5 hunks)Examples/Python/python/acts/examples/reconstruction.py(1 hunks)Examples/Python/src/TrackFitting.cpp(1 hunks)Examples/Scripts/Python/truth_tracking_kalman_refitting.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, clang++)
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: linux_ubuntu
- GitHub Check: macos
- GitHub Check: build_debug
- GitHub Check: missing_includes
🔇 Additional comments (6)
Examples/Algorithms/TrackFitting/src/KalmanFitterFunction.cpp (2)
89-89: Approve the outlier finder integration, I do.Clean and consistent with existing patterns, this integration is. Well done, young padawan!
Also applies to: 116-117
175-175: Approve the chi2Cut parameter handling, I do.Properly initialized and passed through, the chi2Cut parameter is. Clear and correct, the implementation is.
Also applies to: 207-207
Examples/Algorithms/TrackFitting/include/ActsExamples/TrackFitting/TrackFitterFunction.hpp (1)
71-71: Approve the function signature update, I do.Wise choice of default value to infinity, it is. Backward compatibility, this maintains.
Examples/Python/src/TrackFitting.cpp (1)
56-56: Approve the Python binding changes, I do.Properly exposed to Python, the chi2Cut parameter is. The Force is strong with these bindings.
Also applies to: 60-60, 66-66
Examples/Scripts/Python/truth_tracking_kalman_refitting.py (1)
37-37: Approve the kalmanOptions update, I do.Consistent with C++ defaults, this change is. Balance to the Force, it brings.
Examples/Python/python/acts/examples/reconstruction.py (1)
Line range hint
1299-1308: Verify the chi-squared cut parameter's journey, we must.Through the Python bindings to the C++ implementation, flow this parameter must. Check its path, we shall.
Document the parameter in function's docstring, like this you should:
def addKalmanTracks( s: acts.examples.Sequencer, trackingGeometry: acts.TrackingGeometry, field: acts.MagneticFieldProvider, reverseFilteringMomThreshold: float = 0 * u.GeV, inputProtoTracks: str = "truth_particle_tracks", multipleScattering: bool = True, energyLoss: bool = True, clusters: str = None, calibrator: acts.examples.MeasurementCalibrator = acts.examples.makePassThroughCalibrator(), + chi2Cut: float = float("inf"), logLevel: Optional[acts.logging.Level] = None, ) -> None: """This function steers the seeding Parameters ---------- s: Sequencer the sequencer module to which we add the Seeding steps (returned from addSeeding) trackingGeometry : tracking geometry field : magnetic field reverseFilteringMomThreshold : float momentum threshold for reverse filtering inputProtoTracks : str input proto tracks collection name multipleScattering : bool include multiple scattering energyLoss : bool include energy loss clusters : str input clusters collection name calibrator : MeasurementCalibrator measurement calibrator instance + chi2Cut : float + Chi-squared cut value for outlier rejection. Default is infinity (no cut). logLevel : Optional[acts.logging.Level] logging level """✅ Verification successful
Correct, the chi-squared cut implementation is, young Padawan!
Through the Force, I sense proper flow of the chi-squared cut parameter:
- From Python realm to C++ implementation it travels
- In outlier rejection, its purpose it serves
- Default value of infinity, preserved it remains
Your docstring update suggestion, wisdom it shows. Proceed with it, you should!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for chi2Cut usage in C++ implementation echo "Searching for chi2Cut usage in C++ files..." rg -t cpp "chi2Cut|chi2_cut" # Search for related Kalman filter configuration echo "Searching for Kalman filter configuration..." rg -t cpp "KalmanFitterFunction|KalmanFitterOptions"Length of output: 5309
|
acts-project-service
added
the
Breaks Athena build
self explaining
--- END COMMIT MESSAGE ---
Any further description goes here, @-mentions are ok here!
feat,fix,refactor,docs,choreandbuildtypes.Summary by CodeRabbit
New Features
Improvements