Phase 2 seeding quadruplets: from merging triplets to propagation #16858
Conversation
|
A new Pull Request was created by @ebrondol for CMSSW_9_0_X. It involves the following packages: RecoTracker/IterativeTracking @cmsbuild, @cvuosalo, @slava77, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here #13028 |
|
@cmsbuild , please test |
|
The tests are being triggered in jenkins. |
|
I am finalizing a less technical PR on top of this to further reduce the timing (down to 200 sec w/r/t 290 above) and improve vertex performances that are rather poor at the moment... I expect eventually two more PR: removing PixelPairs (save 10 sec) and removing PixelTrack&Vertices (saves 16 sec) NB the timing above is for TrackingOnly wf: it includes ~30sec of validation that does not run in standard wf |
|
Comparison job queued. |
|
Comparison is ready The workflows 1003.0, 1001.0, 1000.0, 140.53, 136.731, 4.22 have different files in step1_dasquery.log than the ones found in the baseline. You may want to check and retrigger the tests if necessary. You can check it in the "files" directory in the results of the comparisons |
|
Hi @slava77 this PR can be closed if this can simplify the review process. We would like to present both the PRs tomorrow in order to understand how we can proceed best. The PR #16885 involve also other files and the review can take longer.. but of course if this would not be the case, the best option is going with one PR. |
|
On 12/6/16 7:23 AM, ebrondol wrote:
Hi @slava77 <https://github.com/slava77> this PR can be closed if this
can simplify the review process. We would like to present both the PRs
tomorrow in order to understand how we can proceed best. The PR #16885
<#16885> involve also other files
and the review can take longer.. but of course if this would not be the
case, the best option is going with one PR.
From a quick check of the code, I think that having one combined PR is
just as long to review.
The only danger is that the additions in the combined PR have a bug.
Also, since the other PR includes this one, if you still want this
reviewed and merged,
then the other one will have to wait until this is merged, then cleaned
up and rebased.
…
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#16858 (comment)>, or
mute the thread
<https://github.com/notifications/unsubscribe-auth/AEdcbmZyw3Hc02cBleQo7yfiBCp35r2hks5rFX4NgaJpZM4LEGdU>.
|
|
On 6 Dec, 2016, at 4:38 PM, Slava Krutelyov ***@***.***> wrote:
From a quick check of the code, I think that having one combined PR is
just as long to review.
The only danger is that the additions in the combined PR have a bug.
Also, since the other PR includes this one, if you still want this
reviewed and merged,
then the other one will have to wait until this is merged, then cleaned
up and rebased.
My understanding is that we do not have much time.
Otherwise we had queued three (even for PR), waiting the previous to be merged before proceeding.
So this is a one-shot game: either #16885 is ok (or easily fiexed) and gets integrated asap
or we revert to this one (and sorry for the additional time/memory saving).
No major iteration on #16885: if it fails will be closed and moved to January.
v.
|
|
-1 superseded by #16885 |
Before this PR, the Phase 2 quadruplets was using the merging triplets approach, now we move in line with Phase 1 where the quadruplets are created propagating the triplets. This new algorithm has been already extensively discussed in #14327.
Performance without PU on 1000 events ttbar, D4 geometry:
About the eff/fakes performance, the results have been produced here. We recover something at low pT without getting more fakes. In particular I think because the efficiency of the seeding itself with the new algorithm is increasing both eff and fake, but then in the track reconstruction itself, the fakes are rejected pretty good. I suppose it can be consider a pretty good result.
About timing and memory, the baseline results have been produced here while including the PR can be found here. Thankfully, we can see that the PixelQuadrupletMergerEDProducer::produce(edm::Event&, edm::EventSetup const&) goes down quite a bit in positions. So everything seem to work pretty as expected. I am not sure that the absolute comparison is correct - maybe I messed up with some machines - but if we take the MuonId as reference, in the ph2_movingQuadSeeding version should be ~17 counts where instead is ~3.
For higher PU, this is the report from @VinInn:
full job, ttbar 200PU, tracking only incl validation&dqm (step3)
vinavx2 (3.5GHz, 4thread)
91x out-of-the-box TimeReport event loop CPU/event = 406.026545
+git cms-merge-topic ebrondol:ph2_movingQuadSeeding TimeReport event loop CPU/event = 291.797381
and is even more efficient at low-pt
http://innocent.home.cern.ch/innocent/RelVal/tkRecoD4_newQuad/plots_highPurity/effandfake1.pdf
This PR is considered from the TRK POG a prority not only for the much better performance, but also because of further developments expected for Phase 1 by @makortel . Best in the next pre-release.
Informing @boudoul @atricomi @delaere