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: Simplified implementation of StepperExtensionList #817

Merged
merged 4 commits into from
May 27, 2021

Conversation

benjaminhuth
Copy link
Member

In this PR I propose a simplified implementation of the StepperExtensionList. I'm currently working with it, and I found it first quite difficult to look through the structure, and then also not so handy to change things (must be always changed both in implementation and the interface class...)

With this proposal, I reimplemented the same functionality with some fold expressions and lambdas, and at least to me it is now a lot clearer what is done here, and everything is in one file.

@benjaminhuth benjaminhuth changed the title refactor: Proposal for simplified implementation of StepperExtensionList refactor: Proposal for simplified implementation of StepperExtensionList WIP May 26, 2021
@codecov
Copy link

codecov bot commented May 26, 2021

Codecov Report

Merging #817 (55bfe42) into main (31faea5) will decrease coverage by 0.00%.
The diff coverage is 53.12%.

❗ Current head 55bfe42 differs from pull request most recent head 624df01. Consider uploading reports for the commit 624df01 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #817      +/-   ##
==========================================
- Coverage   48.64%   48.63%   -0.01%     
==========================================
  Files         328      327       -1     
  Lines       16900    16906       +6     
  Branches     7935     7944       +9     
==========================================
+ Hits         8221     8223       +2     
+ Misses       3080     3079       -1     
- Partials     5599     5604       +5     
Impacted Files Coverage Δ
...e/include/Acts/Propagator/StepperExtensionList.hpp 59.52% <53.12%> (-21.43%) ⬇️
Core/include/Acts/Propagator/Navigator.hpp 57.54% <0.00%> (ø)
Core/include/Acts/Utilities/BinningData.hpp 66.27% <0.00%> (ø)
Core/include/Acts/Propagator/AtlasStepper.hpp 69.36% <0.00%> (ø)
Core/include/Acts/Propagator/EigenStepper.hpp 67.56% <0.00%> (ø)
Core/include/Acts/Utilities/BoundingBox.ipp 43.93% <0.00%> (+5.12%) ⬆️

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 31faea5...624df01. Read the comment docs.

@benjaminhuth benjaminhuth changed the title refactor: Proposal for simplified implementation of StepperExtensionList WIP refactor: Proposal for simplified implementation of StepperExtensionList May 27, 2021
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.

Yup, good idea. I think it's effectively exactly the same, but you can write it a lot more compact this way and there's no need to add extra types.

I think most of us (or me at least) are not super comfortable with fold expressions yet, but this is the way.

Core/include/Acts/Propagator/StepperExtensionList.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Propagator/StepperExtensionList.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Propagator/StepperExtensionList.hpp Outdated Show resolved Hide resolved
@paulgessinger paulgessinger added this to the next milestone May 27, 2021
@paulgessinger paulgessinger changed the title refactor: Proposal for simplified implementation of StepperExtensionList refactor: Simplified implementation of StepperExtensionList May 27, 2021
benjaminhuth and others added 2 commits May 27, 2021 12:05
Co-authored-by: Paul Gessinger <hello@paulgessinger.com>
@paulgessinger paulgessinger merged commit d00ea63 into acts-project:main May 27, 2021
@paulgessinger paulgessinger modified the milestones: next, v8.3.0 May 27, 2021
@benjaminhuth benjaminhuth deleted the refactor/extensionlist branch December 20, 2021 11:26
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

2 participants