Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added code to set isolated Photon puppi weight to 1 for loose id photons above 10 GeV #13157

Merged
merged 4 commits into from Feb 3, 2016

Conversation

violatingcp
Copy link
Contributor

Code which sets the weight of Loose id Isolated photons to weight 1. This fixes puppet behavior in gamma+jets events where isolated high pT photons get weight 0. The code works by setting the weight weight of all associated pfcandidates to the designated puppiweight (1, but its configurable). This will allow for improved electron and photon isolation since an improved footprint removal can be done.

An example of the inteded use in puppet is shown in the test config.

@nhanvtran @hsatoshi

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 2, 2016

A new Pull Request was created by @violatingcp (Philip Harris) for CMSSW_8_0_X.

It involves the following packages:

CommonTools/PileupAlgos

@cvuosalo, @monttj, @cmsbuild, @slava77, @vadler, @davidlange6 can you please review it and eventually sign? Thanks.
@ahinzmann, @jdolen, @rappoccio this is something you requested to watch as well.
@slava77, @Degano, @smuzaffar you are the release manager for this.

cms-bot commands are list here #13028

return false;
}
// ------------------------------------------------------------------------------------------
void PuppiPhoton::beginJob() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove all empty methods (from cc and from the header file).
There is more maintenance burden than benefits from potential future use of these placeholders.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what you mean, this method is used in a number places in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, I understand

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@slava77
Copy link
Contributor

slava77 commented Feb 2, 2016

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 2, 2016

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

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 2, 2016

-1
Tested at: 3748459
When I ran the RelVals I found an error in the following worklfows:
4.22 step1

DAS Error

you can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-13157/10918/summary.html

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 2, 2016

Pull request #13157 was updated. @cvuosalo, @monttj, @cmsbuild, @slava77, @vadler, @davidlange6 can you please check and sign again.

if(!usePFRef_) continue;
try{
const pat::Photon *pPho = dynamic_cast<const pat::Photon*>(&(*itPho));
if(pPho != 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nullptr instead of 0 is preferred (minor though)

@slava77
Copy link
Contributor

slava77 commented Feb 2, 2016

static analysis of the previous version
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-13157/10918/llvm-analysis/
points to the "try-catch" (see comment inline in the code)

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 2, 2016

Pull request #13157 was updated. @cvuosalo, @monttj, @cmsbuild, @slava77, @vadler, @davidlange6 can you please check and sign again.

@slava77
Copy link
Contributor

slava77 commented Feb 2, 2016

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 2, 2016

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

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 2, 2016

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 2, 2016

@slava77
Copy link
Contributor

slava77 commented Feb 3, 2016

+1

for #13157 759396d

  • code changes are OK, in line with the description. The new code is not a part of running workflows, it will be used either as a part of miniAOD creation or, more likely, at the analysis level on miniAOD as an input
  • jenkins tests pass and show no differences (nothing new is running)
  • the test file runs after a change to process.load('Configuration.StandardSequences.GeometryRecoDB_cff') . Checked on RelValTTbar_13/CMSSW_8_0_0_pre5PU25ns_80X_mcRun2_asymptotic_v1/MINIAODSIM, the puppiPhoton takes about 4 ms/event.

davidlange6 added a commit that referenced this pull request Feb 3, 2016
Added code to set isolated Photon puppi weight to 1 for loose id photons above 10 GeV
@davidlange6 davidlange6 merged commit 3d4de85 into cms-sw:CMSSW_8_0_X Feb 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants