Update Phase 1 pixel gains in digitizer/clusterizer #19181

jkarancs · 2017-06-09T19:18:21Z

Contents of this PR:

Update VCal to electron unit for the phase 1 detector using eras (gains and offsets)
Allow separate values for layer 1
Lower the cluster threshold for Layer 1

This PR will not affect Phase 0 Data/MC. There are minor changes for simulation, because the values were updated both in the digitizer and the clusterizer (to remain consistent). Data on the other hand will have better agreement to MC now.

…), lower L1 cluster threshold

cmsbuild · 2017-06-09T19:18:38Z

A new Pull Request was created by @jkarancs (János Karancsi) for master.

It involves the following packages:

RecoLocalTracker/SiPixelClusterizer
SimGeneral/MixingModule
SimTracker/SiPixelDigitizer

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

cms-bot commands are listed here

jkarancs · 2017-06-09T19:24:16Z

@veszpv @boudoul @tsusa @dkotlins may be also interested about this pr.

slava77 · 2017-06-09T19:36:56Z

@cmsbuild please test

cmsbuild · 2017-06-09T19:37:13Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/20478/console Started: 2017/06/09 21:37

cmsbuild · 2017-06-09T21:11:02Z

+1
Tested at: c090e99
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-19181/20478/summary.html

cmsbuild · 2017-06-09T21:11:05Z

Comparison job queued.

cmsbuild · 2017-06-09T23:24:27Z

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

Comparison Summary:

You potentially added 5 lines to the logs
Reco comparison results: 1359 differences found in the comparisons
DQMHistoTests: Total files compared: 23
DQMHistoTests: Total histograms compared: 1872323
DQMHistoTests: Total failures: 21878
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 1850272
DQMHistoTests: Total skipped: 173
DQMHistoTests: Total Missing objects: 0
Checked 94 log files, 14 edm output root files, 23 DQM output files

VinInn · 2017-06-10T14:27:32Z

RecoLocalTracker/SiPixelClusterizer/plugins/PixelThresholdClusterizer.cc

@@ -427,6 +443,11 @@ PixelThresholdClusterizer::make_cluster( const SiPixelCluster::PixelPos& pix,

  if (dead_flag && doSplitClusters) 
    {
+      // Set separate cluster threshold for L1 (needed for phase1)
+      double clusterThreshold = theClusterThreshold;


not double, use auto

VinInn · 2017-06-10T14:29:36Z

SimTracker/SiPixelDigitizer/plugins/SiPixelDigitizerAlgorithm.h

@@ -324,6 +324,8 @@ class SiPixelDigitizerAlgorithm  {

    const double electronsPerVCAL;          // for electrons - VCAL conversion
    const double electronsPerVCAL_Offset;   // in misscalibrate()
+    const double electronsPerVCAL_L1;       // same for Layer 1


if float are used in the algo, please use float here as well (conversion IS expensive)

VinInn · 2017-06-10T14:32:52Z

RecoLocalTracker/SiPixelClusterizer/plugins/PixelThresholdClusterizer.cc

@@ -138,7 +146,7 @@ void PixelThresholdClusterizer::clusterizeDetUnitT( const T & input,

 	  //  Check if the cluster is above threshold  
 	  // (TO DO: one is signed, other unsigned, gcc warns...)
-	  if ( cluster.charge() >= theClusterThreshold) 
+	  if ( cluster.charge() >= clusterThreshold) 


actually given that cluster.charge() is a int please use a int for clusterThreshold!
moving to int the computation of cluster.charge() was a major speed-up improvement

VinInn · 2017-06-10T14:33:37Z

RecoLocalTracker/SiPixelClusterizer/plugins/PixelThresholdClusterizer.cc

@@ -119,6 +122,11 @@ void PixelThresholdClusterizer::clusterizeDetUnitT( const T & input,

  detid_ = input.detId();

+  // Set separate cluster threshold for L1 (needed for phase1)
+  double clusterThreshold = theClusterThreshold;


VinInn · 2017-06-10T14:34:27Z

RecoLocalTracker/SiPixelClusterizer/plugins/PixelThresholdClusterizer.h

-  const float theClusterThreshold;  // Cluster threshold in electrons
-  const int   theConversionFactor;  // adc to electron conversion factor
-  const int   theOffset;            // adc to electron conversion offset
+  const float theClusterThreshold;    // Cluster threshold in electrons


let's move to int

VinInn · 2017-06-10T15:59:36Z

RecoLocalTracker/SiPixelClusterizer/python/SiPixelClusterizer_cfi.py

+  VCaltoElectronGain      = cms.int32(47),             # L2-4: 47 +- 4.7
+  VCaltoElectronGain_L1   = cms.untracked.int32(50),   # L1:   49.6 +- 2.6
+  VCaltoElectronOffset    = cms.int32(-60),            # L2-4: -60 +- 130
+  VCaltoElectronOffset_L1 = cms.untracked.int32(-670), # L1:   -670 +- 220


this is not allowed to be untracked as it modify physics objects
btw I suggest to move to fillDescriptions

cmsbuild · 2017-06-15T00:44:52Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/20581/console Started: 2017/06/15 03:06
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/20582/console Started: 2017/06/15 07:47

slava77 · 2017-06-15T06:42:13Z

RecoLocalTracker/SiPixelClusterizer/plugins/PixelThresholdClusterizer.cc

+  desc.add<int>("ClusterThreshold", 4000);
+  desc.add<int>("maxNumberOfClusters", -1);
+  descriptions.add("siPixelClusters", desc);
+}


If I'm not mistaken, currently this method is a dead code.
Perhaps in the future refactoring this can become useful.

slava77 · 2017-06-15T06:43:44Z

+1

for #19181 973ad66

changes are in line with the description and the follow up review and discussion.
jenkins tests pass and comparisons with baseline show differences only in phase-1 tracking workflows (2017 and 2018)
local tests show expected behavior (notes below)

In 923patch1:

ttbar PU35 wf 10224
there is a small increase in measured charge in dEdX units

just a small increase here

Compare to data
where the peak moves back up to 3 as it is in MC

On individual layers:
BPIX1
MC ~unchanged

data

This is roughly in line with the plots in the PR description

Higher level tracking quantities have little effect from this change

It seems like there is an epsilon improvement in BPIX1 eff, but that's a small effect.

cmsbuild · 2017-06-15T07:17:01Z

+1
Tested at: 973ad66
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-19181/20582/summary.html

cmsbuild · 2017-06-15T07:17:04Z

Comparison job queued.

cmsbuild · 2017-06-15T09:35:23Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 1370 differences found in the comparisons
DQMHistoTests: Total files compared: 22
DQMHistoTests: Total histograms compared: 1817590
DQMHistoTests: Total failures: 36225
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 1781199
DQMHistoTests: Total skipped: 166
DQMHistoTests: Total Missing objects: 0
Checked 90 log files, 14 edm output root files, 22 DQM output files

slava77 · 2017-06-15T12:13:03Z

@Martin-Grunewald @silviodonato @fwyzard
this PR needs an HLT signature.
Please check and suggest changes or sign.
Thank you.

Martin-Grunewald · 2017-06-15T12:36:46Z

yes, our tests are running....

Martin-Grunewald · 2017-06-15T15:43:27Z

+1

cmsbuild · 2017-06-15T15:43:47Z

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. @davidlange6, @smuzaffar

davidlange6 · 2017-06-15T17:39:10Z

+1

davidlange6 · 2017-06-25T13:38:15Z

from the ops meeting Friday we understand that this PR needs conditions adjustments. We'll add it to 930pre1 but not yet 92x

Update Vcal Calibration (backport of PR #19181 in 92X)

Update Phase 1 gains in digitizer/clusterizer (separate values for L1…

c090e99

…), lower L1 cluster threshold

cmsbuild added this to the CMSSW_9_2_X milestone Jun 9, 2017

cmsbuild added comparison-pending orp-pending pending-signatures reconstruction-pending simulation-pending tests-pending labels Jun 9, 2017

cmsbuild added tests-started and removed tests-pending labels Jun 9, 2017

cmsbuild added tests-approved and removed tests-started labels Jun 9, 2017

cmsbuild added comparison-available and removed comparison-pending labels Jun 9, 2017

VinInn reviewed Jun 10, 2017

View reviewed changes

Optimize variable types

a657fdb

cmsbuild removed comparison-available tests-approved labels Jun 12, 2017

cmsbuild added tests-started and removed tests-pending labels Jun 15, 2017

slava77 reviewed Jun 15, 2017

View reviewed changes

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Jun 15, 2017

cmsbuild added tests-approved and removed tests-started labels Jun 15, 2017

cmsbuild added comparison-available and removed comparison-pending labels Jun 15, 2017

cmsbuild added fully-signed hlt-approved and removed hlt-pending pending-signatures labels Jun 15, 2017

cmsbuild added orp-approved and removed orp-pending labels Jun 15, 2017

cmsbuild merged commit 3e65ff2 into cms-sw:master Jun 15, 2017

davidlange6 mentioned this pull request Jun 25, 2017

Revert "Update Phase 1 pixel gains in digitizer/clusterizer" #19423

Merged

boudoul mentioned this pull request Jul 9, 2017

Update Vcal Calibration (backport of PR #19181 in 92X) #19632

Merged

cmsbuild added a commit that referenced this pull request Jul 9, 2017

Merge pull request #19632 from boudoul/92X_Update_Phase1_Pixel_Vcal

82c77dc

Update Vcal Calibration (backport of PR #19181 in 92X)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Phase 1 pixel gains in digitizer/clusterizer #19181

Update Phase 1 pixel gains in digitizer/clusterizer #19181

jkarancs commented Jun 9, 2017

cmsbuild commented Jun 9, 2017

jkarancs commented Jun 9, 2017

slava77 commented Jun 9, 2017

cmsbuild commented Jun 9, 2017 •

edited

cmsbuild commented Jun 9, 2017

cmsbuild commented Jun 9, 2017

cmsbuild commented Jun 9, 2017

VinInn Jun 10, 2017

VinInn Jun 10, 2017

VinInn Jun 10, 2017

VinInn Jun 10, 2017

VinInn Jun 10, 2017

VinInn Jun 10, 2017

cmsbuild commented Jun 15, 2017 •

edited

slava77 Jun 15, 2017

slava77 commented Jun 15, 2017

cmsbuild commented Jun 15, 2017

cmsbuild commented Jun 15, 2017

cmsbuild commented Jun 15, 2017

slava77 commented Jun 15, 2017

Martin-Grunewald commented Jun 15, 2017

Martin-Grunewald commented Jun 15, 2017

cmsbuild commented Jun 15, 2017

davidlange6 commented Jun 15, 2017

davidlange6 commented Jun 25, 2017

Update Phase 1 pixel gains in digitizer/clusterizer #19181

Update Phase 1 pixel gains in digitizer/clusterizer #19181

Conversation

jkarancs commented Jun 9, 2017

cmsbuild commented Jun 9, 2017

jkarancs commented Jun 9, 2017

slava77 commented Jun 9, 2017

cmsbuild commented Jun 9, 2017 • edited

cmsbuild commented Jun 9, 2017

cmsbuild commented Jun 9, 2017

cmsbuild commented Jun 9, 2017

VinInn Jun 10, 2017

Choose a reason for hiding this comment

VinInn Jun 10, 2017

Choose a reason for hiding this comment

VinInn Jun 10, 2017

Choose a reason for hiding this comment

VinInn Jun 10, 2017

Choose a reason for hiding this comment

VinInn Jun 10, 2017

Choose a reason for hiding this comment

VinInn Jun 10, 2017

Choose a reason for hiding this comment

cmsbuild commented Jun 15, 2017 • edited

slava77 Jun 15, 2017

Choose a reason for hiding this comment

slava77 commented Jun 15, 2017

cmsbuild commented Jun 15, 2017

cmsbuild commented Jun 15, 2017

cmsbuild commented Jun 15, 2017

slava77 commented Jun 15, 2017

Martin-Grunewald commented Jun 15, 2017

Martin-Grunewald commented Jun 15, 2017

cmsbuild commented Jun 15, 2017

davidlange6 commented Jun 15, 2017

davidlange6 commented Jun 25, 2017

cmsbuild commented Jun 9, 2017 •

edited

cmsbuild commented Jun 15, 2017 •

edited