phi calculation independent of charge #1272

Jeet-bhai · 2024-02-02T07:41:46Z

Briefly, what does this PR introduce?

In the standard EICrecon, in track seeding, we measure phi using charge. However, it gives us a lot of error. We have changed the phi formula, which now only depends on the tangential vector.

Here is data that shows more than 50% improvement in phi calculation:

number of events = 10000

particle generated= negative muon
vertex = (0,0,0)
eta = -4 to 4
p = 0.5 to 20 GeV
We measure seeds whose phi differs from generated phi by more than 0.5 rad. We find 33 wrong phi-seeds in standard phi calculation (main branch). With new formula, we find only 15 wrong phi-seeds, an improvement of more than 50%.

Here, the first column is event number, second column is seed number, third column is seed phi, fourth column is phi difference, and fifth column is charge

Main branch, standard phi calculation:

root [1] events->Scan("CentralTrackSeedingResults.phi:CentralTrackSeedingResults.phi-atan2(MCParticles.momentum.y[0],MCParticles.momentum.x[0]):CentralTrackSeedingResults.charge","abs(CentralTrackSeedingResults.phi-atan2(MCParticles.momentum.y[0],MCParticles.momentum.x[0]))>0.5")

Row * Instance * CentralTr * CentralTr * CentralTr *
457 * 0 * 3.1368908 * 6.2784049 * -1 *
457 * 2 * 3.1409514 * 6.2824654 * -1 *
1051 * 0 * 3.1248598 * 6.197864 * -1 *
1541 * 2 * -2.652385 * -3.144119 * -1 *
1593 * 0 * 2.1453850 * 3.1149456 * 1 *
1719 * 0 * 3.1414830 * 6.2790422 * -1 *
1719 * 1 * 3.1376869 * 6.2752461 * -1 *
1759 * 0 * 3.1079447 * 6.2340577 * -1 *
1759 * 3 * 3.1407167 * 6.2668297 * -1 *
2268 * 0 * -2.926911 * -3.144877 * -1 *
2360 * 2 * -0.291468 * -3.144404 * -1 *
2748 * 0 * 1.2643282 * 3.1381311 * -1 *
3101 * 2 * 1.1373429 * 3.139108 * -1 *
3460 * 4 * -0.901838 * -3.160678 * -1 *
3464 * 0 * 3.1107549 * 6.2227986 * 1 *
3533 * 1 * 3.1413474 * 6.2821147 * -1 *
3533 * 2 * 3.1400566 * 6.2808239 * -1 *
3533 * 3 * 3.1406669 * 6.2814343 * -1 *
3602 * 0 * 2.7184147 * 3.1398609 * 1 *
4080 * 2 * 0.1672719 * 1.5374061 * -1 *
5168 * 2 * 1.1847635 * 3.1383791 * -1 *
6023 * 2 * 0.1832947 * 3.1397418 * 1 *
6634 * 2 * 0.1372623 * 3.1390230 * -1 *
6654 * 1 * 3.1401174 * 6.2715614 * -1 *
6979 * 2 * 1.2488478 * 3.1390423 * -1 *
Type to continue or q to quit ==>

7302 * 2 * 0.0834536 * 3.1398086 * 1 *
7641 * 2 * 1.4545679 * 3.1399058 * 1 *
7896 * 0 * 3.1225175 * 6.2049581 * 1 *
8508 * 2 * -2.191989 * -3.143604 * 1 *
9033 * 2 * 1.2532205 * 3.1398001 * 1 *
9094 * 4 * -3.139707 * -6.280495 * -1 *
9779 * 0 * 1.0651279 * 3.1089109 * -1 *
9786 * 2 * -1.552465 * -3.143571 * -1 *
==> 33 selected entries

new phi calculation:

root [1] events->Scan("CentralTrackSeedingResults.phi:CentralTrackSeedingResults.phi-atan2(MCParticles.momentum.y[0],MCParticles.momentum.x[0]):CentralTrackSeedingResults.charge","abs(CentralTrackSeedingResults.phi-atan2(MCParticles.momentum.y[0],MCParticles.momentum.x[0]))>0.5")

