Do NaN-check before accepting a PF candidate for computing photon's isolation #39120

swagata87 · 2022-08-20T10:49:32Z

PR description:

This PR proposes to add a NaN-check on PF candidate's pT, before accepting that candidate for photon's isolation calculation.

It was found, as discussed in this github issue (#39110), that in some rare cases, PF candidate's pT/eta/phi is -nan, and this leads to photon's PF-based isolation to be -nan as well. The patch provided in this PR will bypass that problem for photons, and photon's isolation sum will return a meaningful value.

In recent data (2022C, EGamma dataset) in Run 355872, Event 546279379, LumiSection 508,
without this PR: photon's chargedHadronIso=-nan
with this PR: photon's chargedHadronIso=0.683148
This is checked with these files:
miniAOD: /store/data/Run2022C/EGamma/MINIAOD/PromptReco-v1/000/355/872/00000/035b7e72-7f0d-42c8-98c7-d0a5100211fe.root
RAW: /store/data/Run2022C/EGamma/RAW/v1/000/355/872/00000/3b478527-d206-4b8e-8004-08e9aff7758b.root
and, eventsToProcess = cms.untracked.VEventRange('355872:546279379-355872:546279379')

It was checked that similar nan issues in MC, discussed in the github issue (#39110), are also fixed with this patch, as expected.

Note: we have not found any instance of electron's PFChargedHadIso being nan. Until now, the issue happened only for photons.

PR validation:

runTheMatrix.py -l 11834.0

This PR is not a backport.
Since the issue seem to be happening also in prompt reco data, a backport to current data-taking release cycle (12_4_X) might be helpful, to have meaningful PF isolation values of reconstructed photons, in all events.

cmsbuild · 2022-08-20T10:57:14Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39120/31674

This PR adds an extra 16KB to repository

cmsbuild · 2022-08-20T10:57:39Z

A new Pull Request was created by @swagata87 (Swagata Mukherjee) for master.

It involves the following packages:

RecoEgamma/PhotonIdentification (reconstruction)

@jpata, @cmsbuild, @mandrenguyen, @clacaputo can you please review it and eventually sign? Thanks.
@Sam-Harper, @jainshilpi, @valsdav, @lgray, @sobhatta, @afiqaize, @wrtabb, @varuns23, @ram1123 this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy, @rappoccio you are the release manager for this.

cms-bot commands are listed here

swagata87 · 2022-08-20T11:00:02Z

@cmsbuild please test

swagata87 · 2022-08-20T13:30:06Z

type egamma

cmsbuild · 2022-08-20T14:49:12Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d9bdb3/26946/summary.html
COMMIT: 304d87b
CMSSW: CMSSW_12_5_X_2022-08-20-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/39120/26946/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: 51
DQMHistoTests: Total histograms compared: 3693040
DQMHistoTests: Total failures: 2
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3693016
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 50 files compared)
Checked 212 log files, 49 edm output root files, 51 DQM output files
TriggerResults: no differences found

jpata · 2022-08-22T08:45:11Z

Perhaps a warning message would be appropriate. It seems to me we don't want to hide the problem of NaN PF candidates downstream (other stuff beyond Egamma might be affected).

cmsbuild · 2022-08-22T11:15:18Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39120/31698

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

jpata · 2022-08-25T08:02:49Z

Tests look good. @swagata87 did you confirm it fixes the NaN in photon isolation as well (I wasn't able to reproduce the issue myself so far).

swagata87 · 2022-08-25T08:16:43Z

Tests look good. @swagata87 did you confirm it fixes the NaN in photon isolation as well (I wasn't able to reproduce the issue myself so far).

Hi, yes I confirm that the nan issue is solved for photon isolation.

jpata

A very minor performance improvement suggested (there should be no need to copy the vectors).
I hope we can merge this today.

jpata · 2022-08-25T08:37:51Z

RecoEgamma/PhotonIdentification/plugins/PhotonIDValueMapProducer.cc

@@ -96,7 +96,7 @@ class PhotonIDValueMapProducer : public edm::global::EDProducer<> {
  // HiggsTo2photons anaysis code. Template is introduced to handle reco/pat
  // photons and aod/miniAOD PF candidates collections
  float computeWorstPFChargedIsolation(const reco::Photon& photon,
-                                       const edm::View<reco::Candidate>& pfCands,
+                                       const std::vector<edm::Ptr<reco::Candidate>> pfCands,


Suggested change

const std::vector<edm::Ptr<reco::Candidate>> pfCands,

const std::vector<edm::Ptr<reco::Candidate>>& pfCands,

to avoid a vector copy.

jpata · 2022-08-25T08:38:18Z

RecoEgamma/PhotonIdentification/plugins/PhotonIDValueMapProducer.cc

@@ -358,7 +367,7 @@ void PhotonIDValueMapProducer::fillDescriptions(edm::ConfigurationDescriptions&
 // Charged isolation with respect to the worst vertex. See more
 // comments above at the function declaration.
 float PhotonIDValueMapProducer::computeWorstPFChargedIsolation(const reco::Photon& photon,
-                                                               const edm::View<reco::Candidate>& pfCands,
+                                                               const std::vector<edm::Ptr<reco::Candidate>> pfCands,


Suggested change

const std::vector<edm::Ptr<reco::Candidate>> pfCands,

const std::vector<edm::Ptr<reco::Candidate>>& pfCands,

to avoid a vector copy.

cmsbuild · 2022-08-25T09:22:27Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39120/31791

This PR adds an extra 20KB to repository

cmsbuild · 2022-08-25T09:22:54Z

Pull request #39120 was updated. @jpata, @cmsbuild, @mandrenguyen, @clacaputo can you please check and sign again.

perrotta · 2022-08-25T09:27:36Z

please test

cmsbuild · 2022-08-25T13:19:35Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d9bdb3/27083/summary.html
COMMIT: 20aa664
CMSSW: CMSSW_12_5_X_2022-08-24-2300/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/39120/27083/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: 51
DQMHistoTests: Total histograms compared: 3693084
DQMHistoTests: Total failures: 2
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3693060
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 50 files compared)
Checked 212 log files, 49 edm output root files, 51 DQM output files
TriggerResults: no differences found

jpata · 2022-08-25T14:02:48Z

+reconstruction

mitigates reco::Candidate pt = -nan in some rare cases #39110, the effect of NaN PFCandidates on the photon isolation side
no reco changes in tests (It was confirmed by Swagata that it does indeed fix the rare NaN in the problematic file, which is not included in the tests)

cmsbuild · 2022-08-25T14:03:30Z

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

perrotta · 2022-08-25T14:36:42Z

+1

check for nan in PF cand

304d87b

cmsbuild added this to the CMSSW_12_5_X milestone Aug 20, 2022

cmsbuild added code-checks-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Aug 20, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 20, 2022

cmsbuild added tests-started and removed tests-pending labels Aug 20, 2022

swagata87 mentioned this pull request Aug 20, 2022

reco::Candidate pt = -nan in some rare cases #39110

Closed

cmsbuild added the egamma label Aug 20, 2022

cmsbuild added tests-approved and removed tests-started labels Aug 20, 2022

raise warning if NaN PFcand is found

241675c

cmsbuild added code-checks-pending tests-pending and removed tests-approved code-checks-approved labels Aug 22, 2022

cmsbuild added code-checks-rejected and removed code-checks-pending labels Aug 22, 2022

code check

4a4074c

jpata reviewed Aug 25, 2022

View reviewed changes

avoid vector copy

20aa664

cmsbuild added code-checks-pending tests-pending and removed tests-approved code-checks-approved labels Aug 25, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 25, 2022

cmsbuild added tests-started and removed tests-pending labels Aug 25, 2022

cmsbuild added tests-approved and removed tests-started labels Aug 25, 2022

cmsbuild added fully-signed reconstruction-approved and removed reconstruction-pending pending-signatures labels Aug 25, 2022

cmsbuild added orp-approved and removed orp-pending labels Aug 25, 2022

cmsbuild merged commit 8ac417d into cms-sw:master Aug 25, 2022

swagata87 mentioned this pull request Aug 25, 2022

[12_4_X backport] Do NaN-check before accepting a PF candidate for computing photon's isolation #39202

Merged

cmsbuild mentioned this pull request Aug 26, 2022

[GCC][PATCH] libstdc++: Mark non-exported function always_inline cms-sw/cmsdist#8039

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do NaN-check before accepting a PF candidate for computing photon's isolation #39120

Do NaN-check before accepting a PF candidate for computing photon's isolation #39120

swagata87 commented Aug 20, 2022

cmsbuild commented Aug 20, 2022

cmsbuild commented Aug 20, 2022

swagata87 commented Aug 20, 2022

swagata87 commented Aug 20, 2022

cmsbuild commented Aug 20, 2022

jpata commented Aug 22, 2022

cmsbuild commented Aug 22, 2022

jpata commented Aug 25, 2022

swagata87 commented Aug 25, 2022

jpata left a comment •

edited

Loading

jpata Aug 25, 2022

jpata Aug 25, 2022

cmsbuild commented Aug 25, 2022

cmsbuild commented Aug 25, 2022

perrotta commented Aug 25, 2022

cmsbuild commented Aug 25, 2022

jpata commented Aug 25, 2022

cmsbuild commented Aug 25, 2022

perrotta commented Aug 25, 2022

	const std::vector<edm::Ptr<reco::Candidate>> pfCands,
	const std::vector<edm::Ptr<reco::Candidate>>& pfCands,

Do NaN-check before accepting a PF candidate for computing photon's isolation #39120

Do NaN-check before accepting a PF candidate for computing photon's isolation #39120

Conversation

swagata87 commented Aug 20, 2022

PR description:

PR validation:

cmsbuild commented Aug 20, 2022

cmsbuild commented Aug 20, 2022

swagata87 commented Aug 20, 2022

swagata87 commented Aug 20, 2022

cmsbuild commented Aug 20, 2022

Comparison Summary

jpata commented Aug 22, 2022

cmsbuild commented Aug 22, 2022

jpata commented Aug 25, 2022

swagata87 commented Aug 25, 2022

jpata left a comment • edited Loading

Choose a reason for hiding this comment

jpata Aug 25, 2022

Choose a reason for hiding this comment

jpata Aug 25, 2022

Choose a reason for hiding this comment

cmsbuild commented Aug 25, 2022

cmsbuild commented Aug 25, 2022

perrotta commented Aug 25, 2022

cmsbuild commented Aug 25, 2022

Comparison Summary

jpata commented Aug 25, 2022

cmsbuild commented Aug 25, 2022

perrotta commented Aug 25, 2022

jpata left a comment •

edited

Loading