Add GenFit2-based Track Fitting Functional#77
Add GenFit2-based Track Fitting Functional#77andread3vita wants to merge 47 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
|
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 ;)
| 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.
There was a problem hiding this comment.
Fixed by creating Point2D_Rz
| 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?
There was a problem hiding this comment.
What can we do about this? What was the outcome of the discussion on the encoding scheme? As far as I understand, it will be difficult to use a single encoder for both silicon and gaseous detectors. However, would it be possible to adopt a common encoding scheme for all wire-based detectors?
There was a problem hiding this comment.
If we can't do anything with this, we should at least check whether the cell ID decoding string contains those substrings in advance (otherwise users will need to go to the DD4hep source code to figure out what the error is); probably the following would be a good place to put:
https://github.com/key4hep/k4RecTracker/pull/77/changes#diff-f5800e72dd2692318dbc3570ae10bfb8d5624fd746c72ae636095de982473dd2R155-R164
…y when needed + MAKE DCH name configurable Ã+ ADD comments about ownership
andread3vita
force-pushed
the
PRtrackingPipeline
branch
from
c4d9b37 to
5120c6f
Compare
|
@SanghyunKo I have implemented what we discussed. Let me know if there is something else I should implement/change. |
3 participants
BEGINRELEASENOTES
ENDRELEASENOTES
Hello! This PR includes the implementation of the track fitter using Genfit2.
See more info here.
TODO: