fix: Write correct tracks to file for refitting, prevent FPE #3999
Conversation
WalkthroughIn the realm of track refitting, significant changes have emerged. Two Python scripts, Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (2)
✨ Finishing Touches
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Examples/Scripts/Python/truth_tracking_gsf_refitting.py(2 hunks)
🔇 Additional comments (2)
Examples/Scripts/Python/truth_tracking_gsf_refitting.py (2)
79-82: Correct, this change is! Using GSF refit tracks instead of original tracks, we now are.
Wisdom in this modification I sense. Properly aligned with the GSF refitting output, the RootTrackSummaryWriter now is.
Line range hint 1-120: Verify the flow of tracks through the Force, we must.
Through the sequence of operations, track flow we must validate. From KF tracks to GSF refit tracks, the path leads.
✅ Verification successful
Verified, the flow of tracks is. Consistent, the Force remains.
Through the codebase, a clear path I see:
- From "kf_tracks" to "gsf_refit_tracks" the transformation flows
- Proper track-particle matching maintained through "refit_track_particle_matching"
- Only in performance writer, "track_particle_matching" used as intended
Hmm, yes. In balance, the Force is. No disturbance in the tracking flow I sense.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the consistent usage of track references across the codebase
# Search for track reference patterns to ensure consistency
echo "=== Searching for track reference patterns ==="
rg -A 2 "inputTracks\s*=\s*['\"](?:kf_tracks|gsf_refit_tracks)['\"]"
# Search for track-particle matching patterns
echo "=== Searching for track-particle matching patterns ==="
rg -A 2 "TrackParticleMatching\s*=\s*['\"](?:track_particle_matching|refit_track_particle_matching)['\"]"
Length of output: 9852
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Examples/Algorithms/TrackFitting/include/ActsExamples/TrackFitting/RefittingAlgorithm.hpp (1)
36-37: Approve this change, I do. Yet documentation, it needs.Wise addition of
initialVarInflationparameter, this is. But document its purpose and the meaning of each element in the array, we must.Add documentation like this, you should:
/// Inflate initial covariance. + /// @note Array elements correspond to [d0, z0, phi, theta, q/p, t] parameters std::array<double, 6> initialVarInflation = {1., 1., 1., 1., 1., 1.};Examples/Python/src/TrackFitting.cpp (1)
43-45: Consider documentation updates, you should.When Python bindings change, documentation in both realms - C++ and Python - update we must. For future Padawans, clear guidance provide.
Consider:
- Adding Python docstring examples
- Updating C++ documentation with parameter description
- Including usage examples in README
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Examples/Algorithms/TrackFitting/include/ActsExamples/TrackFitting/RefittingAlgorithm.hpp(1 hunks)Examples/Algorithms/TrackFitting/src/RefittingAlgorithm.cpp(1 hunks)Examples/Python/src/TrackFitting.cpp(1 hunks)Examples/Scripts/Python/truth_tracking_gsf_refitting.py(3 hunks)Examples/Scripts/Python/truth_tracking_kalman_refitting.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Examples/Scripts/Python/truth_tracking_gsf_refitting.py
🔇 Additional comments (3)
Examples/Scripts/Python/truth_tracking_kalman_refitting.py (1)
44-44: Large inflation values, concerning they are. Verify their necessity, we must.
Setting all covariance inflation factors to 100.0, a significant impact on track fitting accuracy it may have. Consider different values for different parameters, you should.
Examples/Algorithms/TrackFitting/src/RefittingAlgorithm.cpp (1)
83-91: Placement of covariance inflation, correct it is.
Good structure maintained, error handling preserved. Approve this part of implementation, I do.
Examples/Python/src/TrackFitting.cpp (1)
44-44: Approve this change, I do. Yet verify its impact, we must.
Hmmmm. Wise addition of initialVarInflation parameter, this is. With the PR's objective of fixing GSF refitting output, aligns well it does.
Run this verification script, we shall:
✅ Verification successful
Verified the change is, proper usage found it has.
Found evidence in the codebase, I have:
- In
truth_tracking_gsf_refitting.pyandtruth_tracking_kalman_refitting.py, properly used the parameter is, with value[100.0] * 6 - In
RefittingAlgorithm.hpp, default values defined correctly they are:{1., 1., 1., 1., 1., 1.} - In
RefittingAlgorithm.cpp, implementation handles the parameter appropriately it does
Matches the PR's objective of fixing GSF refitting output, this change does. Working as intended, the parameter is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usage patterns of initialVarInflation in Python scripts
# and related configuration files
# Search for Python scripts using RefittingAlgorithm
echo "=== Python usage patterns ==="
rg -A 5 "RefittingAlgorithm.*initialVarInflation" --type py
# Search for any documentation or configuration mentioning this parameter
echo "=== Documentation and configuration ==="
rg "initialVarInflation" --type md --type txt
Length of output: 265
Script:
#!/bin/bash
# Search for RefittingAlgorithm usage patterns more broadly
echo "=== RefittingAlgorithm usage patterns ==="
rg -A 10 "RefittingAlgorithm" --type py
# Search for initialVarInflation in C++ files
echo "=== C++ implementation details ==="
rg -A 5 "initialVarInflation" --type cpp
# Look for the algorithm declaration pattern
echo "=== Algorithm declaration patterns ==="
ast-grep --pattern 'ACTS_PYTHON_DECLARE_ALGORITHM($$$)'
Length of output: 31849
github-actions
bot
added
Infrastructure
|
|
@andiwand @benjaminhuth the physmon failure is real, there's a new FPE in the KF refitting job: Negatice covariance value? |
|
Puh... Seems to me like a 2025 problem :D |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
Tests/UnitTests/Core/Propagator/DirectNavigatorTests.cpp (5)
197-202: Clean and purposeful, this struct is! Yet documentation, it lacks.A brief comment explaining the purpose of NavigationBreakAborter, helpful would be.
+/// Struct to abort navigation when a break condition is met in the navigator struct NavigationBreakAborter {
212-213: Remove commented code, we must! Clarity, it brings.The commented logger configuration, distracting it is.
- Propagator<StraightLineStepper, DirectNavigator> prop(std::move(stepper), std::move( - dnav) /*, Acts::getDefaultLogger("DirectNavigatorTest", Acts::Logging::VERBOSE)*/); + Propagator<StraightLineStepper, DirectNavigator> prop(std::move(stepper), + std::move(dnav));
229-232: Magic numbers in the parameters, I sense! Named constants, we should use.More meaningful, the test parameters should be.
+ constexpr auto kDefaultMomentum = 1.0_GeV; + constexpr auto kDefaultTime = 0.0_ns; BoundTrackParameters startParameters = BoundTrackParameters( startSurface->getSharedPtr(), - {0.0_mm, 0.0_mm, 0.0_rad, 0.0_rad, 1.0 / 1.0_GeV, 0.0_ns}, std::nullopt, + {0.0_mm, 0.0_mm, 0.0_rad, 0.0_rad, 1.0 / kDefaultMomentum, kDefaultTime}, std::nullopt, ParticleHypothesis::muon());
258-267: Magic numbers in surface creation, troubling they are! Constants, we need.More readable and maintainable, the test setup should be.
+ constexpr int kNumSurfaces = 10; + constexpr double kSurfaceSpacing = 100.0_mm; std::vector<std::shared_ptr<const Acts::Surface>> surfaces; - for (int i = 0; i < 10; i++) { + for (int i = 0; i < kNumSurfaces; i++) { Transform3 transform = Transform3::Identity(); - transform.translate(Vector3{0.0_mm, 0.0_mm, i * 100.0_mm}); + transform.translate(Vector3{0.0_mm, 0.0_mm, i * kSurfaceSpacing});
263-266: GeometryId creation, more explicit it should be!A helper function for GeometryId creation, clearer intent would show.
+ auto createGeometryId = [](int index) { + return Acts::GeometryIdentifier() + .setVolume(1) + .setLayer(1) + .setSensitive(index + 1); + }; - surface->assignGeometryId( - Acts::GeometryIdentifier().setVolume(1).setLayer(1).setSensitive(i + - 1)); + surface->assignGeometryId(createGeometryId(i));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Core/include/Acts/Propagator/DirectNavigator.hpp(2 hunks)Tests/UnitTests/Core/Propagator/DirectNavigatorTests.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Core/include/Acts/Propagator/DirectNavigator.hpp
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: merge-sentinel
- GitHub Check: unused_files
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, clang++)
- GitHub Check: missing_includes
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: macos
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: linux_ubuntu
- GitHub Check: build_debug
🔇 Additional comments (1)
Tests/UnitTests/Core/Propagator/DirectNavigatorTests.cpp (1)
25-25: Hmmmm, good this include addition is!For the new test cases, necessary this StraightLineStepper include is.
Pulled out of #3999 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **New Features** - Enhanced navigation initialization for direct navigator. - Added more comprehensive testing for navigation scenarios. - **Tests** - Introduced new test case for forward and backward surface navigation. - Added navigation break abort logic for more robust testing. - **Improvements** - Streamlined surface index determination method. - Improved logging for navigation direction. - Updated hash values for test files related to refitting and tracking. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
benjaminhuth
changed the title
|
acts-project-service
added
the
Fails Athena tests
…oject#3999) Unfortunately, there where the KF tracks written to files so far.
Unfortunately, there where the KF tracks written to files so far.
--- END COMMIT MESSAGE ---
Any further description goes here, @-mentions are ok here!
feat,fix,refactor,docs,choreandbuildtypes.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores