Calotower -> HCAL rechit for EGM ID variables #33666

afiqaize · 2021-05-07T22:26:11Z

PR description:

This is the PR implementing the calotower -> rechit transition in calculating EGM ID variables H/E and HCAL isolation. Codes relying on affected variables are updated accordingly. Minor changes are expected due to the different positioning of tower vs rechits. To preserve the ability to read old files, ioread rules are defined but currently this is only successful when reading RECO files, not AOD (see discussion in [1]).

[1] https://hypernews.cern.ch/HyperNews/CMS/get/recoData/126.html

PR validation:

WFs 10802.0, 135.4
validated that 5x5 BC H/E do not change wrt calotower version on a custom ele gun sample

cmsbuild · 2021-05-07T22:33:39Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33666/22563

This PR adds an extra 496KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File RecoEgamma/EgammaElectronAlgos/src/GsfElectronAlgo.cc modified in PR(s): Fix circular dependency between RecoEgamma and RecoEcal #33653
- File RecoEgamma/EgammaElectronProducers/plugins/GsfElectronProducer.cc modified in PR(s): Fix circular dependency between RecoEgamma and RecoEcal #33653
- File RecoEgamma/EgammaPhotonProducers/src/GEDPhotonProducer.cc modified in PR(s): Fix circular dependency between RecoEgamma and RecoEcal #33653

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

cmsbuild · 2021-05-07T22:53:17Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33666/22564

This PR adds an extra 508KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File RecoEgamma/EgammaElectronAlgos/src/GsfElectronAlgo.cc modified in PR(s): Fix circular dependency between RecoEgamma and RecoEcal #33653
- File RecoEgamma/EgammaElectronProducers/plugins/GsfElectronProducer.cc modified in PR(s): Fix circular dependency between RecoEgamma and RecoEcal #33653
- File RecoEgamma/EgammaPhotonProducers/src/GEDPhotonProducer.cc modified in PR(s): Fix circular dependency between RecoEgamma and RecoEcal #33653

cmsbuild · 2021-05-07T22:53:43Z

A new Pull Request was created by @afiqaize (Afiq Anuar) for master.

It involves the following packages:

DQM/Physics
DQMOffline/EGamma
DQMOffline/Trigger
DataFormats/EgammaCandidates
PhysicsTools/NanoAOD
RecoEgamma/EgammaElectronAlgos
RecoEgamma/EgammaElectronProducers
RecoEgamma/EgammaIsolationAlgos
RecoEgamma/EgammaPhotonProducers
RecoEgamma/EgammaTools
RecoEgamma/ElectronIdentification
RecoEgamma/Examples
RecoEgamma/PhotonIdentification
RecoParticleFlow/PFProducer
Validation/RecoEgamma

@perrotta, @andrius-k, @gouskos, @kmaeshima, @ErnestaP, @ahmad3213, @cmsbuild, @jfernan2, @fgolf, @slava77, @jpata, @mariadalfonso, @rvenditti can you please review it and eventually sign? Thanks.
@rappoccio, @jainshilpi, @varuns23, @cericeci, @mmarionncern, @jhgoh, @lgray, @missirol, @sscruz, @seemasharmafnal, @trocino, @rociovilar, @Sam-Harper, @rovere, @hatakeyamak, @mtosi, @HuguesBrun, @swertz, @Fedespring, @calderona, @sobhatta, @lecriste, @cbernet, @gpetruc, @ram1123 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

cmsbuild · 2021-05-07T23:51:09Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33666/22565

This PR adds an extra 816KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File RecoEgamma/EgammaElectronAlgos/src/GsfElectronAlgo.cc modified in PR(s): Fix circular dependency between RecoEgamma and RecoEcal #33653
- File RecoEgamma/EgammaElectronProducers/plugins/GsfElectronProducer.cc modified in PR(s): Fix circular dependency between RecoEgamma and RecoEcal #33653
- File RecoEgamma/EgammaPhotonProducers/src/GEDPhotonProducer.cc modified in PR(s): Fix circular dependency between RecoEgamma and RecoEcal #33653

cmsbuild · 2021-05-07T23:51:35Z

Pull request #33666 was updated. @perrotta, @andrius-k, @gouskos, @kmaeshima, @ErnestaP, @ahmad3213, @cmsbuild, @jfernan2, @fgolf, @slava77, @jpata, @mariadalfonso, @rvenditti can you please check and sign again.

slava77 · 2021-05-08T00:30:20Z

@cmsbuild please test

cmsbuild · 2021-05-08T04:22:46Z

-1

Failed Tests: RelVals RelVals-INPUT
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e76c7f/14947/summary.html
COMMIT: ed8f390
CMSSW: CMSSW_12_0_X_2021-05-07-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/33666/14947/install.sh to create a dev area with all the needed externals and cmssw changes.

CMS Clang-Tidy warnings: There are Clang-Tidy warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e76c7f/14947/llvm-analysis/cmsclangtidy.txt for details.

RelVals

136.7611136.7611_RunJetHT2016E_reminiaod+RunJetHT2016E_reminiaod+REMINIAOD_data2016_HIPM+HARVESTDR2_REMINIAOD_data2016_HIPM/step2_RunJetHT2016E_reminiaod+RunJetHT2016E_reminiaod+REMINIAOD_data2016_HIPM+HARVESTDR2_REMINIAOD_data2016_HIPM.log
140.53140.53_RunHI2011+RunHI2011+RECOHID11+HARVESTDHI/step2_RunHI2011+RunHI2011+RECOHID11+HARVESTDHI.log

RelVals-INPUT

136.7611136.7611_RunJetHT2016E_reminiaod+RunJetHT2016E_reminiaod+REMINIAOD_data2016_HIPM+HARVESTDR2_REMINIAOD_data2016_HIPM/step2_RunJetHT2016E_reminiaod+RunJetHT2016E_reminiaod+REMINIAOD_data2016_HIPM+HARVESTDR2_REMINIAOD_data2016_HIPM.log
136.7721136.7721_RunJetHT2016H_reminiaod+RunJetHT2016H_reminiaod+REMINIAOD_data2016+HARVESTDR2_REMINIAOD_data2016/step2_RunJetHT2016H_reminiaod+RunJetHT2016H_reminiaod+REMINIAOD_data2016+HARVESTDR2_REMINIAOD_data2016.log
140.52140.52_RunHI2010+RunHI2010+RECOHID10+RECOHIR10D11+HARVESTDHI/step2_RunHI2010+RunHI2010+RECOHID10+RECOHIR10D11+HARVESTDHI.log

Expand to see more relval errors ...

afiqaize · 2021-05-08T12:53:00Z

Apparently there are a few other codes that need to be modified that weren't caught by the WFs I used to validate. Will fix on Monday.

mariadalfonso · 2021-06-15T11:17:32Z

I have a general comment:

we call too many times the RecoEgamma/EgammaIsolationAlgos/python/egammaHBHERecHitThreshold_cff.py
and then in each instance (isolationCalculator,ecalDrivenElectronSeeds,gsfElectronProducer,conversionTrackCandidates,conversions,gedPhotons,photons) we look again to the channel quality and hcal severity level (that handle for really HCAL experts).

why not using the pfRecHit directly ? or at least have a produced intermediate that make your hbheforECALuse collection ? I think will save a lot of time if one does this once.

mariadalfonso · 2021-06-15T11:18:50Z

DQMOffline/EGamma/plugins/ElectronTagProbeAnalyzer.cc

@@ -665,11 +665,11 @@ bool ElectronTagProbeAnalyzer::isolationCut(const reco::GsfElectronCollection::c

  if (gsfIter->dr03TkSumPt() > tkIso03Max_)
    return true;
-  if (gsfIter->isEB() && gsfIter->dr03HcalDepth1TowerSumEt() > hcalIso03Depth1MaxBarrel_)
+  if (gsfIter->isEB() && gsfIter->dr03HcalTowerSumEt(1) > hcalIso03Depth1MaxBarrel_)


in Run3 HCAL now have more than one depth also in the barrel, so you should have the same multiple cases of isHB() as in the isEE() ?
This seems true for all the DQMOffline instances

Yes, and this is deliberate. This PR is intended purely for the tower to rechit migration, any other code is updated only to the extent necessary to be able to compile and run cmssw.

The fact that the meaning of things monitored have changed, or whether to monitor individual depths are outside of the scope and should be addressed elsewhere.

Could this be a follow-up PR in DQM?

mariadalfonso · 2021-06-15T11:30:57Z

RecoEgamma/EgammaElectronAlgos/interface/ElectronHcalHelper.h

  HcalTopology const *hcalTopology_ = nullptr;
+  HcalChannelQuality const *hcalChannelQuality_ = nullptr;
+  HcalSeverityLevelComputer const *hcalSevLvlComputer_ = nullptr;
+  CaloTowerConstituentsMap const *towerMap_ = nullptr;


I would like to understand why you still need from the CaloTowerConstituentsMap.
Here and in all other files.

For matching the ecal and hcal rechits behind cluster. See EgammaHcalIsolation::getHcalESumBc()

why this is done twice in the ElectronHcalHelper and the EgammaHcalIsolation.h

I see that for photon and GSFElectron you have hcalTowersBehindClusters()
https://github.com/cms-sw/cmssw/pull/33666/files#diff-2a8819f54a944ade88b9fc2dd659ed4b5c16725c31a250abd7ef89860e908dcdR459

and for Photons you found a way to avoid use the CaloTowerConstituentsMap
https://github.com/cms-sw/cmssw/pull/33666/files#diff-fd3b0e20a0dc203a5747ecc1765f08c85018edee832ab5967e260d1f7d06bb35L353

ElectronHcalHelper uses EgammaHcalIsolation under the hood.

And PhotonProducer doesn't explicitly show CaloTowerConstituentsMap usage only because ElectronHcalHelper takes care of this internally, while EgammaHcalIsolation does not.

is this still an issue? I could ask for a bit more context to understand the problem.

afiqaize · 2021-06-15T11:45:05Z

why not using the pfRecHit directly ?

#33666 (comment)

or at least have a produced intermediate that make your hbheforECALuse collection ? I think will save a lot of time if one does this once.

I'm not aware of an existing filter to do this and didn't want to write a new one; never seen that timing was a serious issue. The test jobs take about the same time. If a filter already exists then would be great if you can point me to it.

jpata · 2021-06-17T14:16:00Z

Just trying to make sure it doesn't get stuck (it's a long PR already), I left some comments inline.

@mariadalfonso are there blockers from XPOG or HCAL side, or could your comments be implemented as a follow-up (I can add them to an issue)?

qliphy · 2021-06-22T01:19:00Z

kindly ping @cms-sw/xpog-l2

jpata · 2021-06-29T14:29:18Z

@mariadalfonso are there blockers from XPOG or HCAL side, or could your comments be implemented as a follow-up (I can add them to an issue)?

kind ping

jpata · 2021-07-09T13:48:43Z

Kind ping @cms-sw/xpog-l2. Please let us know which comments are critical, and which can be done in a follow-up / refactoring. We should aim to get this in in the next 2 weeks for 12_0_0_pre5.

mariadalfonso · 2021-07-13T11:05:38Z

+xpog

the only xPOG change is this one: PhysicsTools/NanoAOD/python/electrons_cff.py
needed update to follow changes in this PR.

general comments:
there are too many repetition of the HCAL dedicated classes (hcalTopology, caloTowerMap hcalChannelQuality, hcalSevLvlComputer, towerMap, re-implementation of the threshold)
Multiple calls were present in the original code as well.
Note in view of the Run3, the severity level will not matter since we are with all siPM, so better to move to pf-rechit directly

cmsbuild · 2021-07-13T11:06: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, @perrotta (and backports should be raised in the release meeting by the corresponding L2)

qliphy · 2021-07-14T00:52:30Z

+1

cmsbuild added this to the CMSSW_12_0_X milestone May 7, 2021

cmsbuild added code-checks-pending dqm-pending orp-pending pending-signatures reconstruction-pending tests-pending xpog-pending labels May 7, 2021

cmsbuild added code-checks-rejected code-checks-pending and removed code-checks-pending code-checks-rejected labels May 7, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels May 7, 2021

cmsbuild added code-checks-pending and removed code-checks-approved labels May 7, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels May 7, 2021

cmsbuild added tests-started and removed tests-pending labels May 8, 2021

cmsbuild added tests-rejected and removed tests-started labels May 8, 2021

mariadalfonso reviewed Jun 15, 2021

View reviewed changes

This was referenced Jun 22, 2021

DQM: migrate modules to esConsumes #34208

Merged

Update L1 tracking for HL-LHC (fixed) #34028

Closed

This was referenced Jul 2, 2021

Phase2 RECO/Egamma RECO modifications v1 #34314

Closed

Use std:: functions instead of TMath:: functions in RecoEgamma #34325

Merged

DRAFT: nano: adaptation to run in Prompt #34329

Closed

cmsbuild mentioned this pull request Jul 10, 2021

Clean/rename FromMultiCl HGCal egamma collections #34439

Merged

cmsbuild added fully-signed xpog-approved and removed pending-signatures xpog-pending labels Jul 13, 2021

cmsbuild added orp-approved and removed orp-pending labels Jul 14, 2021

cmsbuild merged commit 37bf1fa into cms-sw:master Jul 14, 2021

slava77 mentioned this pull request Jul 26, 2021

reco comparisons: add monitoring for hcal depth in egamma and HGC_ egamma collections cms-sw/cms-bot#1598

Merged

swagata87 mentioned this pull request Aug 27, 2021

[EGM HLT] Phase out calotower & use HCAL recHits directly #35049

Merged

slava77 mentioned this pull request Nov 2, 2021

mitigate high CPU cost of HBHE hit position calculation in EgammaHcalIsolation #35955

Merged

jpata mentioned this pull request Feb 14, 2022

Adding JPTjets to MiniAOD #36335

Merged

swagata87 mentioned this pull request Oct 21, 2022

duplicated hcalDepthTowerSumEt in GsfElectron.h #39805

Merged

swagata87 mentioned this pull request Nov 22, 2023

Apply HCal noise thresholds to HCal detector-based isolation of electrons #43355

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Calotower -> HCAL rechit for EGM ID variables #33666

Calotower -> HCAL rechit for EGM ID variables #33666

afiqaize commented May 7, 2021

cmsbuild commented May 7, 2021

cmsbuild commented May 7, 2021

cmsbuild commented May 7, 2021

cmsbuild commented May 7, 2021

cmsbuild commented May 7, 2021

slava77 commented May 8, 2021

cmsbuild commented May 8, 2021

afiqaize commented May 8, 2021 •

edited

mariadalfonso commented Jun 15, 2021

mariadalfonso Jun 15, 2021 •

edited

afiqaize Jun 15, 2021

jpata Jun 17, 2021

mariadalfonso Jun 15, 2021

afiqaize Jun 15, 2021

mariadalfonso Jun 15, 2021

afiqaize Jun 15, 2021

jpata Jun 17, 2021

afiqaize commented Jun 15, 2021

jpata commented Jun 17, 2021

qliphy commented Jun 22, 2021

jpata commented Jun 29, 2021

jpata commented Jul 9, 2021

mariadalfonso commented Jul 13, 2021 •

edited

cmsbuild commented Jul 13, 2021

qliphy commented Jul 14, 2021

Calotower -> HCAL rechit for EGM ID variables #33666

Calotower -> HCAL rechit for EGM ID variables #33666

Conversation

afiqaize commented May 7, 2021

PR description:

PR validation:

cmsbuild commented May 7, 2021

cmsbuild commented May 7, 2021

cmsbuild commented May 7, 2021

cmsbuild commented May 7, 2021

cmsbuild commented May 7, 2021

slava77 commented May 8, 2021

cmsbuild commented May 8, 2021

RelVals

RelVals-INPUT

afiqaize commented May 8, 2021 • edited

mariadalfonso commented Jun 15, 2021

mariadalfonso Jun 15, 2021 • edited

Choose a reason for hiding this comment

afiqaize Jun 15, 2021

Choose a reason for hiding this comment

jpata Jun 17, 2021

Choose a reason for hiding this comment

mariadalfonso Jun 15, 2021

Choose a reason for hiding this comment

afiqaize Jun 15, 2021

Choose a reason for hiding this comment

mariadalfonso Jun 15, 2021

Choose a reason for hiding this comment

afiqaize Jun 15, 2021

Choose a reason for hiding this comment

jpata Jun 17, 2021

Choose a reason for hiding this comment

afiqaize commented Jun 15, 2021

jpata commented Jun 17, 2021

qliphy commented Jun 22, 2021

jpata commented Jun 29, 2021

jpata commented Jul 9, 2021

mariadalfonso commented Jul 13, 2021 • edited

cmsbuild commented Jul 13, 2021

qliphy commented Jul 14, 2021

afiqaize commented May 8, 2021 •

edited

mariadalfonso Jun 15, 2021 •

edited

mariadalfonso commented Jul 13, 2021 •

edited