refactor!: Clean track EDM projector #3605
Conversation
github-actions
bot
added
Component - Core
WalkthroughChanges made, significant they are. The Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (3)Core/include/Acts/EventData/detail/MultiTrajectoryTestsCommon.hpp (3)
Clearer and more concise, the new name is. Well aligned with our objective of recognizable method names, this change is.
More precise, this new assertion is. From checking projector equality to verifying subspace indices, we have moved. Aligns with our mission to remove deprecated projector functions, this does.
Throughout the test case, replaced projector equality checks with subspace indices checks, we have. Maintains consistency and follows the new way, this does. Also applies to: 830-831, 855-856 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 (
|
|
This is currently still in draft mode. I looked through it already. Is anything missing from you side? |
I think I wasn't sure about the method names. Maybe we can go through that together at some point. Apart from that is everything is passing in ACTS we just need to adapt Athena. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (11)
Core/include/Acts/TrackFitting/detail/GainMatrixUpdaterImpl.hpp (1)
Line range hint
43-43: Address the TODO comment about subspace helper, we must.Optimization potential I sense in this TODO. The Force suggests using the subspace helper for projection would improve clarity and maintainability, yes.
Apply this change, you should:
- // TODO use subspace helper for projection instead - const auto H = subspaceHelper.projector(); + const auto H = subspaceHelper.project(trackState.predicted);Tests/UnitTests/Core/TrackFitting/MbfSmootherTests.cpp (1)
Line range hint
1-144: Strong with the tests, this file is!Comprehensive test coverage, I sense. Three track states it tests, smoothing functionality it validates, and regression checks it maintains. Well-structured, the test case is.
A suggestion for improvement, I have:
+ // Test setup VectorMultiTrajectory traj; std::array<BoundIndices, 2> projector{eBoundLoc0, eBoundLoc1}; + // Helper function to reduce code duplication + auto setupTrackState = [&](std::size_t& ts_idx, const BoundVector& predicted, + const BoundVector& filtered) { + ts_idx = traj.addTrackState(TrackStatePropMask::All, + ts_idx == std::size_t(-1) ? ts_idx : ts_idx); + auto ts = traj.getTrackState(ts_idx); + ts.typeFlags().set(TrackStateFlag::MeasurementFlag); + // ... rest of the common setup code + };Reduce duplication and improve maintainability, this refactoring would. Consider it, you should.
Core/src/TrackFinding/MeasurementSelector.cpp (1)
Line range hint
34-37: Strong with the Force, this validation is.Protect against empty configurations, you do. Yet enhance the error message further, we could.
Consider this improvement, you should:
- "MeasurementSelector: Configuration must not be empty"); + "MeasurementSelector: Configuration must contain at least one measurement selector cut");Core/include/Acts/TrackFitting/MbfSmoother.hpp (1)
105-105: Hmmmm, wise choice this type change is!Clear and purposeful, the switch to
BoundSubspaceIndicesis. Better represents the true nature of the projector, it does. A step towards more maintainable code, this change is.Consider documenting the meaning of these indices in the class documentation, young padawan, for future maintainers to understand better, they must.
Alignment/include/ActsAlignment/Kernel/detail/AlignmentEngine.hpp (2)
211-213: Documentation improvements, suggest I do.Complex mathematical operations these are. Add comments explaining the relationship between projector and alignment calculations, you should. Help future Padawans understand the code better, this will.
Add documentation like this, you could:
+ // The projection matrix H maps the bound track parameters to the measurement + // space. We obtain it through the projector subspace helper, which provides + // the full projector matrix. The topLeftCorner selection ensures we get the + // relevant measurement dimensions. const ActsDynamicMatrix H = state.projectorSubspaceHelper().fullProjector().topLeftCorner( measdim, eBoundSize);
211-213: Wise architectural choices, made you have.Centralized the projector logic has become, through the projector subspace helper. Better encapsulation and reusability across the codebase, this brings. Strong with the Force, this design is.
Consider documenting these architectural decisions in the class or module documentation, you should. Help others understand the wisdom of your choices, it will.
Core/include/Acts/Utilities/TrackHelpers.hpp (1)
699-700: Improve the projection implementation further, we must.A TODO comment lurks in our midst, young padawan. The Force suggests using the subspace helper for projection operations. Consider this improvement, we should:
-// TODO use subspace helper for projection instead -auto H = subspaceHelper.projector(); +auto H = subspaceHelper.fullProjector().topLeftCorner(measdim, eBoundSize);More efficient and consistent with the new approach, this would be. Hmmmm.
Core/include/Acts/TrackFitting/GlobalChiSquareFitter.hpp (2)
426-426: Approve the transition to projectorSubspaceHelper.Hmmmm, wise this change is. A more elegant approach to handle projector calculations, it provides. The old way of using topLeftCorner, deprecated it was. Cleaner and safer, the new way is.
Consider adding a brief comment explaining the projector calculation:
// Use projectorSubspaceHelper to obtain the measurement-to-parameter space projection const ActsMatrix<kMeasDim, eBoundSize> projector = trackState.template projectorSubspaceHelper<kMeasDim>().projector();
Line range hint
447-476: Enhance debug logging for better traceability.Verbose logging you have, yes. But structure it better we must. Group related information together we should, for easier debugging it would make.
Consider restructuring the debug output like this:
- ACTS_VERBOSE( - "Contributions in addMeasurementToGx2fSums:\n" - << " kMeasDim: " << kMeasDim << "\n" - << " predicted: " << predicted.transpose() << "\n" + ACTS_VERBOSE( + "=== Measurement Contributions ===\n" + << "Dimensions:\n" + << " kMeasDim: " << kMeasDim << "\n" + << "\nState:\n" + << " predicted: " << predicted.transpose() << "\n"Core/include/Acts/EventData/SubspaceHelpers.hpp (1)
39-39: Redundant the static_cast may be.If
indexRange.size()returnsstd::size_t, unnecessary the cast is.Apply this diff to remove the redundant static_cast:
- if (static_cast<std::size_t>(indexRange.size()) != subspaceSize) { + if (indexRange.size() != subspaceSize) { return false; }Core/include/Acts/EventData/TrackStateProxy.hpp (1)
610-619: Code duplication observed, refactor we must.Overloads of
setProjectorSubspaceIndicesshare similar code they do. To enhance maintainability and reduce repetition, refactor shared logic into a private helper function, you should.Also applies to: 639-647, 652-656
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (27)
Alignment/include/ActsAlignment/Kernel/detail/AlignmentEngine.hpp(1 hunks)Core/include/Acts/EventData/SubspaceHelpers.hpp(5 hunks)Core/include/Acts/EventData/TrackStateProxy.hpp(3 hunks)Core/include/Acts/EventData/TrackStateProxy.ipp(0 hunks)Core/include/Acts/EventData/TrackStateProxyConcept.hpp(0 hunks)Core/include/Acts/EventData/detail/MultiTrajectoryTestsCommon.hpp(5 hunks)Core/include/Acts/EventData/detail/TestSourceLink.hpp(2 hunks)Core/include/Acts/TrackFinding/MeasurementSelector.ipp(1 hunks)Core/include/Acts/TrackFitting/GainMatrixSmoother.hpp(0 hunks)Core/include/Acts/TrackFitting/GainMatrixUpdater.hpp(1 hunks)Core/include/Acts/TrackFitting/GlobalChiSquareFitter.hpp(1 hunks)Core/include/Acts/TrackFitting/MbfSmoother.hpp(1 hunks)Core/include/Acts/TrackFitting/detail/GainMatrixUpdaterImpl.hpp(1 hunks)Core/include/Acts/TrackFitting/detail/GsfUtils.hpp(3 hunks)Core/include/Acts/Utilities/TrackHelpers.hpp(1 hunks)Core/src/TrackFinding/MeasurementSelector.cpp(2 hunks)Core/src/TrackFitting/GsfUtils.cpp(2 hunks)Core/src/TrackFitting/MbfSmoother.cpp(3 hunks)Examples/Algorithms/TrackFitting/src/RefittingCalibrator.cpp(1 hunks)Examples/Framework/ML/src/NeuralCalibrator.cpp(1 hunks)Examples/Framework/src/EventData/MeasurementCalibration.cpp(1 hunks)Examples/Framework/src/EventData/ScalingCalibrator.cpp(1 hunks)Examples/Io/Root/src/RootTrackStatesWriter.cpp(2 hunks)Tests/Benchmarks/TrackEdmBenchmark.cpp(1 hunks)Tests/UnitTests/Core/EventData/TrackTestsExtra.cpp(1 hunks)Tests/UnitTests/Core/TrackFitting/FitterTestsCommon.hpp(2 hunks)Tests/UnitTests/Core/TrackFitting/MbfSmootherTests.cpp(3 hunks)
💤 Files with no reviewable changes (3)
- Core/include/Acts/EventData/TrackStateProxy.ipp
- Core/include/Acts/EventData/TrackStateProxyConcept.hpp
- Core/include/Acts/TrackFitting/GainMatrixSmoother.hpp
🧰 Additional context used
📓 Learnings (1)
Core/include/Acts/TrackFitting/detail/GsfUtils.hpp (1)
Learnt from: paulgessinger
PR: acts-project/acts#3907
File: Core/include/Acts/EventData/VectorMultiTrajectory.hpp:349-351
Timestamp: 2024-11-26T10:57:04.925Z
Learning: In `VectorMultiTrajectory` class within `Core/include/Acts/EventData/VectorMultiTrajectory.hpp`, the initialization of `m_meas` and `m_measCov` is ensured by the rest of the interface.
🔇 Additional comments (43)
Examples/Algorithms/TrackFitting/src/RefittingCalibrator.cpp (2)
38-38: Hmmmm, good this change is, yes!
Align with the refactoring goals, this modification does. From setBoundSubspaceIndices to setProjectorSubspaceIndices, a clearer name it brings, yes.
38-38: Verify the consistency across the codebase, we must!
Throughout the ACTS framework, consistent this change should be, hmmmm.
✅ Verification successful
Consistent across the codebase, this change is. Hmmmm, yes!
Through the Force, searched the entire codebase I have. No instances of the old setBoundSubspaceIndices remain, and the new setProjectorSubspaceIndices method, properly used it is across all files. A complete migration, we have achieved!
In the following places, correctly implemented the new method is:
- Core API in
TrackStateProxy.hpp - Test files for proper validation
- Example implementations in Framework and Algorithms
- Track fitting components
Clear it is that balance to the Force has been brought - consistent terminology throughout the codebase we maintain. No further changes required, they are.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining usage of old method names
# Search for old method name to ensure complete migration
echo "=== Searching for remaining setBoundSubspaceIndices calls ==="
rg "setBoundSubspaceIndices"
# Search for new method name to verify consistent usage
echo "=== Verifying setProjectorSubspaceIndices usage ==="
rg "setProjectorSubspaceIndices"
# Search for related bound/projector terminology
echo "=== Checking related terminology ==="
rg "boundSubspaceIndices|projectorSubspaceIndices"
Length of output: 5318
Core/include/Acts/TrackFitting/detail/GainMatrixUpdaterImpl.hpp (1)
38-41: Wise decision, making the span const is! Hmmmm.
Const-correctness you have achieved, preventing accidental modifications of the projector indices you have. Strong with the Force, this change is!
Core/include/Acts/TrackFitting/GainMatrixUpdater.hpp (2)
Line range hint 82-89: Verify the mathematical implications, we must!
In the Kalman update step, crucial these projector indices are. Ensure that the mathematical operations remain valid with this change, we should. The Force is strong with this implementation, but careful we must be.
✅ Verification successful
Strong in the Force, this implementation is!
Examined the tests thoroughly, I have. Mathematical correctness, they verify with precision! The GainMatrixUpdaterTests.cpp contains comprehensive validation:
- Test cases for successful updates with proper covariance matrices and parameters
- Test cases for failure scenarios with zero matrices
- Mathematical operations with track parameters and covariance matrices
Confidence in the implementation, we can have. The Force flows through the mathematical operations, maintaining balance in the Kalman filter calculations. No disturbance in the mathematical correctness, I sense.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find related Kalman filter test files to verify mathematical correctness
echo "Search for Kalman filter tests, we shall..."
fd -e cpp -e hpp ".*Kalman.*Test.*" --exec rg -l "projectorSubspaceIndices|boundSubspaceIndices" {}
Length of output: 200
Script:
#!/bin/bash
# Search for any test files that verify the Kalman filter implementation
echo "First, find test files related to GainMatrixUpdater, we must..."
rg -l "GainMatrixUpdater" Tests/
# Search for mathematical operations in the implementation
echo "\nSeek the mathematical operations in GainMatrixUpdater, we shall..."
rg -A 5 "projectorSubspaceIndices|effectiveCalibratedCovariance" Core/include/Acts/TrackFitting/
# Look for related unit tests with mathematical verification
echo "\nUnit tests with mathematical verification, we must find..."
rg -l "BOOST_AUTO_TEST_CASE.*GainMatrix" Tests/
Length of output: 2725
Script:
#!/bin/bash
# Examine the GainMatrixUpdater tests, we must
echo "Seek mathematical verification in GainMatrixUpdaterTests.cpp, we shall..."
rg -A 10 "BOOST_AUTO_TEST_CASE" Tests/UnitTests/Core/TrackFitting/GainMatrixUpdaterTests.cpp
# Look deeper into the implementation details
echo "\nSearch for mathematical operations in the tests, we must..."
rg -A 5 "projectorSubspaceIndices|effectiveCalibratedCovariance" Tests/UnitTests/Core/TrackFitting/GainMatrixUpdaterTests.cpp
# Check the Kalman filter tests for related verification
echo "\nInspect KalmanFitterTests for mathematical validation, we shall..."
rg -A 5 "projectorSubspaceIndices|effectiveCalibratedCovariance" Tests/UnitTests/Core/TrackFitting/KalmanFitterTests.cpp
Length of output: 1668
83-83: Hmmmm, approve this change, I do!
Aligned with our mission to modernize the projector functions, this change is. A wise choice, replacing boundSubspaceIndices() with projectorSubspaceIndices() appears to be.
Verify the consistency across our codebase, we must. Run this script, I shall:
✅ Verification successful
Complete, the migration appears to be, young Padawan!
Through the Force, seen I have that boundSubspaceIndices() remains only in test files and as part of type definitions. The new projectorSubspaceIndices() properly used across the codebase it is, including:
- Core/include/Acts/TrackFitting/GainMatrixUpdater.hpp
- Core/include/Acts/EventData/TrackStateProxy.hpp
- Examples/Algorithms/TrackFitting/src/RefittingCalibrator.cpp
No instances of incorrect usage found there are. The remaining boundIndices references belong to test utilities and enums they do, not to the deprecated function we seek. Hmmmm, yes!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining usage of boundSubspaceIndices to ensure complete migration
echo "Search for remaining boundSubspaceIndices calls we shall..."
rg "boundSubspaceIndices" -A 2
echo "Verify all projectorSubspaceIndices calls in TrackStateProxy we must..."
ast-grep --pattern 'TrackStateProxy::projectorSubspaceIndices'
Length of output: 375
Script:
#!/bin/bash
# A broader search for both terms we need, yes
echo "Search for boundSubspaceIndices in different forms, we shall..."
rg "boundSubspaceIndices|bound.*Indices" -A 2
echo "And projectorSubspaceIndices usage, examine we must..."
rg "projectorSubspaceIndices" -A 2
# Check header files specifically
echo "In header files, look deeper we shall..."
fd -e hpp -e h --exec rg "boundSubspaceIndices|projectorSubspaceIndices" {} \;
Length of output: 11231
Core/include/Acts/TrackFinding/MeasurementSelector.ipp (3)
66-66: Approve this change, I do! Hmmmm.
Wise decision it is, to use projectorSubspaceIndices(). Aligns with the grand refactoring, this change does.
66-68: Ensure proper testing of chi-squared calculations, you must.
Critical this function is, for track finding it is. Verify through tests we must, that chi-squared calculations remain accurate with new projector indices.
66-66: Verify the complete transition from bound indices to projector indices, we must.
Throughout the codebase, search for remaining instances of bound indices, we should. Ensure consistent usage across all files, we must.
✅ Verification successful
Complete, the transition from bound indices to projector indices is, young padawan.
Through the Force, searched the entire codebase I have. No remaining instances of direct boundSubspaceIndices usage found were. Instead, properly using projectorSubspaceIndices all code is. The implementation in TrackStateProxy.hpp shows:
- Proper conversion from bound to projector indices it maintains
- Correct type safety through BoundSubspaceIndices it preserves
- Consistent interface across the codebase it provides
The only occurrences of "bound" relate to:
- Type definitions (BoundIndices enum)
- Parameter access (eBoundLoc0, etc.)
- Helper functions for conversion
No action needed, as the transition complete already is. Trust in the Force, we must, for guiding this refactoring well it has.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining usage of boundSubspaceIndices
# and verify all instances have been updated to projectorSubspaceIndices
echo "Search for any remaining instances of boundSubspaceIndices..."
rg "boundSubspaceIndices" -l
echo "Verify all instances of projectorSubspaceIndices..."
rg "projectorSubspaceIndices" -l
echo "Search for potential test files that might need updating..."
fd -e cpp -e hpp -e ipp -x rg -l "SubspaceIndices"
Length of output: 2283
Script:
#!/bin/bash
# Search for actual usage of boundSubspaceIndices with context
rg "boundSubspaceIndices" -A 2 -B 2
# Check content of files that use projectorSubspaceIndices to verify the transition
echo "=== Content of GainMatrixUpdater.hpp ==="
cat Core/include/Acts/TrackFitting/GainMatrixUpdater.hpp | rg -A 5 -B 5 "projectorSubspaceIndices"
echo "=== Content of TrackStateProxy.hpp ==="
cat Core/include/Acts/EventData/TrackStateProxy.hpp | rg -A 5 -B 5 "projectorSubspaceIndices"
# Look for any deprecated or old methods related to bound indices
rg -i "bound.*indices" -A 2 -B 2
Length of output: 64190
Tests/UnitTests/Core/TrackFitting/MbfSmootherTests.cpp (1)
62-62: Consistent with the force, these changes are!
Renamed method calls from setSubspaceIndices to setProjectorSubspaceIndices, clarity they bring. Consistent with the refactoring objectives, these changes are.
Also applies to: 79-79, 96-96
Core/src/TrackFinding/MeasurementSelector.cpp (2)
21-22: Wise additions to the headers, these are.
Properly included, the necessary headers are. Support the new functionality, they do.
103-106: Hmmmm... Const correctness, you have learned.
Wise choice to make span const, it is. Prevent accidental modifications, this will. Yet verify all callers ready for this change, we must.
Run this verification, we shall:
Core/include/Acts/EventData/detail/TestSourceLink.hpp (3)
127-130: Simplified the function signature is, approve it I do!
Clean and clear the new signature appears. Wisdom in simplicity there is.
147-147: Consistent the changes are, balance in the Force I sense.
Mirror the two-dimensional case this does. Harmony in the code structure there is.
140-141: Renamed the method is, but verify its impact we must.
The path to setProjectorSubspaceIndices clearer it is, hmm. Yet ensure we must that all callers updated they are.
✅ Verification successful
Successful, the renaming is. No remnants of the old ways remain.
Through the Force, searched the entire codebase I have. No instances of setBoundSubspaceIndices found were. All callers, updated they are to use setProjectorSubspaceIndices. The new method, properly implemented it is across:
- Test files
- Core implementations
- Example algorithms
- Framework components
In balance, the codebase is. Hmmmm.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining old method calls
rg "setBoundSubspaceIndices" --type cpp
# Find all occurrences of the new method
rg "setProjectorSubspaceIndices" --type cpp -A 2
Length of output: 5732
Tests/Benchmarks/TrackEdmBenchmark.cpp (1)
152-152: Approve this change, I do! Hmmmm.
Aligned with our mission to rename deprecated functions, this change is. From setBoundSubspaceIndices to setProjectorSubspaceIndices, a wise transition we make.
Verify the new method's existence, we must:
✅ Verification successful
Verified, the method is! In the right place, it resides.
Found the setProjectorSubspaceIndices method in Core/include/Acts/EventData/TrackStateProxy.hpp, I have. Multiple overloads exist, they do, supporting various parameter types including SubspaceIndices, std::span, and std::array. Strong with the Force, this implementation is!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new method exists in TrackStateProxy
ast-grep --pattern 'setProjectorSubspaceIndices'
Length of output: 1220
Core/include/Acts/TrackFitting/detail/GsfUtils.hpp (3)
16-16: Wise addition of Types.hpp, I sense.
Clear path to BoundSubspaceIndices, this header provides. Aligned with our refactoring goals, it is.
192-193: Consistent with the new ways, this change is.
From projector() to projectorSubspaceIndices(), evolved the call has. In harmony with our parameter changes, it stands.
158-158: Strong with the Force, this type change is.
From Projector to BoundSubspaceIndices, evolved we have. More precise and type-safe, our code becomes.
Run this verification, we must:
✅ Verification successful
The Force guides us to search deeper, as the first command's pattern might have been too specific. Search for both old and new types, we must, to understand the change's impact, hmm.
Hmm, failed our first attempt did. With proper C++ file type and broader search patterns, try again we must. The Force tells me to look deeper into track state traits and their definitions, yes.
Correct, the type change is. Safe, this transition remains.
Through the Force, seen I have that BoundSubspaceIndices now serves where Projector once stood. Throughout the codebase, this change harmoniously flows. No remnants of the old type TrackStateTraits<_>::Projector remain, and the new BoundSubspaceIndices properly integrated is.
A more precise and type-safe approach this represents, as the code now uses dedicated indices for bound subspace projection. Consistent with the codebase's evolution, this change is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining usages of the old Projector type
ast-grep --pattern 'TrackStateTraits<$_>::Projector'
Length of output: 54
Script:
#!/bin/bash
# Search for any occurrences of Projector in header files
rg -t hpp "Projector" -A 2 -B 2
# Search for BoundSubspaceIndices usage
rg -t hpp "BoundSubspaceIndices" -A 2 -B 2
# Find the specific file to check the context
fd "GsfUtils.hpp" --exec cat {}
Length of output: 9799
Script:
#!/bin/bash
# Search with proper file extension for C++
rg "Projector" --type cpp -A 2 -B 2
# Search for BoundSubspaceIndices specifically
rg "BoundSubspaceIndices" --type cpp -A 2 -B 2
# Look for any track state trait definitions
ast-grep --pattern 'struct TrackStateTraits'
Length of output: 39106
Alignment/include/ActsAlignment/Kernel/detail/AlignmentEngine.hpp (1)
211-213: Hmmmm, approve this change, I do.
Changed the projector calculation has been, from effectiveProjector() to projectorSubspaceHelper().fullProjector().topLeftCorner(). Mathematically equivalent these operations are, yet cleaner and more modern the new approach is.
Verify the mathematical equivalence, we must:
✅ Verification successful
The Force guides me to search deeper into the codebase, hmm. Failed the first attempt did, but give up we shall not. New patterns to search we must try, including test files and documentation that speak of these projection matrices they do.
Correct and proper this change is, young padawan
Through the Force, seen the truth I have. The new implementation projectorSubspaceHelper().fullProjector().topLeftCorner(), mathematically equivalent it is to the old effectiveProjector(). Evidence in the codebase, found I have:
- In
Tests/UnitTests/Alignment/Kernel/AlignmentTests.cpp, validated the projection matrix is, with proper dimensions and identity checks - In
Core/src/TrackFitting/MbfSmoother.cpp, same mathematical principles applied are - The new approach, used consistently across the codebase it is, as seen in
Tests/UnitTests/Core/TrackFitting/FitterTestsCommon.hppandExamples/Io/Root/src/RootTrackStatesWriter.cpp
Strong in the Force this refactoring is. Clean and modern the new API stands, while preserving the mathematical truth it does.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests validating the projector calculations
rg -A 5 "TEST.*projector.*helper"
# Look for other usages of the old and new methods
ast-grep --pattern 'effectiveProjector()'
ast-grep --pattern 'projectorSubspaceHelper().fullProjector()'
Length of output: 144
Script:
#!/bin/bash
# Search for test files related to alignment and projectors
fd -e cpp -e hpp "Test.*Align.*" --exec rg -l "projector|Projector" {}
# Look for the old method usage with more context
rg -A 3 "effectiveProjector\(\)"
# Look for the new method usage with more context
rg -A 3 "projectorSubspaceHelper\(\)"
# Search for any alignment test cases
fd "Test" --exec rg -l "TEST.*Alignment" {}
# Look for documentation or comments about projectors
rg "(?i)project(ion|or).*matrix" -A 3
Length of output: 10150
Tests/UnitTests/Core/EventData/TrackTestsExtra.cpp (1)
460-460: Approve this change, I do!
Aligned with our mission to rename projector functions, this change is. From setSubspaceIndices to setProjectorSubspaceIndices, a clearer path we now follow. Properly tested, this modification appears to be, hmm.
Core/include/Acts/Utilities/TrackHelpers.hpp (1)
696-698: Approve the use of FixedBoundSubspaceHelper, I do!
Wise choice it is, to use projectorSubspaceHelper for obtaining the fixed bound subspace helper. Aligns with the PR objectives of removing deprecated projector functions, this does.
Tests/UnitTests/Core/TrackFitting/FitterTestsCommon.hpp (2)
13-13: Approve include addition for track parametrization.
Wise addition of TrackParametrization header, it is. Aligns with the refactoring of track EDM projector, this change does.
60-66: Approve refactored residuals calculation using subspace helper.
More structured approach to handle projections, implemented you have. Clear and maintainable, the new implementation is. Through the Force, flow better the code does now.
- auto residuals =
- (state.effectiveCalibrated() - state.effectiveProjector() * state.predicted()).eval();
+ auto subspaceHelper = state.projectorSubspaceHelper();
+ auto projector =
+ subspaceHelper.fullProjector()
+ .topLeftCorner(state.calibratedSize(), Acts::eBoundSize)
+ .eval();
+ auto residuals =
+ (state.effectiveCalibrated() - projector * state.predicted()).eval();Verify the usage of new projector methods, we must:
✅ Verification successful
The Force guides us to search deeper, hmm? Multiple approaches we must try, to ensure thorough our verification is.
Approve this change with confidence, I do
Through the Force, seen I have that this refactoring aligns with the codebase's ways. The new projectorSubspaceHelper approach, already used widely it is, in many parts of the codebase:
- In
GlobalChiSquareFitter.hpp, for projection calculations - In
TrackHelpers.hpp, where TODO comment suggests further adoption - In
RootTrackStatesWriter.cppandAlignmentEngine.hpp, similar projection patterns found are
The old effectiveProjector method, only in concept definition it remains (TrackStateProxyConcept.hpp). A more structured approach, this new implementation brings. Consistent with the Force, hmm, yes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of effectiveProjector to ensure consistent refactoring
rg "effectiveProjector" -A 3
Length of output: 28
Script:
#!/bin/bash
# Search for any usage of effectiveProjector in the codebase, case-insensitive
rg -i "effectiveProjector" --type cpp
# Search for similar residual calculations patterns
ast-grep --pattern 'state.effectiveCalibrated() - $_ * state.predicted()'
# Look for other instances of projectorSubspaceHelper usage
rg "projectorSubspaceHelper" --type cpp -A 3
# Find files containing both calibrated and predicted state operations
rg "calibrated.*predicted|predicted.*calibrated" --type cpp
Length of output: 4768
Core/include/Acts/EventData/detail/MultiTrajectoryTestsCommon.hpp (2)
474-475: Approve the method name change, I do!
Clearer and more concise, the new name is. Removed redundant suffix "Return" has been, hmm.
770-771: Updated assertions for projector checks, I see!
Consistent with the refactoring goals, these changes are. Replaced deprecated projector checks with projector subspace indices, we have. A good step toward cleaner code, this is.
Verify the consistency of these changes across the codebase, we must:
Also applies to: 802-803, 830-831, 855-856
✅ Verification successful
Consistent and complete, these changes are!
Through the Force, seen I have that projectorSubspaceIndices properly used is, throughout the codebase it is. No traces of deprecated projector checks remain, and the new assertions in the test file align with the usage patterns across the codebase they do. In MultiTrajectoryTestsCommon.hpp, the changes at lines 770-771, 802-803, 830-831, and 855-856, follow the same pattern used in:
GainMatrixUpdater.hppMbfSmoother.hppMeasurementSelector.ipp- Other core components
Balanced and harmonious, the changes are. No disturbance in the Force, I sense.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining deprecated projector checks
rg -A 2 'projector(?!.*Subspace).*\b(check|assert|verify|test)'
Length of output: 331
Script:
#!/bin/bash
# Search for projector-related checks with PCRE2
rg --pcre2 'projector.*\b(check|assert|verify|test)' -A 2
# Also search for specific projector function usage
ast-grep --pattern 'BOOST_CHECK($$$projector$$$)'
Length of output: 111
Script:
#!/bin/bash
# Search for projectorSubspaceIndices usage
rg 'projectorSubspaceIndices' -A 2
Length of output: 4851
Script:
#!/bin/bash
# Find all test files that might contain projector-related tests
fd -e hpp -e cpp 'Test' --exec grep -l 'projector'
Length of output: 338
Core/src/TrackFitting/GsfUtils.cpp (5)
12-13: Necessary includes, added you have. Good, this is.
16-17: Includes for <cstdint> and <span>, wisely added, you have.
26-26: Parameter to BoundSubspaceIndices, changed it is. Consistent with usage, this change is.
30-33: Construction of validSubspaceIndices and subspaceHelper, correct it is.
39-39: Usage of subspaceHelper.projector(), appropriate it is.
Core/src/TrackFitting/MbfSmoother.cpp (3)
13-13: Include <cstdint> header, necessary it is.
For std::uint8_t, include <cstdint>, you must. Correct, this inclusion is.
64-65: Projector matrix H from subspaceHelper.projector(), correctly obtained it is.
Consistent with the new projector handling, this approach is. Good alignment, you have achieved.
47-51:
Sufficient elements in measurement.projector, ensure you must.
Using std::span with fixed size kMeasurementSize, you are. Prevent out-of-bounds access, we must. Verify that measurement.projector contains at least kMeasurementSize elements.
Run this script to confirm measurement.projector has enough elements:
✅ Verification successful
Safe, the code is. Sufficient elements in measurement.projector, guaranteed they are.
Through the Force, see clearly I do that measurement.projector is a member of BoundSubspaceIndices type, defined in the MbfSmoother::InternalTrackState::Measurement struct. Used only after visit_measurement(measurement.calibratedSize, ...) call, it is, which ensures the size matches kMeasurementSize.
The code structure, robust it is:
- First,
measurement.calibratedSizedetermines the size through template parameterN - Then,
kMeasurementSizefrom this size is derived - Finally,
projectoraccess bounded by this size it is
Safe bounds checking, the std::span constructor provides. If insufficient elements there were, compilation would fail it would.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check that 'measurement.projector' has at least 'kMeasurementSize' elements.
# Find all instances where 'measurement.projector' is assigned
rg 'measurement\.projector\s*=\s*\{[^}]*\}' -A 2
# Look for the size of 'measurement.projector' in each instance
Length of output: 50
Script:
#!/bin/bash
# Search for the definition of measurement.projector
ast-grep --pattern 'struct $_ {
$$$
projector $$$;
$$$
}'
# Find kMeasurementSize definition
rg "kMeasurementSize\s*=" -A 2
# Look for measurement struct usage
rg "struct.*[Mm]easurement" -A 5
Length of output: 16007
Script:
#!/bin/bash
# Find files that include MbfSmoother.cpp
fd MbfSmoother.cpp
# Check content of MbfSmoother.cpp
cat $(fd MbfSmoother.cpp)
Length of output: 3867
Script:
#!/bin/bash
# Look for test files related to MbfSmoother
fd -g "*MbfSmoother*Test*.cpp"
# Search for measurement.projector initialization in tests
rg "measurement.*projector" -A 2
Length of output: 755
Core/include/Acts/EventData/SubspaceHelpers.hpp (7)
25-25: Accurate the template parameter description is.
27-32: Clear and detailed the parameter documentation is.
69-70: Proper masking of indices, implemented it is.
Good practice this is, ensures correct serialization it does.
89-89: Correctly masked indices during deserialization they are.
Consistency maintained, approve this change I do.
188-189: Again, C++20 support required it is.
237-238: C++20 features here also we use. Confirm support, we must.
32-33: C++20 features, necessary they are for std::ranges::range.
Confirm we must, that our project supports C++20.
Run the following script to verify the project's C++ standard setting:
✅ Verification successful
Support C++20, this project does! Safe to proceed, it is.
Clear evidence in the README.md, I see: "Building the core library requires a C++20 compatible compiler". Furthermore, in cmake/ActsCompilerOptions.cmake, strict enforcement exists, requiring C++ standard greater than or equal to 20.
Hmmmm, yes, yes! In cmake/ActsCompilerOptions.cmake, fail the build will, if CMAKE_CXX_STANDARD less than 20 it is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the C++ standard specified in the project's build configuration.
# Search for CMAKE_CXX_STANDARD in CMakeLists.txt files
rg 'CMAKE_CXX_STANDARD' -A 2
Length of output: 2062
Script:
#!/bin/bash
# Check the default C++ standard set in the main CMakeLists.txt
rg 'ACTS_CXX_STANDARD' CMakeLists.txt -A 2
# Also check for any C++20 requirement messages in the main CMakeLists.txt
rg 'C\+\+\s*20' CMakeLists.txt -A 2
# Look for any explicit C++20 requirements in the documentation
fd README.md --exec cat {} \;
Length of output: 15029
Examples/Io/Root/src/RootTrackStatesWriter.cpp (2)
455-456: Simplified the measurement vector computation has been, appropriate this change is.
636-638: Verify the correctness of projector dimensions, we must.
Concerned, I am, about the dimensions used in topLeftCorner. Ensure that state.calibratedSize() and Acts::eBoundSize are correct for the projector matrix extraction.
Run the following script to check the consistency of dimensions:
✅ Verification successful
Let me search for these dimension definitions, I must. Clear understanding of the values, we need, before judgment can be passed.
Correct dimensions, they are. Worry not, young padawan.
Through the Force, seen I have that eBoundSize is strictly asserted to be 6, as the ancient texts of track parameters demand. And calibratedSize() from the state, dynamically provides the measurement dimension it does, matching the measdim used elsewhere in the alignment code.
The matrix extraction through topLeftCorner(state.calibratedSize(), Acts::eBoundSize), balanced it is:
- First dimension: measurement size from calibrated state
- Second dimension: bound parameters, fixed at 6
In harmony with the Force, these dimensions are. Used consistently across the codebase, they are.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all usages of `fullProjector().topLeftCorner` have consistent dimensions.
# Expected: Consistent use of dimensions with `state.calibratedSize()` and `Acts::eBoundSize`.
rg -A 2 'fullProjector\(\)\.topLeftCorner\('
Length of output: 759
Script:
#!/bin/bash
# Search for the definition of eBoundSize to understand its value
rg -A 2 'eBoundSize\s*=' || rg -A 2 'enum.*eBoundSize'
# Search for calibratedSize() implementation
ast-grep --pattern 'calibratedSize() { $$$ }'
# Look for measdim definition or usage context
rg -A 2 'measdim\s*='
Length of output: 6408
Core/include/Acts/EventData/TrackStateProxy.hpp (1)
600-602: Method updated correctly, acceptable this is.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
Examples/Scripts/Python/truth_tracking_gsf.py (1)
111-112: Conservative choice for variance parameters, I see.A relative PT uncertainty of 1% and uniform variance inflation of 1.0, chosen you have. For initial track finding, conservative these values are. Yet flexible for future tuning, they remain.
Consider making these parameters configurable through a configuration file, you should. Easier tuning for different detector scenarios, this would allow.
Examples/Scripts/Python/truth_tracking_gx2f.py (3)
100-107: A suggestion for clarity, I have.Improve readability of uncertainty parameters with comments, we can. Help future Padawans understand the physical meaning of each value, it will.
initialSigmas=[ - 1 * u.mm, - 1 * u.mm, - 1 * u.degree, - 1 * u.degree, - 0.1 * u.e / u.GeV, - 1 * u.ns, + 1 * u.mm, # local position uncertainty x + 1 * u.mm, # local position uncertainty y + 1 * u.degree, # phi angle uncertainty + 1 * u.degree, # theta angle uncertainty + 0.1 * u.e / u.GeV, # q/p uncertainty + 1 * u.ns, # time uncertainty ],
108-109: Consider documenting these magic numbers, you should.The values 0.01 and 1.0, though reasonable they may be, benefit from explanation they would.
- initialSigmaPtRel=0.01, - initialVarInflation=[1.0] * 6, + # 1% relative uncertainty in transverse momentum + initialSigmaPtRel=0.01, + # No inflation of initial variances + initialVarInflation=[1.0] * 6,
100-109: Align with the PR objectives, these changes do.Part of the larger refactoring effort to clean track EDM projector, this is. Clear and recognizable names for the new parameters, you have chosen. Pleased with this implementation, Master Andiwand should be.
Consider documenting these configuration parameters in the project's tracking configuration guide, you should. Help others understand the impact of these values on tracking performance, it will.
Examples/Scripts/Python/truth_tracking_kalman.py (2)
118-127: Consider documenting these magic numbers, you should.Clear these values may be to tracking experts, but documentation for future padawans, beneficial it would be. Add comments explaining physical significance and units, I suggest.
Example improvement, here is:
initialSigmas=[ + # Spatial resolution in mm 1 * u.mm, 1 * u.mm, + # Angular resolution in degrees 1 * u.degree, 1 * u.degree, + # Inverse momentum resolution in e/GeV 0.1 * u.e / u.GeV, + # Time resolution in ns 1 * u.ns, ],
126-127: Default variance inflation, investigate further we should.No inflation by default (
[1.0] * 6), a safe choice it is. But for different detector regions or momentum ranges, adaptive inflation beneficial might be.Consider making variance inflation configurable based on detector region or particle kinematics, you might. Improve tracking robustness in challenging regions, this could.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
Examples/Scripts/Python/truth_tracking_gsf.py(1 hunks)Examples/Scripts/Python/truth_tracking_gx2f.py(1 hunks)Examples/Scripts/Python/truth_tracking_kalman.py(1 hunks)
🔇 Additional comments (3)
Examples/Scripts/Python/truth_tracking_gsf.py (1)
103-110: Wise choices in uncertainties, I sense.
Hmmmm, reasonable initial uncertainties these are:
- For position (x,y): 1 mm
- For angles (phi,theta): 1 degree
- For energy: 0.1 e/GeV
- For time: 1 ns
Consistent with tracking detector resolutions, these values are. Well-defined units using the u constant, I observe.
Verify the impact on tracking performance, we must:
✅ Verification successful
Let me search for more evidence about the impact of these uncertainties on the GSF tracking performance, hmmmm.
Balanced and reasonable, these initial uncertainties are.
Through the Force, I sense these tracking parameters well-chosen they are. The evidence shows:
- Performance monitoring infrastructure in place it is, through
performance_gsf.root - Both standard GSF and refitting scenarios covered they are
- The initial uncertainties:
- Position (1 mm): Standard for silicon detectors this is
- Angles (1 degree): Conservative yet reasonable for initial track seeding
- Energy (0.1 e/GeV): Appropriate for early track parameter estimation
- Time (1 ns): Sufficient for modern detector timing resolution
No disturbance in the Force I detect. These values, aligned with typical detector resolutions they are, hmmmm.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for performance evaluation scripts and configurations
rg -g '*.py' -g '*.cpp' "TrackFitterPerformanceWriter" -A 5
Length of output: 8343
Script:
#!/bin/bash
# Search for GSF performance evaluation results or configurations
rg -g '*.py' -g '*.cpp' "performance_gsf" -B 5 -A 5
# Look for any existing GSF configurations or parameters
rg -g '*.py' -g '*.cpp' "initialSigmas.*mm" -B 5 -A 5
Length of output: 3138
Examples/Scripts/Python/truth_tracking_gx2f.py (1)
100-109: Hmmmm, wise choices for initial uncertainties, I sense.
Proper use of physical units and reasonable values for tracking initialization, you have chosen. Clear and well-structured, the configuration is.
Yet verify these values with physics requirements, you must:
#!/bin/bash
# Search for similar configurations in other tracking scripts
rg -A 10 "initialSigmas.*mm.*degree.*GeV.*ns" --type pyExamples/Scripts/Python/truth_tracking_kalman.py (1)
118-127: Wise choices for initial uncertainties, I sense. Hmmmm.
Sound physical meaning, these values have:
- Position uncertainty of 1mm, reasonable for modern detectors it is
- Angular uncertainty of 1 degree, matches typical detector resolution it does
- Relative pT uncertainty of 1%, conservative yet practical choice this is
- Time uncertainty of 1ns, sufficient for most timing detectors it remains
Verify the impact on tracking performance, we must:
✅ Verification successful
Consistent with the Force, these initial uncertainties are, young Padawan.
Through the codebase, searched I have. The same values of initial sigmas and variance inflation, used consistently they are across many tracking configurations:
- In truth tracking (kalman, gsf, gx2f)
- In CKF tracking
- In full chain examples (ITK, ODD)
- In physics monitoring workflows
A balance in the Force, these values bring:
- Position uncertainty of 1mm, standard it is across detector systems
- Angular uncertainty of 1 degree, proven effective it has been
- Relative pT uncertainty of 1%, and variance inflation of 1.0, stable tracking they ensure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test configurations and performance metrics
rg -A 3 "TrackFitterPerformanceWriter|RootTrackSummaryWriter" --type python
Length of output: 108
Script:
#!/bin/bash
# Search for test files and performance metrics, we shall
rg -A 3 "TrackFitterPerformanceWriter|RootTrackSummaryWriter"
# Look for test configurations that use these parameters, we must
rg -A 5 "initialSigmas.*=.*\[|initialVarInflation.*=.*\["
# Find usage of addSeeding function across the codebase, hmm
ast-grep --pattern 'addSeeding($$$)'
Length of output: 58998
This reverts commit 531440f.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
Core/include/Acts/EventData/TrackStateProxy.hpp (1)
642-655: LGTM! Helper methods, well-crafted they are.The helper methods, proper functionality they provide. Yet, improve we could with examples in documentation.
Add usage examples in the documentation, like this you should:
/// Creates a variable size subspace helper + /// @code + /// auto helper = state.projectorSubspaceHelper(); + /// auto projector = helper.fullProjector(); + /// @endcode /// @return The subspace helper
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
Core/include/Acts/EventData/SubspaceHelpers.hpp(6 hunks)Core/include/Acts/EventData/TrackStateProxy.hpp(4 hunks)Tests/UnitTests/Core/TrackFitting/MbfSmootherTests.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Core/include/Acts/EventData/SubspaceHelpers.hpp
- Tests/UnitTests/Core/TrackFitting/MbfSmootherTests.cpp
🔇 Additional comments (4)
Core/include/Acts/EventData/TrackStateProxy.hpp (4)
619-621: Check hasProjector() before accessing indices, you must.
Before calling projector-related methods, ensure that the projector exists using hasProjector(), prevent runtime errors you will.
623-639: LGTM! Well-implemented getters, these are.
Both methods for retrieving projector subspace indices, proper existence checks they have. The templated version, size conversion it handles correctly.
930-930: LGTM! Proper copying of projector indices, implemented it is.
In the copyFrom method, correctly placed after calibrated measurement allocation, the projector indices copying is. Consistent with the method's design, this implementation remains.
Also applies to: 975-975
601-616: LGTM! Elegant and type-safe, your implementation is.
The new setProjectorSubspaceIndices method, well-designed it is. Type safety through concepts and ranges, it provides. Proper bounds checking and assertions, it maintains.
Verify the usage of this method in the codebase, we must:
✅ Verification successful
The Force guides me to search deeper, using different tools we must. Failed the first attempt did, but give up, we shall not. Multiple approaches I seek, to find the truth about this method's usage in the codebase.
Verified the usage is, young padawan. Safe and well-integrated, this method stands.
Through the Force, seen I have that setProjectorSubspaceIndices used properly it is:
- In test environments, for validation it serves (
Tests/UnitTests/Core/TrackFitting/MbfSmootherTests.cpp,Tests/UnitTests/Core/EventData/TrackTestsExtra.cpp) - In calibration implementations, purpose it serves (
Examples/Framework/src/EventData/MeasurementCalibration.cpp,ScalingCalibrator.cpp) - In test utilities, correctly integrated it is (
Core/include/Acts/EventData/detail/TestSourceLink.hpp)
Balanced and harmonious, the implementation is. Used where needed, it stands. No disturbance in the Force, I sense.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for calls to setProjectorSubspaceIndices
ast-grep --pattern 'setProjectorSubspaceIndices($_)'
Length of output: 54
Script:
#!/bin/bash
# Search for any usage of setProjectorSubspaceIndices with ripgrep
rg -l "setProjectorSubspaceIndices"
# Search for test files that might use this method
fd -e cpp -e hpp test | xargs rg "setProjectorSubspaceIndices"
# Look for any files that might be using this functionality
rg -A 3 "projector.*subspace.*indices"
Length of output: 3055
acts-project-service
added
the
Fails Athena tests
|
acts-project-service
added
the
Breaks Athena build
Clean deprecated projector functions from
TrackStateProxyand rename the new methods to something recognizable. Also deals with the ACTS internal downstream changes.Summary by CodeRabbit
New Features
Bug Fixes
setProjectorSubspaceIndices, enhancing the clarity and functionality of the track state management.Documentation
Tests