refactor!: move source link creation, track state creation, measurement selection into a single unit outside of the CKF #3825
Conversation
WalkthroughSignificant changes made to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (4)
👮 Files not reviewed due to content moderation or server errors (3)
🧰 Additional context used📓 Learnings (3)Tests/UnitTests/Core/TrackFinding/CombinatorialKalmanFilterTests.cpp (3)Examples/Algorithms/TrackFinding/src/TrackFindingAlgorithm.cpp (3)Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp (2)🔇 Additional comments (6)Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp (6)
Wise addition of specialized headers for extensions and track state creation, this is. Modular design principles, it follows.
Removed
Direct copy of delegates to calibrator, updater, and branch stopper, this is. Clean and clear, the implementation is.
From past learnings, know we do that overloading intentional it is. Convenience for users, it provides. Template parameters reduced, making interface cleaner it does. Also applies to: 937-942
From past discussions between andiwand and goetzgaycken, noted it was that
Handle errors with more detail now, we do. Surface ID in error message, helpful for debugging it will be. Run this verification, we must: ✅ Verification successfulLet me search for more context about track state creation and error handling patterns, I must. More context about error handling patterns in the force, seek I must. Clear vision of the implementation, this will bring. Consistent with the Force, this error handling is. Verified the error handling patterns across the file, I have. Consistent approach to error reporting it follows, hmm. Surface geometryId in error message, aligned with other error handling patterns in the CombinatorialKalmanFilter it is. Proper context for debugging, it provides.
The path of wisdom in error handling, followed it has. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check error handling consistency across track state creation
# Test: Search for similar error handling patterns
rg -A 2 "Track state creation failed"
Length of output: 390 Script: #!/bin/bash
# Search for similar error handling patterns in track state creation across the codebase
ast-grep --pattern 'createTrackStates($$$)'
# Also check for other geometryId usage in error messages
rg -A 2 "geometryId.*error"
Length of output: 373 Script: #!/bin/bash
# Search for the broader context of error handling in the file
rg -B 5 -A 5 "ACTS_ERROR" Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp
# Look for similar track state handling patterns
ast-grep --pattern 'Result<void> $_ = $$.createTrackStates($$$)'
Length of output: 3509 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
goetzgaycken
force-pushed
the
refactor_move_sl_creation_measurement_selection_to_single_unit
branch
from
c1ce59a to
d3a565e
Compare
|
goetzgaycken
force-pushed
the
refactor_move_sl_creation_measurement_selection_to_single_unit
branch
from
d3a565e to
b44b8fe
Compare
|
Why should there be brackets arround slAccessorDelegate TrackFindingAlgorithm.cpp:365 (macos build complains): |
andiwand
changed the title
|
Couple more thoughts on this: I think going with a single level of composition would be clearer. Currently there's the base composable class that connects to the previous A single composable struct that by itself has the three (?) original delegates including the What do we need this second layer for after this change? I think having one layer of adapter to the fully separate makes sense, but the surface area of the track state creator seems small enough to me to remove that altogether. |
|
It is only needed for the transition period. In my opinion it would be better to first introduce the new api and then merge on the athena side the track state creator and source link accessor. The amount of code duplication when introducing different helper objects for the two cases is small, though the simplification is also not significant. Therefore I would rather not duplicate the code, and rather remove the unnecessary code and layers later when they are not needed anymore. But I do not have a strong opinion. |
|
The question for me would be, do we need a transition period? The change is breaking either way no? Touching this twice means we have to go through two breaking cycles. |
It is certainly possible to merge at the same time on the athena side the source link accessor and track state creator. I don't have any strong argument to motivate why it is better to break this into smaller steps. |
goetzgaycken
force-pushed
the
refactor_move_sl_creation_measurement_selection_to_single_unit
branch
2 times, most recently
from
b00f570 to
2cc2ecc
Compare
|
Changes from b44b8fe to b00f570 :
changes from b00f570 to 2cc2ecc:
I did not remove the adapter to support the components of the version 2 interface, which is currently used in ATLAS. I agree that there is no good reason to keep that around for long, but I am not sure whether it is a good strategy to make the interface change on the Acts side, and directly migrate on the ATLAS side to the new interface. The changes are not super complicated but also not trivial. Minimal changes to support the new interface on the athena side gitlab:EliminateSourceLinkAccessor_main~1, direct support of the new interface gitlab:EliminateSourceLinkAccessor_main, |
goetzgaycken
force-pushed
the
refactor_move_sl_creation_measurement_selection_to_single_unit
branch
from
2cc2ecc to
dbfba96
Compare
goetzgaycken
force-pushed
the
refactor_move_sl_creation_measurement_selection_to_single_unit
branch
from
dbfba96 to
2fa962c
Compare
goetzgaycken
force-pushed
the
refactor_move_sl_creation_measurement_selection_to_single_unit
branch
from
2fa962c to
9493657
Compare
|
Open discussion points:
|
goetzgaycken
force-pushed
the
refactor_move_sl_creation_measurement_selection_to_single_unit
branch
from
9493657 to
abedde4
Compare
![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
🧹 Outside diff range and nitpick comments (11)
Core/include/Acts/TrackFinding/CombinatorialKalmanFilterExtensions.hpp (2)
29-29: Improve readability, you should, by replacingstd::tuplewith a named structClearer your code will be if
BoundStateis a struct with named members, yes.Consider defining:
struct BoundState { BoundTrackParameters parameters; BoundMatrix jacobian; double pathLength; };Then, use it like this:
using BoundState = BoundState;
95-97: MakevoidBranchStopperconstexpr, you canEnhance performance subtly by declaring
voidBranchStopperasconstexpr, hmmm.Core/include/Acts/TrackFinding/TrackStateCreator.hpp (1)
284-292: Avoid unnecessary semicolon after function definition, you shouldUnneeded semicolon at the end of
voidMeasurementSelectorfunction there is. Remove it, you must.Apply this diff to fix:
-static Result<std::pair<typename std::vector<TrackStateProxy>::iterator, - typename std::vector<TrackStateProxy>::iterator>> -voidMeasurementSelector(typename std::vector<TrackStateProxy>& candidates, - bool& /*isOutlier*/, const Logger& /*logger*/) { - return std::pair{candidates.begin(), candidates.end()}; -}; +static Result<std::pair<typename std::vector<TrackStateProxy>::iterator, + typename std::vector<TrackStateProxy>::iterator>> +voidMeasurementSelector(typename std::vector<TrackStateProxy>& candidates, + bool& /*isOutlier*/, const Logger& /*logger*/) { + return std::pair{candidates.begin(), candidates.end()}; +}Examples/Algorithms/TrackFinding/src/TrackFindingAlgorithm.cpp (4)
326-328: InitializeMeasurementSelectordirectly with config, you canSimplify code, you should, by initializing
measSelwithm_cfg.measurementSelectorCfgdirectly.Apply this diff:
-MeasurementSelector measSel{ - Acts::MeasurementSelector(m_cfg.measurementSelectorCfg)}; +MeasurementSelector measSel(m_cfg.measurementSelectorCfg);
329-331: Use direct member initialization forslAccessor, you shouldSet
slAccessor.containerupon construction, consider.Apply this diff:
-IndexSourceLinkAccessor slAccessor; -slAccessor.container = &measurements.orderedIndices(); +IndexSourceLinkAccessor slAccessor{&measurements.orderedIndices()};
335-341: Improve delegate connections readability, you couldMore explicit your delegate connections can be, yes.
Refactor to:
TrackStateCreatorType trackStateCreator; trackStateCreator.sourceLinkAccessor.connect< &IndexSourceLinkAccessor::range>(&slAccessor); trackStateCreator.calibrator.connect< &MeasurementCalibratorAdapter::calibrate>(&calibrator); trackStateCreator.measurementSelector.connect< &MeasurementSelector::select>(&measSel);
368-370: Remove redundant empty line, you shouldUnnecessary empty line between parameters there is.
TrackFinderOptions firstOptions(ctx.geoContext, ctx.magFieldContext, ctx.calibContext, extensions, - firstPropOptions); + + firstPropOptions);Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp (2)
832-835: Template parameters update in documentation, reflect you mustUpdate the documentation to match the new template parameters, you should.
In the comment:
/// @tparam start_parameters_t Type of the initial parameters -/// @tparam source_link_iterator_t Type of the source link iteratorRemove the outdated
source_link_iterator_tparameter.
865-866: Avoid possible shadowing by renamingr, you shouldVariable
rmay shadow other variables. Descriptive name, use.Consider renaming
rtockfResultor similar.-auto& r = +auto& ckfResult = propState .template get<CombinatorialKalmanFilterResult<track_container_t>>();docs/_extensions/lazy_autodoc.py (1)
79-79: Maintain alphabetical order of entries, you shouldEntries in
role_instances["struct"]unordered they are. Alphabetically, sort them you should.Reorder the entries:
role_instances["struct"] |= { "Acts::Geant4PhysicalVolumeSelectors::AllSelector", "Acts::Geant4PhysicalVolumeSelectors::NameSelector", "Acts::Geant4PhysicalVolumeSelectors::PositionSelector", + "Acts::OrientedSurface", "Acts::TrackStateCreator", - "Acts::OrientedSurface", }Tests/UnitTests/Core/TrackFinding/CombinatorialKalmanFilterTests.cpp (1)
292-310: Well structured the helper function is, but documentation strengthen we could!Good organization of track state creator composition I see. Yet, add return type documentation to function comment we should.
Add return type documentation:
// set up composable track state creator from these components: // - source link accessor, // - measurement selector // - track state candidate creator +// @return TrackStateCreator instance configured with the provided components template <typename source_link_accessor_t> inline auto makeTrackStateCreator(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp(8 hunks)Core/include/Acts/TrackFinding/CombinatorialKalmanFilterExtensions.hpp(1 hunks)Core/include/Acts/TrackFinding/TrackStateCreator.hpp(1 hunks)Examples/Algorithms/TrackFinding/include/ActsExamples/TrackFinding/TrackFindingAlgorithm.hpp(1 hunks)Examples/Algorithms/TrackFinding/src/TrackFindingAlgorithm.cpp(3 hunks)Tests/UnitTests/Core/TrackFinding/CombinatorialKalmanFilterTests.cpp(5 hunks)docs/_extensions/lazy_autodoc.py(1 hunks)
👮 Files not reviewed due to content moderation or server errors (1)
- Examples/Algorithms/TrackFinding/include/ActsExamples/TrackFinding/TrackFindingAlgorithm.hpp
🔇 Additional comments (3)
Tests/UnitTests/Core/TrackFinding/CombinatorialKalmanFilterTests.cpp (3)
42-42: Approve these changes, I do!
Necessary additions these are, for the new track state creation functionality. Simplified template parameters, match the production code they do.
Also applies to: 172-172
287-288: Simplified options creation, approve I do!
Removed the source link accessor parameter we have. Cleaner the interface becomes, yet maintains all necessary context it does.
313-319: Necessary template instantiation this is!
Explicit instantiation prevents linker errors it does. Well commented the necessity is.
|
A bit misleading summary by coder abitai,
|
goetzgaycken
force-pushed
the
refactor_move_sl_creation_measurement_selection_to_single_unit
branch
from
56d0051 to
dca3186
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
Examples/Algorithms/TrackFindingExaTrkX/src/TrackFindingFromPrototrackAlgorithm.cpp (1)
110-110: Unnecessary space, there isRemove the extra space before the scope resolution operator in line 110, you should.
Tests/UnitTests/Core/TrackFinding/CombinatorialKalmanFilterTests.cpp (1)
313-320: Automatic instantiation, resolve you mustExplicit template instantiation indicates an issue with compiler deductions. Investigate why automatic instantiation fails, you should.
Examples/Algorithms/TrackFinding/src/TrackFindingAlgorithm.cpp (1)
348-348: Extra space, present it isIn line 348, before the scope resolution operator, an unnecessary space there is. Remove it for consistency, you should.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp(8 hunks)Core/include/Acts/TrackFinding/CombinatorialKalmanFilterExtensions.hpp(1 hunks)Core/include/Acts/TrackFinding/TrackStateCreator.hpp(1 hunks)Examples/Algorithms/TrackFinding/include/ActsExamples/TrackFinding/TrackFindingAlgorithm.hpp(1 hunks)Examples/Algorithms/TrackFinding/src/TrackFindingAlgorithm.cpp(3 hunks)Examples/Algorithms/TrackFindingExaTrkX/src/TrackFindingFromPrototrackAlgorithm.cpp(2 hunks)Tests/UnitTests/Core/TrackFinding/CombinatorialKalmanFilterTests.cpp(5 hunks)docs/_extensions/lazy_autodoc.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/_extensions/lazy_autodoc.py
- Examples/Algorithms/TrackFinding/include/ActsExamples/TrackFinding/TrackFindingAlgorithm.hpp
- Core/include/Acts/TrackFinding/CombinatorialKalmanFilterExtensions.hpp
🧰 Additional context used
📓 Learnings (2)
Tests/UnitTests/Core/TrackFinding/CombinatorialKalmanFilterTests.cpp (1)
Learnt from: goetzgaycken
PR: acts-project/acts#3825
File: Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp:933-938
Timestamp: 2024-12-02T14:13:24.820Z
Learning: In `Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp`, the overloading of `findTracks` functions is intentional for convenience and not considered code duplication.
Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp (1)
Learnt from: goetzgaycken
PR: acts-project/acts#3825
File: Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp:933-938
Timestamp: 2024-12-02T14:13:24.820Z
Learning: In `Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp`, the overloading of `findTracks` functions is intentional for convenience and not considered code duplication.
🔇 Additional comments (5)
Examples/Algorithms/TrackFindingExaTrkX/src/TrackFindingFromPrototrackAlgorithm.cpp (1)
95-111: Set up, correctly you have
The initialization of TrackStateCreator and connection of delegates, appropriate it is.
Core/include/Acts/TrackFinding/TrackStateCreator.hpp (1)
1-294: Well-structured, this new component is
The TrackStateCreator is thoughtfully designed and integrates smoothly with existing systems.
Tests/UnitTests/Core/TrackFinding/CombinatorialKalmanFilterTests.cpp (1)
292-310: Helper function usage, clear it is
The makeTrackStateCreator function enhances code reusability and readability.
Examples/Algorithms/TrackFinding/src/TrackFindingAlgorithm.cpp (1)
332-341: Delegate connections, consistent they are
Properly connecting the delegates to TrackStateCreator, you are. Aligns with previous discussions, this does.
Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp (1)
933-938: Overloaded functions, intentional they are
Aware we are that overloading findTracks is for convenience. Code duplication, this is not.
goetzgaycken
force-pushed
the
refactor_move_sl_creation_measurement_selection_to_single_unit
branch
from
dca3186 to
7d5548c
Compare
Core/include/Acts/TrackFinding/CombinatorialKalmanFilterExtensions.hpp
Outdated
Show resolved
Hide resolved
goetzgaycken
force-pushed
the
refactor_move_sl_creation_measurement_selection_to_single_unit
branch
from
7d5548c to
3639a5a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
Core/include/Acts/TrackFinding/CombinatorialKalmanFilterExtensions.hpp (2)
18-22: Document the rationale for s_maxBranchesPerSurface value, young padawan must.The comment speaks of container size, but wisdom about performance impact and overflow handling, it lacks. Document these aspects, you should.
91-98: Clean implementation, but documentation improve we can.Well implemented the default branch stopper is. Yet, document when this default behavior suitable is, helpful it would be.
Examples/Algorithms/TrackFinding/src/TrackFindingAlgorithm.cpp (1)
347-349: Simplify template syntax, we can.Remove unnecessary space between
TrackStateCreatorTypeand::, cleaner code it will be.- .template connect<&TrackStateCreatorType ::createTrackStates>( + .template connect<&TrackStateCreatorType::createTrackStates>(Examples/Algorithms/TrackFindingExaTrkX/src/TrackFindingFromPrototrackAlgorithm.cpp (2)
108-113: Fix the template syntax in createTrackStates connection, you must!Extra space in template syntax detected, I have. Remove it, we should:
- .template connect<&TrackStateCreatorType ::createTrackStates>( + .template connect<&TrackStateCreatorType::createTrackStates>(
95-113: Wisdom about the architectural changes, share I must!A significant architectural shift this is, moving source link creation, track state creation, and measurement selection into a unified component. Documentation of performance implications, especially for empty sensitive surfaces, add you should.
Consider these aspects:
- Document the slight increase in computational effort
- Explain the trade-offs made for better encapsulation
- Add performance benchmarks for complex events
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp(8 hunks)Core/include/Acts/TrackFinding/CombinatorialKalmanFilterExtensions.hpp(1 hunks)Core/include/Acts/TrackFinding/TrackStateCreator.hpp(1 hunks)Examples/Algorithms/TrackFinding/include/ActsExamples/TrackFinding/TrackFindingAlgorithm.hpp(1 hunks)Examples/Algorithms/TrackFinding/src/TrackFindingAlgorithm.cpp(3 hunks)Examples/Algorithms/TrackFindingExaTrkX/src/TrackFindingFromPrototrackAlgorithm.cpp(2 hunks)Tests/UnitTests/Core/TrackFinding/CombinatorialKalmanFilterTests.cpp(5 hunks)docs/_extensions/lazy_autodoc.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- docs/_extensions/lazy_autodoc.py
- Examples/Algorithms/TrackFinding/include/ActsExamples/TrackFinding/TrackFindingAlgorithm.hpp
- Tests/UnitTests/Core/TrackFinding/CombinatorialKalmanFilterTests.cpp
- Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp
- Core/include/Acts/TrackFinding/TrackStateCreator.hpp
🧰 Additional context used
📓 Learnings (1)
Core/include/Acts/TrackFinding/CombinatorialKalmanFilterExtensions.hpp (1)
Learnt from: goetzgaycken
PR: acts-project/acts#3825
File: Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp:933-938
Timestamp: 2024-12-02T14:13:24.820Z
Learning: In `Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp`, the overloading of `findTracks` functions is intentional for convenience and not considered code duplication.
🔇 Additional comments (6)
Core/include/Acts/TrackFinding/CombinatorialKalmanFilterExtensions.hpp (2)
35-39: Strong with the Force, this enum is.
Clear purpose each value has. Well-structured and self-documenting, the code is.
41-43: Move to CKF namespace, consider you should.
As suggested in past reviews, moving this into a CKF-related namespace, better organization it would bring.
Examples/Algorithms/TrackFinding/src/TrackFindingAlgorithm.cpp (2)
326-341: Well structured the measurement selection changes are.
Clean and focused the implementation is. Good separation of concerns, achieved you have.
368-375: Properly aligned with new extensions, these changes are.
Clean integration of extensions into TrackFinderOptions, achieved you have.
Examples/Algorithms/TrackFindingExaTrkX/src/TrackFindingFromPrototrackAlgorithm.cpp (2)
12-12: Approve the new include and type definition, I do!
Clear and purposeful, these changes are. Well-defined type alias for TrackStateCreator, you have created. With the PR's objective of consolidating track state creation, this aligns perfectly.
Also applies to: 95-97
98-107: Verify the connections of trackStateCreator, we must!
Proper the setup appears, but verify the connections we should. Three critical components you have united:
- Source link accessor
- Calibrator
- Measurement selector
Run this script to verify the connections:
✅ Verification successful
Verified the connections are, young padawan!
Through the Force, seen the truth I have. The pattern of connections in your code, consistent with the ways of the Acts framework it is. Evidence from across the codebase shows:
- The calibrator connection pattern, found in many places it is, including TrackFitting and Alignment algorithms
- The measurementSelector connection, matching your implementation in the test files it does
- The sourceLinkAccessor pattern, though less visible in logs, follows the established pattern it does
Similar connections observed in:
- KalmanFitter
- GlobalChiSquareFitter
- GsfFitter
- AlignmentAlgorithm
Correct your implementation is, following the ways of the Force and the Acts framework patterns. Trust in your code, we can!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage patterns of TrackStateCreator connections
# Check for consistent connection patterns across the codebase
ast-grep --pattern 'TrackStateCreator<$_,$_> $_ = $_;
$_.sourceLinkAccessor.template connect<$_>($_);
$_.calibrator.connect<$_>($_);
$_.measurementSelector.connect<$_>($_);'
Length of output: 182
Script:
#!/bin/bash
# Let's search for individual components to verify their usage patterns
# Check TrackStateCreator usage
rg -A 5 "TrackStateCreator\s+\w+" --type cpp
# Check sourceLinkAccessor connections
rg -A 2 "sourceLinkAccessor.*connect" --type cpp
# Check calibrator connections
rg -A 2 "calibrator.*connect" --type cpp
# Check measurementSelector connections
rg -A 2 "measurementSelector.*connect" --type cpp
# Look for any similar patterns in header files
rg -A 5 "TrackStateCreator" --type hpp
Length of output: 4201
goetzgaycken
force-pushed
the
refactor_move_sl_creation_measurement_selection_to_single_unit
branch
from
3639a5a to
335974e
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
Examples/Algorithms/TrackFindingExaTrkX/src/TrackFindingFromPrototrackAlgorithm.cpp (2)
108-113: Strong with the Force, these extensions are!Properly connected the updater and track state creator to the extensions, you have. But a suggestion, I have:
Consider template argument deduction to simplify the syntax:
- extensions.updater.connect<&Acts::GainMatrixUpdater::operator()< - typename TrackContainer::TrackStateContainerBackend>>(&kfUpdater); + extensions.updater.connect(&Acts::GainMatrixUpdater::operator(), &kfUpdater);
Line range hint
182-187: Track this TODO, we must!Move computeSharedHits to a free function, the comment suggests. Help you with this task, I can.
Create a GitHub issue to track this refactoring task, shall I? Provide the implementation as a free function, I will.
Core/include/Acts/TrackFinding/CombinatorialKalmanFilterExtensions.hpp (1)
60-89: Clarify the ownership of trackStateCandidates, we must!Documentation, comprehensive it is. Yet, ownership semantics of
trackStateCandidatesbuffer, unclear they remain. Lifetime and responsibility for cleanup, specified they should be.Add this clarification to the documentation:
/// @param trackStateCandidates a temporary buffer that can be used to collect track states + /// @note The ownership of trackStateCandidates remains with the caller. The delegate may modify + /// the buffer but does not take ownership of it. /// @param trajectory the trajectory to which the new states are to be addedTests/UnitTests/Core/TrackFinding/CombinatorialKalmanFilterTests.cpp (1)
336-341: More test cases, need we do!Test the happy path alone, sufficient it is not. Edge cases and error conditions, covered they must be.
Add these test scenarios:
- Test with invalid source link accessor
- Test with null measurement selector
- Test error handling during track state creation
Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp (1)
47-47: Simplified template parameters, you have. Wise decision, this is!Reduced complexity in template parameters, this change brings. To a single template argument, the options class now bound is. Easier to use and understand, the interface becomes.
Consider documenting this breaking change in the migration guide, you should. Help users adapt their code to the new interface, it will.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp(8 hunks)Core/include/Acts/TrackFinding/CombinatorialKalmanFilterExtensions.hpp(1 hunks)Core/include/Acts/TrackFinding/TrackStateCreator.hpp(1 hunks)Examples/Algorithms/TrackFinding/include/ActsExamples/TrackFinding/TrackFindingAlgorithm.hpp(1 hunks)Examples/Algorithms/TrackFinding/src/TrackFindingAlgorithm.cpp(3 hunks)Examples/Algorithms/TrackFindingExaTrkX/src/TrackFindingFromPrototrackAlgorithm.cpp(2 hunks)Tests/UnitTests/Core/TrackFinding/CombinatorialKalmanFilterTests.cpp(5 hunks)docs/_extensions/lazy_autodoc.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/_extensions/lazy_autodoc.py
- Examples/Algorithms/TrackFinding/include/ActsExamples/TrackFinding/TrackFindingAlgorithm.hpp
- Core/include/Acts/TrackFinding/TrackStateCreator.hpp
🧰 Additional context used
📓 Learnings (3)
Tests/UnitTests/Core/TrackFinding/CombinatorialKalmanFilterTests.cpp (1)
Learnt from: goetzgaycken
PR: acts-project/acts#3825
File: Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp:933-938
Timestamp: 2024-12-02T14:13:24.820Z
Learning: In `Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp`, the overloading of `findTracks` functions is intentional for convenience and not considered code duplication.
Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp (1)
Learnt from: goetzgaycken
PR: acts-project/acts#3825
File: Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp:933-938
Timestamp: 2024-12-02T14:13:24.820Z
Learning: In `Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp`, the overloading of `findTracks` functions is intentional for convenience and not considered code duplication.
Core/include/Acts/TrackFinding/CombinatorialKalmanFilterExtensions.hpp (1)
Learnt from: goetzgaycken
PR: acts-project/acts#3825
File: Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp:933-938
Timestamp: 2024-12-02T14:13:24.820Z
Learning: In `Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp`, the overloading of `findTracks` functions is intentional for convenience and not considered code duplication.
🔇 Additional comments (9)
Examples/Algorithms/TrackFindingExaTrkX/src/TrackFindingFromPrototrackAlgorithm.cpp (3)
12-12: Approve the new include, I do!
Wise addition of TrackStateCreator header, this is. Aligns with the refactoring goals, it does.
95-107: Hmmmm... A powerful new TrackStateCreator implementation, I sense!
Consolidated the track state creation logic has been, yes. Through the Force, I see the following strengths:
- Clear separation of concerns
- Strong type safety through template parameters
- Proper connection of source link accessor, calibrator, and measurement selector
But careful we must be! Verify the connections, we should.
✅ Verification successful
Verified, the TrackStateCreator implementation is, hmmmm!
Through the Force, seen consistency across the codebase, I have:
- In
TrackFindingFromPrototrackAlgorithm.cpp, proper connections established are - Similar pattern in
TrackFindingAlgorithm.cpp, following the same wisdom - Unit tests in
CombinatorialKalmanFilterTests.cpp, confirming the way, they do
Consistent with the ancient Jedi texts, this implementation is. The same pattern of connecting sourceLinkAccessor, calibrator, and measurementSelector, followed everywhere it is. No disturbance in the Force, I sense.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any other TrackStateCreator usage to ensure consistency
rg -l "TrackStateCreator" | while read -r file; do
echo "=== $file ==="
rg "TrackStateCreator" "$file" -A 5
done
Length of output: 6539
117-118: Simplified the options initialization has become!
Passed the extensions to the finder options directly, you have. Clean and clear, the code is now. But verify the context usage, we must.
✅ Verification successful
Based on the search results, I can see that the context objects (geoContext, magFieldContext, calibContext) are consistently used together across the codebase, particularly in propagation, fitting, and finding operations. The usage pattern in the review file matches the standard pattern seen throughout the codebase.
Hmmmm, consistent with the Force, this code is!
The context objects flow naturally through the codebase:
- Always passed together to core operations
- Used in proper order: geometry, magnetic field, and calibration contexts
- Maintained across all major tracking operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check consistent usage of context objects across the codebase
rg "geoContext|magFieldContext|calibContext" --type cpp
Length of output: 64676
Core/include/Acts/TrackFinding/CombinatorialKalmanFilterExtensions.hpp (3)
18-31: Well structured, these constants and types are, hmm!
Wise choice, the use of small_vector with static size optimization is. Clear documentation for s_maxBranchesPerSurface, provided it has been.
33-39: Clear as crystal, this enum definition is!
Well chosen, the names are. Self-documenting, the purpose is. Continue, StopAndDrop, and StopAndKeep - wisdom in simplicity, there is.
41-59: Elegant and extensible, this design is!
Strong with the Force, this template structure is. Default implementations, wisely provided they are. Type safety through templates, achieved it has.
Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp (3)
28-29: Approve these includes, I do!
Necessary for the refactoring, these includes are. Well-ordered and properly placed, they appear to be.
832-838: Approve these method signatures, I do. Intentional overloading, this is!
From previous learnings, know we do that overloading of findTracks functions serves convenience, not duplication it represents. Simplified template parameters, these signatures now use, aligning with the greater refactoring effort.
Also applies to: 933-938
460-467: Moved track state creation to extensions, you have. Good separation of concerns, this is!
Properly handled, the error cases are. But verify the performance impact of this change, we must.
✅ Verification successful
Safe and well-implemented, the track state creation changes are, hmmmm.
Through the Force, seen the implementation I have. Used consistently across the codebase, the track state creation extension is - in unit tests, ExaTrkX algorithm, and track finding algorithm. No performance regressions or issues detected in the implementation, there are.
Properly connected through the extension mechanism, the track state creator is. Follow the ways of modular design, it does. Approve this change with confidence, we can.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all track state creation calls to ensure consistent usage
ast-grep --pattern 'createTrackStates($$$)'
# Look for any TODO comments related to performance
rg -i "todo.*perf|performance"
Length of output: 39129
Script:
#!/bin/bash
# Search for performance-related code in the extensions
ast-grep --pattern 'class $_ : public TrackStateExtension {
$$$
createTrackStates($$$) {
$$$
}
$$$
}'
# Look for any performance benchmarks or tests related to track state creation
rg -g '!{docs,CI}/*' -i "benchmark.*track.*state|perf.*track.*state" -A 3
Length of output: 191
Script:
#!/bin/bash
# Search for track state extension implementations
rg -g "*.hpp" -g "*.cpp" "class.*TrackStateExtension" -A 5
# Look for track state creation benchmarks in test directories
rg -g "*/test/*" -g "*/Tests/*" "createTrackStates" -A 5
# Find any performance measurements or timing related code around track states
rg -g "*.cpp" "// *(?i)(timing|performance).*track.*state" -A 3
Length of output: 332
Script:
#!/bin/bash
# Find all files related to track state extensions
fd -e hpp -e cpp . | rg -l "TrackStateExtension"
# Search for implementation files containing createTrackStates
fd -e cpp . | xargs rg "createTrackStates" -A 5
# Look for any performance related comments or code around track states
fd -e cpp -e hpp . | xargs rg "(?i)(perf|benchmark).*track.*state" -A 3
Length of output: 3287
goetzgaycken
force-pushed
the
refactor_move_sl_creation_measurement_selection_to_single_unit
branch
from
335974e to
175a588
Compare
goetzgaycken
force-pushed
the
refactor_move_sl_creation_measurement_selection_to_single_unit
branch
from
175a588 to
5dd2547
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp (1)
47-47: Simplified template parameters, you have. Wise decision, this is!Reduced complexity in template parameters, we have achieved. Better maintainability and clearer code structure, this brings. To the Force, closer we are!
Consider documenting this breaking change in the migration guide, you should. Help other developers adapt their code, it will.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp(8 hunks)Core/include/Acts/TrackFinding/CombinatorialKalmanFilterExtensions.hpp(1 hunks)Core/include/Acts/TrackFinding/TrackStateCreator.hpp(1 hunks)Examples/Algorithms/TrackFinding/include/ActsExamples/TrackFinding/TrackFindingAlgorithm.hpp(1 hunks)Examples/Algorithms/TrackFinding/src/TrackFindingAlgorithm.cpp(3 hunks)Examples/Algorithms/TrackFindingExaTrkX/src/TrackFindingFromPrototrackAlgorithm.cpp(2 hunks)Tests/UnitTests/Core/TrackFinding/CombinatorialKalmanFilterTests.cpp(5 hunks)docs/_extensions/lazy_autodoc.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/_extensions/lazy_autodoc.py
- Core/include/Acts/TrackFinding/TrackStateCreator.hpp
- Core/include/Acts/TrackFinding/CombinatorialKalmanFilterExtensions.hpp
- Examples/Algorithms/TrackFinding/include/ActsExamples/TrackFinding/TrackFindingAlgorithm.hpp
👮 Files not reviewed due to content moderation or server errors (3)
- Examples/Algorithms/TrackFindingExaTrkX/src/TrackFindingFromPrototrackAlgorithm.cpp
- Tests/UnitTests/Core/TrackFinding/CombinatorialKalmanFilterTests.cpp
- Examples/Algorithms/TrackFinding/src/TrackFindingAlgorithm.cpp
🧰 Additional context used
📓 Learnings (3)
Examples/Algorithms/TrackFinding/src/TrackFindingAlgorithm.cpp (4)
Learnt from: goetzgaycken
PR: acts-project/acts#3825
File: Examples/Algorithms/TrackFinding/src/TrackFindingAlgorithm.cpp:332-341
Timestamp: 2024-12-03T09:08:58.612Z
Learning: The `connect` methods for delegates return void; thus, checking their return values for error handling is inappropriate.
Learnt from: goetzgaycken
PR: acts-project/acts#3825
File: Examples/Algorithms/TrackFinding/src/TrackFindingAlgorithm.cpp:347-349
Timestamp: 2024-12-03T09:08:44.134Z
Learning: In `Examples/Algorithms/TrackFinding/src/TrackFindingAlgorithm.cpp`, the `connect` function returns void, so it's unnecessary to check its return value.
Learnt from: goetzgaycken
PR: acts-project/acts#3825
File: Tests/UnitTests/Core/TrackFinding/CombinatorialKalmanFilterTests.cpp:292-310
Timestamp: 2024-12-04T12:26:23.263Z
Learning: The `connect` method of `Delegate` returns void, so error handling through return value is not possible.
Learnt from: goetzgaycken
PR: acts-project/acts#3825
File: Examples/Algorithms/TrackFinding/src/TrackFindingAlgorithm.cpp:326-331
Timestamp: 2024-12-03T09:07:08.386Z
Learning: An empty measurement range is acceptable in `TrackFindingAlgorithm::execute` in `Examples/Algorithms/TrackFinding/src/TrackFindingAlgorithm.cpp`; no error needs to be reported when the measurement container is empty.
Tests/UnitTests/Core/TrackFinding/CombinatorialKalmanFilterTests.cpp (3)
Learnt from: goetzgaycken
PR: acts-project/acts#3825
File: Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp:933-938
Timestamp: 2024-12-02T14:13:24.820Z
Learning: In `Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp`, the overloading of `findTracks` functions is intentional for convenience and not considered code duplication.
Learnt from: goetzgaycken
PR: acts-project/acts#3825
File: Tests/UnitTests/Core/TrackFinding/CombinatorialKalmanFilterTests.cpp:292-310
Timestamp: 2024-12-04T12:26:23.263Z
Learning: The `connect` method of `Delegate` returns void, so error handling through return value is not possible.
Learnt from: goetzgaycken
PR: acts-project/acts#3825
File: Examples/Algorithms/TrackFinding/src/TrackFindingAlgorithm.cpp:332-341
Timestamp: 2024-12-03T09:08:58.612Z
Learning: The `connect` methods for delegates return void; thus, checking their return values for error handling is inappropriate.
Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp (1)
Learnt from: goetzgaycken
PR: acts-project/acts#3825
File: Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp:933-938
Timestamp: 2024-12-02T14:13:24.820Z
Learning: In `Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp`, the overloading of `findTracks` functions is intentional for convenience and not considered code duplication.
🔇 Additional comments (2)
Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp (2)
28-29: Approve these includes, I do!
Necessary for the refactoring, these includes are. Support the consolidation of source link creation and track state creation, they do.
832-838: Method signatures, simplified they are. But verify backward compatibility, we must!
Consistent with template parameter changes, these modifications are. Yet, verify impact on existing code, we should.
Also applies to: 933-938
✅ Verification successful
The Force guides me to search without template syntax, as C++ template usage often omits explicit template parameters. Also seek implementations we must, to understand the full picture.
Safe, these changes are. Backward compatibility, maintained it is!
Examined the usage patterns in the codebase, I have. All existing calls to findTracks(), compatible they remain:
- In TrackFindingAlgorithmFunction.cpp, template parameters deduced automatically they are
- In CombinatorialKalmanFilterTests.cpp, both forward and backward tracking tests, unaffected they stay
The Force reveals that removal of source_link_iterator_t affects not the existing code, as template parameter deduction handles it gracefully. Default parameter type BoundTrackParameters, wisdom it shows.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing findTracks calls to verify compatibility
rg -A 2 'findTracks<'
Length of output: 23
Script:
#!/bin/bash
# Search for findTracks usage without template syntax
rg -A 3 '\.findTracks\(' --type cpp
# Also search for any findTracks method implementations
ast-grep --pattern 'auto findTracks($$$) {
$$$
}'
Length of output: 1399
goetzgaycken
force-pushed
the
refactor_move_sl_creation_measurement_selection_to_single_unit
branch
2 times, most recently
from
65948cf to
dbd9d5e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
Core/include/Acts/TrackFinding/CombinatorialKalmanFilterExtensions.hpp (2)
18-31: Wise choice, the small vector optimization is!Good performance characteristics, this implementation has. The size of 10 for
s_maxBranchesPerSurface, well-chosen it is. When exceeded, dynamic allocation occurs, but for most cases, stack allocation suffices.Consider monitoring the actual number of branches per surface in production. If consistently higher than 10, adjust the constant we should.
42-98: Well-structured and extensible, this design is!Proper separation of concerns through delegates, achieved it has. Default implementations, provided they are. Documentation, thorough it is.
Consider adding examples in documentation for custom delegate implementations, help future developers it will.
Tests/UnitTests/Core/TrackFinding/CombinatorialKalmanFilterTests.cpp (1)
292-310: Helper function, well-structured it is!Clear purpose and good organization of connections, this function has. Documentation for the components it connects, helpful it would be.
Add documentation for each component's role and requirements:
// set up composable track state creator from these components: -// - source link accessor, -// - measurement selector -// - track state candidate creator +// - source link accessor: Provides access to source links for a given surface +// - measurement selector: Selects measurements based on configured criteria +// - track state candidate creator: Creates track states from selected measurementsExamples/Algorithms/TrackFinding/src/TrackFindingAlgorithm.cpp (1)
347-349: Fix the template spacing, you must!Double space after TrackStateCreatorType, there is. Consistency in the Force, important it is.
- .template connect<&TrackStateCreatorType ::createTrackStates>( + .template connect<&TrackStateCreatorType::createTrackStates>(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp(8 hunks)Core/include/Acts/TrackFinding/CombinatorialKalmanFilterExtensions.hpp(1 hunks)Core/include/Acts/TrackFinding/TrackStateCreator.hpp(1 hunks)Examples/Algorithms/TrackFinding/include/ActsExamples/TrackFinding/TrackFindingAlgorithm.hpp(1 hunks)Examples/Algorithms/TrackFinding/src/TrackFindingAlgorithm.cpp(3 hunks)Examples/Algorithms/TrackFindingExaTrkX/src/TrackFindingFromPrototrackAlgorithm.cpp(2 hunks)Tests/UnitTests/Core/TrackFinding/CombinatorialKalmanFilterTests.cpp(5 hunks)docs/_extensions/lazy_autodoc.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/_extensions/lazy_autodoc.py
- Examples/Algorithms/TrackFinding/include/ActsExamples/TrackFinding/TrackFindingAlgorithm.hpp
- Core/include/Acts/TrackFinding/TrackStateCreator.hpp
🧰 Additional context used
📓 Learnings (1)
Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp (1)
Learnt from: goetzgaycken
PR: acts-project/acts#3825
File: Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp:933-938
Timestamp: 2024-12-02T14:13:24.820Z
Learning: In `Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp`, the overloading of `findTracks` functions is intentional for convenience and not considered code duplication.
🔇 Additional comments (10)
Core/include/Acts/TrackFinding/CombinatorialKalmanFilterExtensions.hpp (1)
35-39: Clear and concise, this enum is!
Well-chosen names that clearly express their purpose, these enum values have.
Examples/Algorithms/TrackFindingExaTrkX/src/TrackFindingFromPrototrackAlgorithm.cpp (1)
95-113: Strong with the Force, this integration is!
Properly connected all components are, and clear the flow of data is. The new architecture pattern, followed it has been.
Verify the connections, we must:
✅ Verification successful
The Force guides me to look deeper into the connection patterns and their usage in tests, hmm. Verify the proper implementation across the codebase, we must.
Strong in the Force, this implementation is!
Verified the connections are, across the codebase they are consistent, hmm! In test files and examples alike, the same connection patterns we observe. The TrackStateCreator and CombinatorialKalmanFilterExtensions, properly implemented they are:
- In alignment algorithms, similar patterns we see
- In track finding implementations, consistent the connections remain
- In test files, validated these connections are
No exceptions or error cases found in the connections, and consistent with the established patterns throughout the codebase, this implementation is. Trust in the Force, we can, that this code path is secure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all required components are properly connected
# Check for any TODOs or FIXMEs related to connections
rg -g '*.cpp' -g '*.hpp' 'TODO|FIXME' . | grep -i 'connect'
# Look for any connection-related error handling
rg -g '*.cpp' 'connect.*throw|throw.*connect'
Length of output: 463
Script:
#!/bin/bash
# Search for TrackStateCreator usage patterns
ast-grep --pattern 'TrackStateCreator<$_>'
# Check for any error handling around these connections
rg -g '*.cpp' -A 5 'trackStateCreator.*connect|extensions.*connect'
# Look for similar connection patterns in test files
fd -e cpp -e hpp test | xargs rg 'TrackStateCreator.*connect|CombinatorialKalmanFilterExtensions.*connect'
Length of output: 10380
Tests/UnitTests/Core/TrackFinding/CombinatorialKalmanFilterTests.cpp (2)
171-172: Simpler, the interface has become!
Removed complexity, we have. Clearer to understand, the code now is.
336-341: Well-tested, the new architecture is!
Both forward and backward directions, covered they are. Proper integration testing, achieved it has been.
Also applies to: 398-402
Examples/Algorithms/TrackFinding/src/TrackFindingAlgorithm.cpp (3)
28-29: Wise additions to the includes, these are!
Align with the refactoring objectives, these new headers do. Support the consolidation of track state creation and measurement selection, they will.
326-328: Proper initialization of MeasurementSelector, this is!
Configuration from m_cfg, it uses wisely.
332-341: Well structured, the TrackStateCreator setup is!
Properly connected, the delegates are:
- Source link accessor for range access
- Calibrator for measurement calibration
- Measurement selector for candidate selection
Follow the established pattern of explicit delegate connections, it does. Hmmmm.
Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp (3)
47-47: Simplified template parameters, you have! Hmmmm.
Removed source_link_iterator_t template parameter, you did. Cleaner and more focused, the interface becomes.
832-838: Simplified findTracks signature, you have!
Removed unnecessary template parameter, making the interface cleaner it does. Yet maintain all functionality, it does. Hmmmm, good this is.
865-865: Clean and direct, the extension assignment is!
Copy delegates from options to actor, it does. Simple yet effective, this approach is.
goetzgaycken
force-pushed
the
refactor_move_sl_creation_measurement_selection_to_single_unit
branch
from
dbd9d5e to
b8e7d2b
Compare
|
The corresponding necessary changes for athena (main/2024-12-15) can be found in gitlab-cern:main_support_for_new_extendOrBranchTrajectory_delegate |
Merge retrieval of source link ranges, measurement selection and track state creation into one unit which the CKF interacts with via a single delegate. The delegates for measurement selection track state creation and calibration are removed from the CKF options/extensions. The original building blocks (SourceLink accessor, measurement selector, and track state creator) can still be used, however require to setup an additional helper object which combines these independent components. The algorithmic code is unchanged.
goetzgaycken
force-pushed
the
refactor_move_sl_creation_measurement_selection_to_single_unit
branch
from
b8e7d2b to
12b42bc
Compare
|
Merge retrieval of source link ranges, measurement selection and
track state creation into one unit which the CKF interacts with
via a single delegate. The delegates for measurement selection
track state creation and calibration are removed from the CKF
options/extensions.
The original building blocks (SourceLink accessor, measurement selector,
and track state creator, or the combined measurement selector and track
state creator) can still be used, however require to setup an additional
helper object which combines these independent components. The
algorithmic code is unchanged.
As a prerequisite the track states are converted to bound states also on
empty sensitive surfaces.
This increases the computational effort slightly for empty, sensitive
surfaces, since the computation of bound states is slightly more
demanding than the computation of curvilinear states, but for complex
events like HL-LHC events the degradation was not measurable.
--- END COMMIT MESSAGE ---
Any further description goes here, @-mentions are ok here!
Summary by CodeRabbit
Release Notes
New Features
TrackStateCreatorfor enhanced track state management.CombinatorialKalmanFilterExtensionsfor improved filter behavior customization.Improvements
CombinatorialKalmanFilterOptions, reducing complexity.TrackFindingAlgorithmand related components.Bug Fixes
Documentation