refactor: Allow TrackState measurement setting without extra initialization #3907
Conversation
WalkthroughEnhance the Changes
Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
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
|
I benchmarked this only with the synthetic Track EDM benchmark: main: This PR: |
github-actions
bot
added
Component - Examples
|
@andiwand I don't think so, as the previous API should stay valid, as well as the assumption of memory be zero-initialized. We might want to make use of the direct initialization however. Also, I'll probably want to time the CKF timing with this, although I don't expect significant improvements globally. |
|
I'll also fix the sonar issues tomorrow. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Outside diff range comments (3)
Core/src/EventData/VectorTrackContainer.cpp (1)
Line range hint
1-173: Strong with the Force, this implementation is.Wisdom in the code structure, I sense. Synchronized, the vectors remain, through careful implementation of addTrack_impl, removeTrack_impl, reserve, and clear. Consistency checks through assertions, maintain balance they do.
Consider documenting the relationship between vectors in the header file, help future Padawans it will. The size() method, a natural extension of this balanced design, it is.
Examples/Framework/src/EventData/ScalingCalibrator.cpp (1)
Line range hint
142-182: Strong with the Force, this architecture is!Robust error handling and type safety, your code maintains. Template metaprogramming with
visit_measurementwisely used it is. Through validation checks and assertions, protect against the dark side of runtime errors, you do.Consider documenting performance characteristics in comments, help future Padawans it will. Template metaprogramming, complex it can be.
Core/src/EventData/VectorMultiTrajectory.cpp (1)
Line range hint
219-222: More descriptive error messages, we need, young padawan!In the default case of unset_impl, improve the error message we should. Help future maintainers understand which components are valid for unsetting, it would.
Apply this improvement, you should:
- throw std::domain_error{"Unable to unset this component"}; + throw std::domain_error{"Unable to unset component. Valid targets are: Predicted, Filtered, Smoothed, Jacobian, and Calibrated"};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (20)
Core/include/Acts/EventData/MultiTrajectory.hpp(2 hunks)Core/include/Acts/EventData/MultiTrajectoryBackendConcept.hpp(1 hunks)Core/include/Acts/EventData/TrackProxy.hpp(0 hunks)Core/include/Acts/EventData/TrackStateProxy.hpp(3 hunks)Core/include/Acts/EventData/TrackStateProxyConcept.hpp(1 hunks)Core/include/Acts/EventData/VectorMultiTrajectory.hpp(3 hunks)Core/include/Acts/EventData/VectorTrackContainer.hpp(1 hunks)Core/include/Acts/EventData/detail/MultiTrajectoryTestsCommon.hpp(12 hunks)Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp(0 hunks)Core/src/EventData/VectorMultiTrajectory.cpp(1 hunks)Core/src/EventData/VectorTrackContainer.cpp(1 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)Plugins/Podio/include/Acts/Plugins/Podio/PodioTrackStateContainer.hpp(4 hunks)Tests/Benchmarks/TrackEdmBenchmark.cpp(2 hunks)Tests/UnitTests/Core/EventData/MultiTrajectoryTests.cpp(1 hunks)Tests/UnitTests/Core/EventData/TrackTestsExtra.cpp(0 hunks)Tests/UnitTests/Plugins/Podio/PodioTrackStateContainerTest.cpp(1 hunks)
💤 Files with no reviewable changes (3)
- Core/include/Acts/EventData/TrackProxy.hpp
- Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp
- Tests/UnitTests/Core/EventData/TrackTestsExtra.cpp
🔇 Additional comments (20)
Examples/Algorithms/TrackFitting/src/RefittingCalibrator.cpp (1)
33-35: Verify the allocation behavior, we must!
Ensure proper error handling when allocation fails, we should. Check if the trackState already has an allocation of different size, we must.
✅ Verification successful
Let me search deeper into the Force to understand the allocation mechanisms, hmm. These commands will help us see the implementation details and any existing error handling, they will.
Hmm, deeper into the implementation we must look, to find error handling and size validation patterns, we shall. The Force will guide us to the truth about allocation safety.
Safe the allocation is, young padawan!
Revealed by the Force, proper safeguards I see:
- Validation of measurement dimension through
throw_assert(measdim > 0 && measdim <= eBoundSize) - Protection against different size reallocation, documented clearly it is
- Zero-initialization of allocated storage, guaranteed it is
No additional error handling needed, hmm. The existing checks, sufficient they are. The Dark Side of memory corruption, prevented it shall be.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for error handling patterns in allocation calls
rg -A 5 "allocateCalibrated.*throw"
# Look for size validation before allocation
ast-grep --pattern 'if ($size != $_) {
throw $_
}'
Length of output: 94
Script:
#!/bin/bash
# Search for allocateCalibrated implementation and usage
ast-grep --pattern 'void allocateCalibrated($_) {
$$$
}'
# Search for any error handling or exceptions related to allocation
rg -B 2 -A 5 "allocateCalibrated"
# Look for size-related checks or validations
rg -B 2 -A 5 "calibrated.*Size"
Length of output: 77138
Script:
#!/bin/bash
# Search for error handling in allocateCalibrated implementation
ast-grep --pattern 'void allocateCalibrated($_) {
if ($condition) {
throw $_
}
$$$
}'
# Look for size validation in MultiTrajectory implementation
rg -B 2 -A 5 "throw_assert.*measdim"
# Search for reallocation checks
rg -B 2 -A 5 "existing allocation.*different size"
Length of output: 2687
Core/src/EventData/VectorTrackContainer.cpp (1)
170-172: Approve the size implementation, I do. Hmmmm.
Simple and effective, this implementation is. The way of the Jedi, it follows - doing one thing well, it does.
Verify the consistency of all vectors, we must. Run this script, we shall:
✅ Verification successful
Strong with the Force, this implementation is, hmmmm.
In balance, all vectors are kept, yes. Through the Force, I have seen:
- The
checkConsistency()method, guard the synchronization it does - In
reserve(),clear(), andaddTrack_impl(), together all vectors move - The sacred texts (header file) show
assert(checkConsistency())insize() - Protected by assertions, the size consistency is
Wisdom in using m_tipIndex.size() I sense, for:
- Primary container it is, yes
- Before each size check, consistency verified it is
- All vectors, same size they must maintain, enforced by the sacred assertions
May the Force be with your code, young Padawan. Hmmmmmm.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for vector member declarations and verify they maintain consistent sizes
# Find all vector member declarations
ast-grep --pattern 'std::vector<$_> m_$_'
# Find size-related operations on vectors
rg -A 2 '\b(push_back|pop_back|clear|erase|reserve)\b.*m_[a-zA-Z]+' Core/src/EventData/VectorTrackContainer.cpp
Length of output: 465
Script:
#!/bin/bash
# Search for vector member declarations and their initialization
ast-grep --pattern 'std::vector<$_> m_$_'
# Find constructor and reserve operations
rg -A 5 'VectorTrackContainer::VectorTrackContainer|reserve\(' Core/src/EventData/VectorTrackContainer.cpp
# Find push_back operations to verify synchronized additions
rg 'push_back.*m_' Core/src/EventData/VectorTrackContainer.cpp
# Find clear operations
rg '\bclear\(' Core/src/EventData/VectorTrackContainer.cpp
# Find the header file to check member declarations
fd VectorTrackContainer.hpp
Length of output: 1225
Script:
#!/bin/bash
# Check the header file for member declarations
cat Core/include/Acts/EventData/VectorTrackContainer.hpp
# Find any add/addTrack methods to verify synchronized additions
rg -A 10 'add(Track|\w+Track)' Core/src/EventData/VectorTrackContainer.cpp
Length of output: 10403
Core/include/Acts/EventData/MultiTrajectoryBackendConcept.hpp (2)
133-136: Approve the first overload for 1D calibrated allocation, I do!
Hmmmm, wise addition this is! For single-dimensional measurements, a path this provides. Clean and precise the implementation is, yes.
138-141: Strong with the Force, this bound-size overload is!
Flexibility it brings, allowing full bound-size allocation. Performance improvements this shall bring, as foretold in the prophecy of benchmarks. Aligned with our objectives, this change is.
Yet verify we must, that all implementations of this concept properly handle these new requirements.
✅ Verification successful
The previous pattern might have been too complex. Let's try simpler patterns and multiple approaches to find the implementations. We search for class declarations inheriting from MutableMultiTrajectoryBackend, use ripgrep for a broader text search, and also list trajectory-related files to ensure we don't miss anything.
Hmm, found trajectory files we have. But their contents, examine we must. The VectorMultiTrajectory, the primary implementation it might be. The test files, valuable insights they may hold. The usage patterns of allocateCalibrated, crucial they are to our investigation.
Strong in the Force, this implementation is!
Verified the bound-size overload has been, throughout the codebase. In VectorMultiTrajectory, properly implemented it is, with full dimension support and error checking. The test files, comprehensive coverage they show. Used widely across the codebase, this feature is - in calibrators, fitters, and benchmarks.
Key observations, these are:
- Proper dimension validation exists, throwing errors when mismatched dimensions appear
- Zero initialization handled correctly it is
- Existing implementations properly support the full bound-size allocation
- Test coverage extensive it is, including edge cases and error conditions
Through the Force, see no issues I do. Working as intended, this change is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for implementations of MutableMultiTrajectoryBackend
ast-grep --pattern 'class $_ : public $$$, public Acts::MutableMultiTrajectoryBackend<$_> {
$$$
}'
Length of output: 102
Script:
#!/bin/bash
# Search for implementations using different pattern
ast-grep --pattern 'class $_ : public MutableMultiTrajectoryBackend<$_>'
# Also search for direct implementations
rg "class.*MutableMultiTrajectoryBackend" -A 5
# Search for files that might contain implementations
fd -e hpp -e cpp "Trajectory"
Length of output: 744
Script:
#!/bin/bash
# Check VectorMultiTrajectory implementation specifically
cat Core/include/Acts/EventData/VectorMultiTrajectory.hpp
# Check test implementations for usage patterns
cat Core/include/Acts/EventData/detail/MultiTrajectoryTestsCommon.hpp
# Look for allocateCalibrated usage
rg "allocateCalibrated" -A 3
Length of output: 87172
Tests/Benchmarks/TrackEdmBenchmark.cpp (2)
114-115: Hmmmm, wise addition this is!
Clear the output container between runs, we must. Prevent contamination of benchmark results, this does. Essential for accurate performance measurements, such cleanliness is.
138-148: Strong with the Force, this change is!
Improved, the measurement allocation has been. Two benefits, I sense:
- Calculate measurement dimension first, we now do
- Initialize values directly during allocation, we can
Matches the PR objectives, this does. Benchmark results, verify we must.
✅ Verification successful
Hmm, clear the path forward has become
Through the Force, see the changes I do. A pattern emerges:
- Old way: Two steps it took - allocate first, then set values
- New way: Single step it is - allocate and set values together
- Throughout the codebase, similar pattern exists in production code
Performance benefits, this brings:
- One memory allocation instead of two
- Initialization combined with allocation, fewer operations needed
- Clean and consistent with existing production code patterns, it is
In RefittingCalibrator, ScalingCalibrator, and MeasurementCalibration, same approach already used they do. Aligned with the ways of the Force, this change is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the benchmark improvements across git history
# Get the benchmark results from the main branch
git show main:Tests/Benchmarks/TrackEdmBenchmark.cpp > main_benchmark.cpp
# Compare implementations
diff main_benchmark.cpp Tests/Benchmarks/TrackEdmBenchmark.cpp
# Look for other measurement allocation patterns that might need updating
ast-grep --pattern 'trackState.allocateCalibrated($$$)'
Length of output: 2112
Examples/Framework/src/EventData/ScalingCalibrator.cpp (1)
181-181: Approve the consolidation of calibrated allocation, I do!
Wise changes these are, hmm! Consolidating separate allocations into single call allocateCalibrated, efficiency brings. Benchmarks show improvement in performance they do - from 17.270 to 16.553 seconds the execution time drops.
Verify the impact across codebase, we must:
✅ Verification successful
Consistent usage of allocateCalibrated across the codebase, I see!
Searched through the Force, hmm, and found wisdom I have! All instances of allocateCalibrated follow same pattern they do - parameters and covariance together they keep. No separate allocation patterns remaining that require consolidation, there are.
In harmony with the Force, this change is:
- In benchmarks, properly used it is
- In test files, correctly implemented it stands
- In other calibrators (Neural, Refitting, Measurement), same pattern followed it is
- In core functionality, consistently applied it remains
Strong with the Force, this consolidation is! Performance benefits it brings, while maintaining consistency across the codebase it does.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of allocateCalibrated to ensure consistent usage
ast-grep --pattern 'allocateCalibrated($$$)'
# Look for any remaining separate allocation patterns that could benefit from consolidation
rg -A 2 'trackState\.(allocate|set)(Calibrated|Parameters|Covariance)'
Length of output: 2578
Examples/Framework/ML/src/NeuralCalibrator.cpp (1)
192-192: Approve the simplified allocation approach, I do!
Wise decision it is, to consolidate multiple calls into single allocateCalibrated. Cleaner code this creates, and performance benefits it may bring, as shown in the benchmarks, hmm.
Verify the impact on calibration process, we must:
✅ Verification successful
Consistent usage of allocateCalibrated across codebase, I see!
Through the Force, observed I have that the allocateCalibrated method is used consistently across the codebase. The change in NeuralCalibrator.cpp aligns with established patterns, it does:
- In ScalingCalibrator.cpp and MeasurementCalibration.cpp, same pattern of allocation followed by setSubspaceIndices, there is
- In unit tests and benchmarks, proper initialization with parameters and covariance, demonstrated it is
- In core implementations (VectorMultiTrajectory.hpp, MultiTrajectory.hpp), robust error checking and dimension validation, present they are
Strong with the Force, this change is. Proper initialization and consistent usage patterns throughout the codebase, maintained they are.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other calibrator implementations to ensure consistency
rg -l "allocateCalibrated" --type cpp | while read -r file; do
echo "=== $file ==="
rg "allocateCalibrated" "$file" -C 2
done
Length of output: 13020
Tests/UnitTests/Core/EventData/MultiTrajectoryTests.cpp (1)
206-209: Approve this test case addition, I do.
Follows the established patterns of testing, this new case does. Well structured and consistent with other test cases in the suite, it is. Random number generator for reproducible results, wisely used it has been.
Verify the test coverage for the new initialization functionality, we must:
✅ Verification successful
Strong with the Force, this test case is, hmmmm.
Comprehensive testing of calibrated initialization, I see. Well implemented the test case is, with proper assertions it has:
- Initial invalid state, verified it is
- Allocation of calibrated parameters and covariance, tested it has been
- Correct size and values after allocation, confirmed they are
In the common test implementation, thorough coverage exists. Multiple test files using this common test, I observe. The Force flows through both core and plugin tests, it does.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage of the new initialization functionality
# Search for related test implementations
echo "=== Related test implementations ==="
rg -A 5 "testMultiTrajectoryAllocateCalibratedInit" --type cpp
# Search for test assertions related to calibrated initialization
echo "=== Test assertions for calibrated initialization ==="
ast-grep --pattern 'BOOST_CHECK$$$allocateCalibrated$$$'
Length of output: 2019
Script:
#!/bin/bash
# Search for the implementation details of the test case
echo "=== Test implementation details ==="
rg -B 5 -A 15 "testMultiTrajectoryAllocateCalibratedInit" Core/include/Acts/EventData/detail/MultiTrajectoryTestsCommon.hpp
# Search for the allocateCalibrated method usage
echo "=== allocateCalibrated method usage ==="
ast-grep --pattern 'allocateCalibrated($_)'
# Search for related test assertions
echo "=== Related test assertions ==="
rg "BOOST_CHECK.*calibrated" --type cpp
Length of output: 8194
Core/src/EventData/VectorMultiTrajectory.cpp (1)
215-215: Hmmmm, proper cleanup of measurement state, this is!
Wise addition, this line is. Complete state cleanup it ensures, when calibrated measurements are unset. Prevent issues with stale measurement dimensions, it does.
Run this verification, we must:
✅ Verification successful
Proper cleanup of measurement state, this change ensures!
Verified the change is, hmmmm. Throughout the codebase, consistent pattern of invalidating measdim we see:
- In
PodioTrackStateContainer, same pattern of settingmeasdim = kInvalidduring cleanup exists - In
VectorMultiTrajectory, proper handling of measurement dimension in allocation and cleanup we observe - Consistent with the Force's balance, this change is - invalidating all measurement-related components when unsetting calibrated state
A wise addition to maintain proper state cleanup, this change is. No issues in the Force, I sense.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent cleanup in similar unset operations
# Search for other measurement-related cleanup patterns
rg "measdim.*=.*kInvalid" --type cpp
# Look for related initialization patterns
ast-grep --pattern 'measdim = $_'
Length of output: 907
Script:
#!/bin/bash
# Search for the unset_impl method implementation to understand the full context
ast-grep --pattern 'unset_impl($_) {
$$$
}'
# Look for other measurement-related state handling
rg "measdim" -B 2 -A 2 --type cpp
Length of output: 48410
Tests/UnitTests/Plugins/Podio/PodioTrackStateContainerTest.cpp (1)
205-208: Hmmmm, test case added well, it has been!
Align with PR objectives, this test does. Validate the new functionality for TrackState measurement initialization without extra steps, it will. Follow the established patterns of other test cases in this file, it does.
Verify the test implementation in CommonTests, we must:
✅ Verification successful
Verified, the test implementation is, hmmmm!
Found the implementation in MultiTrajectoryTestsCommon.hpp, I have. Proper test it is, checking the calibrated size initialization and parameter allocation, it does. Common test suite shared between PodioTrackStateContainerTest and core MultiTrajectoryTests, this is. Follows the way of proper testing, it does:
- Verifies initial calibrated size is invalid
- Tests allocation with parameters and covariance
- Validates size and content after allocation
- Checks zero initialization behavior
The Force is strong with this test case, yes, yes!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the test implementation
ast-grep --pattern 'testMultiTrajectoryAllocateCalibratedInit($_) {
$$$
}'
Length of output: 78
Script:
#!/bin/bash
# Search for the test implementation with ripgrep
rg -l "testMultiTrajectoryAllocateCalibratedInit" --type cpp
# If found, show the implementation
rg "testMultiTrajectoryAllocateCalibratedInit" -A 20 --type cpp
Length of output: 6057
Core/include/Acts/EventData/MultiTrajectory.hpp (3)
11-11: LGTM! Essential include for algebra operations added.
Necessary for vector and matrix operations in the enhanced allocation methods, this include does.
691-692: Documentation clarity improved, it has.
Clear documentation of behavior when existing allocation present, crucial for API users it is.
697-703: Implementation refactored for better efficiency, hmm.
Wise use of visit_measurement pattern, I sense. Zero initialization through Eigen's methods, cleaner it is.
✅ Verification successful
The previous pattern didn't yield results, so I'm adjusting the search to get more context about the visit_measurement usage and related implementations. Using both ripgrep for textual context and ast-grep for the specific implementation pattern.
Wise implementation indeed, hmm, yes!
Through the Force, seen the truth I have. The visit_measurement pattern, widely used throughout the codebase it is, serving as a powerful template dispatch mechanism for handling measurements of different dimensions. Zero initialization through Eigen's methods, a clean and efficient approach it is.
In the implementation:
- Handles measurement dimensions dynamically it does
- Proper zero initialization through
ActsVector<DIM>::Zero()andActsSquareMatrix<DIM>::Zero(), memory-safe it is - Consistent with the codebase's pattern, as seen in many other files, hmm
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other visit_measurement usage patterns
ast-grep --pattern 'visit_measurement($_, $_)'
Length of output: 48
Script:
#!/bin/bash
# Search for visit_measurement pattern with more context
rg -U "visit_measurement" -A 5 -B 5
# Also search for similar patterns to understand the implementation
ast-grep --pattern 'template allocateCalibrated_impl($_, $_, $_)'
Length of output: 28846
Plugins/Podio/include/Acts/Plugins/Podio/PodioTrackStateContainer.hpp (4)
73-73: Proper invalid measurement dimension check, you have implemented.
Using kInvalid ensures uninitialized measurement dimensions are correctly identified.
Also applies to: 75-75
475-475: Initializing data.measdim with kInvalid, wisely you do.
Enhances error handling and prevents misinterpretation of uninitialized measurement dimensions, this change does.
Also applies to: 595-595
618-646: New overload for allocateCalibrated_impl enhances flexibility, it does.
Allows direct initialization of measurement values and covariances, improving efficiency and usability.
623-628:
Ensure C++20 support for requires clause, you must.
The use of requires clauses mandates C++20 support. Verify that the build environment and project guidelines permit C++20 features to avoid compilation issues.
Core/include/Acts/EventData/TrackStateProxy.hpp (1)
868-875: Allocate storage method added, useful it is.
This method properly initializes storage for measurements, handling existing allocations wisely.
Core/include/Acts/EventData/detail/MultiTrajectoryTestsCommon.hpp (1)
1230-1264: Well-designed, your new test 'testMultiTrajectoryAllocateCalibratedInit' is.
Ensures correct behavior when initializing calibrated measurements and covariances, it does. Validates allocation and error handling, this test effectively does.
this will also exclude dynamic eigen matrices
|
There might be a tiny improvement in CKF time, but it's likely at the noise level.
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
Core/include/Acts/EventData/TrackStateProxy.hpp(3 hunks)Core/include/Acts/EventData/VectorMultiTrajectory.hpp(3 hunks)
🧰 Additional context used
📓 Learnings (1)
Core/include/Acts/EventData/VectorMultiTrajectory.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 (4)
Core/include/Acts/EventData/VectorMultiTrajectory.hpp (3)
350-352: Initialization ensured by the interface, it is.
As confirmed you have, safe to proceed we are.
494-531: Logic sound, code acceptable is.
Implementation correct appears. Approve this change, I do.
60-63: 🧹 Nitpick (assertive)
Consider defining operator!=, you should.
Defined operator== is, but operator!= is not. For completeness and consistency, define operator!=, you might.
⛔ Skipped due to learnings
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.
Core/include/Acts/EventData/TrackStateProxy.hpp (1)
875-895: Addition of overloaded allocateCalibrated, commendable it is.
Allows direct initialization with measurement and covariance, this enhancement does. Improves usability and flexibility, it will.
|
acts-project-service
added
the
Breaks Athena build
Previously,
VectorMultiTrajectorywould initialize measurement values and covariances to zero. This has some amount of overhead.With this
allocateCalibratedon a track state with an existing allocation of different size is now an errorallocateCalibratednow accepts arguments for the value and covariance that it will initialize the allocation to.Summary by CodeRabbit
New Features
size()method toVectorTrackContainerfor easy retrieval of track count.CombinatorialKalmanFilterwith a newfindTracksmethod for improved track finding capabilities.MultiTrajectoryAllocateCalibratedInittest case to validate the allocation of calibrated initial states.Bug Fixes
findTracksmethod to log propagation errors.VectorMultiTrajectoryto ensure proper state invalidation.Tests