Correctly add HIPhotonIsolation to pat::photon #32462

guitargeek · 2020-12-13T18:01:10Z

PR description:

This is to address issue #32451.

I was asked if there is a simpler python-level-only solution for that issue than to hard-code the propagation of the HI isolation value map in the ReducedEGProducer. Well, I couldn't find any and I don't think it's even possible because the HIPhotonIsolation is a custom class. Hence, we can't use the existing mechanisms for int and float ValueMaps.

Accordingly, this PR proposes the solution via editing the ReducedEGProducer source.

@mandrenguyen @ttrk

PR validation:

CMSSW compiles, local matrix tests pass.

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

No backport intened.

cmsbuild · 2020-12-13T18:07:41Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32462/20362

This PR adds an extra 24KB 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-32462/20362/code-format.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32462/20362/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2020-12-13T18:28:10Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32462/20363

This PR adds an extra 24KB to repository

cmsbuild · 2020-12-13T18:28:31Z

A new Pull Request was created by @guitargeek (Jonas Rembser) for master.

It involves the following packages:

RecoEgamma/EgammaPhotonProducers
RecoEgamma/EgammaTools

@perrotta, @jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@Sam-Harper, @jainshilpi, @rovere, @lgray, @sobhatta, @lecriste, @afiqaize, @varuns23, @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

slava77 · 2020-12-13T18:29:22Z

@cmsbuild please test

slava77 · 2020-12-13T18:36:21Z

RecoEgamma/EgammaPhotonProducers/src/ReducedEGProducer.cc

@@ -238,6 +242,13 @@ ReducedEGProducer::ReducedEGProducer(const edm::ParameterSet& config)
          config.getParameter<edm::InputTag>("photonsPFValMap"))),
      gsfElectronPfCandMapT_(consumes<edm::ValueMap<std::vector<reco::PFCandidateRef>>>(
          config.getParameter<edm::InputTag>("gsfElectronsPFValMap"))),
+      recoHIPhotonIsolationMapInputToken_{
+          config.existsAs<edm::InputTag>("HIPhotonIsolationMapInput")


other cases in this producer rely on empty inputTags; I suggest to stay with that logic, also considering that use of existsAs is generally not welcome.

also, please stay with lower case

Suggested change

config.existsAs<edm::InputTag>("HIPhotonIsolationMapInput")

config.existsAs<edm::InputTag>("hiPhotonIsolationMapInput")

Okay, I will drop the existsAs after the tests. I know it's not welcome because fillDescriptions is preferred. But since there is no fillDescriptions, isn't it better than an empty tag? I thought existsAs would make an eventual fillDescription migration easier because it makes it explicit that the parameter is optional. In the end I think the consistency with the other tags is the strongest argument for dropping the existsAs here, so I agree with you in the end.

cmsbuild · 2020-12-13T21:27:56Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-13de91/11582/summary.html
CMSSW: CMSSW_11_3_X_2020-12-13-0000/slc7_amd64_gcc900

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 2 differences found in the comparisons
DQMHistoTests: Total files compared: 36
DQMHistoTests: Total histograms compared: 2747284
DQMHistoTests: Total failures: 10
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2747252
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 35 files compared)
Checked 153 log files, 37 edm output root files, 36 DQM output files

guitargeek · 2020-12-13T21:33:26Z

FYI it's expected that there are no reco differences. The HI events in the regular matrix tests have no photons. Privately, I used wf 158.2, the Photon+Jets HI workflow, to validate this PR.

cmsbuild · 2020-12-13T21:37:06Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32462/20366

This PR adds an extra 28KB to repository

cmsbuild · 2020-12-18T13:16:53Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32462/20495

This PR adds an extra 28KB to repository

cmsbuild · 2020-12-18T13:17:15Z

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

slava77 · 2020-12-18T13:45:16Z

@cmsbuild please test

cmsbuild · 2020-12-18T21:10:58Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-13de91/11792/summary.html
CMSSW: CMSSW_11_3_X_2020-12-18-1100/slc7_amd64_gcc900

DAS Queries failed

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 3 differences found in the comparisons
DQMHistoTests: Total files compared: 36
DQMHistoTests: Total histograms compared: 2716967
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 2716938
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.004 KiB( 35 files compared)
DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
Checked 153 log files, 37 edm output root files, 36 DQM output files

slava77 · 2020-12-19T01:13:05Z

+1

for #32462 e299916

code changes are in line with the PR description and the follow up review: the association of the photon isolation is appropriately propagated via reducedEgamma
jenkins tests pass and comparisons with the baseline show no (relevant) difference: on one hand the used HI tests apparently do not have any slimmedPhotons; on the other hand the bugfix is for the alignment of the isolation variable with different photons, which would not be visible in the simple inclusive plots

the signoff relies mainly on the expected behavior of the modified code (the old variant relied on simple alignment by virtue of having photon_config.photonSrc defined; now it defaults through the originalObjectRef as follows from

cmssw/RecoEgamma/EgammaTools/interface/EGExtraInfoModifierFromValueMaps.h

Lines 212 to 217 in af19244

    
           edm::Ptr<reco::Candidate> ptr(pho.originalObjectRef()); 
        
           if (!ph_conf.tok_photon_src.isUninitialized()) 
        
             ptr = phos_by_oop.at(pho_idx); 
        
           //now we go through and modify the objects using the valuemaps we read in 
        
           EGXtraModFromVMObjFiller<OutputType>::addValuesToObject( 
        
               pho, ptr, ph_conf.tok_valuemaps, pho_vmaps, overrideExistingValues_);

)

No backport intened.

actually, since the main target for HI miniAOD is 11_2_X, please provide a backport to 11_2_X

cmsbuild · 2020-12-19T01:13:29Z

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-12-19T02:08:03Z

+1

mandrenguyen · 2021-01-08T14:37:35Z

@guitargeek Would you mind backporting this to 11_2_X, which is the release for HI miniAOD? Sorry, I should have caught that before the holiday.

guitargeek · 2021-01-08T15:06:46Z

Okay! There you go #32619

cmsbuild added this to the CMSSW_11_3_X milestone Dec 13, 2020

cmsbuild added code-checks-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Dec 13, 2020

cmsbuild added code-checks-rejected code-checks-pending and removed code-checks-pending code-checks-rejected labels Dec 13, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Dec 13, 2020

cmsbuild added tests-started and removed tests-pending labels Dec 13, 2020

slava77 reviewed Dec 13, 2020

View reviewed changes

cmsbuild added tests-approved code-checks-pending tests-pending and removed tests-started code-checks-approved tests-approved labels Dec 13, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Dec 13, 2020

cmsbuild added code-checks-pending tests-pending and removed tests-approved labels Dec 18, 2020

implement review comments to reducedEgamma_cfi.py

e299916

cmsbuild added code-checks-approved and removed code-checks-pending labels Dec 18, 2020

cmsbuild added tests-started and removed tests-pending labels Dec 18, 2020

cmsbuild added tests-approved and removed tests-started labels Dec 18, 2020

cmsbuild added fully-signed and removed pending-signatures reconstruction-pending labels Dec 19, 2020

cmsbuild added the reconstruction-approved label Dec 19, 2020

cmsbuild added orp-approved and removed orp-pending labels Dec 19, 2020

cmsbuild merged commit d495a3c into cms-sw:master Dec 19, 2020

guitargeek deleted the slimmedPhotons_HI_1 branch December 19, 2020 09:57

guitargeek mentioned this pull request Jan 8, 2021

[11_2_X] Correctly add HIPhotonIsolation to pat::photon #32619

Merged

guitargeek mentioned this pull request Jan 10, 2021

Use EDPutToken in ReducedEGProducer and modernize GEDPhotonCoreProducer #32626

Merged

slava77 mentioned this pull request Feb 11, 2021

HIPhotonIsolation not correctly added to pat::photon #32451

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correctly add HIPhotonIsolation to pat::photon #32462

Correctly add HIPhotonIsolation to pat::photon #32462

guitargeek commented Dec 13, 2020

cmsbuild commented Dec 13, 2020

cmsbuild commented Dec 13, 2020

cmsbuild commented Dec 13, 2020

slava77 commented Dec 13, 2020

slava77 Dec 13, 2020

slava77 Dec 13, 2020

guitargeek Dec 13, 2020

cmsbuild commented Dec 13, 2020

guitargeek commented Dec 13, 2020

cmsbuild commented Dec 13, 2020

cmsbuild commented Dec 18, 2020

cmsbuild commented Dec 18, 2020

slava77 commented Dec 18, 2020

cmsbuild commented Dec 18, 2020

slava77 commented Dec 19, 2020

cmsbuild commented Dec 19, 2020

qliphy commented Dec 19, 2020

mandrenguyen commented Jan 8, 2021

guitargeek commented Jan 8, 2021

	config.existsAs<edm::InputTag>("HIPhotonIsolationMapInput")
	config.existsAs<edm::InputTag>("hiPhotonIsolationMapInput")

Correctly add HIPhotonIsolation to pat::photon #32462

Correctly add HIPhotonIsolation to pat::photon #32462

Conversation

guitargeek commented Dec 13, 2020

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 Dec 13, 2020

cmsbuild commented Dec 13, 2020

cmsbuild commented Dec 13, 2020

slava77 commented Dec 13, 2020

slava77 Dec 13, 2020

Choose a reason for hiding this comment

slava77 Dec 13, 2020

Choose a reason for hiding this comment

guitargeek Dec 13, 2020

Choose a reason for hiding this comment

cmsbuild commented Dec 13, 2020

Comparison Summary

guitargeek commented Dec 13, 2020

cmsbuild commented Dec 13, 2020

cmsbuild commented Dec 18, 2020

cmsbuild commented Dec 18, 2020

slava77 commented Dec 18, 2020

cmsbuild commented Dec 18, 2020

Comparison Summary

slava77 commented Dec 19, 2020

cmsbuild commented Dec 19, 2020

qliphy commented Dec 19, 2020

mandrenguyen commented Jan 8, 2021

guitargeek commented Jan 8, 2021