Skip to content
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

New cluster shape filter for phase2 + adding PixelPair #17891

Merged
merged 4 commits into from Mar 16, 2017

Conversation

VinInn
Copy link
Contributor

@VinInn VinInn commented Mar 11, 2017

This Pr includes three main changes (all connected )

  1. add a New cluster shape filter for phase2
    1a) I explicitly added two files for two efficiency WP 99.5 and 99.9 the default (at the moment a copy) is 99.5
  2. add (back) a PixelPair iteration to recover any inefficiency of the triplet step
    (so PixelPair cover only the regions not covered by quadruplets)
  3. make HighPtTriplets and PixelPair seed-extensions...

MTV
1000 single muon (blue pre5, red WP99p5, orange Wp99p9, black this PR)
http://innocent.home.cern.ch/innocent/RelVal/Muon10csf99/plots_ootb/effandfake1.pdf
250 ttbar PU200 (blue pre5, red WP99p9, black Wp99p5, orange this PR)
http://innocent.home.cern.ch/innocent/RelVal/ttbar200PUcsf/plots_ootb/effandfake1.pdf

timing ttbar PU200 tracking only +DQM
900_pre5

TimeReport ---------- Event  Summary ---[sec]----
TimeReport       event loop CPU/event = 202.108402
TimeReport      event loop Real/event = 35.805349
TimeReport     sum Streams Real/event = 203.915857

this PR

TimeReport ---------- Event  Summary ---[sec]----
TimeReport       event loop CPU/event = 146.413452
TimeReport      event loop Real/event = 25.872114
TimeReport     sum Streams Real/event = 148.02895

obsoletes #17810

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @VinInn (Vincenzo Innocente) for master.

It involves the following packages:

RecoPixelVertexing/PixelLowPtUtilities
RecoTracker/FinalTrackSelectors
RecoTracker/IterativeTracking

@perrotta, @cmsbuild, @slava77, @davidlange6 can you please review it and eventually sign? Thanks.
@ghellwig, @makortel, @felicepantaleo, @GiacomoSguazzoni, @rovere, @VinInn, @mschrode, @gpetruc, @dgulhan this is something you requested to watch as well.
@Muzaffar, @davidlange6, @smuzaffar you are the release manager for this.

cms-bot commands are listed here #13028

@VinInn
Copy link
Contributor Author

VinInn commented Mar 11, 2017

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 11, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/18350/console Started: 2017/03/11 14:44

@VinInn
Copy link
Contributor Author

VinInn commented Mar 11, 2017

assign upgrade

@VinInn
Copy link
Contributor Author

VinInn commented Mar 11, 2017

@kandrosov @venturia

@VinInn
Copy link
Contributor Author

VinInn commented Mar 11, 2017

@boudoul

@VinInn
Copy link
Contributor Author

VinInn commented Mar 11, 2017

@ebrondol

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

@@ -150,10 +158,12 @@
)
trackingPhase1PU70.toModify(highPtTripletStepTrajectoryBuilder,
MeasurementTrackerName = '',
maxCand = 4,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@makortel do you want to change this? I thought that this era was frozen.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't matter, trackingPhase1PU70 is not used in any workflow, and I'll clean it up sometime soon.

@ebrondol
Copy link
Contributor

Thanks @VinInn . Can you please describe the legend in the files you have posted?

@slava77
Copy link
Contributor

slava77 commented Mar 13, 2017

assign upgrade

@cmsbuild
Copy link
Contributor

New categories assigned: upgrade

@kpedro88 you have been requested to review this Pull request/Issue and eventually sign? Thanks

@slava77
Copy link
Contributor

slava77 commented Mar 16, 2017

@VinInn
what is the purpose of the new files?
They are not used in this PR.

Is it "for upcoming developments"?

@VinInn
Copy link
Contributor Author

VinInn commented Mar 16, 2017

I added the files to allow people to easily test different options changing just the config...

@slava77
Copy link
Contributor

slava77 commented Mar 16, 2017

Some plots from PU200 (baseline 900pre6):

some large reduction of "bad" PVs
all_sign880vsorig_ttbar14tev2023d4timingpu200localwf23034p0c_log10recovertexs_offlineprimaryvertices__reco_obj_zerror

Pretty large reduction in electron seeds (/2.5) with better matching ones retained
all_sign880vsorig_ttbar14tev2023d4timingpu200localwf23034p0c_recoelectronseeds_electronmergedseeds__reco_obj_drz1

downstream we have two times less electronGsfTracks
all_sign880vsorig_ttbar14tev2023d4timingpu200localwf23034p0c_recogsftracks_electrongsftracks__reco_obj_eta

ecal driven electron reduction is mainly in the barrel (apparently mostly fakes; based on MTV GSF eff plots)
all_sign880vsorig_ttbar14tev2023d4timingpu200localwf23034p0c_recogsfelectrons_ecaldrivengsfelectrons__reco_obj_eta

Inclusive muon distribution is still full of fakes [a part of it is a feature of what's reconstructed/saved, not necessarily fake tracks]
all_sign880vsorig_ttbar14tev2023d4timingpu200localwf23034p0c_recomuons_muons__reco_obj_eta

number of generalTracks is down (almost all of this reduction is from tracks with less than 7 hits)
all_sign880vsorig_ttbar14tev2023d4timingpu200localwf23034p0c_recotracks_generaltracks__reco_obj_eta

PF CH (~high purity tracks) are less affected suggesting relatively high purity here already
all_sign880vsorig_ttbar14tev2023d4timingpu200localwf23034p0c_recopfcandidates_particleflow__reco_obj_eta43

.... ..... ..... ......

DQM plots:

wf 21211 (10 muons extended eta range)
muon efficiencies have some visible loss in eta>3.5, while below that the efficiency looks about the same (a bit up/down in a few bins)
wf21211_hp_eff_eta

In PU200 ttbar
track fake rate is down by x2 or even more in some regions [in line with the supplied MTV plots]
mainly at low pt
wf23034pu200_hp_fake_eta
wf23034pu200_hp_fake_eta

PV fake rate is down
wf23034pu200_offpv_fake_vs_numv

b-tagging is improved moderately
wf23034pu200_csvv2_l_v_b

.... ..... ..... ......

On the technical side, based on PU200:

  • miniAOD output size is down by 5%; RECO (which is actually FEVT) is down by 10%
  • total CPU time in reco+miniAOD is reduced by 1.5 from 600 to 400 s/event [in my 10HS06 seconds]
    • iterative tracking "proper" (*Step* module names from tracking) is down by a bit more than twice: from 270 s to 120 s/evt
    • note that this check is before 4D PV timing went up by x3; after this PR in 91X it's probably heavier than all of tracking
  • memory is down by about 200 MB/core (the job includes validation sequences; proper reco may reduce by less)

Overall, looks like a very welcome change.

@kpedro88
Copy link
Contributor

Indeed, a nice improvement both for physics and resource usage

@slava77
Copy link
Contributor

slava77 commented Mar 16, 2017

+1

for #17891 bb2e881

  • implemented as described; main improvement are from the cluster shape cuts with some recovery from the pixel doublets
  • jenkins tests pass and comparisons with baseline show changes only in 2023 workflows; changes start from tracking as expected
  • local tests with 10-mu and TTbarPU200 samples show improvements in behavior, as posted above

@kpedro88
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @Muzaffar, @davidlange6, @smuzaffar

@davidlange6
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 5e507f4 into cms-sw:master Mar 16, 2017
@ebrondol
Copy link
Contributor

@slava77 did you also performed the check on D11? Or just a rough idea if my report in PR #17919 is compatible with your report?

@slava77
Copy link
Contributor

slava77 commented Mar 17, 2017 via email

@ebrondol
Copy link
Contributor

As far as I understood, the nomenclature T3 and T5 will remain the same with the idea of having a "validated tracker" (T3) and a "testing tracker" (T5). If T5 will succeed the tests and the validation we are going to do with the next RelVal production, it will become the new default (T3).
@boudoul @kpedro88 please confirm if I am correct.

@slava77
Copy link
Contributor

slava77 commented Mar 17, 2017 via email

@kpedro88
Copy link
Contributor

@ebrondol that wasn't really my understanding... if there's going to be another new tracker, I assumed it would get its own number. What you propose could be done, but since we are preparing for more TDR productions, we need to be very careful about changing the baseline geometry.

@boudoul
Copy link
Contributor

boudoul commented Mar 17, 2017

Dear all , we are keeping T3 , mostly because all production which has been done so far (and presumably more to come) are all based on T3 - Physics results for the tracker TDR are and will be based on T3 - However T3 is not a version that eventually will be in the real detector, and we are devloping T5 with with respect- T5 will be used to derive tracker-tracking performance (eventually to be also added within the TDR) , but indeed we should keep T3 (not merging or at least not in a short time scale defined by TDRs)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants