Add GenFit2-based Track Fitting Functional#77
Add GenFit2-based Track Fitting Functional#77andread3vita wants to merge 23 commits intokey4hep:mainfrom
Conversation
FIX comments and track finder test
FIX fit initialization ADD fitter test ADD README for fitter FIX small fix FIX hit ordering function for fitter ADD comments Remove compatibility with older Gaudi versions (key4hep#76)
andread3vita
force-pushed
the
PRtrackingPipeline
branch
from
89d430a to
f173001
Compare
…NAME m_skip_background to m_skip_unmatchedTracks + COMMENT m_singleEvaluation
|
For every comment everyone watching the repository gets a notification. In addition, if someone reviews the PR and there are conversations, it will be very hard to find them between all your comments. As an alternative you can make a todo list in the original post with boxes and click the boxes once they are done. Or you can put comments in your own code. |
|
You are right! Sorry! I can modify the first comment with the TO-DO list |
andread3vita
force-pushed
the
PRtrackingPipeline
branch
from
435c1d3 to
d0d5fce
Compare
andread3vita
force-pushed
the
PRtrackingPipeline
branch
from
53a1fe0 to
d49e537
Compare
andread3vita
force-pushed
the
PRtrackingPipeline
branch
from
887e90c to
2cfda9b
Compare
|
@SanghyunKo I have finalized all modifications for standalone muon system fitting. The PR is ready for review. |
|
As there were quite a number of formatting issues, I ran the clang hook and pushed to the PR myself. I will start reviewing the PR based on the updated one. Next time, it'll be very helpful for reviewers if you can run the pre-commit in advance, especially for a big PR like this :) I'll come back soon with the real comments. |
SanghyunKo
left a comment
SanghyunKo
left a comment
There was a problem hiding this comment.
Impressive amount of work! I made my first round of passes. Frankly, I'm not confident enough that I caught everything properly (or even that my comments were proper). I'd highly appreciate comments from others.
I think most of my comments are rather easy to address, except the ones in GenfitTrack.cpp; Personally I feel that we might need to iterate on GenfitTrack.cpp a couple of times more. Anyway, let's make it converge soon ;)
| namespace GenfitInterface { | ||
|
|
||
| GenfitField::GenfitField(dd4hep::OverlayedField dd4hepField) : m_dd4hepField(dd4hepField) { | ||
| genfit::FieldManager::getInstance()->init(this); |
There was a problem hiding this comment.
I think you're calling the init function twice - you already have the call in L138 of GenfitTrackFitter.cpp. I'd remove this call in this constructor.
|
|
||
| double rx = center.X() - pos.X(); | ||
| double ry = center.Y() - pos.Y(); | ||
| double signed_param = init_mom.X() * ry - init_mom.y() * rx; |
There was a problem hiding this comment.
Perhaps you wanted the capital letter Y? (everywhere else uses the capital letter)
| points_Rz.emplace_back(R, z_coord); | ||
| } | ||
|
|
||
| double sumR = 0.0; | ||
| double sumZ = 0.0; | ||
| double sumRZ = 0.0; | ||
| double sumR2 = 0.0; | ||
|
|
||
| for (const auto& p : points_Rz) { | ||
| sumR += p.x; | ||
| sumZ += p.y; | ||
| sumRZ += p.x * p.y; | ||
| sumR2 += p.x * p.x; | ||
| } |
There was a problem hiding this comment.
Why do you use Point2D_xy to store R & Z? It seems really confusing. You could use something else (e.g. std::pair) if you simply wanted to store the two doubles.
| double denominator = N * sumR2 - sumR * sumR; | ||
|
|
||
| double a = (N * sumRZ - sumR * sumZ) / denominator; | ||
| double b = (sumZ - a * sumR) / N; | ||
| pZ = a * init_pT; | ||
| init_mom.SetZ(pZ); | ||
|
|
||
| if (std::abs(denominator) < 1e-12) { | ||
| pZ = 0; | ||
| init_mom.SetZ(pZ); | ||
| } | ||
|
|
||
| double z_PCA = b; | ||
|
|
||
| // CHARGE | ||
| TVector3 pos(closestPoint.x, closestPoint.y, z_PCA); |
There was a problem hiding this comment.
Zero guard seems to be imperfect; if the denominator is zero, then z_PCA will be NaN.
| // Initialize the Genfit: FieldManager and MaterialEffects | ||
| m_detector = m_geoSvc->getDetector(); | ||
| auto fieldMap = m_detector->field(); | ||
| m_genfitField = new GenfitInterface::GenfitField(fieldMap); |
There was a problem hiding this comment.
The m_genfitField is created and never deleted. Please delete this in the right place, or simply make this std::unique_ptr.
| pz = gen_momentum.Z(); // gev | ||
| pt = gen_momentum.Perp(); // gev | ||
|
|
||
| double Bz = m_fieldMap->getBz(gen_position) / 10.; // Tesla |
There was a problem hiding this comment.
Same comment regarding the B field unit above (should be *dd4hep::kilogauss).
| pz = gen_momentum.Z(); // gev | ||
| pt = gen_momentum.Perp(); // gev | ||
|
|
||
| Bz = m_fieldMap->getBz(gen_position) / 10.; // Tesla |
| trackStateFirstHit.D0 = d0; | ||
| trackStateFirstHit.Z0 = z0; | ||
| trackStateFirstHit.phi = phi; | ||
| trackStateFirstHit.omega = omega; | ||
| trackStateFirstHit.tanLambda = tanLambda; | ||
| trackStateFirstHit.time = 0.; |
There was a problem hiding this comment.
Probably you could make a function for setting the track state and save a couple of hundred lines here (same operations are repeated for the first hit, last hit, and at IP).
(+ avoid the battle with units)
| const int layer = decoder->get(cellid, "layer"); | ||
| const int superlayer = decoder->get(cellid, "superlayer"); | ||
| const int nphi = decoder->get(cellid, "nphi"); |
There was a problem hiding this comment.
Are we 100% sure that all users of the SenseWireHit will use the same cell ID encoding scheme?
| rawHitCov(7, 7) = positionAlongWireError * positionAlongWireError; | ||
|
|
||
| // Create the genfit::WirePointMeasurement | ||
| m_genfitHit = new genfit::WirePointMeasurement(rawHitCoords, rawHitCov, det_idx, hit_idx, nullptr); |
There was a problem hiding this comment.
Same comment with the planar measurement (explain the ownership).
3 participants
BEGINRELEASENOTES
ENDRELEASENOTES
Hello! This PR includes the implementation of the track fitter using Genfit2.
See more info here.
TODO: