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!: Steppers use MagneticFieldProvider (2/2) #675

Merged

Conversation

paulgessinger
Copy link
Member

@paulgessinger paulgessinger commented Jan 27, 2021

Based on #666, this PR changes the way the steppers store and use the B fields. The B field template parameter goes away and they accept instances of std::shared_ptr<MagneticFieldProvider>. The use of std::shared_ptr should be fine, since the propagators and steppers are only rarely created, copied or destroyed. This change then also allows getting rid of BFieldVariant in the examples, since the generic MagneticFieldProvider can be used as an interchange type. Doing this reduces the compile time of two or our most compile-time resource hungry compilation units by over 20%. More improvements might be possible, but require more work.

There are some additional cleanups that become possible but I would like to keep this PR reasonably (to some extent) small, so those will be independent.

I did not observe decreases in propagation preformance.

steppers take b field type steppers take MagneticFieldProvider
test master any sbo sbo
prop 37.11 ms/event 38.09 ms/event 37.74 ms/event 35.51 ms/event
prop cov 74.44 ms/event 75.09 ms/event 74.29 ms/event 71.84 ms/event

This includes #666, #670, #673

BREAKING CHANGE: The stepper type structure changes. None of the steppers accept a B field template argument anymore. Constructors are changed to accept an std::shared_ptr<Acts::BFieldProvider> instead of an instance of the concrete B field type.

@paulgessinger paulgessinger added this to the next milestone Jan 27, 2021
@paulgessinger
Copy link
Member Author

@baschlag could give the vertexing based on this branch a shot and see if anything sticks out?

@paulgessinger paulgessinger added the 🚧 WIP Work-in-progress label Jan 27, 2021
@paulgessinger paulgessinger changed the title feat!: Steppers use BFieldProvider feat!: Steppers use BFieldProvider (2/2) Jan 27, 2021
@codecov
Copy link

codecov bot commented Jan 27, 2021

Codecov Report

Merging #675 (772b82a) into master (213d97d) will increase coverage by 0.02%.
The diff coverage is 79.16%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #675      +/-   ##
==========================================
+ Coverage   48.94%   48.96%   +0.02%     
==========================================
  Files         325      325              
  Lines       16622    16639      +17     
  Branches     7760     7762       +2     
==========================================
+ Hits         8135     8147      +12     
- Misses       3039     3042       +3     
- Partials     5448     5450       +2     
Impacted Files Coverage Δ
Core/include/Acts/Propagator/Propagator.ipp 37.33% <0.00%> (ø)
...clude/Acts/Vertexing/AdaptiveMultiVertexFinder.hpp 78.57% <0.00%> (ø)
...clude/Acts/Vertexing/AdaptiveMultiVertexFinder.ipp 42.19% <0.00%> (ø)
...re/include/Acts/Vertexing/ImpactPointEstimator.ipp 37.80% <0.00%> (ø)
...include/Acts/Vertexing/FullBilloirVertexFitter.hpp 66.66% <50.00%> (-13.34%) ⬇️
.../include/Acts/Vertexing/HelicalTrackLinearizer.ipp 29.78% <50.00%> (ø)
...clude/Acts/Vertexing/AdaptiveMultiVertexFitter.hpp 69.56% <66.66%> (+1.38%) ⬆️
...e/include/Acts/Vertexing/IterativeVertexFinder.hpp 82.35% <75.00%> (+9.01%) ⬆️
Core/include/Acts/Propagator/EigenStepper.ipp 51.88% <90.90%> (-0.04%) ⬇️
Core/include/Acts/Propagator/AtlasStepper.hpp 69.53% <100.00%> (-0.49%) ⬇️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 213d97d...fbd1118. Read the comment docs.

@baschlag
Copy link
Contributor

Hi, I finally managed to get this branch running in Athena and compared the Acts AMVF performance of this branch vs. the current default Acts AMVF in Athena (baseline). Physics performance (as expected) unchanged, CPU comparable to previous version (within measurement uncertainties I guess).

timing_vs_ntracks

timing
validation_AllPrimaryVertices_vx_err_z
validation_AllPrimaryVertices_vx_nTracks
validation_AllPrimaryVertices_vx_z

@paulgessinger
Copy link
Member Author

Excellent news. This seems to be within precision, so we can go ahead with this one and #666

@paulgessinger paulgessinger changed the title feat!: Steppers use BFieldProvider (2/2) feat!: Steppers use MagneticFieldProvider (2/2) Feb 24, 2021
@paulgessinger paulgessinger removed the 🚧 WIP Work-in-progress label Feb 24, 2021
@paulgessinger
Copy link
Member Author

Ok with #666 merged, this is ready for review.

@paulgessinger paulgessinger changed the title feat!: Steppers use MagneticFieldProvider (2/2) refactor!: Steppers use MagneticFieldProvider (2/2) Feb 24, 2021
Copy link
Contributor

@asalzburger asalzburger left a comment

Choose a reason for hiding this comment

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

I approve the changes here (there's one question regaring a TODO), I think we should create three follow-up PRs to clean out the unneccessary shorthand definitions, i.e. we I would do:

We should basically make a rule that
using Stepper = Acts::EigenStepper<> should not be used, but e.g. `using KalmanFitter = Acts::KalmanFitter<Acts::EigenStepper<>, ..., ..>' is still fine.

What do you think?

@paulgessinger paulgessinger merged commit 12f1247 into acts-project:master Mar 2, 2021
@paulgessinger paulgessinger deleted the bfield-inheritance-steppers branch March 2, 2021 09:37
@paulgessinger paulgessinger modified the milestones: next, v6.0.0 Mar 2, 2021
asalzburger pushed a commit that referenced this pull request Mar 4, 2021
With the vertexing code previously being templated on the bfield type, a hardcoded ConstantBField was used for simplicity in the Examples. After #675 went in, a configurable BField can now easily be used and set in the example options.
This PR changes the Examples accordingly.
Additionally some clean-up is done and Stepper typedefs like
using Stepper = Acts::EigenStepper<>;
are removed and the type is used explicitly instead.

Closes #735
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants