Adding the codes for ECAL Phase2 amplitude local reconstruction #34505

BiancaPinolini · 2021-07-15T17:27:11Z

This PR adds the codes for the amplitude local reconstruction for ECAL for Phase 2. Details about the implemented weights method to reconstruct the amplitude have been presented in the ECAL DPG meeting here.
The PR was prepared with the help of @amassiro and @thomreis.

cmsbuild · 2021-07-15T17:34:24Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34505/23993

This PR adds an extra 16KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File RecoLocalCalo/EcalRecProducers/plugins/EcalUncalibRecHitPhase2WeightsProducer.cc modified in PR(s): Bianca pinolini ecal reco phase2 commit unimib #34212
- File RecoLocalCalo/EcalRecProducers/python/ecalUncalibRecHitPhase2_cff.py modified in PR(s): Bianca pinolini ecal reco phase2 commit unimib #34212
- File RecoLocalCalo/EcalRecProducers/python/ecalUncalibRecHitPhase2_cfi.py modified in PR(s): Bianca pinolini ecal reco phase2 commit unimib #34212
- File RecoLocalCalo/EcalRecProducers/test/testEcalUncalibRecHitPhase2WeightsProducer.py modified in PR(s): Bianca pinolini ecal reco phase2 commit unimib #34212

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-34505/23993/code-format.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34505/23993/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2021-07-16T15:46:39Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34505/24025

This PR adds an extra 20KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File RecoLocalCalo/EcalRecProducers/plugins/EcalUncalibRecHitPhase2WeightsProducer.cc modified in PR(s): Bianca pinolini ecal reco phase2 commit unimib #34212
- File RecoLocalCalo/EcalRecProducers/python/ecalUncalibRecHitPhase2_cff.py modified in PR(s): Bianca pinolini ecal reco phase2 commit unimib #34212
- File RecoLocalCalo/EcalRecProducers/python/ecalUncalibRecHitPhase2_cfi.py modified in PR(s): Bianca pinolini ecal reco phase2 commit unimib #34212
- File RecoLocalCalo/EcalRecProducers/test/testEcalUncalibRecHitPhase2WeightsProducer.py modified in PR(s): Bianca pinolini ecal reco phase2 commit unimib #34212

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-34505/24025/code-format.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34505/24025/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

slava77 · 2021-07-19T13:56:46Z

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

please address this; this is blocking the further test

cmsbuild · 2021-07-19T14:43:17Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34505/24078

This PR adds an extra 20KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File RecoLocalCalo/EcalRecProducers/plugins/EcalUncalibRecHitPhase2WeightsProducer.cc modified in PR(s): Bianca pinolini ecal reco phase2 commit unimib #34212
- File RecoLocalCalo/EcalRecProducers/python/ecalUncalibRecHitPhase2_cff.py modified in PR(s): Bianca pinolini ecal reco phase2 commit unimib #34212
- File RecoLocalCalo/EcalRecProducers/python/ecalUncalibRecHitPhase2_cfi.py modified in PR(s): Bianca pinolini ecal reco phase2 commit unimib #34212
- File RecoLocalCalo/EcalRecProducers/test/testEcalUncalibRecHitPhase2WeightsProducer.py modified in PR(s): Bianca pinolini ecal reco phase2 commit unimib #34212

cmsbuild · 2021-07-19T14:43:44Z

A new Pull Request was created by @BiancaPinolini (Bianca Sofia Pinolini) for master.

It involves the following packages:

RecoLocalCalo/EcalRecProducers (reconstruction)

@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, @perrotta you are the release manager for this.

cms-bot commands are listed here

slava77 · 2021-07-19T23:04:26Z

@cmsbuild please test

cmsbuild · 2021-07-20T02:29:21Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-01dbfc/16995/summary.html
COMMIT: 6510980
CMSSW: CMSSW_12_0_X_2021-07-19-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/34505/16995/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

@slava77 comparisons for the following workflows were not done due to missing matrix map:

/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-01dbfc/11634.912_TTbar_14TeV+2021_DD4hepDB+TTbar_14TeV_TuneCP5_GenSim+Digi+Reco+HARVEST+ALCA

Summary:

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

cmsbuild · 2021-08-10T17:27:46Z

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

slava77 · 2021-08-10T17:29:32Z

@cmsbuild please test

cmsbuild · 2021-08-10T22:03:20Z

-1

Failed Tests: RelVals-INPUT
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-01dbfc/17676/summary.html
COMMIT: 524da38
CMSSW: CMSSW_12_1_X_2021-08-10-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/34505/17676/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals-INPUT

23434.2123434.21_TTbar_14TeV+2026D49PU_ProdLike+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14INPUT+DigiTriggerPU+RecoGlobalPU+MiniAODPU/step2_TTbar_14TeV+2026D49PU_ProdLike+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14INPUT+DigiTriggerPU+RecoGlobalPU+MiniAODPU.log
23434.9923434.99_TTbar_14TeV+2026D49PU_PMXS1S2+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14INPUT+PREMIX_PremixHLBeamSpot14PU+DigiTriggerPU+RecoGlobalPU+HARVESTGlobalPU/step2_TTbar_14TeV+2026D49PU_PMXS1S2+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14INPUT+PREMIX_PremixHLBeamSpot14PU+DigiTriggerPU+RecoGlobalPU+HARVESTGlobalPU.log
23434.992123434.9921_TTbar_14TeV+2026D49PU_PMXS1S2ProdLike+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14INPUT+PREMIX_PremixHLBeamSpot14PU+DigiTriggerPU+RecoGlobalPU+MiniAODPU/step2_TTbar_14TeV+2026D49PU_PMXS1S2ProdLike+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14INPUT+PREMIX_PremixHLBeamSpot14PU+DigiTriggerPU+RecoGlobalPU+MiniAODPU.log

Expand to see more relval errors ...

Comparison Summary

Summary:

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

slava77 · 2021-08-12T17:26:25Z

@cmsbuild please test

it looks like the phase-2 test workflows that failed the last time were fixed by now

slava77 · 2021-08-12T19:15:33Z

RecoLocalCalo/EcalRecProducers/test/testEcalUncalibRecHitPhase2WeightsProducer.py

+process.L1Reco_step = cms.Path(process.L1Reco)
+process.reconstruction_step = cms.Path(cms.Sequence(cms.Task(
+    bunchSpacingProducer,
+    ecalUncalibRecHitPhase2Task


addition to a task is not enough if there is nothing to consume this. So, this module does not run.
you will also need smth like

process.FEVTDEBUGHLToutput.outputCommands += ["keep *_ecalUncalibRecHitPhase2_*_*"]

added in this file.

I will not delay the signature because of this though.

cmsbuild · 2021-08-12T20:35:16Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-01dbfc/17746/summary.html
COMMIT: 524da38
CMSSW: CMSSW_12_1_X_2021-08-12-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/34505/17746/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: 6 differences found in the comparisons
DQMHistoTests: Total files compared: 39
DQMHistoTests: Total histograms compared: 2999420
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 2999391
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.004 KiB( 38 files compared)
DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
Checked 165 log files, 37 edm output root files, 39 DQM output files
TriggerResults: no differences found

slava77 · 2021-08-12T23:26:11Z

I finally managed to remind myself of the other ECAL (uncalib) rechit producer structure.
Essentially everything is based on the factory pattern.
E.g. the EcalUncalibRecHitProducer.cc currently produces EB and EE uncalib rechits (compared to the same type but EB rechits only in the proposed EcalUncalibRecHitPhase2WeightsProducer), while a specific worker (e.g. EcalUncalibRecHitWorkerWeights) creates rechits by calling an instance of EcalUncalibRecHitRecWeightsAlgo. In this PR the EcalUncalibRecHitPhase2WeightsProducer does everything in situ with some fairly simple logic, which looks more like a simplified toy (considering also a few hardcoded/fixed parameters).

What is the plan for future developments?
Wouldn't it be better to simply fit in the existing structure, but modify it to work separately on EB vs EE? Currently, it looks like the further phase-2 producers may end up being written in full and from scratch without much structure design allowing for alternatives.

amassiro · 2021-08-13T13:37:22Z

Hi,
I'm not sure about the Phase2 global plan for ECAL, and let me ping directly ecal dpg @cms-sw/ecal-dpg-l2 , in case they want to comment.
The idea is to have right now at least a simple ecal local reconstruction for Phase2 for ECAL (missing otherwise).
For Phase2 we will need also GPU versions, and I think that, for sake of maintenance, having simple separate reconstruction algorithms work better, but this is just my opinion.
It's worth having this discussed in an ECAL DPG meeting too.

About EB vs EE, EE will not be there anymore in Phase2, so this is just EB.

I finally managed to remind myself of the other ECAL (uncalib) rechit producer structure.
Essentially everything is based on the factory pattern.
E.g. the EcalUncalibRecHitProducer.cc currently produces EB and EE uncalib rechits (compared to the same type but EB rechits only in the proposed EcalUncalibRecHitPhase2WeightsProducer), while a specific worker (e.g. EcalUncalibRecHitWorkerWeights) creates rechits by calling an instance of EcalUncalibRecHitRecWeightsAlgo. In this PR the EcalUncalibRecHitPhase2WeightsProducer does everything in situ with some fairly simple logic, which looks more like a simplified toy (considering also a few hardcoded/fixed parameters).

What is the plan for future developments?
Wouldn't it be better to simply fit in the existing structure, but modify it to work separately on EB vs EE? Currently, it looks like the further phase-2 producers may end up being written in full and from scratch without much structure design allowing for alternatives.

slava77 · 2021-08-13T23:26:07Z

+reconstruction

for #34505 524da38

code changes are in line with the PR description and the follow up review. From the discussion in the thread it seems more important to have a starting point with a simple implementation to make uncalib rechits so that further developments can continue.
jenkins tests pass and comparisons with the baseline show no differences (the new EcalUncalibRecHitPhase2WeightsProducer is not in the standard workflows)
- the test configuration provided with the PR runs after a small modification to actually enable the module

cmsbuild · 2021-08-13T23:26:27Z

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

slava77 · 2021-08-13T23:27:08Z

The idea is to have right now at least a simple ecal local reconstruction for Phase2 for ECAL (missing otherwise).

OK, I created #34880 in order to hopefully come back to this relatively soon

qliphy · 2021-08-15T02:23:54Z

+1

Adding the codes for ECAL Phase2 amplitude local reconstruction

099b90b

cmsbuild added this to the CMSSW_12_0_X milestone Jul 15, 2021

cmsbuild added code-checks-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Jul 15, 2021

cmsbuild added code-checks-rejected and removed code-checks-pending labels Jul 15, 2021

Fixing code-format

0aa39d3

cmsbuild added code-checks-pending and removed code-checks-rejected labels Jul 16, 2021

cmsbuild added code-checks-rejected and removed code-checks-pending labels Jul 16, 2021

thomreis mentioned this pull request Jul 16, 2021

Bianca pinolini ecal reco phase2 commit unimib #34212

Closed

Fixing code format 2

6510980

cmsbuild added code-checks-pending and removed code-checks-rejected labels Jul 19, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Jul 19, 2021

cmsbuild added tests-started and removed tests-pending labels Jul 19, 2021

cmsbuild removed the tests-started label Jul 20, 2021

cmsbuild added the code-checks-approved label Aug 10, 2021

cmsbuild added tests-started and removed tests-pending labels Aug 10, 2021

cmsbuild added tests-rejected and removed tests-started labels Aug 10, 2021

cmsbuild added tests-started and removed tests-rejected labels Aug 12, 2021

slava77 reviewed Aug 12, 2021

View reviewed changes

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

slava77 mentioned this pull request Aug 13, 2021

establish/review the design plans for phase-2 ECAL rechit reconstruction #34880

Open

cmsbuild added fully-signed reconstruction-approved and removed reconstruction-pending pending-signatures labels Aug 13, 2021

cmsbuild added orp-approved and removed orp-pending labels Aug 15, 2021

cmsbuild merged commit 07b7c6f into cms-sw:master Aug 15, 2021

thomreis mentioned this pull request Jan 19, 2022

ECAL Phase 2 Development WF 28234.61 fix #36748

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding the codes for ECAL Phase2 amplitude local reconstruction #34505

Adding the codes for ECAL Phase2 amplitude local reconstruction #34505

BiancaPinolini commented Jul 15, 2021

cmsbuild commented Jul 15, 2021

cmsbuild commented Jul 16, 2021

slava77 commented Jul 19, 2021

cmsbuild commented Jul 19, 2021

cmsbuild commented Jul 19, 2021

slava77 commented Jul 19, 2021

cmsbuild commented Jul 20, 2021

cmsbuild commented Aug 10, 2021

slava77 commented Aug 10, 2021

cmsbuild commented Aug 10, 2021

slava77 commented Aug 12, 2021

slava77 Aug 12, 2021

cmsbuild commented Aug 12, 2021

slava77 commented Aug 12, 2021

amassiro commented Aug 13, 2021

slava77 commented Aug 13, 2021

cmsbuild commented Aug 13, 2021

slava77 commented Aug 13, 2021

qliphy commented Aug 15, 2021

Adding the codes for ECAL Phase2 amplitude local reconstruction #34505

Adding the codes for ECAL Phase2 amplitude local reconstruction #34505

Conversation

BiancaPinolini commented Jul 15, 2021

cmsbuild commented Jul 15, 2021

cmsbuild commented Jul 16, 2021

slava77 commented Jul 19, 2021

cmsbuild commented Jul 19, 2021

cmsbuild commented Jul 19, 2021

slava77 commented Jul 19, 2021

cmsbuild commented Jul 20, 2021

Comparison Summary

cmsbuild commented Aug 10, 2021

slava77 commented Aug 10, 2021

cmsbuild commented Aug 10, 2021

RelVals-INPUT

Comparison Summary

slava77 commented Aug 12, 2021

slava77 Aug 12, 2021

Choose a reason for hiding this comment

cmsbuild commented Aug 12, 2021

Comparison Summary

slava77 commented Aug 12, 2021

amassiro commented Aug 13, 2021

slava77 commented Aug 13, 2021

cmsbuild commented Aug 13, 2021

slava77 commented Aug 13, 2021

qliphy commented Aug 15, 2021