Row * Instance * CentralTr * CentralTr * CentralTr *
457 * 0 * 3.1368837 * 6.2783978 * -1 *
457 * 2 * 3.1409020 * 6.2824161 * -1 *
1051 * 0 * 3.1248586 * 6.1978628 * -1 *
1719 * 0 * 3.1415927 * 6.2791518 * -1 *
1719 * 1 * 3.1376864 * 6.2752456 * -1 *
1759 * 0 * 3.1079473 * 6.2340603 * -1 *
1759 * 3 * 3.1407468 * 6.2668598 * -1 *
3464 * 0 * 3.1107542 * 6.2227979 * 1 *
3533 * 1 * 3.1415927 * 6.2823600 * -1 *
3533 * 2 * 3.1400876 * 6.2808549 * -1 *
3533 * 3 * 3.1406791 * 6.2814464 * -1 *
4080 * 2 * 0.1672727 * 1.5374069 * -1 *
6654 * 1 * 3.1401691 * 6.2716131 * -1 *
7896 * 0 * 3.1225180 * 6.2049586 * 1 *
9094 * 4 * -3.139701 * -6.280489 * -1 *
==> 15 selected entries

What kind of change does this PR introduce?

Bug fix (issue #__)
New feature (issue #__)
[x ] Documentation update
Other: __

Please check if this PR fulfills the following:

[x ] Tests for the changes have been added
[ x] Documentation has been added / updated
Changes have been communicated to collaborators

Does this PR introduce breaking changes? What changes might users need to make to their code?

Does this PR change default behavior?

for more information, see https://pre-commit.ci

github-actions · 2024-02-02T08:34:23Z

Capybara summary for PR 1272

veprbl · 2024-02-03T21:38:22Z

src/algorithms/tracking/TrackSeeding.cc

+        auto dot_1 = tangent_vector_candidate_1*first_hit_vector;
+        auto dot_2 = tangent_vector_candidate_2*first_hit_vector;
+
+        TVector2 tangent_vector = (dot_1>dot_2) ? tangent_vector_candidate_1 : tangent_vector_candidate_2;


From what I can tell, this basically calculates the charge but using only one hit instead of two. Would it be possible to instead find a way to improve the charge calculation?

Once we find the PCA point to (x,y) = (0,0), there are two vectors that are tangential to the circle around (0,0) through that PCA point.

In the current official implementation, we pick the vector based on the seed charge. One issue with this -- which is admittedly a rare occurrence -- is that combination of the charge seed and the seed circle center position can be such that the phi gets flipped by 180 degrees, causing the CKF to fail for that seed.

In the proposed approach, we see which of the two candidate vectors points in the direction of the first hit point and then use that to set phi. For the majority of seeds, this causes no change. But it corrects the phi for the few seeds that Jeet showed above, which previously had an error close to +-180 degrees.

For these seeds, which now have the correct phi and the wrong charge, they will at least go through the CFK successfully. And the CKF can then correct the charge to the correct sign.

@veprbl I have created a new branch where I calculate the charge in this way, and then use the previous phi calculation. I get the same results. So, we can say, that the new charge calculation is an improved version. Thank you for the suggestion!

src/algorithms/tracking/TrackSeeding.cc

veprbl · 2024-02-03T21:38:51Z

src/algorithms/tracking/TrackSeeding.cc


-      auto phi = atan2(vypos,vxpos);
+        const auto& firstpos = xyHitPositions.at(0);


Please don't change indentation inside the block.

bschmookler · 2024-02-06T04:16:47Z

I probably was too involved in creating this PR to review it. So either @veprbl and/or @osbornjd should review.

Co-authored-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>

Jeet-bhai · 2024-02-07T00:10:53Z

src/algorithms/tracking/TrackSeeding.cc

+
+        TVector2 tangent_vector = (dot_1>dot_2) ? tangent_vector_candidate_1 : tangent_vector_candidate_2;
+
+        auto phi = tangent_vector.Phi();


This gives phi from 0 to 2pi. The earlier code gave phi from -pi to pi.

Jeet-bhai · 2024-02-07T06:52:50Z

src/algorithms/tracking/TrackSeeding.cc

+        auto dot_1 = tangent_vector_candidate_1*first_hit_vector;
+        auto dot_2 = tangent_vector_candidate_2*first_hit_vector;
+
+        TVector2 tangent_vector = (dot_1>dot_2) ? tangent_vector_candidate_1 : tangent_vector_candidate_2;


@veprbl I have created a new branch where I calculate the charge in this way, and then use the previous phi calculation. I get the same results. So, we can say, that the new charge calculation is an improved version. Thank you for the suggestion!

osbornjd · 2024-02-07T19:32:15Z

src/algorithms/tracking/TrackSeeding.cc

@@ -4,6 +4,7 @@

 #include "TrackSeeding.h"

+#include "TVector2.h"


This is a super nitpicky and minor comment, but Acts has a Vector2 object that uses eigen as the backend that is already included in this file in Acts/Definitions/Algebra.hpp. We could use that instead of TVector2 which would

Reduce the number of includes

use eigen as the backend for matrix math, which while it probably doesn't make a difference in CPU speed for a simple dot product at least keeps us consistent

Closes: #1271 Closes: #1272 ### Briefly, what does this PR introduce? It calculates the charge more correctly. Before the change ``` events->Scan("CentralTrackSeedingResults.phi:CentralTrackSeedingResults.phi-atan2(MCParticles.momentum.y[0],MCParticles.momentum.x[0]):CentralTrackSeedingResults.charge","abs(CentralTrackSeedingResults.phi-atan2(MCParticles.momentum.y[0],MCParticles.momentum.x[0]))>0.5") *********************************************************** * Row * Instance * CentralTr * CentralTr * CentralTr * *********************************************************** * 457 * 0 * 3.1368908 * 6.2784049 * -1 * * 457 * 2 * 3.1409514 * 6.2824654 * -1 * * 1051 * 0 * 3.1248598 * 6.197864 * -1 * * 1541 * 2 * -2.652385 * -3.144119 * -1 * * 1593 * 0 * 2.1453850 * 3.1149456 * 1 * * 1719 * 0 * 3.1414830 * 6.2790422 * -1 * * 1719 * 1 * 3.1376869 * 6.2752461 * -1 * * 1759 * 0 * 3.1079447 * 6.2340577 * -1 * * 1759 * 3 * 3.1407167 * 6.2668297 * -1 * * 2268 * 0 * -2.926911 * -3.144877 * -1 * * 2360 * 2 * -0.291468 * -3.144404 * -1 * * 2748 * 0 * 1.2643282 * 3.1381311 * -1 * * 3101 * 2 * 1.1373429 * 3.139108 * -1 * * 3460 * 4 * -0.901838 * -3.160678 * -1 * * 3464 * 0 * 3.1107549 * 6.2227986 * 1 * * 3533 * 1 * 3.1413474 * 6.2821147 * -1 * * 3533 * 2 * 3.1400566 * 6.2808239 * -1 * * 3533 * 3 * 3.1406669 * 6.2814343 * -1 * * 3602 * 0 * 2.7184147 * 3.1398609 * 1 * * 4080 * 2 * 0.1672719 * 1.5374061 * -1 * * 5168 * 2 * 1.1847635 * 3.1383791 * -1 * * 6023 * 2 * 0.1832947 * 3.1397418 * 1 * * 6634 * 2 * 0.1372623 * 3.1390230 * -1 * * 6654 * 1 * 3.1401174 * 6.2715614 * -1 * * 6979 * 2 * 1.2488478 * 3.1390423 * -1 * Type <CR> to continue or q to quit ==> * 7302 * 2 * 0.0834536 * 3.1398086 * 1 * * 7641 * 2 * 1.4545679 * 3.1399058 * 1 * * 7896 * 0 * 3.1225175 * 6.2049581 * 1 * * 8508 * 2 * -2.191989 * -3.143604 * 1 * * 9033 * 2 * 1.2532205 * 3.1398001 * 1 * * 9094 * 4 * -3.139707 * -6.280495 * -1 * * 9779 * 0 * 1.0651279 * 3.1089109 * -1 * * 9786 * 2 * -1.552465 * -3.143571 * -1 * *********************************************************** ==> 33 selected entries ``` after the change: ``` events->Scan("CentralTrackSeedingResults.phi:CentralTrackSeedingResults.phi-atan2(MCParticles.momentum.y[0],MCParticles.momentum.x[0]):CentralTrackSeedingResults.charge","abs(CentralTrackSeedingResults.phi-atan2(MCParticles.momentum.y[0],MCParticles.momentum.x[0]))>0.5") *********************************************************** * Row * Instance * CentralTr * CentralTr * CentralTr * *********************************************************** * 457 * 0 * 3.1368908 * 6.2784049 * -1 * * 457 * 2 * 3.1409514 * 6.2824654 * -1 * * 1051 * 0 * 3.1248598 * 6.197864 * -1 * * 1719 * 0 * 3.1414830 * 6.2790422 * -1 * * 1719 * 1 * 3.1376869 * 6.2752461 * -1 * * 1759 * 0 * 3.1079447 * 6.2340577 * -1 * * 1759 * 3 * 3.1407167 * 6.2668297 * -1 * * 3464 * 0 * 3.1107549 * 6.2227986 * 1 * * 3533 * 1 * 3.1413474 * 6.2821147 * -1 * * 3533 * 2 * 3.1400566 * 6.2808239 * -1 * * 3533 * 3 * 3.1406669 * 6.2814343 * -1 * * 4080 * 2 * 0.1672719 * 1.5374061 * -1 * * 6654 * 1 * 3.1401174 * 6.2715614 * -1 * * 7896 * 0 * 3.1225175 * 6.2049581 * 1 * * 9094 * 4 * -3.139707 * -6.280495 * -1 * *********************************************************** ==> 15 selected entries ``` ### What kind of change does this PR introduce? - [x ] Bug fix (issue #__) - [ ] New feature (issue #__) - [x ] Documentation update - [ ] Other: __ ### Please check if this PR fulfills the following: - [ x] Tests for the changes have been added - [x ] Documentation has been added / updated - [x ] Changes have been communicated to collaborators ### Does this PR introduce breaking changes? What changes might users need to make to their code? no ### Does this PR change default behavior? no --------- Co-authored-by: Jeetendra Gupta <jeetendragupta@Jeetendras-MacBook-Air.local> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>

phi calculation independent of charge

6641546

Jeet-bhai requested a review from bschmookler February 2, 2024 07:41

Jeet-bhai linked an issue Feb 2, 2024 that may be closed by this pull request

new phi calculation of real seeding #1271

Closed

github-actions bot added the topic: tracking Relates to tracking reconstruction label Feb 2, 2024

[pre-commit.ci] auto fixes from pre-commit.com hooks

7cccf20

for more information, see https://pre-commit.ci

Jeet-bhai self-assigned this Feb 2, 2024

pre-commit-ci bot temporarily deployed to github-pages February 2, 2024 08:33 Inactive

veprbl reviewed Feb 3, 2024

View reviewed changes

bschmookler requested a review from osbornjd February 6, 2024 04:16

Update src/algorithms/tracking/TrackSeeding.cc

4eedc5b

Co-authored-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>

Jeet-bhai temporarily deployed to github-pages February 6, 2024 06:26 — with GitHub Actions Inactive

Jeet-bhai commented Feb 7, 2024

View reviewed changes

osbornjd reviewed Feb 7, 2024

View reviewed changes

veprbl mentioned this pull request Feb 15, 2024

improvement in charge calculation in trackseeding #1281

Merged

2 tasks

veprbl closed this in #1281 Feb 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

phi calculation independent of charge #1272

phi calculation independent of charge #1272

Jeet-bhai commented Feb 2, 2024

github-actions bot commented Feb 2, 2024

veprbl Feb 3, 2024

bschmookler Feb 7, 2024

Jeet-bhai Feb 7, 2024

veprbl Feb 3, 2024

bschmookler commented Feb 6, 2024 •

edited

Loading

Jeet-bhai Feb 7, 2024

Jeet-bhai Feb 7, 2024

osbornjd Feb 7, 2024


		auto phi = atan2(vypos,vxpos);
		const auto& firstpos = xyHitPositions.at(0);


		TVector2 tangent_vector = (dot_1>dot_2) ? tangent_vector_candidate_1 : tangent_vector_candidate_2;

		auto phi = tangent_vector.Phi();

		@@ -4,6 +4,7 @@

		#include "TrackSeeding.h"

		#include "TVector2.h"

phi calculation independent of charge #1272

phi calculation independent of charge #1272

Conversation

Jeet-bhai commented Feb 2, 2024

Briefly, what does this PR introduce?

What kind of change does this PR introduce?

Please check if this PR fulfills the following:

Does this PR introduce breaking changes? What changes might users need to make to their code?

Does this PR change default behavior?

github-actions bot commented Feb 2, 2024

Capybara summary for PR 1272

veprbl Feb 3, 2024

Choose a reason for hiding this comment

bschmookler Feb 7, 2024

Choose a reason for hiding this comment

Jeet-bhai Feb 7, 2024

Choose a reason for hiding this comment

veprbl Feb 3, 2024

Choose a reason for hiding this comment

bschmookler commented Feb 6, 2024 • edited Loading

Jeet-bhai Feb 7, 2024

Choose a reason for hiding this comment

Jeet-bhai Feb 7, 2024

Choose a reason for hiding this comment

osbornjd Feb 7, 2024

Choose a reason for hiding this comment

bschmookler commented Feb 6, 2024 •

edited

Loading