Updates to precision timing integration in reconstruction and miniaod #20338
Conversation
|
The code-checks are being triggered in jenkins. |
|
A new Pull Request was created by @bendavid (Josh Bendavid) for master. It involves the following packages: CommonTools/RecoAlgos @perrotta, @smuzaffar, @civanch, @Dr15Jones, @mdhildreth, @monttj, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
+code-checks |
|
(The actual implementation of the workflow to reproduce miniaod with timing on top of AODSIM produced with this requires some modest additional changes and can come later) |
|
@cmsbuild please test |
|
Does this pull request include unnecessary commits? 48 commits starting from Dec 2016 seems strange. |
|
The tests are being triggered in jenkins. |
|
No, this is really 48 commits with work done starting in Dec. 2016. We started a pull request with some of these changes at the beginning of the year, but got stuck because we did not manage to disentangle the changes to the default reco behaviour at the time. |
|
+1 |
|
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
Comparison job queued. |
|
The tests are being triggered in jenkins. |
|
Pull request #20338 was updated. @perrotta, @smuzaffar, @civanch, @Dr15Jones, @mdhildreth, @monttj, @cmsbuild, @kpedro88, @slava77 can you please check and sign again. |
|
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
| generalTrackTimes.push_back(time); | ||
| } | ||
| else { | ||
| float rndtime = CLHEP::RandGauss::shoot(rng_engine, meanSimTime, rmsSimTime); |
There was a problem hiding this comment.
[the functionality has been here before; I perhaps notice it more obviously only now]
this leads to non-reproducibility of RECO in multithreaded jobs and is technically in violation of the policy.
The seed should be reset to something based on event or track quantities.
It may be OK for now as long as the 4D vertex and track times are used only for checks or tests and do not contribute to the downstream reco.
I made #20621 to keep track of this issue.
There was a problem hiding this comment.
I get the appropriate random number engine based on the stream ID from the RNG service in the produce() method and pass it down. It should be OK... no?
There was a problem hiding this comment.
I guess not. It's not about streams. Events can be executed in different order, even if there is a continuous tracking of the seed/random engine state per stream.
There was a problem hiding this comment.
The RandomNumberGeneratorService associates random number sequences to streams. However, when running a job, the association of events to streams is not-reproducible (the same event may be assigned to different streams in different jobs).
The standard for reconstruction, even before multi-threading, was to have the module seed the random number generator based on a property of the event.
There was a problem hiding this comment.
Gotcha. I'll try to make a PR soon to address this. Trying to attack the 4D vertex speed issue from a slightly different direction first.
|
+1
|
|
+1 |
|
+1 |
|
+1 |
|
@monttj Please sign! |
|
@davidlange6 |
|
@davidlange6 We need this relatively soon! Thanks! |
|
merge |
Update low level assignment of MIP timing to tracks. (Implement acceptance cuts, improved handling of fake tracks, and fixes the handling of displaced tracks)
Update 4D Vertexing (including the changes in #19935)
Updates to integration of timing in reconstruction (but no change to default behaviour or collections beyond the time stored in pfcandidates, which remains unused downstream)
Addition of timing to packed candidates (but this is only partially useful unless miniaod is moved to the 4d vertices, currently not possible to have both sets of vertices in parallel, so miniaod stays with the non-timing vertices by default)
AODSIM produced with this pull request included can be easily processed into consistent miniaod with timing for further timing studies, so it would be useful to have for further TDR production.
There should be no changes to default reco/collections/miniaod beyond the not-used-downstream timestamps in the PFCandidates and PackedCandidates as said, but also understood if the code changes are considered too large to integrate for HGCal TDR production. These changes accumulated into our development branches for quite a while because we had so far not managed to disentangle them from changes to the default reco behaviour as finally done here.
@lgray