Change the isolation in the reco::Photon to the value computed by CITK #16759

ishvetso · 2016-11-25T12:44:13Z

This PR changes the isolation in the reco::Photon object to the value computed by CITK as presented here:
https://indico.cern.ch/event/589257/contributions/2376119/attachments/1376717/2090803/Shvetsov_23November2016.pdf

Following changes are done the particleFlowEGammaFinal sequence:

creation particleBasedIsolationTmp object
add pfNoPileUpCandidates to the
CITK module to compute isolation

Additional minor changes in CITK:

isolation cone definitions is move to a standalone file RecoEgamma/EgammaIsolationAlgos/python/egmIsoConeDefinitions_cfi.py
fillDescriptions() function is added to PhysicsTools/IsolationAlgos/plugins/CITKPFIsolationSumProducer.cc

include @gzevi , @ikrav, @fcouderc

cmsbuild · 2016-11-25T12:44:30Z

A new Pull Request was created by @ishvetso (Ivan Shvetsov) for CMSSW_9_0_X.

It involves the following packages:

PhysicsTools/IsolationAlgos
RecoEgamma/EgammaIsolationAlgos
RecoEgamma/EgammaPhotonProducers
RecoParticleFlow/PFProducer

@cmsbuild, @cvuosalo, @slava77, @monttj, @davidlange6 can you please review it and eventually sign? Thanks.
@mmarionncern, @Sam-Harper, @rafaellopesdesa, @lgray, @cbernet, @bachtis this is something you requested to watch as well.
@slava77, @smuzaffar you are the release manager for this.

cms-bot commands are listed here #13028

cvuosalo · 2016-11-25T13:09:46Z

@cmsbuild please test

cmsbuild · 2016-11-25T13:10:03Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/16600/console

cmsbuild · 2016-11-25T14:02:42Z

+1
Tested at: 4c44738
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16759/16600/summary.html

cmsbuild · 2016-11-25T14:02:45Z

Comparison job queued.

cmsbuild · 2016-11-25T14:35:34Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16759/16600/summary.html

slava77 · 2016-11-28T12:59:20Z

@cmsbuild please test

cmsbuild · 2016-11-28T12:59:34Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/16628/console

cmsbuild · 2016-11-28T16:09:18Z

+1
Tested at: 9068b11
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16759/16628/summary.html

cmsbuild · 2016-11-28T16:09:21Z

Comparison job queued.

cmsbuild · 2016-11-28T18:14:22Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16759/16628/summary.html

The workflows 1003.0, 1001.0, 1000.0, 140.53, 136.731, 4.22 have different files in step1_dasquery.log than the ones found in the baseline. You may want to check and retrigger the tests if necessary. You can check it in the "files" directory in the results of the comparisons

cvuosalo · 2016-11-28T19:46:18Z

@ishvetso: The "All Conversions" plot I included here (#16759 (comment)) is a standard DQM plot that is checked by Jenkins for every pull request. I don't know the details of the selection in the plot.

ishvetso · 2016-11-28T20:54:44Z

this seems to be caused by relinking in ReducedEGProducer here:

cmssw/RecoEgamma/EgammaPhotonProducers/python/reducedEgamma_cfi.py

Line 6 in 5332703

    
           relinkPhotons = cms.string("(r9()>0.8 || chargedHadronIso()<20 || chargedHadronIso()<0.3*pt())"), #keep all associated clusters/rechits/conversions

That should explain the first picture but I'm not sure about conversions.

cvuosalo · 2016-11-29T20:04:56Z

@gzevi: Could you please comment on how changes to isolation in this PR could change photon conversion plots like the one shown here:

#16759 (comment)

Is isolation used by the gedPhotonAnalyzer?

gzevi · 2016-11-29T21:09:36Z

@cvuosalo: I am not sure how this works. I can’t find the plot you pasted in the folder [1], so I don’t know exactly where it comes from. Based on its title, and what you say in the email, it looks like this plot is produced here [2] based on input from this analyzer [3]. The code seems to have no isolation selection on the conversions that are plotted (only nTracks, isValid and Chi2prob), and no isolation selection for photons of type “All”. So I think this plot is not applying isolation, but I might be reading the wrong code. The question is whether conversion reconstruction applies isolation, upstream of this code. [1] https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_9_0_X_2016-11-24-2300+16759/17173/ [2] http://cmslxr.fnal.gov/source/DQMOffline/EGamma/plugins/PhotonOfflineClient.cc#0267 [3] http://cmslxr.fnal.gov/source/DQMOffline/EGamma/plugins/PhotonAnalyzer.cc#1017

…

On Nov 29, 2016, at 2:06 PM, Carl Vuosalo ***@***.***> wrote: @gzevi <https://github.com/gzevi>: Could you please comment on how changes to isolation in this PR could change photon conversion plots like the one shown here: #16759 (comment) <#16759 (comment)> Is isolation used by the gedPhotonAnalyzer? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16759 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFH0Vy-V5ehNyqe0HJ8cbo1EgUWCzf_tks5rDIWxgaJpZM4K8aE1>.

cvuosalo · 2016-11-29T21:24:42Z

@gzevi: The plots 1 come from a test of workflow 1332.0_H125GGgluonfusion_13 with 1000 events against baseline CMSSW_9_0_X_2016-11-22-2300. I used the standard Jenkins code to compare the PR with baseline and make the plots.
It looks like we still have a mystery. Why are many Reco quantities from AOD/RECO being slightly changed by this PR, which should only be changing isolation? We have an answer for Mini-AOD quantities like those shown in the reducedEGamma plot, but not the others.
@rafaellopesdesa: Could you please contact EGM experts to get some information about the downstream effects of changing photon isolation?

gzevi · 2016-11-29T21:35:43Z

Just to make sure I understand: Reco quantities in miniAOD are expected to change, as shown above, because the photons are treated differently depending on their isolation. The issue is Reco quantities in Reco (for example the conversion plot you show, which I understand is made from AOD). Please correct if I misunderstood.

cvuosalo · 2016-11-29T22:06:16Z

@gzevi: Yes, your statement of the issue is correct. (I edited my previous comment to be more clear.)

lgray · 2016-11-29T22:19:32Z

I'm taking a look...

cvuosalo · 2016-11-29T22:32:16Z

Looking more carefully, I see that the differences are limited to DQM photon conversion plots produced by the gedPhotonAnalyzer (except for the Mini-AOD changes that are expected).

lgray · 2016-11-29T22:36:58Z

OK - I also made sure that the photon analyzer wasn't being run with the same settings for the MiniAOD DQM.

lgray · 2016-11-29T22:53:20Z

Hi, I found the issue.
The isolation in "photonAnalyzer" for the DQM uses the charged hadron isolation to select photons.

See here: https://github.com/cms-sw/cmssw/blob/CMSSW_9_0_X/DQMOffline/EGamma/plugins/PhotonAnalyzer.cc#L1181-L1207

No mystery, just very convoluted code. Changes expected, in that case.

lgray · 2016-11-29T23:10:08Z

Also the plot labelling is misleading, "AllPhotons" actually means "non isolated photons" according to the DQM code.

lgray · 2016-11-29T23:10:33Z

This last point explains all other differences.

i.e. #16759 (comment)

gzevi · 2016-11-29T23:17:10Z

Thanks Lindsey!
For the record, the feature that fooled some of us was that the "All" category of plots only contains photons that fail the "GoodCandidate" selection. Should be easy to confirm by looking at the whole set of plots.

cvuosalo · 2016-11-30T00:08:50Z

+1

For #16759 9068b11

Changing to use CITK for computing photon isolation.

The code changes are satisfactory. Jenkins tests against baseline CMSSW_9_0_X_2016-11-27-2300 show many small differences expected from the change in isolation, as discussed extensively above. Additional tests results discussed above also show only expected changes, no increase in CPU time, and only a slight increase in memory usage.

ishvetso added 12 commits November 24, 2016 17:42

changes for test

3681ad1

add header

b54d3e0

fix bug with name

4b1be38

uncomment getByToken

32ddef8

add CITK module to egamma sequence: for test

8eb7355

fix a bug with renaming of particleFlowTmpPtrs

50f3805

add pfNoPileCandidates

8933add

fix conflict

289732a

fix conflict

350f43d

add filldescriptions

3f1ba48

move isoCone definitions in standalone file

c865116

fix syntax for IsoConeDefinitionsTmp

4c44738

cmsbuild added this to the Next CMSSW_9_0_X milestone Nov 25, 2016

cmsbuild added analysis-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Nov 25, 2016

cmsbuild added tests-started and removed tests-pending labels Nov 25, 2016

ishvetso changed the title ~~Change this isolation in the reco::Photon to the value computed by CITK~~ Change the isolation in the reco::Photon to the value computed by CITK Nov 25, 2016

cmsbuild added tests-approved and removed tests-started labels Nov 25, 2016

cmsbuild added tests-started and removed tests-pending labels Nov 28, 2016

cmsbuild added tests-approved and removed tests-started labels Nov 28, 2016

cmsbuild added comparison-available and removed comparison-pending labels Nov 28, 2016

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Nov 30, 2016

davidlange6 merged commit 370ecf5 into cms-sw:CMSSW_9_0_X Dec 1, 2016

ishvetso mentioned this pull request Feb 12, 2017

Change the isolation in the reco::GsfElectron to the value computed by CITK #17494

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change the isolation in the reco::Photon to the value computed by CITK #16759

Change the isolation in the reco::Photon to the value computed by CITK #16759

ishvetso commented Nov 25, 2016

cmsbuild commented Nov 25, 2016

cvuosalo commented Nov 25, 2016

cmsbuild commented Nov 25, 2016 •

edited

cmsbuild commented Nov 25, 2016

cmsbuild commented Nov 25, 2016

cmsbuild commented Nov 25, 2016

slava77 commented Nov 28, 2016

cmsbuild commented Nov 28, 2016 •

edited

cmsbuild commented Nov 28, 2016

cmsbuild commented Nov 28, 2016

cmsbuild commented Nov 28, 2016

cvuosalo commented Nov 28, 2016

ishvetso commented Nov 28, 2016 •

edited

cvuosalo commented Nov 29, 2016

gzevi commented Nov 29, 2016 via email

cvuosalo commented Nov 29, 2016 •

edited

gzevi commented Nov 29, 2016

cvuosalo commented Nov 29, 2016

lgray commented Nov 29, 2016

cvuosalo commented Nov 29, 2016

lgray commented Nov 29, 2016

lgray commented Nov 29, 2016 •

edited

lgray commented Nov 29, 2016

lgray commented Nov 29, 2016 •

edited

gzevi commented Nov 29, 2016

cvuosalo commented Nov 30, 2016

Change the isolation in the reco::Photon to the value computed by CITK #16759

Change the isolation in the reco::Photon to the value computed by CITK #16759

Conversation

ishvetso commented Nov 25, 2016

cmsbuild commented Nov 25, 2016

cvuosalo commented Nov 25, 2016

cmsbuild commented Nov 25, 2016 • edited

cmsbuild commented Nov 25, 2016

cmsbuild commented Nov 25, 2016

cmsbuild commented Nov 25, 2016

slava77 commented Nov 28, 2016

cmsbuild commented Nov 28, 2016 • edited

cmsbuild commented Nov 28, 2016

cmsbuild commented Nov 28, 2016

cmsbuild commented Nov 28, 2016

cvuosalo commented Nov 28, 2016

ishvetso commented Nov 28, 2016 • edited

cvuosalo commented Nov 29, 2016

gzevi commented Nov 29, 2016 via email

cvuosalo commented Nov 29, 2016 • edited

gzevi commented Nov 29, 2016

cvuosalo commented Nov 29, 2016

lgray commented Nov 29, 2016

cvuosalo commented Nov 29, 2016

lgray commented Nov 29, 2016

lgray commented Nov 29, 2016 • edited

lgray commented Nov 29, 2016

lgray commented Nov 29, 2016 • edited

gzevi commented Nov 29, 2016

cvuosalo commented Nov 30, 2016

cmsbuild commented Nov 25, 2016 •

edited

cmsbuild commented Nov 28, 2016 •

edited

ishvetso commented Nov 28, 2016 •

edited

cvuosalo commented Nov 29, 2016 •

edited

lgray commented Nov 29, 2016 •

edited

lgray commented Nov 29, 2016 •

edited