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

Enable pt-dependent JER-SF and phi-dependent JEC application in PAT #28096

Merged
merged 2 commits into from Oct 9, 2019

Conversation

ahinzmann
Copy link
Contributor

PR description:

Enable pt-dependent jet resolution scale factors in PAT. This is needed to apply Autumn18_V7 JER-SF, released last week.
Enable phi-dependent jet energy scale in PAT. This will be needed for 2018 JEC to deal with the HEM issue.
This PR has no impact with the current global tags in any release. However, it will enable using a new global tag (currently in the makings) with pt-dependent JER-SFs and later with phi-dependent JECs.

A test config to apply resolution smearing from a db file has been added under /test.
Here is an example db-file containing pt-dependent JER-SFs:
https://github.com/cms-jet/JRDatabase/blob/master/SQLiteFiles/Autumn18_V7_MC.db

PR validation:

Test config runs after inclusion of this PR. It was checked that this does not change the memory consumption, compared to using the old code with Autumn18_V1_MC.db (no pt-dependence).

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 1, 2019

The code-checks are being triggered in jenkins.

@ahinzmann ahinzmann changed the title pt-JER and phi-JEC Enable pt-dependent JER and phi-dependent JEC application in PAT Oct 1, 2019
@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 1, 2019

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28096/12089

  • This PR adds an extra 28KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 1, 2019

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

It involves the following packages:

JetMETCorrections/Modules
PhysicsTools/PatAlgos
PhysicsTools/PatUtils

@perrotta, @cmsbuild, @santocch, @slava77 can you please review it and eventually sign? Thanks.
@emilbols, @gouskos, @rappoccio, @HeinerTholen, @peruzzim, @seemasharmafnal, @mmarionncern, @smoortga, @acaudron, @jdolen, @ferencek, @jdamgov, @nhanvtran, @gkasieczka, @schoef, @andrzejnovak, @clelange, @riga, @JyothsnaKomaragiri, @mverzett, @gpetruc, @mariadalfonso, @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 Oct 1, 2019

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 1, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/2734/console Started: 2019/10/01 15:06

@ahinzmann ahinzmann changed the title Enable pt-dependent JER and phi-dependent JEC application in PAT Enable pt-dependent JER-SF and phi-dependent JEC application in PAT Oct 1, 2019
@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 1, 2019

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 1, 2019

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 1, 2019

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2958092
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2957750
  • 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

# Input source
process.source = cms.Source("PoolSource",
fileNames = cms.untracked.vstring(
'file:////afs/cern.ch/user/h/hinzmann/workspace/tmp/102D73A7-5B87-E611-936E-0CC47A1E0472.root',
Copy link
Contributor

Choose a reason for hiding this comment

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

private files from AFS should not be used in the released source code.
Please change to something available centrally.

label = cms.untracked.string('AK4PFchs')
),
),
connect = cms.string('sqlite:Autumn18_V7_MC.db')
Copy link
Contributor

Choose a reason for hiding this comment

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

is this an extremely temporary solution?

Shouldn't the payloads be uploaded already to the DB, or is the expectation that this mechanism will always be done with a local db file?

Is this test file really needed in the release?
The SmearedPATJetProducer is apparently used in the standard workflows. So, to enable the new corrections one would ideally just need to run the standard PAT with a candidate GT.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main reason to provide this config was, to be able to test this before the global tag is available. It will come in a couple of days.
The SmearedPATJetProducer is only used for MET uncertainties in standard workflows, thus not the most direct test of it.
If the file is nevertheless not desired I can remove it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that a commented out variant provided as an example is OK

@peruzzim
Copy link
Contributor

peruzzim commented Oct 8, 2019

Can we please proceed more quickly with the preparation of this PR? It's needed for NanoAODv6 (and in general for analysis). @ahinzmann have you implemented the requested changes?

@ahinzmann
Copy link
Contributor Author

yes, all changes are in

@slava77
Copy link
Contributor

slava77 commented Oct 8, 2019

+1

for #28096 89b308a

  • code changes are in line with the PR description and the follow up review
  • jenkins tests pass and comparisons with the baseline show no differences in the PR tests by itself (the new features in JECs are not exercised)

@santocch
Copy link

santocch commented Oct 9, 2019

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 9, 2019

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)

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 9, 2019

The tests are being triggered in jenkins.
Tested with other pull request(s) #28122,#28118
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/2848/console Started: 2019/10/09 14:09

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 9, 2019

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 9, 2019

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 9, 2019

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

Comparison Summary:

  • No significant changes to the logs found
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 15 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2961053
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2960710
  • DQMHistoTests: Total skipped: 341
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 1.285 KiB( 33 files compared)
  • DQMHistoSizes: changed ( 1325.7 ): 1.285 KiB Physics/NanoAODDQM
  • Checked 147 log files, 16 edm output root files, 34 DQM output files

@fabiocos
Copy link
Contributor

fabiocos commented Oct 9, 2019

+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

6 participants