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

refactor: Propagator deduce parameter type from stepper #2413

Merged

Conversation

benjaminhuth
Copy link
Member

@benjaminhuth benjaminhuth commented Aug 31, 2023

The main change is that the Acts::Propagator now deduces the parameters type (bound and curvilinear) from the stepper.

This allows the MultiStepper to return multi-component-parameters, and makes the whole structure of the GSF more straight.

Related changes to GSF infrastructure:

  • Add multi component curvilinear track parameters
  • Remove workaround to get final parameters (this leads to changed output, because with the workaround there was one additional transport-to-bound, which causes floating-point-differences)

@benjaminhuth benjaminhuth added this to the next milestone Aug 31, 2023
@github-actions github-actions bot added Component - Core Affects the Core module Component - Examples Affects the Examples module Event Data Model Track Fitting labels Aug 31, 2023
@benjaminhuth
Copy link
Member Author

benjaminhuth commented Aug 31, 2023

@andiwand @paulgessinger Let me know what you think about the change.

I think this is NOT breaking, because for all non-GSF code the acutal types involved don't change, and the GSF is still in Acts::Experimental.

If preffered, I could split of the part with the optional covariance...

@benjaminhuth benjaminhuth added the 🚧 WIP Work-in-progress label Aug 31, 2023
Copy link
Member

@paulgessinger paulgessinger left a comment

Choose a reason for hiding this comment

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

Seems reasonable

@codecov
Copy link

codecov bot commented Sep 1, 2023

Codecov Report

Merging #2413 (0b1a961) into main (a9ab6a5) will increase coverage by 0.02%.
The diff coverage is 40.38%.

@@            Coverage Diff             @@
##             main    #2413      +/-   ##
==========================================
+ Coverage   49.82%   49.84%   +0.02%     
==========================================
  Files         468      468              
  Lines       26539    26524      -15     
  Branches    12197    12187      -10     
==========================================
- Hits        13224    13222       -2     
- Misses       4632     4634       +2     
+ Partials     8683     8668      -15     
Files Coverage Δ
.../include/Acts/Propagator/MultiEigenStepperLoop.hpp 69.02% <100.00%> (-0.17%) ⬇️
Core/include/Acts/Propagator/Propagator.hpp 92.68% <ø> (ø)
Core/include/Acts/TrackFitting/detail/GsfActor.hpp 45.11% <ø> (-0.72%) ⬇️
Core/include/Acts/TrackFitting/detail/GsfUtils.hpp 60.00% <ø> (ø)
.../TrackFitting/detail/SymmetricKlDistanceMatrix.hpp 65.00% <ø> (ø)
Core/include/Acts/Propagator/Propagator.ipp 43.58% <0.00%> (ø)
...re/include/Acts/TrackFitting/GaussianSumFitter.hpp 43.42% <42.85%> (-0.95%) ⬇️
.../include/Acts/Propagator/MultiEigenStepperLoop.ipp 41.74% <44.44%> (+3.46%) ⬆️
...e/Acts/EventData/MultiComponentTrackParameters.hpp 54.21% <36.00%> (ø)

... and 1 file with indirect coverage changes

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

@paulgessinger paulgessinger removed the 🚧 WIP Work-in-progress label Sep 25, 2023
@paulgessinger
Copy link
Member

What's the plan for this @benjaminhuth?

@benjaminhuth
Copy link
Member Author

Hmm I resolved the merge conflicts. If physmon is green I think I would propose to merge it

@github-actions github-actions bot added Infrastructure Changes to build tools, continous integration, ... Changes Performance labels Oct 2, 2023
@benjaminhuth benjaminhuth marked this pull request as ready for review October 2, 2023 13:04
paulgessinger
paulgessinger previously approved these changes Oct 2, 2023
andiwand
andiwand previously approved these changes Oct 19, 2023
@kodiakhq kodiakhq bot removed the automerge label Oct 20, 2023
@kodiakhq
Copy link
Contributor

kodiakhq bot commented Oct 20, 2023

This PR currently has a merge conflict. Please resolve this and then re-add the automerge label.

andiwand
andiwand previously approved these changes Oct 20, 2023
@kodiakhq kodiakhq bot merged commit f54faba into acts-project:main Oct 20, 2023
55 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changes Performance Component - Core Affects the Core module Component - Examples Affects the Examples module Event Data Model Infrastructure Changes to build tools, continous integration, ... Track Fitting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants