New FWD tracking mode based on FST-track finding #571
Conversation
|
Note, this PR includes the changes from PR #569 but the only changes relevant here are those in StRoot/StFwdTrackMaker and StRoot/StFwdUtils/ |
|
Also the CI jobs seem to be failing on code unrelated to my changes (things in StEvent) |
|
These errors should be fixed first: |
klendathu2k
left a comment
klendathu2k
left a comment
There was a problem hiding this comment.
Few initial comments made. Will review in more detail tomorrow.
|
@klendathu2k I will remove everything StFtt related from this PR. Everything you suggest is valid - but it I've addressed it in PR #569 instead of here |
jdbrice
force-pushed
the
fst-tracking-mode
branch
from
68cb393 to
7786bd9
Compare
|
I finally updated this PR to include only the needed changes for this feature (FST based tracking) |
jdbrice
requested review from
R-Witt,
fisyak,
genevb,
iraklic and
plexoos
as code owners
|
commit e65362f made changes to verify everything works for the ideal simulation cases:
I added: to make running the code in this way easy. It can be tested with: |
…Finder node present
…listic + fst/ftt seeds)
…Genfit tracks not deleted (use shared_ptr where possible)
|
OK with 99752c4
|
genevb
left a comment
genevb
left a comment
There was a problem hiding this comment.
Just a few minor observations. I didn't dig through all the sim and Tracker changes, so I can't comment on those.
|
|
||
| mIdTruth = evTrack->idTruth(); | ||
| mQATruth = evTrack->qaTruth(); | ||
|
|
There was a problem hiding this comment.
From what I can tell, you rely on the default behavior of C++ to initialize the value of StEvent/StFwdTrack.cxx::mIdTruth to zero (for all real tracks), as it isn't explicitly initialized to have a default value neither there nor here. Is this the desired approach?
| // needed since I use the StMuTrack | ||
| gSystem->Load("StarClassLibrary"); | ||
| gSystem->Load("StStrangeMuDstMaker"); | ||
| gSystem->Load("StMuDSTMaker"); |
There was a problem hiding this comment.
loadSharedLibraries() already loads StarClassLibrary, StStrangeMuDstMaker, StMuDSTMaker, so not needed here. Not sure if I missed others, but this isn't critical.
| cout << "LL1" << endl; | ||
| gROOT->LoadMacro("$STAR/StRoot/StMuDSTMaker/COMMON/macros/loadSharedLibraries.C"); | ||
| loadSharedLibraries(); | ||
| cout << "LL2" << endl; |
There was a problem hiding this comment.
These three LL0/1/2 cout lines can probably be dropped off now?
|
The last time all checks passed was with this commit: @jdbrice However, the next commit failed: @jdbrice Let's try to revert it to see if it really introduced a bug. |
This reverts commit 0241f63.
|
The test job 125 completed successfully after reverting the changes in @jdbrice's commit. Use FST z locations from geom to identify hit plane index This is the only test that uses the From this, I conclude that something in that commit causes the test to fail. |
|
Thank you @plexoos - I will investigate and try to resolve. |
…ult config handeling
…k projections with zero points with measurements
…ro. add LOG_INFO to identify test 125 failure
|
@plexoos, @klendathu2k - I have finally identified the seg fault, it occurs when I try to find from various possible sources an event primary vertex. This is the code that fails: if(mMuDstMaker && mMuDstMaker->muDst() && mMuDstMaker->muDst()->primaryVertex() ) {
mEventVertex.SetX(mMuDstMaker->muDst()->primaryVertex()->position().x());
mEventVertex.SetY(mMuDstMaker->muDst()->primaryVertex()->position().y());
mEventVertex.SetZ(mMuDstMaker->muDst()->primaryVertex()->position().z());
mFwdVertexSource = kFwdVertexSourceTpc;
return mEventVertex;
} as you can see I am testing for nullptrs, and in the most recent code I print out each pointer: The So I will temporarily remove this code to fix this PR so we can move forward with the next library, but this is likely indicative of some deeper issues with the MuDst Primary Vertex. Let me know if you have any thoughts |
|
Good catch, @jdbrice! Yeah, dereferencing an invalid pointer is a classic case of undefined behavior in C++, where it might work on one machine but crash on another. |
…f pointer being null
|
@plexoos - thanks for the suggestions that helped me find it. OK I 'd like to close this PR without adding anything else to it (except anything identified necessary by further review). I prefer to make any final changes needed for a new library in smaller more scoped PRs. So if you and @klendathu2k or @genevb etc can take a look, I will leave this alone for now. But it is ready from my side |
klendathu2k
left a comment
klendathu2k
left a comment
There was a problem hiding this comment.
I remain concerned about the GetPrimaryEventVertex method. Specifically in a full simulation, we should be fitting to the reconstructed rather than the simulated event vertex. That said, we should not hold up getting this ready for data production.
| mTreeData.fcsY.push_back( y0 ); | ||
| mTreeData.fcsZ.push_back( zepd ); | ||
| mTreeData.fcsDet.push_back( det ); | ||
| TVector3 StFwdTrackMaker::GetEventPrimaryVertex(){ |
There was a problem hiding this comment.
I haven't traced this through, but... if GetEventPrimaryVertex is used in the track fit, then I have some concerns with this implementation.
Here is my reading of what this does...
If the MC vertex is available, return it. If not, use the MuDst. If not... check to see if StEvent is available and return what we have. If not, return some default values.
Here are my concerns...
-
StEvent is our event data model. We do not need to create the MuDst to run our code, but StEvent must be available. It is the preferred source for the primary vertex.
-
Simulated vertices are returned if they are found, taking precedence if the vertex is available in StEvent / MuDst. This means that you will be treating simulated data and real data differently... you'll take the exact primary vertex for simulation (which is always the correct one) and the reconstructed primary vertex from data (which might not point to the real primary vertex).
There was a problem hiding this comment.
@klendathu2k Thanks for pointing this out - I agree with your concerns. I will address this in new PR - The primary vertex handling can still be improved in a couple of ways in addition to your comment here. I will follow this up promptly with a (draft) PR so that this is addressed.
| FillTrackDeltas(); | ||
|
|
||
| LOG_INFO << "Forward tracking on this event took " << (FwdTrackerUtils::nowNanoSecond() - itStart) * 1e-6 << " ms" << endm; | ||
| /**********************************************************************/ |
There was a problem hiding this comment.
Not a show stopper ... but I would split visualization into a separate maker.
There was a problem hiding this comment.
@klendathu2k - thanks for the suggestion. While I was able to separate almost all of the other non-essential parts of the tracking (matching, QA+ttree generation, etc.) I have not yet been able to move this out of the core FwdTrackMaker because it needs the in-memory GenFit track object. I will continue to look into ways to separate it out.
The visualization is OFF by default (only turned on if the user turns it on)
6 participants
These updates provide a new FWD tracking mode in which the track finding is one with FST information.
Tracks are then fit to valid Seeds and then (optionally) any FTT hits are added to the track if found along the trajectory.
This mode is needed for any analysis of real data at this point. That would include fast offline for ongoing data taking.