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

Simplify PUPPI Photon producer and change PUPPI isolation algo #27929

Merged
merged 12 commits into from Nov 1, 2019

Conversation

ahinzmann
Copy link
Contributor

@ahinzmann ahinzmann commented Sep 4, 2019

PR description:

Simplify PUPPI MET computation, by removing (over-complicated) PUPPI photon algorithm and replace it by a simple option in PUPPI itself (avoid any kind of matching etc.).
The PR is supposed to keep tuning and physics of puppi (for jets) and puppiForMET untouched, however puppiNoLeptons (used for lepton isolation) is modified to include the same protection for high pT photons as puppiForMET.

PR validation:

Checked on 3000 QCD events that the high-pT photon candidates get the weight increased to 1 in old and new implementation.
Small differences arise when particle pt is close to the pT threshold of 20 within packedCandidate precision.

@lathomas @zdemirag @abenecke @nhanvtran @violatingcp

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 4, 2019

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 4, 2019

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27929/11764

  • This PR adds an extra 28KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 4, 2019

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 4, 2019

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27929/11769

  • This PR adds an extra 32KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 4, 2019

A new Pull Request was created by @ahinzmann for master.

It involves the following packages:

CommonTools/PileupAlgos
PhysicsTools/PatAlgos

@perrotta, @cmsbuild, @santocch, @slava77 can you please review it and eventually sign? Thanks.
@rappoccio, @gouskos, @emilbols, @HeinerTholen, @peruzzim, @seemasharmafnal, @mmarionncern, @smoortga, @acaudron, @jdolen, @ferencek, @jdamgov, @nhanvtran, @gkasieczka, @schoef, @mariadalfonso, @clelange, @riga, @JyothsnaKomaragiri, @mverzett, @gpetruc, @andrzejnovak, @pvmulder this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

@slava77
Copy link
Contributor

slava77 commented Sep 4, 2019

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 4, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/2365/console Started: 2019/09/04 20:13

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 4, 2019

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 4, 2019

Comparison job queued.

@cmsbuild
Copy link
Contributor

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 30 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2961036
  • DQMHistoTests: Total failures: 168
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2960527
  • DQMHistoTests: Total skipped: 341
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
  • Checked 147 log files, 16 edm output root files, 34 DQM output files

@slava77
Copy link
Contributor

slava77 commented Oct 29, 2019

+1

for #27929 71d320d

  • code changes are in line with the PR description and the follow up review
  • jenkins tests pass and comparisons with the baseline show differences related to puppiNoLep photons.
  • local tests show somewhat expected and acceptable (eventually, after checking with EGM) behavior
    • the most visible changes are in slimmedElectrons puppiNoLep photon isolation in photon-reach sample Simplify PUPPI Photon producer and change PUPPI isolation algo #27929 (comment). This is a result of pf photons getting now a larger weight in puppiNoLep.
    • some visible changes also appear in the deepTau ID variables; the effect appears to be rather small based on ten-tau (RelValTenTau_15_500 from 1100pre7) sample there is a rather tiny improvement in rejection vs electron and small degradation in rejection vs jets (below)
    • technical costs are reduced by about 0.3% in CPU time wrt miniAOD job, by removing puppiMerged and changing puppiForMET

deepTau2017v2p1:VSe
all_sign1088VSorig_RelValTenTau_15_500_2018c_patTaus_slimmedTaus__PAT_obj___tauIDs__72__second
VSeVVTight
all_sign1088VSorig_RelValTenTau_15_500_2018c_patTaus_slimmedTaus__PAT_obj___tauIDs__91__second

VSjet
all_sign1088VSorig_RelValTenTau_15_500_2018c_patTaus_slimmedTaus__PAT_obj___tauIDs__73__second
VSjetVVTight
all_sign1088VSorig_RelValTenTau_15_500_2018c_patTaus_slimmedTaus__PAT_obj___tauIDs__92__second

@mbluj @swozniewski

@mbluj
Copy link
Contributor

mbluj commented Oct 30, 2019

I think that differences in deepTau scores are small enough to be accepted. However, it can be an issue for analyses if there is a plan to backport this development to releases used for ongoing Run-2 analyses (mainly 102X).

@fabiocos
Copy link
Contributor

@santocch as far as I am concerned this PR could technically be integrated in master, I am not sure we really want to backport it (up to proponents to say). In any case please check
@peruzzim @fgolf FYI for possible downstream impact on nanoAOD

@ahinzmann
Copy link
Contributor Author

We don't want this change in 10_2.
We only plan to use it for Run3 and possibly UL.
Possibly a back port to 10_6 is desired (but not now) once we actually finalize and change the tune of PUPPI possibly a re-MiniAOD of UL.

@slava77
Copy link
Contributor

slava77 commented Oct 30, 2019

Regarding a backport,
my understanding of this PR is that it's purpose is to simplify the software use by avoiding using a "complicated" PuppiPhoton algorithm.
As such, a backport should not be needed, unless it becomes a part of a future development which has to be backported. (perhaps in line with the re-mini #27929 (comment))

@peruzzim
Copy link
Contributor

My understanding, after chatting with a few people:

  • this change does not affect NanoAOD, where (at the moment) we do not rerun PUPPI in any release;
  • also DeepTau scores in NanoAOD are, therefore, not affected;
  • it would affect MiniAOD output in production if backported, because there is a technical change in the algorithm;
  • there is a planned further update of the algorithm (the PUPPI tune) that is
    expected to come maybe in time for the final re-miniAOD of the UL. This also connects, I think, with the switch to PUPPI jets for Run-3, for which we will have to discuss what to leave as "legacy" in the Run-2 UL datasets.

On the basis of this, I would propose that:

  • this gets merged in master, as an algorithm update (that has the positive effect of also simplifying the maintenance of this module);
  • we treat the PUPPI output as the result of a new algorithm, and specifically:
  • as it seems impractical to backport this under the protection of the run2_miniAOD_devel modifier, we consider the backport to 10_6_X only when the PUPPI tune also gets updated, and will be switched "by release version" for UL re-miniAOD. Please note this in features for UL re-miniAOD in 10_6_X #27889!

@peruzzim
Copy link
Contributor

I would also recommend, for clarity, to change the title of this PR to make it clear that it's an update of the algorithm and not just a technical simplification of the code.

@ahinzmann ahinzmann changed the title Simplify PUPPI Photon producer Simplify PUPPI Photon producer and change PUPPI isolation input Oct 30, 2019
@ahinzmann ahinzmann changed the title Simplify PUPPI Photon producer and change PUPPI isolation input Simplify PUPPI Photon producer and change PUPPI isolation algo Oct 30, 2019
@santocch
Copy link

+1

@cmsbuild
Copy link
Contributor

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

@fabiocos
Copy link
Contributor

fabiocos commented Nov 1, 2019

+1

@fabiocos
Copy link
Contributor

fabiocos commented Nov 1, 2019

the title now seems clear enough to explain an update in the algorithm

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

9 participants