SiPM nonlinearity, parameters from CondDB, update tau, cleanup #16315

kpedro88 · 2016-10-22T05:06:31Z

This PR supersedes #16070 (which should be closed). One of the issues under discussion for that PR, HO SiPM parameters in the database, has already been resolved. To recap: in #15058, when the new features were added to the SiPM simulation for HE, we propagated the same parameter values to HO (but only for Run 1). There was then a bit of confusion when transferring those parameters to the database. Upon reflection, we decided these new features should be avoided for HO entirely, since they have not been tuned for it. This resets the Run 1 HO sim back to how it was before 810pre9, and the Run 2 HO sim should be unchanged now that the database conditions are fixed.

After several weeks of internal discussions, a few decisions were made about the nonlinearity implementation. The SiPM simulation already includes nonlinearity using a saturation/recovery model for pixels with recovery time constant tau. Therefore, it would be redundant to include the saturation/recovery model and apply the inverted nonlinearity correction (derived from bench data) in the simulation. For now, we have decided to stick with the saturation/recovery model. However, the code to invert the nonlinearity correction is left in place, in case of future studies.

This implies a few things:

The nonlinearity correction from MC needs to be derived from the SiPM simulation. A tool has been added for this: SimCalorimetry/HcalSimAlgos/test/sipmnonlinearityanalyzer_cfg.py.
The value for tau has been updated to 10 ns, based on a recommendation from experts. This significantly improves the agreement of the simulated nonlinearity with the bench data: slides
Tau is now a configurable python parameter, and left as 5ns for HO to avoid unnecessary changes to that simulation (since the HO signals tend to be small, the nonlinear effect is also small and generally neglected).
The new nonlinearity parameters have been updated in the hardcode conditions. This PR now activates the reco application of the nonlinearity correction (since Phase 1 HBHE fixes related to SiPM nonlinearity and cosmic run mode #16197 from @igv4321 is already merged), so the corrections in the database need to be updated to give the most appropriate results. @abdoulline is working on this. In the meantime, to pick up the hardcode condition values:

from SLHCUpgradeSimulations.Configuration.HCalCustoms import load_HcalHardcode
process = load_HcalHardcode(process)
process.es_hardcode.toGet = cms.untracked.vstring("GainWidths","SiPMCharacteristics")

@mariadalfonso is checking the effect of the nonlinearity correction on M2 performance (hopefully it should provide some improvement at high energy).

In addition, I checked off a few other items on my todo list for the SiPM code:
5) remove iConfig.exists() from HcalSimParameters (recall this contributed to the bug/digression fixed in #15834).
6) Restore the use of doSiPMSmearing for HO (only for 2017 and beyond, leaving the old versions as they were) with the old pulse shape (suitable for HO SiPMs). This required adding HcalMCParams to HcalDbService to get access to the pulse shape number on a per-channel basis.
7) Remove some unused fragments in the SiPM code that had been lying around.

Note: there may be a separate PR in the near future updating some SiPM parameters, as we have been notified that the TB data used to measure the parameters was taken at V-VB = 4.4 V, while the SiPMs installed in HE will be operated at V-VB = 3 V. However, some data still needs to be analyzed for this, and it should be totally orthogonal to this PR.

The other item still coming is ZS thresholds for HE (from @lihux25), but these of course depend on any parameter changes.

@civanch, @slava77: I would appreciate a rapid review of this PR if possible. I know it has a long description, but most of the new changes are rather minor. (Also, the majority of added lines are in the new standalone tool to measure the simulated nonlinearity.)

cmsbuild · 2016-10-22T05:06:50Z

A new Pull Request was created by @kpedro88 (Kevin Pedro) for CMSSW_8_1_X.

It involves the following packages:

CalibCalorimetry/HcalAlgos
CalibCalorimetry/HcalPlugins
CalibFormats/HcalObjects
CondFormats/HcalObjects
RecoLocalCalo/HcalRecProducers
SimCalorimetry/HcalSimAlgos
SimCalorimetry/HcalSimProducers
SimCalorimetry/HcalTestBeam

@ghellwig, @civanch, @cvuosalo, @mdhildreth, @cmsbuild, @franzoni, @cerminar, @slava77, @ggovi, @mmusich, @davidlange6 can you please review it and eventually sign? Thanks.
@ghellwig, @mariadalfonso, @apfeiffer1, @tocheng 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

kpedro88 · 2016-10-22T05:09:28Z

@cmsbuild please test

cmsbuild · 2016-10-22T05:09:43Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/15901/console

cmsbuild · 2016-10-22T07:17:32Z

+1
Tested at: eadbc53
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16315/15901/summary.html

cmsbuild · 2016-10-22T07:17:35Z

Comparison job queued.

cmsbuild · 2016-10-22T07:53:33Z

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

kpedro88 · 2016-10-22T15:09:34Z

@smuzaffar this PR is having the issue where comparisons still say running even though the bot reports they're available, can you take a look?

slava77 · 2016-10-22T15:48:40Z

@kpedro88 if appropriate calibration payloads should be in GT for testing this PR, the autoGT change should be a part of this PR

slava77 · 2016-10-22T15:55:11Z

@kpedro88 please provide some basic plots of performance.
Ideally eReco / eGen vs eGen for HCAL hits or at least some proxies like M2/M0, jet reco/gen from DQM.
IMO, all non-technical PRs should come with this for self-documentation.

kpedro88 · 2016-10-22T16:10:45Z

@mariadalfonso is working on tests of the M2 performance.

We can update autocond in this PR once the new records are inserted (@abdoulline is working on it), but this might conflict with #16301.

abdoulline · 2016-10-22T16:22:37Z

I've responded via e-mail, but I don't see the message arrived there in PR thread, and the same (absence of comment sent via e-mail) happened yesterday with my rather long comment in another PR thread (#16150 RPD implementation)

So I'm copy-pasting:

"The appropriate tag HcalSiPMCharacteristics_2017_v5.0_mc has been submitted. Alas... now we need to bug Marco @mmusich again..."

kpedro88 · 2016-10-28T19:26:09Z

@slava77 @civanch @mmusich @ggovi please sign ASAP

civanch · 2016-10-29T13:59:04Z

+1

kpedro88 · 2016-10-29T18:11:45Z

@slava77 @mmusich @ggovi @davidlange6 this PR is important for pre16, and other PRs need to come on top of it

kpedro88 · 2016-10-30T17:44:56Z

@slava77 @mmusich @ggovi @davidlange6 I am obliged to keep nagging about this PR for pre16

davidlange6 · 2016-10-30T17:50:00Z

Look so like it's had a fairly active and productive review during usual (and recent) working days....

kpedro88 · 2016-10-30T17:53:27Z

It did have a productive review and now needs signatures. #16365 is needed for phase2 and conflicts with this, so needs at least a day to be rebased/tested/signed after this is merged. We also expect another PR to add ZS thresholds (still being finalized). Each PR does not exist in a vacuum, and if the Nov 1 deadline for pre16 is going to be enforced, there remains a lot of work to do.

slava77 · 2016-10-30T17:55:22Z

On 10/30/16 10:44 AM, Kevin Pedro wrote:

@slava77 https://github.com/slava77 @mmusich
https://github.com/mmusich @ggovi https://github.com/ggovi
@davidlange6 https://github.com/davidlange6 I am obliged to keep
nagging about this PR for pre16

As usual, any basic validation plots for the full dynamic range are
missing for the last update.
So, it takes some time to remake. (Also, it's the weekend.)
This is on top of two already failed iterations.
Please take this in consideration for the future HCAL PRs and your
expectations and ideas on how to speedup review.

I'm hoping to get to this in time for pre16.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#16315 (comment), or
mute the thread
https://github.com/notifications/unsubscribe-auth/AEdcbn1kkaQSBgf89uLWoHGAEXXjqxSgks5q5NeZgaJpZM4KdxA-.

kpedro88 · 2016-10-30T18:02:58Z

Well, the change since the last round was only in the SiPMParameters tag to remove dark current in HO.

No HO plots show up in https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_8_1_X_2016-10-28-1100+16315/16651/validateJR/all_OldVSNew_TTbar13TeV2017wf10024p0/, which indicates the change was successful. (If we don't trust the code that identifies plots with differences to be shown in the comparison, that is a problem that should be addressed.) The HBHE plots there show minor differences compatible with the change in the sim-level saturation/recovery parameter and corresponding nonlinearity correction introduced in reco.

slava77 · 2016-10-30T18:12:04Z

On 10/30/16 11:02 AM, Kevin Pedro wrote:

Well, the change since the last round was only in the SiPMParameters tag
to remove dark current in HO.

No HO plots show up in
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_8_1_X_2016-10-28-1100+16315/16651/validateJR/all_OldVSNew_TTbar13TeV2017wf10024p0/,
which indicates the change was successful. (If we don't trust the code
that identifies plots with differences to be shown in the comparison,
that is a problem that should be addressed.) The HBHE plots there show
minor differences compatible with the change in the sim-level
saturation/recovery parameter and corresponding nonlinearity correction
introduced in reco.

Where can I see clearly that the last GT update
touched only HO?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#16315 (comment), or
mute the thread
https://github.com/notifications/unsubscribe-auth/AEdcbgGgycJsblTZhucvDls4wuIa_SpMks5q5NvTgaJpZM4KdxA-.

davidlange6 · 2016-10-30T18:17:09Z

Right but complaining that updates made Friday late are not reviewed on Sunday sounds like the wrong expectation (even if Slava is indeed working)

Cheers
David

On 30 Oct 2016, at 18:53, Kevin Pedro <notifications@github.com mailto:notifications@github.com> wrote:

It did have a productive review and now needs signatures. #16365 #16365 is needed for phase2 and conflicts with this, so needs at least a day to be rebased/tested/signed after this is merged. We also expect another PR to add ZS thresholds (still being finalized). Each PR does not exist in a vacuum, and if the Nov 1 deadline for pre16 is going to be enforced, there remains a lot of work to do.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHubhttps://github.com//pull/16315#issuecomment-257166563, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AEzyw-xWxevolcNoqKyGl06pow6H3cXOks5q5NmYgaJpZM4KdxA-.

kpedro88 · 2016-10-30T18:23:30Z

https://cms-conddb.cern.ch/cmsDbBrowser/diff/Prod/gts/81X_upgrade2017_realistic_v20/81X_upgrade2017_realistic_v21 shows that only HcalSiPMParameters was changed.

Using the config dumpHcalCond_test_cfg.py, I dumped HcalSiPMParameters_2017_v2.0_mc and HcalSiPMParameters_2017_v4.0_mc and made a diff, showing only the expected change in the HO dark current value.

I don't like working on weekends either, but I don't decide the deadlines, I just have to live with them.

mmusich · 2016-10-31T08:59:45Z

+1

slava77 · 2016-10-31T13:31:16Z

+1

for #16315 b41c70b

code and GT changes now appear to be in line with expectations
jenkins tests pass and show changes in HO in run1 (as posted in SiPM nonlinearity, parameters from CondDB, update tau, cleanup #16315 (comment)) and in HE in 2017
local tests with higher stats show small impact on jet response

based on dijet 15 to 3 TeV sample (wf 1338 equivalent) in 2017 setup

kpedro88 · 2016-10-31T13:44:48Z

thanks!

@ggovi the DB change is just adding HcalMCParams to HcalDbService

davidlange6 · 2016-10-31T15:20:00Z

CalibCalorimetry/HcalAlgos/src/HcalSiPMnonlinearity.cc

+int HcalSiPMnonlinearity::getPixelsFired(int inpes) const
+{
+  ROOT::Math::Polynomial p(a2,b1,c0,-inpes);
+  std::vector<double> roots = p.FindRealRoots();


calling gsl directly would likely be better in the long term (seems this function is not yet used)

Yes, right now this is just left in place for future studies.

The ROOT functions do a fair amount of useful processing (it's not just a direct clone of gsl), so I would prefer to leave it this way.

davidlange6 · 2016-10-31T15:22:52Z

SimCalorimetry/HcalSimAlgos/src/HcalSiPM.cc

@@ -164,7 +174,15 @@ void HcalSiPM::setTemperatureDependence(double dTemp) {

 double HcalSiPM::cellCharge(double deltaTime) const {
  if (deltaTime <= 0.) return 0.;
-  if (deltaTime > 10./theTauInv) return 1.;
-  double result(1. - std::exp(-deltaTime*theTauInv));
+  if (deltaTime > 10.*theTau) return 1.;


if this is a performance issue, then

if (deltaTime_theTauInv > 10. ) return 1.;
double result(1. - std::exp(-deltaTime_theTauInv));

would be best

I was just trying to avoid multiple inversions of the tau parameter. I will run igprof to check if there is a performance penalty.

davidlange6 · 2016-10-31T15:26:16Z

SimCalorimetry/HcalSimAlgos/src/HcalSiPMShape.cc

-    onePulse(t,A2,sigma2_shape,theta2_loc,m2_scale);
+double HcalSiPMShape::analyticPulseShape(double t, unsigned int signalShape) const {
+  if(signalShape==HcalShapes::ZECOTEK || signalShape==HcalShapes::HAMAMATSU){
+    double A1(0.08757), c1(-0.5257), t01(2.4013), s1(0.6721);


please document the source of these magic numbers

Well, these were never documented to begin with... I just restored the HO pulse shape from earlier versions: https://github.com/cms-sw/cmssw/blob/CMSSW_8_1_0_pre8/SimCalorimetry/HcalSimAlgos/src/HcalSiPMShape.cc

I can add a note like "HO SiPM pulse shape fit from Jake Anderson ca. 2013".

davidlange6 · 2016-10-31T15:27:11Z

@kpedro88 -three small comments that can be considered on one of the promised follow-on prs. thanks

davidlange6 · 2016-10-31T15:30:04Z

On Oct 31, 2016, at 4:24 PM, Kevin Pedro notifications@github.com wrote:

@kpedro88 commented on this pull request.

In CalibCalorimetry/HcalAlgos/src/HcalSiPMnonlinearity.cc:

@@ -0,0 +1,19 @@
+#include
+#include "Math/Polynomial.h"
+#include "CalibCalorimetry/HcalAlgos/interface/HcalSiPMnonlinearity.h"
+
+// Assume parameters come to us from the reco side; i.e.,
+// true pes = corfun(pixelsfired). But we want to invert that.
+//
+int HcalSiPMnonlinearity::getPixelsFired(int inpes) const
+{

ROOT::Math::Polynomial p(a2,b1,c0,-inpes);

std::vector roots = p.FindRealRoots();

Yes, right now this is just left in place for future studies.

The ROOT functions do a fair amount of useful processing (it's not just a direct clone of gsl), so I would prefer to leave it this way.

almost none of the ROOT overhead is actually taken advantage of in this usage. but yes, it saves you a handful of lines (at a substantial cost of operational overhead) - of course it may or may not matter depending on how this function gets used

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

davidlange6 · 2016-10-31T15:31:04Z

right, you need my change to go back to one inversion…

On Oct 31, 2016, at 4:27 PM, Kevin Pedro notifications@github.com wrote:

@kpedro88 commented on this pull request.

In SimCalorimetry/HcalSimAlgos/src/HcalSiPM.cc:

@@ -164,7 +174,15 @@ void HcalSiPM::setTemperatureDependence(double dTemp) {

double HcalSiPM::cellCharge(double deltaTime) const {
if (deltaTime <= 0.) return 0.;

if (deltaTime > 10./theTauInv) return 1.;

double result(1. - std::exp(-deltaTime*theTauInv));

if (deltaTime > 10.*theTau) return 1.;

I was just trying to avoid multiple inversions of the tau parameter. I will run igprof to check if there is a performance penalty.

—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub, or mute the thread.

cmsbuild added this to the Next CMSSW_8_1_X milestone Oct 22, 2016

cmsbuild added alca-pending comparison-pending db-pending orp-pending pending-signatures reconstruction-pending simulation-pending tests-pending labels Oct 22, 2016

cmsbuild added tests-started and removed tests-pending labels Oct 22, 2016

cmsbuild added tests-approved and removed tests-started labels Oct 22, 2016

cmsbuild added comparison-available and removed comparison-pending labels Oct 22, 2016

mariadalfonso mentioned this pull request Oct 22, 2016

HBHE: siPM M2 reco fixes #15959

Merged

cmsbuild added comparison-pending and removed comparison-available tests-approved labels Oct 22, 2016

cmsbuild added comparison-available and removed comparison-pending labels Oct 28, 2016

cmsbuild added simulation-approved and removed simulation-pending labels Oct 29, 2016

cmsbuild added alca-approved and removed alca-pending labels Oct 31, 2016

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Oct 31, 2016

davidlange6 reviewed Oct 31, 2016

View reviewed changes

davidlange6 merged commit 4019c3f into cms-sw:CMSSW_8_1_X Oct 31, 2016

kpedro88 mentioned this pull request Nov 1, 2016

SiPM followups #16417

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SiPM nonlinearity, parameters from CondDB, update tau, cleanup #16315

SiPM nonlinearity, parameters from CondDB, update tau, cleanup #16315

kpedro88 commented Oct 22, 2016

cmsbuild commented Oct 22, 2016

kpedro88 commented Oct 22, 2016

cmsbuild commented Oct 22, 2016 •

edited

cmsbuild commented Oct 22, 2016

cmsbuild commented Oct 22, 2016

cmsbuild commented Oct 22, 2016

kpedro88 commented Oct 22, 2016

slava77 commented Oct 22, 2016

slava77 commented Oct 22, 2016

kpedro88 commented Oct 22, 2016

abdoulline commented Oct 22, 2016

kpedro88 commented Oct 28, 2016

civanch commented Oct 29, 2016

kpedro88 commented Oct 29, 2016

kpedro88 commented Oct 30, 2016

davidlange6 commented Oct 30, 2016

kpedro88 commented Oct 30, 2016

slava77 commented Oct 30, 2016

kpedro88 commented Oct 30, 2016

slava77 commented Oct 30, 2016

davidlange6 commented Oct 30, 2016

kpedro88 commented Oct 30, 2016

mmusich commented Oct 31, 2016

slava77 commented Oct 31, 2016

kpedro88 commented Oct 31, 2016

davidlange6 Oct 31, 2016

kpedro88 Oct 31, 2016

davidlange6 Oct 31, 2016

kpedro88 Oct 31, 2016

davidlange6 Oct 31, 2016

kpedro88 Oct 31, 2016

davidlange6 commented Oct 31, 2016

davidlange6 commented Oct 31, 2016

davidlange6 commented Oct 31, 2016

SiPM nonlinearity, parameters from CondDB, update tau, cleanup #16315

SiPM nonlinearity, parameters from CondDB, update tau, cleanup #16315

Conversation

kpedro88 commented Oct 22, 2016

cmsbuild commented Oct 22, 2016

kpedro88 commented Oct 22, 2016

cmsbuild commented Oct 22, 2016 • edited

cmsbuild commented Oct 22, 2016

cmsbuild commented Oct 22, 2016

cmsbuild commented Oct 22, 2016

kpedro88 commented Oct 22, 2016

slava77 commented Oct 22, 2016

slava77 commented Oct 22, 2016

kpedro88 commented Oct 22, 2016

abdoulline commented Oct 22, 2016

kpedro88 commented Oct 28, 2016

civanch commented Oct 29, 2016

kpedro88 commented Oct 29, 2016

kpedro88 commented Oct 30, 2016

davidlange6 commented Oct 30, 2016

kpedro88 commented Oct 30, 2016

slava77 commented Oct 30, 2016

kpedro88 commented Oct 30, 2016

slava77 commented Oct 30, 2016

davidlange6 commented Oct 30, 2016

kpedro88 commented Oct 30, 2016

mmusich commented Oct 31, 2016

slava77 commented Oct 31, 2016

kpedro88 commented Oct 31, 2016

davidlange6 Oct 31, 2016

Choose a reason for hiding this comment

kpedro88 Oct 31, 2016

Choose a reason for hiding this comment

davidlange6 Oct 31, 2016

Choose a reason for hiding this comment

kpedro88 Oct 31, 2016

Choose a reason for hiding this comment

davidlange6 Oct 31, 2016

Choose a reason for hiding this comment

kpedro88 Oct 31, 2016

Choose a reason for hiding this comment

davidlange6 commented Oct 31, 2016

davidlange6 commented Oct 31, 2016

davidlange6 commented Oct 31, 2016

cmsbuild commented Oct 22, 2016 •

edited