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

Make track algo priority order dynamic #17771

Merged
merged 3 commits into from Mar 11, 2017
Merged

Conversation

makortel
Copy link
Contributor

@makortel makortel commented Mar 3, 2017

Currently the "track algo priority order", that is used in track collection merging to dictate the order of iterations, is hardcoded. The order is the one of run2 tracking, but phase1 and phase2 tracking have the iterations in different orders. This PR proposes to make this iteration ordering dynamic via ESProduct TrackAlgoPriorityOrder that is configured via tracking eras using iterativeTkConfig.py. The TrackAlgoPriorityOrder starts with the hardcoded order, and reorders only the algos that are given as parameters.

Tested in CMSSW_9_1_X_2017-03-02-2300. There can be tiny changes in non-run2-tracking in cases the "best algo" gets changed by this reordering and the other track has slightly different parameters. Anything else (run2 tracking, HLT) should have no changes. (and all changes should be caused by the second commit alone, the first one being purely technical)

@rovere @VinInn

…oPriorityOrder ESProduct

The order of (offline) tracking iterations depends on tracking era, so
something like this is needed for proper bookkeeping of originalAlgo.
@makortel
Copy link
Contributor Author

makortel commented Mar 3, 2017

@cmsbuild, please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 3, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/18133/console Started: 2017/03/03 18:58

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 3, 2017

A new Pull Request was created by @makortel (Matti Kortelainen) for master.

It involves the following packages:

DQM/BeamMonitor
FastSimulation/Tracking
HLTrigger/Configuration
RecoHI/HiMuonAlgos
RecoHI/HiTracking
RecoLocalTracker/SubCollectionProducers
RecoMuon/Configuration
RecoMuon/L3MuonProducer
RecoTracker/FinalTrackSelectors
RecoTracker/IterativeTracking

@perrotta, @cmsbuild, @civanch, @lveldere, @silviodonato, @cvuosalo, @fwyzard, @ssekmen, @mdhildreth, @dmitrijus, @Martin-Grunewald, @slava77, @vanbesien, @davidlange6 can you please review it and eventually sign? Thanks.
@ghellwig, @felicepantaleo, @forthommel, @abbiendi, @MiheeJo, @Martin-Grunewald, @threus, @geoff-smith, @battibass, @jhgoh, @jlagram, @HuguesBrun, @OlivierBondu, @mandrenguyen, @rociovilar, @yduhm, @GiacomoSguazzoni, @rovere, @VinInn, @bellan, @jalimena, @mschrode, @dgulhan, @echapon, @trocino, @gbenelli, @jazzitup, @calderona, @nickmccoll, @yenjie, @kurtejung, @gpetruc, @matt-komm, @yetkinyilmaz, @bachtis 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

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 3, 2017

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 3, 2017

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 3, 2017

# CMSSW version specific customizations
def customizeHLTforCMSSW(process, menuType="GRun"):
# add call to action function in proper order: newest last!
# process = customiseFor12718(process)
process = customiseForXXXXX(process)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please replace, twice, XXXXX by 17771.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, fixed, thanks.

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 4, 2017

Pull request #17771 was updated. @perrotta, @cmsbuild, @civanch, @lveldere, @silviodonato, @fwyzard, @ssekmen, @mdhildreth, @dmitrijus, @Martin-Grunewald, @slava77, @vanbesien, @davidlange6 can you please check and sign again.

@makortel
Copy link
Contributor Author

makortel commented Mar 4, 2017

@cmsbuild, please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 4, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/18143/console Started: 2017/03/04 18:45

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 4, 2017

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 4, 2017

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 4, 2017

@silviodonato
Copy link
Contributor

+1

@dmitrijus
Copy link
Contributor

+1

@perrotta
Copy link
Contributor

perrotta commented Mar 9, 2017

Tiny differences are observed in the phase 1 and phase 2 tracking distribution and none in the run2 ones, as expected.

Jenkins pretends there are also difference in HLT, see e.g.
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_9_1_X_2017-03-03-1100+17771/18573/10824.0_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2018_GenSimFull+DigiFull_2018+RecoFull_2018+ALCAFull_2018+HARVESTFull_2018/HLT__Tracking_ValidationWRTtp_hltIter0PFlowTrackSelectionHighPurity_hltAssociatorByHits_h_phipulleta_Mean.png
(where it is indeed hard to spot any difference by eyes in those histos, though...)
or
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_9_1_X_2017-03-03-1100+17771/18573/21234.0_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2023D4_GenSimHLBeamSpotFull14+DigiFullTrigger_2023D4+RecoFullGlobal_2023D4+HARVESTFullGlobal_2023D4/Tracking__TrackParameters_highPurityTracks_pt_0to1_GeneralProperties_originalAlgorithm_GenTk.png
(where it looks like one track is actually selected by a different iteration)

@makortel : do you have any explanation for those tiny differences in HLT tracking (as you claimed they were not expected)?

@slava77
Copy link
Contributor

slava77 commented Mar 9, 2017

@makortel
do we expect further changes in the order of iterations in phase-1?

@makortel
Copy link
Contributor Author

makortel commented Mar 9, 2017

@slava77

do we expect further changes in the order of iterations in phase-1?

No(t for this year at least).

@makortel
Copy link
Contributor Author

makortel commented Mar 9, 2017

@perrotta The first plot (*_Mean, also *_Sigma) is produced by gaussian fits in the harvesting step. In practice if the input plots for one of them changes, it may induce tiny numerical changes in others that had no changes in their input histograms (IIRC caused by internal state of minuit not being reset between fit calls or something).

The second plot is from offline tracking, where tiny changes in the algo assignment are expected.

@perrotta
Copy link
Contributor

+1
Because of #17771 (comment) and the answer in #17771 (comment) (indeed, the second plot was from offline tracking, not from HLT...)

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