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 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
genevb,
plexoos,
fisyak,
R-Witt and
iraklic
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
|
| @@ -19,6 +19,9 @@ void StMuFwdTrack::set( StFwdTrack * evTrack) { | |||
| mCharge = evTrack->charge(); | |||
| mPrimaryMomentum = TVector3( evTrack->momentum().x(), evTrack->momentum().y(), evTrack->momentum().z() ); | |||
|
|
|||
| 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?
| @@ -121,6 +121,8 @@ class StMuFwdTrack : public TObject { | |||
|
|
|||
| // Number of points used in the track seed step | |||
| short numberOfSeedPoints() const; | |||
| UShort_t idTruth() const { return mIdTruth; } | |||
| UShort_t qaTruth() const { return mQATruth; } | |||
|
|
|||
|
|
|||
| void setPrimaryMomentum( TVector3 mom ) { mPrimaryMomentum = mom; } | |||
There was a problem hiding this comment.
I suggest switching to StThreeVector for speed. TVector3 has an additional overhead b/c it inherits from TObject. Which means that every instance of every vector you create will be registered in a global ROOT map, and subsequently destroyed when you call the destructor.
Looking at the MuDST codes... I see that we are using both TVector3 and StThreeVector (with x=float or double) to store vector quantities.... so consider this a soft suggestion.
| @@ -769,11 +828,12 @@ class TrackFitter { | |||
|
|
|||
| // get the seed info from our hits | |||
| TVector3 seedMom, seedPos; | |||
There was a problem hiding this comment.
[very small optimization...]
Since you are initializing these below... you can make these static variables and save the overhead of registering them with ROOT every time the ctor is called.
There was a problem hiding this comment.
Just took a quick look, and commented on some possible optimizations. TVector3 is used several places and, where ever possible, you should avoid calling the constructor. TVector3 registers itself with ROOT, which introduces an overhead. (We ran into this slowing down Sti O(10%) in the past as I recall). Otherwise I think this looks good. I'll take a closer look tomorrow.
| // we use d+4 so that both FTT and FST start at 4 | ||
| FwdHit *hit = new FwdHit(count++, x0, y0, vZ, d+4, 0, hitCov3, nullptr); | ||
| // Add the hit to the hit map | ||
| hitMap[d+4].push_back(hit); |
There was a problem hiding this comment.
I think this is the case where you want to be a bit more explicit with the code. You may know that you use smart pointers but it does look scary to a casual reviewer who is not aware of the internals.
| // we use d+4 so that both FTT and FST start at 4 | |
| FwdHit *hit = new FwdHit(count++, x0, y0, vZ, d+4, 0, hitCov3, nullptr); | |
| // Add the hit to the hit map | |
| hitMap[d+4].push_back(hit); | |
| auto hit = std::make_unique<FwdHit>(count++, hitInfo.x, hitInfo.y, hitInfo.z, hitInfo.d + START_INDEX_FOR_FST, 0, hitCov3, nullptr); | |
| hitMap[d + START_INDEX_FOR_FST].push_back(std::move(hit)); |
And BTW, why not create the hits on the stack?
| const float dz0 = fabs( vZ - 151.75 ); | ||
| const float dz1 = fabs( vZ - 165.248 ); | ||
| const float dz2 = fabs( vZ - 178.781 ); | ||
|
|
||
| int d = 0 * ( dz0 < 1.0 ) + 1 * ( dz1 < 1.0 ) + 2 * ( dz2 < 1.0 ); |
There was a problem hiding this comment.
Magic numbers... It took me a while to understand the meaning of 1.0 which is probably a pretty arbitrary value.
| @@ -37,13 +37,15 @@ class FwdDataSource { | |||
| for ( auto kv : mFttHits ){ | |||
| for ( auto h : kv.second ){ | |||
| delete h; | |||
| h = 0; | |||
There was a problem hiding this comment.
Oh, actually you are not using smart pointers for KiTrack hits... If you did
using HitMap_t = std::map<int, std::vector<std::unique_ptr<KiTrack::IHit>>>;it would simplify the cleanup code
for (auto& kv : mFttHits) {
kv.second.clear();
}
for (auto& kv : mFstHits) {
kv.second.clear();
}
mFttHits.clear();
mFstHits.clear();There was a problem hiding this comment.
Hits are now created on the stack and cleared each event by the StFwdTrackMaker
| mForwardTracker->initialize( geoCache, mGenHistograms ); | ||
| mForwardTracker->initialize( mGeoCache, mGenHistograms ); | ||
|
|
||
| // geometry should be available from here (mForwardTracker will initialize cache if needed) |
There was a problem hiding this comment.
I'm slightly concerned about pulling the z-locations from the geometry and having fallback values hardcoded here.
Is there at least logging in place to keep track of when / if we fall back to the default?
|
Thank you, Daniel, for addressing the comments! Everything looks good as far as I can tell. As a last request we discussed, could you point me to a recent data file with raw FST data from this run? I would like to extend our CI tests at https://github.com/star-bnl/star-sw/blob/main/tests/joblist.json#L1126, which already include data from 2023. |
- Improve memory management of FwdHits - Add FST and FTT detection methods to FwdHit class and update FwdGeomUtils - cleanup track fitter logic - Add additional track seed modes - use enums where possible for clearer intentions - remove histogram and ttree output from FwdTrackMaker and TrackFitter - Tests against data and simulation
Sorry for not answering so long. I am not ignoring your PR. I just have been super busy and this seems to be a substantial one, with the discussion that I have not really followed. I this it is better if I defer this to others, but I will go over this once I am back to the office if it is not done till then. |
|
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. |
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.