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: optimise BinningData, remove dead code #657

Merged
merged 2 commits into from
Jan 21, 2021

Conversation

asalzburger
Copy link
Contributor

This PR cleans the BinUtility and BinningData objects:

  • removes dead code
  • removes hand-written search in vector in favour of std::lower_bound

It adds/removes relevant UnitTests and adds a new benchmark test, the benchmark before the changes shows:

16:36:17    BinUtility     INFO      Execution stats small: 1000 runs of 1 iteration(s), 0.0ms total, 0.0430+/-0.0001µs per run, 43.000+/-0.058ns per iteration
16:36:17    BinUtility     INFO      Fraction is: 21968011 vs. 21968011
16:36:19    BinUtility     INFO      Execution stats medium: 1000 runs of 1 iteration(s), 0.1ms total, 0.0510+/-0.0003µs per run, 51.000+/-0.346ns per iteration
16:36:19    BinUtility     INFO      Fraction is: 19641811 vs. 19641811
16:36:21    BinUtility     INFO      Execution stats many: 1000 runs of 1 iteration(s), 0.1ms total, 0.0780+/-0.0002µs per run, 78.000+/-0.173ns per iteration
16:36:21    BinUtility     INFO      Fraction is: 12780128 vs. 12780128
16:36:23    BinUtility     INFO      Execution stats many (lower_bounds): 1000 runs of 1 iteration(s), 0.0ms total, 0.0420+/-0.0001µs per run, 42.000+/-0.086ns per iteration
16:36:23    BinUtility     INFO      Fraction is: 23670634 vs. 23670634

to the new implementation:

17:25:25    BinUtility     INFO      Execution stats small: 1000 runs of 1 iteration(s), 0.0ms total, 0.0450+/-0.0001µs per run, 45.000+/-0.115ns per iteration
17:25:25    BinUtility     INFO      Fraction is: 21444179 vs. 21444179
17:25:27    BinUtility     INFO      Execution stats medium: 1000 runs of 1 iteration(s), 0.0ms total, 0.0460+/-0.0001µs per run, 46.000+/-0.115ns per iteration
17:25:27    BinUtility     INFO      Fraction is: 21030434 vs. 21030434
17:25:29    BinUtility     INFO      Execution stats many: 1000 runs of 1 iteration(s), 0.0ms total, 0.0480+/-0.0001µs per run, 48.000+/-0.115ns per iteration
17:25:29    BinUtility     INFO      Fraction is: 20028335 vs. 20028335
17:25:31    BinUtility     INFO      Execution stats equidistant: 1000 runs of 1 iteration(s), 0.0ms total, 0.0450+/-0.0001µs per run, 45.000+/-0.058ns per iteration
17:25:31    BinUtility     INFO      Fraction is: 20992746 vs. 20992746

The new implementation is faster in all bin scenarios.

Eventually, the BinUtility should go in favour of Grid usage, the removal of dead code is a first step towards this.

@asalzburger asalzburger self-assigned this Jan 20, 2021
@asalzburger asalzburger added Component - Core Affects the Core module Feature Development to integrate a new feature labels Jan 20, 2021
@asalzburger asalzburger added this to the next milestone Jan 20, 2021
@codecov
Copy link

codecov bot commented Jan 20, 2021

Codecov Report

Merging #657 (e494981) into master (bc366d9) will increase coverage by 0.02%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #657      +/-   ##
==========================================
+ Coverage   49.04%   49.06%   +0.02%     
==========================================
  Files         329      329              
  Lines       16508    16438      -70     
  Branches     7692     7657      -35     
==========================================
- Hits         8097     8066      -31     
+ Misses       3028     2998      -30     
+ Partials     5383     5374       -9     
Impacted Files Coverage Δ
Core/include/Acts/Utilities/BinUtility.hpp 45.83% <ø> (-0.96%) ⬇️
Core/include/Acts/Utilities/BinnedArray.hpp 33.33% <ø> (ø)
Core/include/Acts/Utilities/BinnedArrayXD.hpp 34.21% <ø> (+11.40%) ⬆️
Core/include/Acts/Utilities/BinningData.hpp 66.27% <50.00%> (+0.08%) ⬆️
Core/include/Acts/Propagator/Navigator.hpp 58.62% <0.00%> (ø)
Core/include/Acts/Propagator/EigenStepper.ipp 50.48% <0.00%> (+0.97%) ⬆️

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 f3f5479...e494981. Read the comment docs.

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 fine to me.

@paulgessinger
Copy link
Member

This changes the public API, but we probably wouldn't consider this breaking, right @asalzburger ?

@asalzburger asalzburger merged commit cae92cb into acts-project:master Jan 21, 2021
@paulgessinger paulgessinger modified the milestones: next, v5.0.0 Jan 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Core Affects the Core module Feature Development to integrate a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants