Fix bug in CKF and add script for KF timing test #126

XiaocongAi · 2020-04-24T06:46:48Z

Fix bug in CKF: before adding the track state, the last tip should be removed from the active tips. But when creating multiple states on ONE surface, the update of active tips should be done only ONCE so that states have the same previous state. In the current master branch, each call to addSourcelinkState will remove the last tip, which is not correct.
Fix a bug in Pythia8 option: include the pileup generators as well instead of including the hard configurations twice.
Clean the KF smoothing
Add script for KF timing test (To produce plot as below)

codecov · 2020-04-24T21:25:55Z

Codecov Report

Merging #126 into master will increase coverage by 0.00%.
The diff coverage is 47.36%.

@@           Coverage Diff           @@
##           master     #126   +/-   ##
=======================================
  Coverage   45.82%   45.82%           
=======================================
  Files         351      351           
  Lines       17446    17447    +1     
  Branches     8095     8095           
=======================================
+ Hits         7994     7995    +1     
- Misses       4335     4338    +3     
+ Partials     5117     5114    -3

Impacted Files	Coverage Δ
Core/include/Acts/Fitter/KalmanFitter.hpp	`37.80% <30.00%> (ø)`
...ude/Acts/TrackFinder/CombinatorialKalmanFilter.hpp	`27.94% <66.66%> (+0.21%)`	⬆️

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 be3d87b...3038d5f. Read the comment docs.

XiaocongAi · 2020-04-27T06:09:21Z

@paulgessinger @asalzburger , could you help take a look and get it in?

paulgessinger · 2020-04-27T07:19:18Z

Can you comment what the bugs were? I'm not sure I understand just from the code.

XiaocongAi · 2020-04-27T17:20:32Z

@paulgessinger , sorry. The bugs are explained in the first comment.

asalzburger · 2020-04-27T18:10:26Z

@XiaocongAi can you rebase, please - this should remove the build error with macOS as well.

asalzburger

There's one comment change I would do, otherwise I am happy with the PR.

asalzburger · 2020-04-27T18:12:07Z

Core/include/Acts/Fitter/KalmanFitter.hpp

@@ -1100,6 +1099,7 @@ class KalmanFitter {
    }

    if (!kalmanResult.result.ok()) {
+      ACTS_ERROR("KalmanFilter failed: " << kalmanResult.result.error());
      return kalmanResult.result.error();
    }



I like the meaningful error output messages.

asalzburger · 2020-04-27T18:21:06Z

Examples/Scripts/KF_timing.py

@@ -0,0 +1,55 @@
+import ROOT


I think we will have to do a cleanup of the scripts, probably as preparation for the Tutorial/Workshop.
This is unrelated to this PR though, it should go into an issue.

That's a good idea!

asalzburger · 2020-04-27T18:21:22Z

Examples/Algorithms/GeneratorsPythia8/ACTFW/Options/Pythia8Options.cpp

@@ -72,7 +72,7 @@ FW::EventGenerator::Config FW::Options::readPythia8Options(
       Pythia8Generator::makeFunction(hard, lvl)},
      {PoissonMultiplicityGenerator{mu},
       GaussianVertexGenerator{{vtxStdXY, vtxStdXY, vtxStdZ, vtxStdT}},
-       Pythia8Generator::makeFunction(hard, lvl)},
+       Pythia8Generator::makeFunction(pileup, lvl)},


Well spotted!

asalzburger · 2020-04-27T18:23:12Z

Core/include/Acts/Fitter/KalmanFitter.hpp

-  /// The navigator has DirectNavigator type or not
-  static constexpr bool isDirectNavigator =
-      std::is_same<KalmanNavigator, DirectNavigator>::value;
-


Just shifting lines, hence ok.

asalzburger · 2020-04-27T18:35:00Z

Core/include/Acts/Fitter/KalmanFitter.hpp

+        if (isMeasurement) {
+          measurementIndices.emplace_back(st.index());
+        } else if (measurementIndices.empty()) {
+          // No smoothed parameter if the last measurment state has not been


"No smoothed parameters if the last measurement stat has not been found yet."

asalzburger

Ok, the changes are integrated. I approve.

* Clean-up the KalmanFitter * Fix the pythia8 option * Add the script for KF timing test * Add the python script for plotting KF timing test result * Fix a bug in CKF * Fix the comment

acts-issue-bot bot added the Triage label Apr 24, 2020

XiaocongAi self-assigned this Apr 24, 2020

XiaocongAi added the Bug Something isn't working label Apr 24, 2020

acts-issue-bot bot removed the Triage label Apr 24, 2020

XiaocongAi added the Improvement Changes to an existing feature label Apr 24, 2020

XiaocongAi changed the title ~~WIP: Add script for KF timing test~~ Add script for KF timing test Apr 24, 2020

XiaocongAi changed the title ~~Add script for KF timing test~~ Fix bug in CKF and add script for KF timing test Apr 25, 2020

XiaocongAi force-pushed the add_KF_test_script branch from 9df781b to 4d0086c Compare April 27, 2020 06:15

XiaocongAi force-pushed the add_KF_test_script branch from 4d0086c to 7fc67bf Compare April 27, 2020 16:57

XiaocongAi force-pushed the add_KF_test_script branch from 7fc67bf to 850f3fe Compare April 27, 2020 18:14

asalzburger requested changes Apr 27, 2020

View reviewed changes

XiaocongAi added 6 commits April 27, 2020 12:04

Clean-up the KalmanFitter

cb36dff

Fix the pythia8 option

d7cddf6

Add the script for KF timing test

84a8f40

Add the python script for plotting KF timing test result

dbb0c9e

Fix a bug in CKF

70e9d10

Fix the comment

3038d5f

XiaocongAi force-pushed the add_KF_test_script branch from d8776c1 to 3038d5f Compare April 27, 2020 19:04

asalzburger approved these changes Apr 28, 2020

View reviewed changes

asalzburger merged commit 8a05bdb into acts-project:master Apr 28, 2020

asalzburger added this to the v0.23.00 milestone Apr 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug in CKF and add script for KF timing test #126

Fix bug in CKF and add script for KF timing test #126

XiaocongAi commented Apr 24, 2020 •

edited

Loading

codecov bot commented Apr 24, 2020 •

edited

Loading

XiaocongAi commented Apr 27, 2020

paulgessinger commented Apr 27, 2020

XiaocongAi commented Apr 27, 2020

asalzburger commented Apr 27, 2020

asalzburger left a comment

asalzburger Apr 27, 2020

asalzburger Apr 27, 2020

XiaocongAi Apr 27, 2020

asalzburger Apr 27, 2020

asalzburger Apr 27, 2020

asalzburger Apr 27, 2020

XiaocongAi Apr 27, 2020

asalzburger left a comment

Fix bug in CKF and add script for KF timing test #126

Fix bug in CKF and add script for KF timing test #126

Conversation

XiaocongAi commented Apr 24, 2020 • edited Loading

codecov bot commented Apr 24, 2020 • edited Loading

Codecov Report

XiaocongAi commented Apr 27, 2020

paulgessinger commented Apr 27, 2020

XiaocongAi commented Apr 27, 2020

asalzburger commented Apr 27, 2020

asalzburger left a comment

Choose a reason for hiding this comment

asalzburger Apr 27, 2020

Choose a reason for hiding this comment

asalzburger Apr 27, 2020

Choose a reason for hiding this comment

XiaocongAi Apr 27, 2020

Choose a reason for hiding this comment

asalzburger Apr 27, 2020

Choose a reason for hiding this comment

asalzburger Apr 27, 2020

Choose a reason for hiding this comment

asalzburger Apr 27, 2020

Choose a reason for hiding this comment

XiaocongAi Apr 27, 2020

Choose a reason for hiding this comment

asalzburger left a comment

Choose a reason for hiding this comment

XiaocongAi commented Apr 24, 2020 •

edited

Loading

codecov bot commented Apr 24, 2020 •

edited

Loading