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.

Sign up for GitHub

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

phi calculation independent of charge #1272

Closed
wants to merge 3 commits into from
Closed

phi calculation independent of charge #1272

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6641546
phi calculation independent of charge
Feb 2, 2024
7cccf20
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Feb 2, 2024
4eedc5b
Update src/algorithms/tracking/TrackSeeding.cc
Jeet-bhai Feb 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions src/algorithms/tracking/TrackSeeding.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#include "TrackSeeding.h"

#include "TVector2.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

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

  1. Reduce the number of includes
  2. 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

#include <Acts/Definitions/Algebra.hpp>
#include <Acts/Seeding/Seed.hpp>
#include <Acts/Seeding/SeedConfirmationRangeConfig.hpp>
Expand Down Expand Up @@ -211,10 +212,20 @@ std::unique_ptr<edm4eic::TrackParametersCollection> eicrecon::TrackSeeding::make
auto xpos = xypos.first;
auto ypos = xypos.second;

auto vxpos = -1.*charge*(ypos-Y0);
auto vypos = charge*(xpos-X0);

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

Choose a reason for hiding this comment

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

Please don't change indentation inside the block.


TVector2 tangent_vector_candidate_1( (Y0-ypos) , -(X0-xpos) );
TVector2 tangent_vector_candidate_2( -(Y0-ypos) , (X0-xpos) );

TVector2 first_hit_vector(firstpos.first-xpos,firstpos.second-ypos);

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;
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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!


auto phi = atan2(tangent_vector.Py(),tangent_vector.Px());
Jeet-bhai marked this conversation as resolved.
Show resolved Hide resolved

const float z0 = seed.z();
auto perigee = Acts::Surface::makeShared<Acts::PerigeeSurface>(Acts::Vector3(0,0,0));
Expand Down
Loading