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

Vectorized 2D simulated annealing vertexing #19935

Merged
merged 16 commits into from Sep 12, 2017

Conversation

lgray
Copy link
Contributor

@lgray lgray commented Jul 27, 2017

Implement vectorized 2D simulated annealing for vertices with timing.
Use as default vertexing.

Performance measurements in progress.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @lgray (Lindsey Gray) for master.

It involves the following packages:

RecoVertex/Configuration
RecoVertex/PrimaryVertexProducer

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

cms-bot commands are listed here

@lgray
Copy link
Contributor Author

lgray commented Jul 27, 2017

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 27, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/21815/console Started: 2017/07/27 13:25

@lgray
Copy link
Contributor Author

lgray commented Jul 27, 2017

@bendavid FYI

@lgray
Copy link
Contributor Author

lgray commented Jul 27, 2017

@cmsbuild please abort

@cmsbuild
Copy link
Contributor

Jenkins tests are aborted.

@kpedro88
Copy link
Contributor

@slava77 you should be able to see the vertex validation results in WF 20434.0 (2023D19, w/ timing layer)

@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-19935/22871/summary.html

There are some workflows for which there are errors in the baseline:
1330.0 step 4
The results for the comparisons for these workflows could be incomplete
This means most likely that the IB is having errors in the relvals.The error does NOT come from this pull request

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 17 differences found in the comparisons
  • DQMHistoTests: Total files compared: 26
  • DQMHistoTests: Total histograms compared: 2563849
  • DQMHistoTests: Total failures: 1014
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2562649
  • DQMHistoTests: Total skipped: 186
  • DQMHistoTests: Total Missing objects: 0
  • Checked 107 log files, 14 edm output root files, 26 DQM output files

@kpedro88
Copy link
Contributor

+1

@slava77
Copy link
Contributor

slava77 commented Sep 12, 2017

Compared to my last checks #19935 (comment)
with the latest version the agreement with the baseline is much better:

There are 2% fewer vertices, but the parameters appear to be much more similar than before
all_sign948-2023pu200vssign948base-2023pu200_ttbar14tev2023d17putimingwf20234p2c_recovertexs_offlineprimaryvertices4d__reco_obj_trackssize

all_sign948-2023pu200vssign948base-2023pu200_ttbar14tev2023d17putimingwf20234p2c_log10recovertexs_offlineprimaryvertices4d__reco_obj_zerror

Most relevant diffs since then are:

-        Tpurge = cms.double(3.0),         # cleaning
+        Tpurge = cms.double(4.0),         # cleaning

-  double Tmin = conf.getParameter<double> ("Tmin");
-  double Tpurge = conf.getParameter<double> ("Tpurge");
-  double Tstop = conf.getParameter<double> ("Tstop");
+  double minT = conf.getParameter<double> ("Tmin")*std::sqrt(2.0);
+  double purgeT = conf.getParameter<double> ("Tpurge")*std::sqrt(2.0);
+  double stopT = conf.getParameter<double> ("Tstop")*std::sqrt(2.0);
+  coolingFactor_ = std::sqrt(coolingFactor_);

PV validation also makes sense
Efficiency is ~unchanged
wf20234p2pu200_pv_eff_ntrk
Duplicate rate is down
wf20234p2pu200_pv_dup_vs_pv
There are more "pure" vertices (matched and not split or duplicates)
wf20234p2pu200_pv_reco2gen_props

Matched vertices are slightly more separated
wf20234p2pu200_pv_r2gen_match_dz

Still, there is some loss of response in matched PVs
wf20234p2pu200_pv_reco2gen_resolpt2

CPU time is down by a factor of 6 (from 240 to ~40 s/evt).

@slava77
Copy link
Contributor

slava77 commented Sep 12, 2017

+1

for #19935 163dda9

  • vectorized and retuned ZT DA vertexing, to be used in timing eras for phase-2 (the old non-vector version is removed)
  • jenkins tests pass and comparisons show small changes in 4D PV reco plots in wf 20434 (which includes the timing vertices)
  • local tests show roughly expected behavior: expected speedup and reasonably similar physics performance as observed in Vectorized 2D simulated annealing vertexing #19935 (comment)

At some point, follow up to this PR is expected following #20417

@lgray
Copy link
Contributor Author

lgray commented Sep 12, 2017

@vanbesien, @kmaeshima, @vazzolini could one of you please sign for DQM? Thanks!

@lgray
Copy link
Contributor Author

lgray commented Sep 12, 2017

@slava77 OK looks like there can be further tuning of the clustering parameters to further improve the situation. Similarly, looking how how the 1D annealing was spruced up for Phase 1, I have a few ideas to follow up for this. However, that will come at a later time.

What's here is enough to perform reasonable studies.

@dmitrijus
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 will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar (and backports should be raised in the release meeting by the corresponding L2)

@davidlange6
Copy link
Contributor

+1

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