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

Replace trackingPhase1PU70 workflows with CA seeding #16911

Merged
merged 6 commits into from Dec 16, 2016

Conversation

makortel
Copy link
Contributor

@makortel makortel commented Dec 8, 2016

This PR switches the default phase1 pixel seeding to cellular automaton (tuning by @felicepantaleo). The workflows of trackingPhase1PU70 are replaced with workflows of the previous seeding (trackingPhase1QuadProp, suggestions for better name are welcome) to be kept as a reference for some time. This PR replaces the trackingPhase1PU70 workflows with CA seeding (tuning by @felicepantaleo). The goal is to switch the default phase1 tracking to CA seeding once the duplicate rate increase is better understood (and fixed/mitigated), therefore the "CA seeding" is already implemented under trackingPhase1 sub-era, and the default trackign under trackingPhase1QuadProp era (suggestions for better name are welcome). Full cleanup of trackingPhase1PU70 will come later (after the pre2 dust settles).

Here are MTV plots for 1000 ttbar+35PU events in 900pre1 (using 810pre16 DIGI-RAW as an input) comparing the CA seeding to the default.
https://mkortela.web.cern.ch/mkortela/tracking/validation/CMSSW_9_0_0_pre1_caseeding/
Timing decreases by ~5 % for the full trackingOnly reco job (becomes ~10 % when excluding most expensive other-than-iterative-tracking modules, mostly validation).

The previous seeding is kept workflows 10024.[45] (full/trackingOnly), so there should be no changes between old 10024.[01] and new 10024.[45].

To minimize merge conflicts the developments are based on @ebrondol's #16858 (in the hope of getting this in pre2).

Tested in CMSSW_9_0_X_2016-11-30-2300 (rebased on top of CMSSW_9_0_X after IB CMSSW_9_0_X_2016-12-09-1100 and before the next). No changes expected in standard workflows (special 10024.[45] show the changes from trackingPhase1PU70 to CA). for phase0, phase1 changes are described above. and for phase2 there are changes because of #16858 (but no changes on top of that).

@rovere @VinInn @felicepantaleo @ebrondol @mtosi

@makortel
Copy link
Contributor Author

makortel commented Dec 8, 2016

@cmsbuild, please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 8, 2016

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/16862/console Started: 2016/12/08 15:32

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 8, 2016

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

It involves the following packages:

Configuration/Eras
Configuration/PyReleaseValidation
Configuration/StandardSequences
RecoPixelVertexing/PixelTriplets
RecoTracker/FinalTrackSelectors
RecoTracker/IterativeTracking
RecoTracker/TkSeedingLayers
Validation/RecoTrack

@dmitrijus, @cvuosalo, @franzoni, @fabozzi, @cmsbuild, @srimanob, @slava77, @vanbesien, @hengne, @davidlange6 can you please review it and eventually sign? Thanks.
@ghellwig, @felicepantaleo, @GiacomoSguazzoni, @rovere, @VinInn, @Martin-Grunewald, @mschrode, @wmtford, @gpetruc, @dgulhan this is something you requested to watch as well.
@slava77, @smuzaffar you are the release manager for this.

cms-bot commands are listed here #13028

@makortel
Copy link
Contributor Author

makortel commented Dec 8, 2016

@felicepantaleo Could you double-check the CA parameters?

@felicepantaleo
Copy link
Contributor

@makortel looks good to me

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 8, 2016

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 8, 2016

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 8, 2016

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16911/16862/summary.html

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

@slava77
Copy link
Contributor

slava77 commented Dec 9, 2016

if #16885 is merged on Friday, this will likely need a rebase

@makortel
Copy link
Contributor Author

makortel commented Dec 9, 2016

Seems that the system is smart-enough to not claim a conflict here after #16885 merge (as the commits from #16858 are exactly the same as in #16885).

@slava77
Copy link
Contributor

slava77 commented Dec 9, 2016

@makortel
for diff and incremental testing purposes, could you please rebase to get rid of the other commits.

@makortel
Copy link
Contributor Author

makortel commented Dec 9, 2016

Sure, is there preference to do it now or wait an IB?

@slava77
Copy link
Contributor

slava77 commented Dec 9, 2016

do it now, I'll wait for IB to test

@rovere
Copy link
Contributor

rovere commented Dec 12, 2016

Ciao @slava77
at this stage your proposal sounds reasonable. I let @makortel work out all the necessary changes on top of this PR.

@cmsbuild
Copy link
Contributor

Pull request #16911 was updated. @dmitrijus, @cvuosalo, @franzoni, @fabozzi, @cmsbuild, @srimanob, @slava77, @vanbesien, @hengne, @davidlange6 can you please check and sign again.

@makortel
Copy link
Contributor Author

@cmsbuild, please test

Following the discussion above, I switched the "old" phase1 seeding back to the default. So essentially this PR now replaced trackingPhase1PU70 workflows with "CA seeding".

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 13, 2016

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/16997/console Started: 2016/12/13 14:33

@makortel makortel changed the title Switch default phase1 seeding to CA (and disable trackingPhase1PU70) Replace trackingPhase1PU70 workflows with CA seeding Dec 13, 2016
@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16911/16997/summary.html

@slava77 comparisons for the following workflows were not done due to missing matrix map:

  • 20034.0_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2023D7_GenSimHLBeamSpotFull14+DigiFull_2023D7+RecoFullGlobal_2023D7+HARVESTFullGlobal_2023D7
  • 23234.0_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2023D8_GenSimHLBeamSpotFull14+DigiFull_2023D8+RecoFullGlobal_2023D8+HARVESTFullGlobal_2023D8

@slava77
Copy link
Contributor

slava77 commented Dec 15, 2016

+1

for #16911 8a86cb5

  • code changes are as described (after rescoping this from a change of the default to just addition of a CA scenario)
  • jenkins tests pass and comparisons with baseline show no differences
  • some notes on results with CA enabled were posted earlier in the thread before the descope

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

6 participants