HBHE: use proper quantization noise in the M2

[mariadalfonso]

Hi.
Added proper quantization noise introduced in #16951
Validation test in
http://dalfonso.web.cern.ch/dalfonso/dalfonso_M2adcFIX.pdf

Fix only the HE for the 2017 scenario.
It affect both data and MC.

[cmsbuild]

A new Pull Request was created by @mariadalfonso for CMSSW_9_0_X.

It involves the following packages:

RecoLocalCalo/HcalRecAlgos

@cmsbuild, @cvuosalo, @slava77, @davidlange6 can you please review it and eventually sign? Thanks.
@argiro 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

[slava77]

@mariadalfonso
if the fix applies to QIE8 as well, I think we should apply changes everywhere.
If we need to compare 2016 data with 2017 data or MC or other permutations, we better have consistent reconstruction for the same detectors.

[slava77]

@cmsbuild please test

[cmsbuild]

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

[slava77]

if pre-2017 QIE8 reco can not be handled uniformly with 2017 with the new quantization uncertainties, I think it would be better to keep all QIE8/HPD reco the same in the same release.

[mariadalfonso]

@slava77

To have in the 2016 scenario as well I have to clone the function here
https://github.com/cms-sw/cmssw/pull/16951/files#diff-5bffd97385f18e7da1326160e40795c1R139
inside the RecoLocalCalo/HcalRecAlgos/src/PulseShapeFitOOTPileupCorrection.cc

I do think is better to have the fix for whole new 2017 detector (HB,HE) as we should aim to have the best hcal reco.
What is the pre-2017 use ?

1. comparison of the relVal ?
2. The "phase1" code can also be used for the run2 legacy re-reco and in case we have the full detector with the HPD thanks to the switch channelData.hasTimeInfo()
   @igv4321

[slava77]

On 12/14/16 7:08 AM, mariadalfonso wrote: @slava77 <https://github.com/slava77> To have in the 2016 scenario as well I have to clone the function here https://github.com/cms-sw/cmssw/pull/16951/files#diff-5bffd97385f18e7da1326160e40795c1R139 inside the RecoLocalCalo/HcalRecAlgos/src/PulseShapeFitOOTPileupCorrection.cc I do think is better to have the fix for whole new 2017 detector (HB,HE) as we should aim to have the best hcal reco. What is the pre-2017 use ? 1. comparison of the relVal ? 2. The "phase1" code can also be used for the run2 legacy re-reco and in case we have the full detector with the HPD thanks to the switch channelData.hasTimeInfo() @igv4321 <https://github.com/igv4321>

We should have the best reco available everywhere a given detector is active. If the code is not flexible enough, it should be made so, and then the reco applied.

…

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

[mariadalfonso]

@slava77
ok, will change the 2016 scenario as well.
actually happy to do this for this and other fixes

[slava77]

On 12/14/16 7:20 AM, mariadalfonso wrote: @slava77 <https://github.com/slava77> ok, will change the 2016 scenario as well. actually happy to do this for this and other fixes

Thank you very much.

…

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

[igv4321]

tsDFcPerADC is actually the differential gain: the width of that particular ADC count in fC. The nominal noise is actually tsDFcPerADC(ip)/sqrt(12.0). The factor 1/sqrt(12) is probably too optimistic, so (in an email thread "Re: QIE slope") I suggested to make it configurable.

[cmsbuild]

+1
Tested at: f290d99
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-17037/17031/summary.html

[cmsbuild]

Comparison job queued.

[abdoulline]

Thanks for the reminder, Igor.
Indeed, effective quantization noise is certainly lower than the size of the ADC count in fC.
Maria, please, consider modifying the code accordingly.

[cmsbuild]

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

Alternative comparison was/were failed for workflow(s):
25202.0
25.0
1306.0
21234.0
1003.0
23234.0
140.53
136.731
1330.0
1000.0
135.4
10024.0
5.1
8.0
1001.0
10021.0
50202.0
20034.0
9.0
4.22

[cmsbuild]

+1
Tested at: 0035245
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-17037/17097/summary.html

[cmsbuild]

Comparison job queued.

[cmsbuild]

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

[slava77]

@mariadalfonso
there is a new quantum in chi2 values (~exactly 1)

Is this some numerical coincidence for "empty" TSs?

[mariadalfonso]

@slava77

For low energy recHit, it often happen as follow
depth==1 chi2=0.831883
depth>1 chi2=0.893766
http://dalfonso.web.cern.ch/dalfonso/HCAL/depth1_afterPR17037_Elt5.txt
http://dalfonso.web.cern.ch/dalfonso/HCAL/depthgt1_afterPR17037_Elt5.txt
indeed those correspond to empty recHit

other repetitive empty events are also with
depth==1 chi2=7.25356
depth>1 chi2=8.06138

This happen for the high energy (E>5 and chi2>100)
http://dalfonso.web.cern.ch/dalfonso/HCAL/depth1_afterPR17037.txt
http://dalfonso.web.cern.ch/dalfonso/HCAL/depthgt1_afterPR17037.txt

[slava77]

Some plots from pi E 5 to 5000 gun (no PU), from endcap

(e_new - e_old)/e_raw vs e_raw

and the same zoomed to +/-0.1

Looks like the 3-pulse fit results change by as much as 100%.
Above 20 GeV we now get about 10% less energy.

here is an overlay of the energy/eraw vs log10(eraw) profiles ("prof")
to show the change in response (new is in red)

For your amusement, the scatter plot of new energy/eraw vs log10(eraw) is

chi2_new/chi2_old vs log10(eraw)

at lower energies the chi2 is smaller as expected
at high energy the chi2 gets worse as well as the response

Given that PF backs up calorimeter with tracking at lower energies, issues at higher energy are more important.
It's good that there are no tails here at high energy. So, a few % loss in response is probably OK since it can be calibrated away.

Is it clear that the high energy ADC uncertainty is correct?

[slava77]

@mariadalfonso @igv4321 @kpedro88 @abdoulline
I will need some confirmation/clarification regarding behavior of M2 at high energy to sign off on this PR.

It looks like the errors decrease with this PR compared to the current CMSSW version.
If it is appropriate and expected from first principles, we can move on, hoping that the upcoming incorporation of shape variations or other updates will take care of the excess chi2.

[mariadalfonso]

@slava77

Thanks for the studies
I will compare the noise before as I obtained here
https://indico.cern.ch/event/563410/contributions/2277004/attachments/1324249/2063825/ADCgranularity.pdf
Before the linear approximation, I should get the same error from igor implementation
I will come back to you by the end of my day

[slava77]

piE5to5000 sample calojet response.
this is essentially a single (charged) hadron response test

~5% drop, partly from averaging out over the hadron shower energy sample.

[abdoulline]

Slava, I do believe this implementation of ADC quantization error estimate is the move in the right direction, as it's based on a real ADC properties of QIE10 chip.

…

On Thu, 22 Dec 2016, Slava Krutelyov wrote: @mariadalfonso @igv4321 @kpedro88 @abdoulline I will need some confirmation/clarification regarding behavior of M2 at high energy to sign off on this PR. It looks like the errors decrease with this PR compared to the current CMSSW version. If it is appropriate and expected from first principles, we can move on, hoping that the upcoming incorporation of shape variations or other updates will take care of the excess chi2. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.[AEx02imLl09-7D9dcnYRU_sI-x4Yccvnks5rKpLagaJpZM4LM-fy.gif]

[slava77]

On 12/22/16 1:17 PM, Salavat Abdullin wrote: Slava, I do believe this implementation of ADC quantization error estimate is the move in the right direction, as it's based on a real ADC properties of QIE10 chip.

QIE11 (just a typo) ?

[abdoulline]

Right, mea cupla. Actually the only essential difference of QIE11 from QIE10 is a programmable shunt (of the input current) which is taken into account in the QIE11 calibration, other properties are very much the same.

…

On Thu, 22 Dec 2016, Slava Krutelyov wrote: On 12/22/16 1:17 PM, Salavat Abdullin wrote: > Slava, I do believe this implementation of ADC quantization error > estimate is the move in the right direction, as it's based on a real ADC > properties of QIE10 chip. QIE11 (just a typo) ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.[AEx02qv-WPD2Jq49GOwDGDvf7ntNsw7_ks5rKvACgaJpZM4LM-fy.gif]

[mariadalfonso]

@slava77

I plotted here the relative error vs the input charge
http://dalfonso.web.cern.ch/dalfonso/HCAL/ADCnoiseVsCharge.gif

it's close enought to what I see in the slides of the electronic's guys see page22
https://indico.cern.ch/event/192695/contributions/353256/attachments/277181/387778/Elliot-Hughes.pdf
We can merge from my side

[slava77]

+1

for #17037 0035245

• code change is as described
• jenkins tests pass and show small differences in HCAL QIE11 and downstream variables in 2017 and in 2023 D4 and D8 workflows as expected
• local tests with more events confirm expected behavior: reduction of chi2 for lower energy hits (more so for very low-energy hits E<1 GeV). Also chi2 gets a bit worse for high energy and energy response decreases as discussed above. This specific change in uncertainty is more appropriate to what is done in digitization and what's expected for real QIE10/11 chips.
• technical performance with ttbar PU35 wf 10224 shows reduction in hbheprereco (M2, mostly) CPU by 0.4s/evt (~12% relative to the baseline module time or about 2% of the total reco time).

Note from PU35:
chi2 reduction is much more moderate

[cmsbuild]

This pull request is fully signed and it will be integrated in one of the next CMSSW_9_0_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @smuzaffar

[davidlange6]

+1