code updates to add a noise correlation factor between adjacent time slices for HB and HE channels #31064

TaeunKwon · 2020-08-05T16:28:00Z

PR description:

This PR updates MAHI to add a pedestal correlation factor between adjacent time slices to pedestal covariance matrix for HB and HE channels. A new class member is introduced in class HBHEChannelInfo to take and pass over the noise correlation factor to MAHI. This PR corresponds to the previously merged PR, which introduces the pedestal correlation factor to HcalSiPMParametersRcd tag for Run-3 MC scenario. The motivation and effect of changes by this PR are shown in presentation by Taeun Kwon.

links of igprof report for CPU time measurements are given below. (Please see slide 15 and 19 of above presentation for the details)

igprof report for RECO without the pedestal correlation factor:
https://ktaeun.web.cern.ch/ktaeun/cgi-bin/igprof-navigator/igreport_RECO

igprof report for RECO with the pedestal correlation factor:
https://ktaeun.web.cern.ch/ktaeun/cgi-bin/igprof-navigator/igreport_RECO_NoiseCorrelation

igprof report for RECO without the pedestal correlation factor with PU:
https://ktaeun.web.cern.ch/ktaeun/cgi-bin/igprof-navigator/igreport_RECO_wPU

igprof report for RECO with the pedestal correlation factor with PU:
https://ktaeun.web.cern.ch/ktaeun/cgi-bin/igprof-navigator/igreport_RECO_NoiseCorrelation_wPU

links of igprof reports for DIGI are given below for the measurment of HLT timing cost change.
The sample used here : TTbar RelVal w/o PU

igprof report for DIGI without the pedestal correlation factor:
10.44 sec / 1000 evts taken for mahiFit:minimization module.
https://ktaeun.web.cern.ch/ktaeun/cgi-bin/igprof-navigator/igreport_DIGI_woNC

igprof report for DIGI with the pedestal correlation factor:
11.25 sec / 1000 evts taken for mahiFit:minimization module.
https://ktaeun.web.cern.ch/ktaeun/cgi-bin/igprof-navigator/igreport_DIGI_wNC

This PR is expected to affect on the HLT timing cost by 7~8% of time spent on mahiFit:minimization module.

PR validation:

A basic technical test was performed: runTheMatrix.py -l limited -i all --ibeos

if this PR is a backport please specify the original PR and why you need to backport that PR:

This PR is not a backport and shouldn't be backported.

…ance matrix

correction on indentation

cmsbuild · 2020-08-05T16:28:21Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-08-05T16:36:48Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31064/17576

This PR adds an extra 40KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31064/17576/code-format.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31064/17576/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2020-08-05T16:43:31Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-08-05T16:48:42Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31064/17577

This PR adds an extra 40KB to repository

cmsbuild · 2020-08-05T16:49:07Z

A new Pull Request was created by @TaeunKwon for master.

It involves the following packages:

DataFormats/HcalRecHit
RecoLocalCalo/HcalRecAlgos
RecoLocalCalo/HcalRecProducers

@perrotta, @jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@rchatter, @rovere, @argiro, @apsallid, @mariadalfonso, @abdoulline this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

RecoLocalCalo/HcalRecAlgos/src/MahiFit.cc

cmsbuild · 2020-08-05T17:51:17Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-08-05T17:58:14Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31064/17581

This PR adds an extra 40KB to repository

cmsbuild · 2020-08-12T08:22:45Z

+1
Tested at: 600bf92
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ce097e/8720/summary.html
CMSSW: CMSSW_11_2_X_2020-08-11-2300
SCRAM_ARCH: slc7_amd64_gcc820

cmsbuild · 2020-08-12T08:22:48Z

Comparison job queued.

cmsbuild · 2020-08-12T10:06:28Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 2396 differences found in the comparisons
DQMHistoTests: Total files compared: 35
DQMHistoTests: Total histograms compared: 2612401
DQMHistoTests: Total failures: 18169
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 2594183
DQMHistoTests: Total skipped: 48
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.004 KiB( 34 files compared)
DQMHistoSizes: changed ( 10224.0 ): 0.004 KiB MessageLogger/Warnings
Checked 149 log files, 22 edm output root files, 35 DQM output files

slava77 · 2020-08-12T14:57:31Z

current status from my review side:

it looks like the igprof links in the pr description were updated (a note to confirm it would be nice)
an update to the study initially linked in the PR, but using 2023 or 2024 setup instead of 2021 (which has little noise) is expected
in code updates to add a noise correlation factor between adjacent time slices for HB and HE channels #31064 (comment) the behavior of the fit in 2023 setup is unclear, perhaps it will become clear after the above item is clear
).

slava77 · 2020-08-12T15:59:07Z

here is a chi2 plot (from the bin-by-bin DQM outputs, after suppressing the large "overflow" bin)

2021 case (wf 11634)

2023 case (wf 12434), which is supposedly dominated by noise

The 2023 is more "self-consistent" fitting of the noise alone, but the behavior (increased chi2) seems to be the same as in the 2021 case, which was investigated in more detail in the DPG talk linked in the PR description.
Was the correlation meant to reduce the chi2 (from first principles)? (I'm trying to understand if the sign in the correlation term is right)

TaeunKwon · 2020-08-12T15:59:37Z

Thanks for the clarification!

it looks like the igprof links in the pr description were updated (a note to confirm it would be nice)

I'm sorry, I updated igprof report for DIGI and CPU time increased after the Noise correlation factor as expected. I will leave a note next time.

an update to the study initially linked in the PR, but using 2023 or 2024 setup instead of 2021 (which has little noise) is expected

The analysis in the presentation was done in 2024 setup with conditions of phase1_2024_realistic GT to test extreme case with large noise.

in #31064 (comment) the behavior of the fit in 2023 setup is unclear, perhaps it will become clear after the above item is clear

I'm so sorry I think I missed your question in the comment. For the energy of hits, the increase of energy of Hits in low energy region is expected and we observed that it causes the increase of the number of Jets. But we expect that it will be fine because newly emerged Jets after the noise correlation factor has lower Pt, less than 20 GeV. Distributions of Jet Pt and nJets are in my slide. If you think it would be better to have more checks, please let me know.

TaeunKwon · 2020-08-12T16:09:41Z

Was the correlation meant to reduce the chi2 (from first principles)? (I'm trying to understand if the sign in the correlation term is right)

No, the noise correlation factor increases the chi2 value of the fit. But as discussed in the DPG talk, chi2 value before the correlation was too small, considering chi2/ndof. So, the conclusion was the increase of chi2 value is okay because it makes the mean of chi2/ndof more closer to 1.

slava77 · 2020-08-12T16:44:02Z

an update to the study initially linked in the PR, but using 2023 or 2024 setup instead of 2021 (which has little noise) is expected

The analysis in the presentation was done in 2024 setup with conditions of phase1_2024_realistic GT to test extreme case with large noise.

Ah, sorry, I was "mislead" by 2021 in the introductory slide.
OK for this item then.

slava77 · 2020-08-12T16:49:11Z

Was the correlation meant to reduce the chi2 (from first principles)? (I'm trying to understand if the sign in the correlation term is right)

No, the noise correlation factor increases the chi2 value of the fit. But as discussed in the DPG talk, chi2 value before the correlation was too small, considering chi2/ndof. So, the conclusion was the increase of chi2 value is okay because it makes the mean of chi2/ndof more closer to 1.

Thank you for clarifying. When I read the slides on this part, it just seemed a bit more observational than from first principles.
For some future follow up it may be useful to have simplified injection tests where the input signal is well understood (Gaussian noise, properly unbiased pulse shape).

TaeunKwon · 2020-08-12T17:00:15Z

Thank you for clarifying. When I read the slides on this part, it just seemed a bit more observational than from first principles.
For some future follow up it may be useful to have simplified injection tests where the input signal is well understood (Gaussian noise, properly unbiased pulse shape).

Thanks for the suggestion, we will include such tests in further updates.

slava77 · 2020-08-12T17:21:37Z

+1

for #31064 600bf92

code changes are in line with the PR description and the follow up review
jenkins tests pass and comparisons with the baseline show differences only in Run3 workflows, starting from HBHE rechit variables, propagating downstream. The HBHE changes seen in the tests are in line with the studies shown in the DPG meeting (slides linked in the PR description). The igprof show that some increase in CPU time is expected, partly from the MAHI fit itself and partly from somewhat minor increase in energy of the reconstructed hits leading to possibly more work in the downstream modules. In reports for HLT and offline with PU the cost increase in calling MahiFit::phase1Apply is about 7%, which translates to about 0.1% of offline reco time.

cmsbuild · 2020-08-12T17:22:04Z

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. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

qliphy · 2020-08-13T05:21:33Z

+1

mariadalfonso · 2020-08-13T06:58:22Z

As a note for everybody.
The correlation is not properly implemented.

It's calculated in minimumBias where the main noise contribution is on the diagonal is the dark current, while in the hard scattering the photo statistics term dominate.
Should be derived and applied in reference to the pedval term.

TaeunKwon added 3 commits August 5, 2020 11:23

adding noise correlation between adjacent time slices to noise covari…

f28b567

…ance matrix

Update HBHEChannelInfo.h

644125b

correction on indentation

Update MahiFit.cc

38c4f88

cmsbuild added this to the CMSSW_11_2_X milestone Aug 5, 2020

cmsbuild added code-checks-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Aug 5, 2020

cmsbuild added code-checks-rejected and removed code-checks-pending labels Aug 5, 2020

Update HBHEChannelInfo.h

3bf261d

cmsbuild added code-checks-pending and removed code-checks-rejected labels Aug 5, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 5, 2020

slava77 reviewed Aug 5, 2020

View reviewed changes

RecoLocalCalo/HcalRecAlgos/src/MahiFit.cc Outdated Show resolved Hide resolved

Update MahiFit.cc

eba3186

cmsbuild added code-checks-pending and removed code-checks-approved labels Aug 5, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 5, 2020

cmsbuild added tests-approved and removed tests-started labels Aug 12, 2020

cmsbuild added comparison-available and removed comparison-pending labels Aug 12, 2020

cmsbuild removed pending-signatures reconstruction-pending labels Aug 12, 2020

cmsbuild added fully-signed reconstruction-approved labels Aug 12, 2020

cmsbuild added orp-approved and removed orp-pending labels Aug 13, 2020

cmsbuild merged commit 38b2bdb into cms-sw:master Aug 13, 2020

mariadalfonso mentioned this pull request Aug 21, 2020

HCAL - Run3 preparation cms-patatrack/cmssw#539

Closed

TaeunKwon mentioned this pull request Sep 18, 2020

HCAL MAHI code update to take only pedestal noise terms for correlation terms of a noise covariance matrix #31513

Merged

mariadalfonso mentioned this pull request Dec 3, 2020

Patatrack integration - HCAL local reconstruction (8/N) #31720

Merged

fwyzard mentioned this pull request Dec 8, 2020

Open issues regarding the HCAL local reconstruction on GPU #32413

Open

8 tasks

TaeunKwon deleted the NoiseCorrelation branch August 5, 2021 17:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

code updates to add a noise correlation factor between adjacent time slices for HB and HE channels #31064

code updates to add a noise correlation factor between adjacent time slices for HB and HE channels #31064

TaeunKwon commented Aug 5, 2020 •

edited

cmsbuild commented Aug 5, 2020

cmsbuild commented Aug 5, 2020

cmsbuild commented Aug 5, 2020

cmsbuild commented Aug 5, 2020

cmsbuild commented Aug 5, 2020

cmsbuild commented Aug 5, 2020

cmsbuild commented Aug 5, 2020

cmsbuild commented Aug 12, 2020

cmsbuild commented Aug 12, 2020

cmsbuild commented Aug 12, 2020

slava77 commented Aug 12, 2020

slava77 commented Aug 12, 2020

TaeunKwon commented Aug 12, 2020

TaeunKwon commented Aug 12, 2020

slava77 commented Aug 12, 2020

slava77 commented Aug 12, 2020

TaeunKwon commented Aug 12, 2020

slava77 commented Aug 12, 2020

cmsbuild commented Aug 12, 2020

qliphy commented Aug 13, 2020

mariadalfonso commented Aug 13, 2020

code updates to add a noise correlation factor between adjacent time slices for HB and HE channels #31064

code updates to add a noise correlation factor between adjacent time slices for HB and HE channels #31064

Conversation

TaeunKwon commented Aug 5, 2020 • edited

PR description:

PR validation:

if this PR is a backport please specify the original PR and why you need to backport that PR:

cmsbuild commented Aug 5, 2020

cmsbuild commented Aug 5, 2020

cmsbuild commented Aug 5, 2020

cmsbuild commented Aug 5, 2020

cmsbuild commented Aug 5, 2020

cmsbuild commented Aug 5, 2020

cmsbuild commented Aug 5, 2020

cmsbuild commented Aug 12, 2020

cmsbuild commented Aug 12, 2020

cmsbuild commented Aug 12, 2020

slava77 commented Aug 12, 2020

slava77 commented Aug 12, 2020

TaeunKwon commented Aug 12, 2020

TaeunKwon commented Aug 12, 2020

slava77 commented Aug 12, 2020

slava77 commented Aug 12, 2020

TaeunKwon commented Aug 12, 2020

slava77 commented Aug 12, 2020

cmsbuild commented Aug 12, 2020

qliphy commented Aug 13, 2020

mariadalfonso commented Aug 13, 2020

TaeunKwon commented Aug 5, 2020 •

edited