Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat!: MultiTrajectory backends #1262

Merged
merged 65 commits into from
Aug 3, 2022

Conversation

paulgessinger
Copy link
Member

@paulgessinger paulgessinger commented May 20, 2022

This PR decouples the interface of MultiTrajectory from the storage backend. The default storage backend is practically identical to the previous implementation, but it's now behind the interface. This change is intended to allow seamless integration with experiment specific EDM backends, like ATLAS' xAOD.

See these slides for some more information.

BREAKING CHANGE: The template parameters of CombinatorialKalmanFilter, KalmanFitter and GaussianSumFitter changes to

template <typename propagator_t, typename traj_t>
class CombinatorialKalmanFilter;
template <typename propagator_t, typename traj_t>
class KalmanFitter;
template <typename propagator_t, typename traj_t,
          typename bethe_heitler_approx_t = detail::BetheHeitlerApprox<6, 5>>
struct GaussianSumFitter;

Related classes (options, extensions, result) also change, gaining a traj_t template parameter. This parameter is the type of the memory backend.

@paulgessinger paulgessinger added the 🚧 WIP Work-in-progress label May 20, 2022
@paulgessinger paulgessinger added this to the next milestone May 20, 2022
@codecov
Copy link

codecov bot commented May 20, 2022

Codecov Report

Merging #1262 (96894f6) into main (e72db5d) will increase coverage by 0.36%.
The diff coverage is 58.18%.

@@            Coverage Diff             @@
##             main    #1262      +/-   ##
==========================================
+ Coverage   47.46%   47.83%   +0.36%     
==========================================
  Files         375      380       +5     
  Lines       19842    20149     +307     
  Branches     9293     9371      +78     
==========================================
+ Hits         9418     9638     +220     
- Misses       4045     4063      +18     
- Partials     6379     6448      +69     
Impacted Files Coverage Δ
.../include/Acts/EventData/MultiTrajectoryHelpers.hpp 50.00% <ø> (ø)
Core/include/Acts/EventData/TrackStatePropMask.hpp 100.00% <ø> (ø)
...re/include/Acts/TrackFitting/GainMatrixUpdater.hpp 10.00% <0.00%> (ø)
...e/Acts/TrackFitting/detail/KalmanUpdateHelpers.hpp 15.25% <0.00%> (-6.78%) ⬇️
Core/src/EventData/TrackStatePropMask.cpp 0.00% <ø> (ø)
Core/src/TrackFitting/GainMatrixSmoother.cpp 7.69% <7.69%> (-2.95%) ⬇️
Core/src/TrackFitting/GainMatrixUpdater.cpp 20.00% <16.66%> (+8.88%) ⬆️
Core/include/Acts/TrackFitting/detail/GsfUtils.hpp 39.24% <24.32%> (-13.15%) ⬇️
Core/src/TrackFinding/MeasurementSelector.cpp 33.33% <33.33%> (+0.90%) ⬆️
.../include/Acts/TrackFinding/MeasurementSelector.hpp 38.09% <36.06%> (-61.91%) ⬇️
... and 27 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@paulgessinger
Copy link
Member Author

Following @timadye's suggestions, I'll try to replace void* with std::any to get at least type checking.

Copy link
Contributor

@tboldagh tboldagh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few comments (more questions realy).
One thing that I missed to spot, and was curious was creation of new track states
in CKF, KF or GSF and how this changes with backend?

Core/include/Acts/Utilities/HashedString.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Utilities/HashedString.hpp Show resolved Hide resolved
Core/include/Acts/EventData/MultiTrajectory.hpp Outdated Show resolved Hide resolved
Core/include/Acts/EventData/MultiTrajectory.hpp Outdated Show resolved Hide resolved
Core/include/Acts/EventData/MultiTrajectory.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Propagator/Propagator.ipp Outdated Show resolved Hide resolved
Core/include/Acts/Propagator/Propagator.ipp Outdated Show resolved Hide resolved
@paulgessinger
Copy link
Member Author

FYI I'm working on a follow-up update that uses CRTP instead of virtual functions.

@paulgessinger paulgessinger removed the 🚧 WIP Work-in-progress label Jul 5, 2022
@Corentin-Allaire Corentin-Allaire self-requested a review July 5, 2022 15:16
Copy link
Contributor

@andiwand andiwand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just took a quick look - will follow up later

Core/include/Acts/TrackFitting/GainMatrixSmoother.hpp Outdated Show resolved Hide resolved
Core/include/Acts/TrackFitting/GainMatrixSmoother.hpp Outdated Show resolved Hide resolved
Core/include/Acts/TrackFitting/GainMatrixSmoother.hpp Outdated Show resolved Hide resolved
Copy link
Contributor

@andiwand andiwand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, I could not provide valuable feedback but highlighted some todo / fixme / commented code

Core/src/TrackFitting/GainMatrixSmoother.cpp Outdated Show resolved Hide resolved
Core/include/Acts/TrackFitting/detail/GsfUtils.hpp Outdated Show resolved Hide resolved
Core/include/Acts/TrackFitting/detail/GsfUtils.hpp Outdated Show resolved Hide resolved
Core/include/Acts/EventData/VectorMultiTrajectory.hpp Outdated Show resolved Hide resolved
Core/include/Acts/TrackFitting/GainMatrixSmoother.hpp Outdated Show resolved Hide resolved
@paulgessinger
Copy link
Member Author

Thanks @andiwand, mostly things I missed when cleaning up. Should all be addressed with the most recent commit.

@paulgessinger paulgessinger changed the title feat: MultiTrajectory backends feat!: MultiTrajectory backends Jul 27, 2022
@paulgessinger
Copy link
Member Author

paulgessinger commented Aug 3, 2022

I updated this with a merge commit, but the approval was dismissed. @tboldagh or @andiwand can you reapprove?

@kodiakhq kodiakhq bot merged commit 5cbcbf0 into acts-project:main Aug 3, 2022
@paulgessinger paulgessinger deleted the feat/mtj-backends-pivot branch August 3, 2022 15:46
@paulgessinger paulgessinger modified the milestones: next, v20.0.0 Aug 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants