Cross-correlation algorithm for the ECAL timing reconstruction #33119

nminafra · 2021-03-09T15:16:32Z

This PR introduces a new algorithm for the timing reconstruction of ECAL. It looks for the maximum cross-correlation between the acquired pulse and a template with different time shifts.
This method uses the same pulse template ad the energy reconstruction, hence it doesn't need any parameter.
Also, Out-Of-Time (OOT) Pileup is mitigated by removing the OOT amplitudes using the result of the multi-fit algorithm.

The performance was discussed in various DPG meetings.
Some references with more details are available:
ECAL general meeting:
https://indico.cern.ch/event/1002949/contributions/4211423/attachments/2182555/3687212/ECALtiming_2021_2_2.pdf

Performance on Monte Carlo:
https://indico.cern.ch/event/966023/contributions/4135611/attachments/2163562/3652215/ecaldpg_121620.pdf

Description of the algorithm (called KUCC here) and performance on data:
https://indico.cern.ch/event/953451/contributions/4026342/attachments/2108199/3545710/ecaldpg_gentime.pdf

Notes for this PR:
this PR is including the code into the main repository, but the default Timing reco method will remain unchanged. It is, of course, possible to enable this method changing the "timingMethod" variable.

Example of test file:
RecoLocalCalo/EcalRecProducers/test/testEcalUncalibRechitProducerWithCC_cfg.py

@thomreis @jking79 @crogan

…go.h

…ingCCAlgo obj created once)

cmsbuild · 2021-03-09T15:22:42Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33119/21462

This PR adds an extra 88KB to repository

cmsbuild · 2021-03-09T15:23:07Z

A new Pull Request was created by @nminafra (Nicola Minafra) for master.

It involves the following packages:

RecoLocalCalo/EcalRecAlgos
RecoLocalCalo/EcalRecProducers

@perrotta, @jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@rchatter, @apsallid, @argiro, @thomreis, @simonepigazzini 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

slava77 · 2021-03-10T02:52:53Z

@cmsbuild please test

slava77 · 2021-03-16T15:50:16Z

RecoLocalCalo/EcalRecAlgos/interface/EcalUncalibRecHitTimingCCAlgo.h

  double computeTimeCC(const EcalDataFrame& dataFrame,
                       const std::vector<double>& amplitudes,
                       const EcalPedestals::Item* aped,
                       const EcalMGPAGainRatio* aGain,
                       const FullSampleVector& fullpulse,
                       EcalUncalibratedRecHit& uncalibRecHit,
-                       float& errOnTime);
+                       float errOnTime) const;


Suggested change

float errOnTime) const;

float& errOnTime) const;

I noticed only now thanks to the static analyzer https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-74f75c/13543/llvm-analysis/report-9ba007.html#EndPath.

slava77 · 2021-03-16T16:17:36Z

RecoLocalCalo/EcalRecAlgos/src/EcalUncalibRecHitTimingCCAlgo.cc

+  if (counter < MIN_NUM_OF_ITERATIONS || counter > MAX_NUM_OF_ITERATIONS - 1) {
+    tM = TIME_WHEN_NOT_CONVERGING * ecalPh1::Samp_Period;
+    //Negative error means that there was a problem with the CC
+    errOnTime = -targetTimePrecision_ / ecalPh1::Samp_Period;


BTW, I do not see a value assigned to errOnTime for normal conditions.
Is it intentional?

nminafra · 2021-03-16T17:36:27Z

Thanks @slava77 for catching the reference pass. I rushed the correction too much!
errOnTime was not set, but indeed it would be more correct to set it to the minimization target if the minimization is successful.
I've also removed uncalibRecHit.setJitterError(0.); that is not needed here.

cmsbuild · 2021-03-16T17:43:47Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33119/21609

This PR adds an extra 28KB to repository

cmsbuild · 2021-03-16T17:44:07Z

Pull request #33119 was updated. @perrotta, @jpata, @cmsbuild, @slava77 can you please check and sign again.

slava77 · 2021-03-16T17:46:38Z

@cmsbuild please test

cmsbuild · 2021-03-16T20:23:42Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-74f75c/13556/summary.html
COMMIT: e10760c
CMSSW: CMSSW_11_3_X_2021-03-16-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/33119/13556/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 37
DQMHistoTests: Total histograms compared: 2639881
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2639858
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 36 files compared)
Checked 155 log files, 37 edm output root files, 37 DQM output files
TriggerResults: no differences found

slava77 · 2021-03-16T23:01:20Z

+1

for #33119 e10760c

code changes are in line with the PR description and the follow up review; the new feature (crossCorrelationMethod) is not enabled by default
jenkins tests pass and comparisons with the baseline show no differences
signoff is based on visual code review and the profiler reports provided in Cross-correlation algorithm for the ECAL timing reconstruction #33119 (comment) (the new way of computing the rechit time takes slightly smaller CPU time than the old/current default ratio method)

cmsbuild · 2021-03-16T23:01:42Z

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 · 2021-03-17T01:08:12Z

+1

nminafra · 2021-03-17T08:07:03Z

Thanks everyone and thanks @slava77 for all the improvements to the code.

Best regards,
Nicola

nminafra added 19 commits February 12, 2021 15:15

Rebasing kucc for 11_3

7ba20c8

Simplified inclusion of kucc

38506d2

Implemented thomreis suggestions iteration 1

a93f1a2

scram build code-format

c0bdba3

kansasMethodCC renamed as crossCorrelationMethod

a7b115f

added comment for magic constants from EcalUncalibRecHitTimeWeightsAl…

857a61c

…go.h

kansasMethodCC renamed as crossCorrelationMethod complete

4d1c340

Second round of thomreis comments

13690d6

Code cleanup and small performance optimization (EcalUncalibRecHitTim…

465da15

…ingCCAlgo obj created once)

Added parameters in python files

514e38d

Added parameters to ParameterSetDescription

d183be0

Adding test file dedicated to CC method

a74b1fc

working test file

75833b8

LogWarning to LogInfo when too many CC iterations

21901ad

slightly more efficient

3be47f2

Cleanup

a916b16

Cleanup of test

1e4a5df

code-checks and code-format

252a9e4

Further cleanup

45c5972

cmsbuild added this to the CMSSW_11_3_X milestone Mar 9, 2021

cmsbuild added code-checks-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Mar 9, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 9, 2021

slava77 reviewed Mar 16, 2021

View reviewed changes

errOnTime fix

e10760c

cmsbuild added code-checks-pending tests-pending and removed code-checks-approved tests-approved labels Mar 16, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 16, 2021

cmsbuild added tests-started and removed tests-pending labels Mar 16, 2021

cmsbuild added tests-approved and removed tests-started labels Mar 16, 2021

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels Mar 16, 2021

cmsbuild added orp-approved and removed orp-pending labels Mar 17, 2021

cmsbuild merged commit 4bcdf96 into cms-sw:master Mar 17, 2021

thomreis mentioned this pull request Mar 17, 2021

Cross Correlation algorithm for ECAL timing, first implementation thomreis/cmssw#30

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cross-correlation algorithm for the ECAL timing reconstruction #33119

Cross-correlation algorithm for the ECAL timing reconstruction #33119

nminafra commented Mar 9, 2021

cmsbuild commented Mar 9, 2021

cmsbuild commented Mar 9, 2021

slava77 commented Mar 10, 2021

slava77 Mar 16, 2021 •

edited

slava77 Mar 16, 2021

nminafra commented Mar 16, 2021

cmsbuild commented Mar 16, 2021

cmsbuild commented Mar 16, 2021

slava77 commented Mar 16, 2021

cmsbuild commented Mar 16, 2021

slava77 commented Mar 16, 2021

cmsbuild commented Mar 16, 2021

qliphy commented Mar 17, 2021

nminafra commented Mar 17, 2021

Cross-correlation algorithm for the ECAL timing reconstruction #33119

Cross-correlation algorithm for the ECAL timing reconstruction #33119

Conversation

nminafra commented Mar 9, 2021

cmsbuild commented Mar 9, 2021

cmsbuild commented Mar 9, 2021

slava77 commented Mar 10, 2021

slava77 Mar 16, 2021 • edited

Choose a reason for hiding this comment

slava77 Mar 16, 2021

Choose a reason for hiding this comment

nminafra commented Mar 16, 2021

cmsbuild commented Mar 16, 2021

cmsbuild commented Mar 16, 2021

slava77 commented Mar 16, 2021

cmsbuild commented Mar 16, 2021

Comparison Summary

slava77 commented Mar 16, 2021

cmsbuild commented Mar 16, 2021

qliphy commented Mar 17, 2021

nminafra commented Mar 17, 2021

slava77 Mar 16, 2021 •

edited