feat: Allow propagator initialization to fail #4036
Conversation
WalkthroughChanged, many initialization methods have been, yes. Methods once returning Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Propagator
Client->>Propagator: makeState(options)
Note right of Propagator: State created without start parameters
Propagator-->>Client: state returned
Client->>Propagator: initialize(state, start)
alt Initialization successful
Propagator-->>Client: Result<void>::success()
else Initialization fails
Propagator-->>Client: Return error code
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
Component - Core
|
7e4563d comes with some measurable slowdowns
|
|
With the previous implementation no slowdowns are observed |
andiwand
force-pushed
the
allow-prop-init-failure
branch
from
76fd85c to
ef7010e
Compare
Disentangles the state creation and initialization for the steppers. This is analog to what is done in the navigator. The idea is that a state can be used for multiple propagations so its creation should be decoupled from the initialization to allow the reuse of the existing state. At the same time `resetState` is removed as it has no use anymore. `initialize` will reset and reinitialize the state. Pulled out of #4036 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Refactor** - Redesigned the state management workflow across propagation and tracking features to separate object creation from parameter initialization, resulting in a clearer, more streamlined API. - Removed legacy reset mechanisms in favor of explicit initialization methods that accommodate configurable parameters. - **Tests** - Updated unit tests to align with the new initialization process, ensuring improved consistency and robustness across propagation-related functionalities. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
andiwand
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
Core/include/Acts/TrackFitting/GlobalChiSquareFitter.hpp (1)
1254-1262: Duplicate error handling code for propagator initialization, refactor to reduce repetition you could.Each time the propagator state is initialized, similar error handling code you use. Consider extracting this into a helper function to improve maintainability and reduce duplication.
Apply this refactor to introduce a helper function:
+auto initializePropagatorState(auto& propagator, auto& state, auto& params) { + auto initRes = propagator.initialize(state, params); + if (!initRes.ok()) { + ACTS_ERROR("Propagation initialization failed: " << initRes.error()); + return initRes.error(); + } + return Result<void>::success(); +}Then update the initialization code:
- auto propagatorState = m_propagator.makeState(propagatorOptions); - - auto propagatorInitResult = - m_propagator.initialize(propagatorState, params); - if (!propagatorInitResult.ok()) { - ACTS_ERROR("Propagation initialization failed: " - << propagatorInitResult.error()); - return propagatorInitResult.error(); - } + auto propagatorState = m_propagator.makeState(propagatorOptions); + auto initResult = initializePropagatorState(m_propagator, propagatorState, params); + if (!initResult.ok()) { + return initResult.error(); + }Core/include/Acts/Propagator/NavigatorError.hpp (1)
16-20: Document the error codes, you must!For each error code, add documentation explaining when it occurs and how to handle it.
enum class NavigatorError { // ensure all values are non-zero - NotInsideExpectedVolume = 1, - NotOnExpectedSurface = 2, + /// Occurs when the navigator fails to find itself within the expected volume + NotInsideExpectedVolume = 1, + /// Occurs when the stepper is not positioned on the expected surface + NotOnExpectedSurface = 2, };Core/src/Propagator/NavigatorError.cpp (1)
24-31: More descriptive error messages, provide we must!Clear guidance to users, these messages should give. Help them understand and resolve the issues, they must.
switch (static_cast<NavigatorError>(c)) { case NavigatorError::NotInsideExpectedVolume: - return "We did not end up inside the volume."; + return "Navigation failed: Current position not within expected volume. Check volume boundaries and navigation parameters."; case NavigatorError::NotOnExpectedSurface: - return "Stepper not on surface"; + return "Navigation failed: Stepper position does not align with expected surface. Verify surface parameters and stepper configuration."; default: return "unknown"; }Examples/Algorithms/Propagation/include/ActsExamples/Propagation/PropagatorInterface.hpp (1)
93-98: Debug logging for initialization failures, add we should!When initialization fails, helpful it would be to log the error details.
auto state = m_propagator.makeState(options); auto resultInit = m_propagator.initialize(state, startParameters); if (!resultInit.ok()) { + ACTS_DEBUG("Propagator initialization failed: " << resultInit.error()); return resultInit.error(); }Tests/UnitTests/Core/Navigation/DetectorNavigatorTests.cpp (1)
100-103: Approve the separation of state creation and initialization, I do. Additional test cases, suggest I must!Good changes these are, separating state creation from initialization. But test more error cases, we should.
Add test cases for these scenarios, you should:
// Test initialization with invalid parameters auto state = propagator.makeState(options); auto invalidStart = Acts::CurvilinearTrackParameters(); // Invalid parameters BOOST_CHECK_THROW(propagator.initialize(state, invalidStart), std::invalid_argument);Core/include/Acts/Propagator/TryAllNavigator.hpp (1)
157-202: Return Result, wisdom it shows. Approve this change, I do!Better error handling, this brings. When initialization fails, know it we will.
Consider adding error codes for specific failure cases:
if (state.currentVolume == nullptr) { return Result<void>::failure(NavigatorError::StartVolumeNotFound); }Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp (1)
872-882: Handle initialization errors with care, we must. Approve these changes, I do!Proper error handling and logging, this brings. When initialization fails, inform the user we must.
Consider adding more detailed error messages:
if (!initResult.ok()) { ACTS_ERROR("Propagation initialization failed for track with parameters: " << initialParameters.parameters().transpose() << "\nError: " << initResult.error()); return initResult.error(); }Core/include/Acts/TrackFitting/KalmanFitter.hpp (1)
1236-1244: In the ways of initialization, consistent we must be. Approve these changes, I do!Handle errors with grace and wisdom, this implementation does. When initialization fails, clear message it provides.
Consider adding performance logging:
ACTS_DEBUG("Initialization time: " << std::chrono::duration_cast<std::chrono::microseconds>( std::chrono::high_resolution_clock::now() - startTime).count() << "us");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
Core/include/Acts/Navigation/DetectorNavigator.hpp(2 hunks)Core/include/Acts/Propagator/DirectNavigator.hpp(3 hunks)Core/include/Acts/Propagator/Navigator.hpp(4 hunks)Core/include/Acts/Propagator/NavigatorError.hpp(1 hunks)Core/include/Acts/Propagator/Propagator.hpp(1 hunks)Core/include/Acts/Propagator/Propagator.ipp(5 hunks)Core/include/Acts/Propagator/TryAllNavigator.hpp(4 hunks)Core/include/Acts/Propagator/VoidNavigator.hpp(2 hunks)Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp(2 hunks)Core/include/Acts/TrackFitting/GaussianSumFitter.hpp(2 hunks)Core/include/Acts/TrackFitting/GlobalChiSquareFitter.hpp(4 hunks)Core/include/Acts/TrackFitting/KalmanFitter.hpp(2 hunks)Core/src/Propagator/CMakeLists.txt(1 hunks)Core/src/Propagator/NavigatorError.cpp(1 hunks)Examples/Algorithms/Propagation/include/ActsExamples/Propagation/PropagatorInterface.hpp(1 hunks)Tests/Benchmarks/StepperBenchmarkCommons.hpp(1 hunks)Tests/UnitTests/Core/Navigation/DetectorNavigatorTests.cpp(5 hunks)
🧰 Additional context used
📓 Learnings (2)
Core/include/Acts/Propagator/VoidNavigator.hpp (1)
Learnt from: paulgessinger
PR: acts-project/acts#3449
File: Core/include/Acts/Propagator/Navigator.hpp:153-153
Timestamp: 2024-12-13T08:31:09.422Z
Learning: The developer prefers to avoid using `-1` as a sentinel value in indices and considers using `std::optional<unsigned int>` as an alternative.
Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp (2)
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: Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp:460-467
Timestamp: 2024-12-05T10:58:53.433Z
Learning: In the `Acts` project, within `Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp`, the cache `result.trackStateCandidates` is expected to be empty before calling `createTrackStates` in the function `filter` of `CombinatorialKalmanFilter`.
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: merge-sentinel
- GitHub Check: CI Bridge / lcg_106a: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc14]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc13]
- GitHub Check: CI Bridge / lcg_105: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- GitHub Check: CI Bridge / linux_ubuntu_2204_clang
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: CI Bridge / build_gnn_tensorrt
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / clang_tidy
🔇 Additional comments (33)
Core/include/Acts/Propagator/Propagator.ipp (12)
31-32: Enhance error handling, this change does.Changing the return type to
Acts::Result<void>improves error handling consistency throughout the propagator. Good practice, it is.
199-205: Initialize state before propagation, wisely you have.By calling
initializeand handling potential errors, ensure proper state setup before propagation begins, you do.
226-229: Simplify state creation, you have.By removing redundant parameters from
makeState, clarity and simplicity, you achieve.
231-234: Handle initialization errors, correctly you do.Checking the result of
initializeand returning errors appropriately, commendable this is.
243-246: Ensure extended options are properly constructed, you must.Creating
eOptionswith care prevents potential issues with state initialization. Verify all necessary fields are correctly set, you should.
253-260: Extend actor list with path aborter, correctly you have.Appending
pathAborterto the actor list enhances control over propagation limits. Good, this is.
272-273: Initialize internal propagator state, properly you do.Setting up
statewith extended options and stepper state ensures readiness for propagation.
315-342: Introducinginitializemethod, improve state management it does.By initializing the stepper and navigator states, ensure consistent starting conditions, you do. Handling errors from navigator initialization, prudent this is.
330-335: Handle navigator initialization errors, carefully you must.Returning the error if navigator fails to initialize ensures issues are not ignored. Vigilant you are.
337-340: Set up loop protection, you have.Applying
detail::setupLoopProtectionresets internal path limits, preventing infinite loops. Wise precaution this is.
373-375: Check for curvilinear state preparation failure, you must.If information to compute curvilinear state is incomplete, return the propagation error you should. Ensures correctness, this does.
473-475: Handle unsuccessful propagation, properly you do.Returning the propagation error when propagation fails, essential it is. Maintain reliability, this will.
Core/include/Acts/TrackFitting/GaussianSumFitter.hpp (6)
267-268: Initialization of propagator state, properly done it is.
269-277: Type deductions for error handling, correctly implemented they are.
278-281: State initialization with error handling, well performed it is.
338-341: Backward propagator state creation, correctly done it is.
342-350: Type deductions for backward error handling, appropriately set they are.
351-354: State initialization for backward pass, correctly with error handling performed it is.Core/include/Acts/Propagator/Navigator.hpp (2)
16-16: Including NavigatorError.hpp, necessary it is.
264-365: Enhanced error handling in initialize method, well implemented it is.The method now returns
Result<void>, allowing for proper error checks when the navigator is initialized. The added conditions ensure that propagation does not proceed when the initial state is invalid.Core/include/Acts/TrackFitting/GlobalChiSquareFitter.hpp (2)
1427-1435: Duplicate error handling code for propagator initialization, refactor to reduce repetition you could.As previously noted in lines 1254-1262, the same error handling code you have here. Apply the suggested refactoring to reduce code duplication.
1583-1591: Duplicate error handling code for propagator initialization, refactor to reduce repetition you could.Again, the initialization and error handling code is repeated here. Refactoring as suggested before, consider you should.
Core/include/Acts/Propagator/NavigatorError.hpp (1)
22-22: Well structured, this error handling is!Integration with std::error_code system, a wise choice it is. Standard error handling mechanisms, it leverages.
Core/include/Acts/Propagator/VoidNavigator.hpp (1)
66-70: Wise changes, these are!Consistent with the new error handling system, this change is. Always success it returns, as a void navigator should.
Examples/Algorithms/Propagation/include/ActsExamples/Propagation/PropagatorInterface.hpp (1)
101-104: Well handled, these errors are!Consistent error propagation throughout the execution path, maintained it is. Early returns on failures, prevent further issues they do.
Tests/Benchmarks/StepperBenchmarkCommons.hpp (1)
120-121:⚠️ Potential issuePerformance impact, this change has. Investigate we must.
Split state creation and initialization into separate steps, we have. Performance slowdown in benchmarks, this causes. Consider combining these operations or optimizing initialization path, we should.
Run this benchmark to measure the impact:
Core/include/Acts/Propagator/DirectNavigator.hpp (1)
175-177: Approve this change, I do. Better error handling, it brings.Return Result instead of void, we now do. Consistent with other navigator classes, this makes us. Improved error handling capabilities, we gain.
Also applies to: 219-219
Core/include/Acts/Propagator/Propagator.hpp (1)
311-325: Well documented and consistent, this new method is.Clear initialization point it provides. With navigator changes, it aligns well. Documentation, thorough it is.
Tests/UnitTests/Core/Navigation/DetectorNavigatorTests.cpp (2)
120-123: Consistent with the Force, this change is!Wise separation of concerns, I sense. Explicit stepper initialization, better control it provides.
154-157: Handle errors gracefully, we must. Approve this change, I do!Test the boundaries of our universe, this does. When initialization fails, catch it we must.
Core/include/Acts/Propagator/TryAllNavigator.hpp (2)
289-302: Chain of error handling, strong it grows. Approve this pattern, I do!Properly handle base class errors, it does. Through the chain of initialization, flow they must.
614-629: Consistency in the Force, I sense. Approve this implementation, I must!Like its siblings, handle errors it does. Through the navigator hierarchy, consistency flows.
Core/src/Propagator/CMakeLists.txt (1)
6-6: Strong addition, this is!
The inclusion ofNavigatorError.cppin the target sources aligns perfectly with the new error handling strategy—robust and clear, it is. Ensure that the implementation inNavigatorError.cppfully embraces our updated error reporting and propagator initialization logic, so the Force of stability remains with us.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
Core/include/Acts/Navigation/DetectorNavigator.hpp(2 hunks)Core/include/Acts/Propagator/DirectNavigator.hpp(3 hunks)Core/include/Acts/Propagator/Navigator.hpp(4 hunks)Core/include/Acts/Propagator/Propagator.hpp(1 hunks)Core/include/Acts/Propagator/TryAllNavigator.hpp(4 hunks)Core/include/Acts/Propagator/VoidNavigator.hpp(2 hunks)Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp(2 hunks)Core/include/Acts/TrackFitting/KalmanFitter.hpp(3 hunks)Tests/UnitTests/Core/Navigation/DetectorNavigatorTests.cpp(5 hunks)Tests/UnitTests/Core/Propagator/NavigatorTests.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- Core/include/Acts/Propagator/VoidNavigator.hpp
- Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp
- Tests/UnitTests/Core/Navigation/DetectorNavigatorTests.cpp
- Core/include/Acts/Navigation/DetectorNavigator.hpp
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: merge-sentinel
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc14]
- GitHub Check: CI Bridge / lcg_106a: [alma9, clang16]
- GitHub Check: unused_files
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc13]
- GitHub Check: CI Bridge / lcg_105: [alma9, clang16]
- GitHub Check: missing_includes
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- GitHub Check: CI Bridge / build_gnn_tensorrt
- GitHub Check: macos
- GitHub Check: CI Bridge / linux_ubuntu_2204_clang
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, clang++)
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: linux_ubuntu
- GitHub Check: build_debug
🔇 Additional comments (15)
Tests/UnitTests/Core/Propagator/NavigatorTests.cpp (2)
193-195: Approve the addition of initialization result checks, I do.Consistent with the PR objectives, these changes are. Verify the success of navigator initialization, they do. A good pattern of error handling, this establishes.
Also applies to: 206-208, 216-218, 226-228, 260-262
193-195: Verify error messages in failure cases, we must.When initialization fails, error messages should be checked. Important for debugging and error handling, this is.
Also applies to: 206-208, 216-218, 226-228, 260-262
Core/include/Acts/Propagator/DirectNavigator.hpp (2)
23-23: Hmm, wise addition of Result.hpp, this is.For error handling capabilities, essential this header is.
175-177: Return Result, the initialize function now does. Mandatory handling of errors, this ensures.The
nodiscardattribute, prevent silent error ignoring it does. Return success explicitly, we now do.Hmm, past review comments about
nodiscard, I see. Agree with them, I do.Also applies to: 219-219
Core/include/Acts/Propagator/Propagator.hpp (2)
311-325: Public initialize function, added it is. Error handling, improved it has.Clear documentation and return type, essential they are. The
nodiscardattribute, prevent silent error ignoring it does.
288-290: Hmm, simpler interface this brings, but performance impact we must watch.Separated state creation from parameter initialization, we have. Cleaner design this is, yes. But watch performance impact, we must.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for performance impact by analyzing call patterns # Search for chained initialization calls that might indicate performance overhead rg -n "makeState.*initialize" .Length of output: 33
🏁 Script executed:
#!/bin/bash # Attempting a more comprehensive search for any chaining of # makeState and initialize calls that might indicate performance overhead. # This script displays 5 lines of context after each makeState occurrence. rg -n -A5 "makeState\(" .Length of output: 62831
Performance impact appears manageable, yes.
Separated state creation from initialization, a cleaner design it is. A search across the codebase reveals the familiar chaining of makeState followed by initialize is common practice, and no extra overhead obvious it appears. Still, wise to monitor profiling in production if any anomalies arise.Core/include/Acts/Propagator/Navigator.hpp (3)
16-16: NavigatorError.hpp, added it is. Error types, define it does.For structured error handling, essential this header is.
333-341: Volume check with error handling, improved it is.Replace assertion with conditional check, we do. Clear error message and specific error type, provide we do.
351-361: Surface check with error handling, improved it is.Replace assertion with conditional check, we do. Clear error message and specific error type, provide we do.
Core/include/Acts/Propagator/TryAllNavigator.hpp (3)
157-159: Base class initialize, return Result it now does.Consistent with other navigator classes, this change is.
Also applies to: 202-202
292-296: Error propagation from base class, added it is.Check base class result and propagate error, we now do. Proper error handling chain, this creates.
617-621: Same error handling pattern in TryAllOverstepNavigator, implemented it is.Consistency in error handling across navigator classes, maintained it is.
Core/include/Acts/TrackFitting/KalmanFitter.hpp (3)
560-567: Improved error handling in navigation initialization, hmm, yes!Direct setting of navigation options and proper error propagation from navigator initialization, I see. Wise changes these are!
1049-1054: Consistent error handling in finalize method, implemented well it is!Proper error propagation from navigator initialization, maintained here too. Consistency in error handling, brings balance to the Force, it does!
1242-1250: Complete error handling chain in fit implementation, established it is!From propagator to navigator, a complete chain of error handling flows. Useful error messages for debugging, provided they are. Strong with the Force, this implementation is!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Tests/Benchmarks/StepperBenchmarkCommons.hpp (1)
120-125: Optimization suggestions, have I.To mitigate performance impact, consider these changes we should:
- Cache the initialization result for reuse, when multiple propagations with same parameters occur
- Move error checking to debug builds only, we could
Apply this optimization, you might:
+ // Cache initialization result for reuse + std::optional<Result<void>> cachedInitResult; auto state = propagator.makeState(options); - auto initRes = propagator.initialize(state, pars); + if (!cachedInitResult) { + cachedInitResult = propagator.initialize(state, pars); + } + auto& initRes = *cachedInitResult; if (!initRes.ok()) { ACTS_ERROR("initialization failed: " << initRes.error()); return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Tests/Benchmarks/StepperBenchmarkCommons.hpp(1 hunks)Tests/UnitTests/Core/Navigation/DetectorNavigatorTests.cpp(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Tests/UnitTests/Core/Navigation/DetectorNavigatorTests.cpp
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: merge-sentinel
🔇 Additional comments (1)
Tests/Benchmarks/StepperBenchmarkCommons.hpp (1)
120-125: Split initialization from state creation, hmm. Wise this change is.Separated the concerns have been, yes. State creation and initialization, two distinct steps they now are. Error handling improved it has been.
But careful we must be. Reported slowdowns in performance, the dark side they are. Measure we must.
Run this benchmark comparison we shall:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Core/include/Acts/Utilities/Result.hpp (1)
421-433: Well-implemented error checking, this is!Proper error handling with std::optional, you have achieved. Clear error messages for std::error_code, you provide. Consistent with non-void Result class implementation, it remains.
A suggestion for documentation, I have:
void checkValueAccess() const { + // Throws std::runtime_error if the Result contains an error + // For std::error_code, includes detailed error information in the message if (m_opt.has_value()) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Core/include/Acts/Utilities/Result.hpp(1 hunks)Tests/UnitTests/Core/Navigation/DetectorNavigatorTests.cpp(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Tests/UnitTests/Core/Navigation/DetectorNavigatorTests.cpp
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: merge-sentinel
- GitHub Check: macos
🔇 Additional comments (1)
Core/include/Acts/Utilities/Result.hpp (1)
416-416: Approve the addition of value() method, I do!Symmetry with non-void Result class, this brings. Consistent API across Result variants, it provides.
|
Didn't measure any significant change in CPU timings for fatras or CKF |
|
acts-project-service
added
the
Fails Athena tests
This change allows the propagator initialization to fail which can happen if for example pre conditions of the navigator are not met. To achieve this the
initializefunction in the propagator was elevated to be part of the public interface.I think it would make sense to use this
initializefunction to also initialize the stepper and to remove that frommakeState. That way the user of the propagator can reuse the propagation state afterinitialize.Breakdown of the changes
Navigator::initializePropagator::initializeto public APIPropagator::initializeinPropagator::makeStatenamespace Acts {inPropagator.ippSummary by CodeRabbit