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

Puppi isolation miniAOD electrons & photons v2 #16172

Conversation

ishvetso
Copy link
Contributor

@ishvetso ishvetso commented Oct 11, 2016

PUPPI isolation for electrons and photons for miniAOD.
this is a rebase of #16055

@cmsbuild
Copy link
Contributor

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

@slava77
Copy link
Contributor

slava77 commented Oct 18, 2016

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 18, 2016

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

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Oct 18, 2016

+1

for #16172 af5583c

  • code changes are in line with the PR description and the follow up code review. For miniAOD electrons 6 new isolation floats are added and 3 new floats are added for photons.
  • jenkins tests pass and comparisons with baseline show no differences as expected (based on previous commit tests)
  • local tests with 1000 ttbar PU35 events show that new variables are filled with somewhat reasonable distributions for isolations. On the technical side:
    • the event size increased by 55 bytes/evt in slimmedPhotons and slimmedElectrons (with a few bytes of compression noise) or by 0.1%;
    • egmPhotonPUPPIIsolationForPhotons module is gone to be replaced by egmElectronPUPPIIsolation, egmElectronPUPPINoLeptonsIsolation, and egmPhotonPUPPIIsolation for a net increase in CPU time of about 3 ms/evt (0.15% of miniAOD processing time).

@cmsbuild
Copy link
Contributor

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

@slava77 comparisons for the following workflows were not done due to missing matrix map:

  • 20024.0_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2023D1_GenSimHLBeamSpotFull+DigiFull_2023D1+RecoFullGlobal_2023D1+HARVESTFullGlobal_2023D1
  • 22424.0_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2023D3Timing_GenSimHLBeamSpotFull+DigiFull_2023D3Timing+RecoFullGlobal_2023D3Timing+HARVESTFullGlobal_2023D3Timing

@monttj
Copy link
Contributor

monttj commented Oct 19, 2016

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_8_1_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @smuzaffar

@davidlange6
Copy link
Contributor

+1

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

7 participants