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: Propagator accepts prepared result instance #1317

Merged
merged 11 commits into from
Jul 21, 2022

Conversation

paulgessinger
Copy link
Member

This PR adds overloads to the Propagator::propagate method which allow passing in a prepared result object. I want to use this to have the CKF reuse a single MultiTrajectory object for all input seeds, and @benjaminhuth might be able to use this to simplify the GSF internals.

@paulgessinger paulgessinger added this to the next milestone Jul 11, 2022
@codecov
Copy link

codecov bot commented Jul 11, 2022

Codecov Report

Merging #1317 (093d1b7) into main (e7edccb) will increase coverage by 0.02%.
The diff coverage is 28.57%.

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

@@            Coverage Diff             @@
##             main    #1317      +/-   ##
==========================================
+ Coverage   47.45%   47.47%   +0.02%     
==========================================
  Files         375      375              
  Lines       19780    19781       +1     
  Branches     9274     9271       -3     
==========================================
+ Hits         9386     9391       +5     
  Misses       4010     4010              
+ Partials     6384     6380       -4     
Impacted Files Coverage Δ
Core/include/Acts/Propagator/Propagator.hpp 89.74% <ø> (ø)
Core/include/Acts/Propagator/Propagator.ipp 50.63% <28.57%> (+7.04%) ⬆️
Core/include/Acts/Utilities/Result.hpp 72.00% <0.00%> (-2.00%) ⬇️

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

Copy link
Member

@benjaminhuth benjaminhuth left a comment

Choose a reason for hiding this comment

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

Looks good to me!

The only thing I thought about is if it would be better to use r-value references and std::forward for the input_result since it would allow perfect forwarding... However, this has probably no measurable impact.

@paulgessinger
Copy link
Member Author

Hm, I didn't expect this to change any outputs.

@paulgessinger
Copy link
Member Author

I updated the changes hashes now, @benjaminhuth.

@benjaminhuth
Copy link
Member

hmm it seems that only the tracksummary-hashes have changed, not the trackstates, right?
This is more weird than before, since that means that the propagation/fitting itself does not change on the bit-level, but the summary does... Maybe this is worth investigating @paulgessinger?

@paulgessinger
Copy link
Member Author

I guess so, yes. We don't have performance plots running in CI. Do you have an implementation for diagnostics plots other than what's directly in tracksummary_gsf.root?

@benjaminhuth
Copy link
Member

Hmm that should be possible with the same tools used for the KalmanFitter, since the result type and the writers are identical...
@paulgessinger is there a way to download these tracksummary_gsf.root from the CI? Or should I build acts in both configurations?

@paulgessinger
Copy link
Member Author

The GSF currently doesn't run in the physics perf monitoring, so it's not stored in the physmon artifact unfortunately.

@andiwand
Copy link
Contributor

but we could just add another artifact in the github actions yaml right?

is this a pure CI problem and not reproducible locally?

@benjaminhuth
Copy link
Member

is this a pure CI problem and not reproducible locally?

No I don't think so, it would be just convenience and time saving not to rebuild ACTS in 2 different versions...

@benjaminhuth
Copy link
Member

benjaminhuth commented Jul 20, 2022

Okay, locally I see also differences in both files (states and summary). I see also "high-level differences" between the runs, in the main-branch there are two MultiStepperError:4 which do not happen in your feature branch @paulgessinger ...
I'll investigate this.

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

All green I think. Can you approve if you're happy @benjaminhuth ?

@kodiakhq kodiakhq bot merged commit 201b308 into acts-project:main Jul 21, 2022
@paulgessinger paulgessinger modified the milestones: next, v19.5.0 Jul 25, 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

3 participants