refactor!: Particle hypothesis for track parameters #2181

andiwand · 2023-06-06T17:32:06Z

In this PR I make the particle hypothesis part of the track parameters. This allows us to consistently use the same hypothesis across various parts of our code. Before that the particle hypothesis was a bit scattered in the propagator and stepper.

TrackParameters gain ParticleHypothesis
ParticleHypothesis is a new class which holds a charge type, the mass and the absolute PDG value
- This interface is consistently used to get the charge or momenta from qOverP or vice versa
Single*TrackParameters are renamed and typedefed in TrackParameters.hpp
- The templated TrackParameters implementations are now called Generic*TrackParameters
I aligned absoluteMomentum, direction, momentum in the steppers, truth particles and track parameters

alternative to #2178

andiwand · 2023-06-18T11:09:09Z

2023-06-18T10:53:09.4149100Z The following tests FAILED:
2023-06-18T10:53:09.4149417Z 	 27 - Charge (Failed)
2023-06-18T10:53:09.4149733Z 	 28 - CurvilinearTrackParameters (Failed)
2023-06-18T10:53:09.4150015Z 	 37 - Track (Failed)
2023-06-18T10:53:09.4150269Z 	 91 - Interactions (Failed)
2023-06-18T10:53:09.4150546Z 	122 - EingenStepper (Failed)
2023-06-18T10:53:09.4150853Z 	155 - CombinatorialKalmanFilter (Failed)
2023-06-18T10:53:09.4151196Z 	158 - KalmanFitter (Failed)
2023-06-18T10:53:09.4154127Z 	159 - Gsf (Failed)
2023-06-18T10:53:09.4154388Z 	162 - Chi2Fitter (Failed)
2023-06-18T10:53:09.4154675Z 	200 - ImpactPointEstimator (Failed)
2023-06-18T10:53:09.4158101Z Errors while running CTest
2023-06-18T10:53:09.4158605Z 	213 - EventDataView3D (Failed)
2023-06-18T10:53:09.4161257Z 	265 - ConvertTrackEDM4hep (Failed)
2023-06-18T10:53:09.4161737Z 	266 - Alignment (Failed)

codecov · 2023-06-19T16:28:20Z

Codecov Report

Merging #2181 (d42a8b5) into main (60bb598) will increase coverage by 0.25%.
The diff coverage is 63.65%.

❗ Current head d42a8b5 differs from pull request most recent head e7a7c2e. Consider uploading reports for the commit e7a7c2e to get more accurate results

@@            Coverage Diff             @@
##             main    #2181      +/-   ##
==========================================
+ Coverage   49.26%   49.52%   +0.25%     
==========================================
  Files         449      447       -2     
  Lines       25403    25293     -110     
  Branches    11723    11620     -103     
==========================================
+ Hits        12516    12527      +11     
+ Misses       4554     4513      -41     
+ Partials     8333     8253      -80

Impacted Files	Coverage Δ
Core/include/Acts/EventData/TrackContainer.hpp	`89.04% <ø> (ø)`
Core/include/Acts/EventData/TrackProxy.hpp	`78.70% <ø> (ø)`
...re/include/Acts/EventData/VectorTrackContainer.hpp	`49.03% <0.00%> (-1.97%)`	⬇️
Core/include/Acts/Navigation/NextNavigator.hpp	`47.65% <0.00%> (ø)`
...lude/Acts/Propagator/DenseEnvironmentExtension.hpp	`100.00% <ø> (ø)`
...ore/include/Acts/Propagator/MaterialInteractor.hpp	`39.72% <0.00%> (ø)`
.../include/Acts/Propagator/MultiEigenStepperLoop.ipp	`40.62% <0.00%> (+1.56%)`	⬆️
Core/include/Acts/Propagator/Propagator.hpp	`92.68% <ø> (-0.66%)`	⬇️
Core/include/Acts/Propagator/StandardAborters.hpp	`71.73% <ø> (+3.11%)`	⬆️
.../include/Acts/Propagator/detail/LoopProtection.hpp	`42.10% <0.00%> (ø)`
... and 47 more

... and 11 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

github-actions · 2023-06-19T18:29:39Z

📊 Physics performance monitoring for `e7a7c2e`

Summary
Full report
Seeding: seeded, truth estimated, orthogonal
CKF: seeded, truth smeared, truth estimated, orthogonal
IVF: seeded, truth smeared, truth estimated, orthogonal
AMVF: seeded, truth smeared, truth estimated, orthogonal
Ambiguity resolution: seeded, orthogonal
Truth tracking
Truth tracking (GSF)

Vertexing

Vertexing vs. mu

IVF seeded

IVF truth_smeared

IVF truth_estimated

IVF orthogonal

AMVF seeded

AMVF truth_smeared

AMVF truth_estimated

AMVF orthogonal

Seeding

Seeding seeded

Seeding truth_estimated

Seeding orthogonal

CKF

CKF seeded

CKF truth_smeared

CKF truth_estimated

CKF orthogonal

Ambiguity resolution

seeded

Truth tracking (Kalman Filter)

Truth tracking

Truth tracking (GSF)

Truth tracking

benjaminhuth

I took a first round, didn't found anything critical, just a few comments! Great work!
I think I'll do a second round once the CI is green

Core/include/Acts/EventData/detail/Charge.hpp

Core/include/Acts/EventData/detail/BoundTrackParameters.hpp

Core/include/Acts/EventData/detail/CurvilinearTrackParameters.hpp

Core/include/Acts/EventData/detail/ParticleHypothesis.hpp

Core/include/Acts/Utilities/detail/periodic.hpp

paulgessinger

Ok, I looked through this until the point where GitHub stops auto-showing the diff anymore because too many files are touched.
I think in the future, it would be nice to separate the actual changes from the extra ones, so the PR stays reviewable.

paulgessinger · 2023-06-21T11:51:45Z

Core/include/Acts/EventData/detail/ParticleHypothesis.hpp

+    return ParticleHypothesis<ChargeType>(pion().absPdg(), pion().mass(), absQ);
+  }
+
+  constexpr ParticleHypothesis(PdgParticle absPdg, float mass,


Should we make this private?

not sure - I feel like a user might want to add a particle we do not provide without writing their own particle hypothesis class and modified track parameters

Core/include/Acts/EventData/detail/ParticleHypothesis.hpp

paulgessinger · 2023-06-21T12:33:45Z

Core/include/Acts/Propagator/EigenStepper.hpp

+  /// Momentum accessor
+  ///
+  /// @param state [in] The stepping state (thread-local cache)
+  Vector3 momentum(const State& state) const {


So momentum becomes absolute momentum and momentum is now the absolute momentum vector?

yes I tried to align this with the methods we have in the track parameters

Core/include/Acts/EventData/detail/Charge.hpp

Examples/Algorithms/Propagation/include/ActsExamples/Propagation/PropagatorInterface.hpp

Core/src/EventData/NeutralTrackParameters.cpp

Examples/Algorithms/TrackFitting/src/RefittingAlgorithm.cpp

Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/TrackParameterSelector.cpp

Examples/Io/EDM4hep/src/EDM4hepUtil.cpp

paulgessinger · 2023-06-23T06:32:44Z

I think before we can consider merging, we should discuss it in the meeting.

Also, I've been thinking about the downstream implications and support. Could we actually separate just the hypothesis changes from everything else?

I'm thinking that a lot of users will have a hard time adjusting to the particle hypothesis change, so if we cut a release right before merging we'd be able to backport changes for a while.

I would like to move `ParticleData` to `Core` so we have one central spot to get mass and charge for a PDG particle identifier. This can the be utilized in #2181

…e interface (#2250) Currently the `Surface` classes have `momentum` parameters which is misleading IMO as we do not care about the magnitude of that input. Here I try to improve things by renaming the occasions with `direction`. I pulled these changes out of #2181.

With this PR I try to align the naming in the Stepper interface to `qOverP` and `absoluteMomentum` which is also my goal for the track parameters. I pulled these changes out of #2181.

I would like to move `ParticleData` to `Core` so we have one central spot to get mass and charge for a PDG particle identifier. This can the be utilized in acts-project#2181

I noticed that we are quite inconsistent with the use of `direction` and `unitDirection`. Personally I would opt for `direction` and have it as a unit vector implicitly. Also, I changed `qop` to `qOverP` which I will also align for the Stepper and TrackParameter interfaces in follow-up PRs I pulled these changes out of #2181.

paulgessinger · 2023-07-24T08:20:00Z

We're ready to go for v28 now. Can you update the PR so we can merge it?

andiwand · 2023-07-24T08:46:49Z

We're ready to go for v28 now. Can you update the PR so we can merge it?

I think this one is ready to be closed. I split it off into multiple PRs the ones we can merge next are

…s-project#2251) I noticed that we are quite inconsistent with the use of `direction` and `unitDirection`. Personally I would opt for `direction` and have it as a unit vector implicitly. Also, I changed `qop` to `qOverP` which I will also align for the Stepper and TrackParameter interfaces in follow-up PRs I pulled these changes out of acts-project#2181.

…s` (#2269) I am unhappy with the name `Single*TrackParameters` as it could refer to singly charged or single track. The track actually supports different kinds of charge hypothesis so I thought `Generic*TrackParameter` might fit better. I also merged `NeutralTrackParameters` into `TrackParameters` and removed the external template instantiation as it could be a performance hit because it disables inlining. Pulled these changes out of #2181

In this PR I introduce the concept of a particle hypothesis which will later be used by the track parameters. I also refactored the Charge interface a bit. This is the foundation for upcoming refactor PRs for the particle hypothesis. Pulled out from #2181. Co-authored-by: Paul Gessinger <[email protected]>

In this PR I make the particle hypothesis part of the track parameters. This allows us to consistently use the same hypothesis across various parts of our code. Before that the particle hypothesis was a bit scattered in the propagator and stepper. replaces #2181 after splitting it up blocked by - #2366 - #2396 - #2398 - #2397

In this PR I make the particle hypothesis part of the track parameters. This allows us to consistently use the same hypothesis across various parts of our code. Before that the particle hypothesis was a bit scattered in the propagator and stepper. replaces acts-project#2181 after splitting it up blocked by - acts-project#2366 - acts-project#2396 - acts-project#2398 - acts-project#2397

andiwand added Component - Core Affects the Core module 🚧 WIP Work-in-progress labels Jun 6, 2023

andiwand added this to the next milestone Jun 6, 2023

andiwand mentioned this pull request Jun 7, 2023

refactor!: Remove momentum and charge from track parameters #2178

Closed

github-actions bot added Component - Examples Affects the Examples module Component - Fatras Affects the Fatras module Component - Plugins Affects one or more Plugins Event Data Model Seeding SP formation Track Finding Track Fitting Vertexing labels Jun 13, 2023

github-actions bot added Infrastructure Changes to build tools, continous integration, ... and removed Infrastructure Changes to build tools, continous integration, ... labels Jun 19, 2023

benjaminhuth reviewed Jun 21, 2023

View reviewed changes

andiwand commented Jun 21, 2023

View reviewed changes

Core/include/Acts/Utilities/detail/periodic.hpp Outdated Show resolved Hide resolved

paulgessinger reviewed Jun 21, 2023

View reviewed changes

andiwand mentioned this pull request Jun 22, 2023

refactor: Move ParticleData from Fatras to Core #2233

Merged

paulgessinger added the Needs Discussion label Jun 23, 2023

draft

e7a7c2e

andiwand force-pushed the refactor-trackparam-particle-hypothesis branch from abd20dd to e7a7c2e Compare June 29, 2023 08:27

This was referenced Jun 29, 2023

refactor: direction and qOverP for Fatras particle interface #2251

Merged

refactor: Explicit direction parameter instead of momentum for Surface interface #2250

Merged

refactor: qOverP and absoluteMomentum for Stepper interface #2253

Merged

andiwand mentioned this pull request Jun 30, 2023

feat: Introduce particle hypothesis #2254

Merged

andiwand mentioned this pull request Jul 4, 2023

refactor!: Rename Single*TrackParameters to Generic*TrackParameters #2269

Merged

paulgessinger added the Breaking change This change breaks backwards compatibility label Jul 5, 2023

paulgessinger modified the milestones: next, v28.0.0 Jul 5, 2023

andiwand removed the Needs Discussion label Jul 6, 2023

andiwand marked this pull request as draft July 6, 2023 12:08

andiwand closed this Jul 24, 2023

andiwand mentioned this pull request Aug 3, 2023

refactor!: Particle hypothesis for track parameters #2341

Merged

andiwand deleted the refactor-trackparam-particle-hypothesis branch September 18, 2023 16:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor!: Particle hypothesis for track parameters #2181

refactor!: Particle hypothesis for track parameters #2181

andiwand commented Jun 6, 2023 •

edited

Loading

andiwand commented Jun 18, 2023

codecov bot commented Jun 19, 2023 •

edited

Loading

github-actions bot commented Jun 19, 2023 •

edited

Loading

benjaminhuth left a comment

paulgessinger left a comment

paulgessinger Jun 21, 2023

andiwand Jun 21, 2023

paulgessinger Jun 21, 2023

andiwand Jun 21, 2023

paulgessinger commented Jun 23, 2023

paulgessinger commented Jul 24, 2023

andiwand commented Jul 24, 2023

refactor!: Particle hypothesis for track parameters #2181

refactor!: Particle hypothesis for track parameters #2181

Conversation

andiwand commented Jun 6, 2023 • edited Loading

andiwand commented Jun 18, 2023

codecov bot commented Jun 19, 2023 • edited Loading

Codecov Report

github-actions bot commented Jun 19, 2023 • edited Loading

📊 Physics performance monitoring for e7a7c2e

Vertexing

Seeding

CKF

Ambiguity resolution

Truth tracking (Kalman Filter)

Truth tracking (GSF)

benjaminhuth left a comment

Choose a reason for hiding this comment

paulgessinger left a comment

Choose a reason for hiding this comment

paulgessinger Jun 21, 2023

Choose a reason for hiding this comment

andiwand Jun 21, 2023

Choose a reason for hiding this comment

paulgessinger Jun 21, 2023

Choose a reason for hiding this comment

andiwand Jun 21, 2023

Choose a reason for hiding this comment

paulgessinger commented Jun 23, 2023

paulgessinger commented Jul 24, 2023

andiwand commented Jul 24, 2023

andiwand commented Jun 6, 2023 •

edited

Loading

codecov bot commented Jun 19, 2023 •

edited

Loading

github-actions bot commented Jun 19, 2023 •

edited

Loading

📊 Physics performance monitoring for `e7a7c2e`