-
Notifications
You must be signed in to change notification settings - Fork 15
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.
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
More validation overhaul: get pulls correctly, plus more fixups. Include Steve's fixes for compiling on mic, and use of SDE. #69
More validation overhaul: get pulls correctly, plus more fixups. Include Steve's fixes for compiling on mic, and use of SDE. #69
Conversation
actually, even after @srlantz makes his commit, can you wait? I want to double-check something before the merge. |
…ARK switches that can be used by SDE to sync its output with the __itt_pause() and __itt_resume() calls used by VTune (all are from the same -littnotify).
OK ... thanks ... waiting for green light :) |
…some gymnastics to work additional changes: * removed flat pz from toyMC simulation --> factor of 2.3 was to get tracks within 1.0 in eta * fixed up a warning on read-in * removed HitID and layerHitMap_ from Event --> made objects local, or replaced with simpler structures * ensured that values used in pulls correspond the track actually matched
… into full-det-tracking
} | ||
|
||
// XXMT4K What does this do? Needs change? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is used by the SMatrix building. Before, we chunked hits into eta-phi bins by sorting them and then getting the hit indices.
so this is legacy for smatrix building... I made the necessary changes to the remapping, while dropping HitID and layerHitMap_ from Event.
} | ||
#ifdef TBB | ||
}); | ||
#endif | ||
|
||
// do some work to ensure everything is aligned after multithreading |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
simTrackStates_ now just a single vector indexed directly by mcHitID, since at least every MC hit is produced by a propagated simtrack trackstate. With multithreading, the indices are all screwed up, so we have to sort both simHitsInfo_ and simTrackStates_.
Since simTrackStates_ is a simple vector of track states (could imagine making it a struct with the trackstate vector + an int for mcHitID), have to do unfortunately do a temporary copy.
// XXMT4K My attempt at rewriting the code below - not all layers are traversed with endcap. | ||
// Note ... this still asusmes single hit per layer, right? | ||
// What is the comment about thread safety??? The final index is on itrack and this will clash due to cache-line overlaps. | ||
// XXKM4MT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this needs discussion: i.e. how do we want to handle simtracks that produce more than one layer per hit.
this would not take too much of a code rewrite, but would require a little thought. given that reco tracks only pick up one hit/layer, it might make sense to make a separate inherited SimTrack class to handle the case where a simtrack produces two hits (or more) per layer.
For example, if a simtrack passes through an overlap region, we will still want to store each hit into the simtrack indices (so expand the array inside the track class), but denote them in the simHitInfo with either ithLayerHit, or an inner/outer flag. To say nothing of loopers...
#else | ||
// generate flat in pz | ||
|
||
// XXMT4K WTF is 2.3 coming from? What do we do here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2.3 is the factor needed to get the tracks between |eta|<1.0, if you do the log, tangent, etc. since we moved away from flat pz awhile ago, I went ahead and removed it.
if we really want it back, we should do it when we do not have the hack in place to avoid the transition region so we can more elegantly write the proper pz function based on max eta.
@@ -479,6 +473,20 @@ void TTreeValidation::mapSeedTkToRecoTk(const TrackVec& evt_tracks, const TrackE | |||
} | |||
} | |||
|
|||
int TTreeValidation::getLastGoodHit(const int trackMCHitID, const int mcTrackID, const Event& ev) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this section of the code checks to make sure that the sim trackstate we want to make for the pull is actually on the sim track the reco track was matched to.
if not, then try to find the corresponding mc hit to get the sim track state for the matched sim track.
if that fails, fill -101 (see L333, etc).
{ | ||
return hitsOnTrk_[nGoodHitIdx_-1].layer; | ||
} | ||
int getLastGoodMCHitID(const std::vector<HitVec>& globalHitVec) const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gah, have to remember that nGoodHitIdx starts at 0, not -1 like hitIdxPos!
these functions will return the corresponding hit idx, hit layer, or mcHitID for the last good hit on a track
Okay, whew. A bit more than a few line change. Needed to redo a significant amount to get proper layers for endcaps, which meant rewriting a bit of the validation code. To get an idea of where we are now in terms of efficiency, fake rate, duplicate rate (which is identically 0 by definition when seeds == sim tracks), and some pT pulls: One thing to notice right away is that the pulls are not the best. We are biased by a good amount to the right, well outside the error on the fit. The pulls also do not full reach the top of peak (and in log, the tails are non-gaussian). I guess for now this is okay, given the efficiency. One oddity is the fake rate phi... you may notice it is outside the range of atan2 (i.e. |pi|). Since the value being plotted is the reco phi, it is possible that the propagation + update on any given layer does not have to land with pi, given we are using CCS coordinates, and so phi is not computed from atan2(py,px), but rather by the KF derivatives. This may suggest that before moving onto any layer to check the phi on every layer, and if it outside the bounds, reset the track state phi parameter to within |pi|. This is ready to be merged whenever. |
Hi @osschar
I forgot to add this file to the PR last night... sorry about that. Although, by chance, this is a blessing in disguise as @srlantz wanted to piggyback on my previous PR with some minor fixups (didn't see the email until I woke up this morning).
So Steve, can you add your commits directly to this PR, and then Matevž can make the merge?
Peace,
Kevin