Conversation
Add flags for tracking/seeding production Conditional creation of products Per-iteration heap allocations moved out of the loop Remove unnecessary OT hit loop + sort for pLS candidates Seed creator initiation out of the loop
VourMa
force-pushed
the
simplifyLSTInputOutput
branch
from
72ae121 to
8f4f2fa
Compare
| edm::EDPutTokenT<TrajectorySeedCollection> trajectorySeedpLSPutToken_; | ||
| edm::EDPutTokenT<TrackCandidateCollection> trackCandidatePutToken_; | ||
| edm::EDPutTokenT<TrackCandidateCollection> trackCandidatepTCPutToken_; | ||
| edm::EDPutTokenT<TrackCandidateCollection> trackCandidateT4T5TCPutToken_; |
There was a problem hiding this comment.
I think these could still be const if you fold the initialization to the constructor initializer list
There was a problem hiding this comment.
I wanted to make these collections be produced conditionally. Unless I am missing some trick, to do that I would have to add them to the constructor (and not the constructor initializer list), and hence they cannot be made const. Or maybe I misunderstood your comment?
There was a problem hiding this comment.
check with git grep --cached -2 "\? [^$]*consumes[^$]*" for existing patterns.
It will be token_(flag ? consumes() : defaultConstructed )
| using Hit = SeedingHitSet::ConstRecHitPointer; | ||
| std::vector<Hit> hitsForSeed; | ||
| GlobalTrackingRegion region; | ||
| seedCreator_->init(region, iSetup, nullptr); |
There was a problem hiding this comment.
is it a large saving? The interface of SeedCreator to making seeds is non-const
virtual void makeSeed(TrajectorySeedCollection& seedCollection, const SeedingHitSet& hits) = 0;
I'd rather figure out how to make this seed making call const before moving the init outside the use loop
There was a problem hiding this comment.
The timing gain is really minor:
LSTOutputConverter: 55.0 → 54.6 ms (-1%)
I think it makes sense to drop this change, if we suspect it could cause issues.
| std::vector<Hit> hits; | ||
| for (unsigned int iHit = 0, nHits = proto.recHitsSize(); iHit < nHits; ++iHit) { | ||
| TrackingRecHitRef refHit = proto.recHit(iHit); | ||
| if (refHit->isValid()) | ||
| hits.push_back((Hit)&(*refHit)); | ||
| } | ||
| sort(hits.begin(), hits.end(), HitLessByRadius()); |
There was a problem hiding this comment.
sorting and invalid hit filtering should probably be configurable and done regardless of removeOTRechits_
There was a problem hiding this comment.
These lines are a direct copy from:
so this part just replicates the behavior from the code below.
Would you like me to test what happens if I don't apply this sorting+filtering?
The only case that this sorting+filtering is not applied is when useProtoTrackKinematics and not removeOTRechits_. But I wouldn't like to change this general behavior, as it will have effects to the Run 3 reconstruction potentially.
There was a problem hiding this comment.
I implied that after my updates removeOTRechits_ can be set to false, then we'd likely want to have the same filtering and sorting still present here
| if (hits.size() > 2) { | ||
| SeedFromProtoTrack seedFromProtoTrack(config_, proto, SeedingHitSet(hits), es); | ||
| if (seedFromProtoTrack.isValid()) | ||
| (*result).push_back(seedFromProtoTrack.trajectorySeed()); | ||
| } |
There was a problem hiding this comment.
as a part of debugging the efficiency loss: please check if this part ever fails to produce seeds (IIUC the number of pixel hits is still expected to be at least 3, the question is if the SeedFromProtoTrack construction sometimes makes invalid seeds
2 participants
Commits d35a0eb to 43379c7 are fully technical, resulting in the same physics performance (validation plots here) and the following timing reduction (baseline timing vs. PR timing):
Commit 8f4f2fa applies a physics change for the new tracking baseline (validation plots here) and the following timing reduction (baseline timing vs. PR timing):
I have removed the commit where the
LSTOutputConverterproducts are completely removed (it will have to wait for #227).