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

Enabling re-application of Type1 Corrections for PuppiMET and adding PUPPI MET Uncertainties #30922

Merged
merged 11 commits into from
Sep 16, 2020

Conversation

saghosh
Copy link
Contributor

@saghosh saghosh commented Jul 27, 2020

PR description:

Reapplication of JECs for Type-1 corrections for PuppiMET is not performed to include the latest JECs during NanoAOD production.
https://github.com/cms-sw/cmssw/blob/master/PhysicsTools/NanoAOD/python/nano_cff.py#L198

This is particularly relevant for 2018D in which the MiniAOD is PROMPT RECO.
It is not possible to re-apply JECs on the fly since the AK4PuppiJet collection is not stored in NanoAOD.

Also uncertainties for PuppiMET, particularly UnclusteredEnergy Uncertainties and the JER, JES down uncertainties are missing.
Only part of the uncertainties were added previously : https://github.com/cms-nanoAOD/cmssw/pull/516/files

This issue is already raised in the NanoAOD git area :
cms-nanoAOD#523
And also CMSSW github issue:
#30304

PR validation:

For verification of code, please see the slides linked below
https://indico.cern.ch/event/924615/contributions/3885484/attachments/2049952/3435787/Pre_EPR_MET_June3.pdf

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…missing uncertainties
@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@saghosh
Copy link
Contributor Author

saghosh commented Jul 27, 2020

Sorry, PR description was not included for some reason. Please find it here:

Issues that this PR intends to fix are :

Reapplication of JECs for Type-1 corrections for PuppiMET is not performed to include the latest JECs during NanoAOD production.
https://github.com/cms-sw/cmssw/blob/master/PhysicsTools/NanoAOD/python/nano_cff.py#L198

This is particularly relevant for 2018D in which the MiniAOD is PROMPT RECO.
It is not possible to re-apply JECs on the fly since the AK4PuppiJet collection is not stored in NanoAOD.

Also uncertainties for PuppiMET, particularly UnclusteredEnergy Uncertainties and the JER, JES down uncertainties are missing.
Only part of the uncertainties were added previously : https://github.com/cms-nanoAOD/cmssw/pull/516/files

This issue is already raised in the NanoAOD git area :
cms-nanoAOD#523
and in CMSSW github:
#30304

-- Verification of code changes :
For more details about the issue and resolution and the verification of it, please see the slides linked below
https://indico.cern.ch/event/924615/contributions/3885484/attachments/2049952/3435787/Pre_EPR_MET_June3.pdf

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30922/17318

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @saghosh (Saranya Ghosh) for master.

It involves the following packages:

PhysicsTools/NanoAOD

@gouskos, @cmsbuild, @fgolf, @mariadalfonso, @santocch, @peruzzim can you please review it and eventually sign? Thanks.
@gpetruc this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@@ -266,7 +266,7 @@ def nanoAOD_runMETfixEE2017(process,isData):
process.nanoSequenceCommon.insert(process.nanoSequenceCommon.index(jetSequence),process.fullPatMetSequenceFixEE2017)

def nanoAOD_customizeCommon(process):
# makePuppiesFromMiniAOD(process,True) # call this here as it calls switchOnVIDPhotonIdProducer
makePuppiesFromMiniAOD(process,True) # call this here as it calls switchOnVIDPhotonIdProducer
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be put beyond a process modifier since V14 Puppi is available already in the re-miniAOD

for master:
if run2_nanoAOD_106Xv1 (+ any other old):
makePuppiesFromMiniAOD(process,True)

for 10_6:
if NOT run2_miniAOD_devel:
makePuppiesFromMiniAOD(process,True)

Copy link
Contributor

Choose a reason for hiding this comment

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

You need to decide which tune you want for
run2_miniAOD_80XLegacy, run2_nanoAOD_94X2016, run2_nanoAOD_94XMiniAODv1, run2_nanoAOD_94XMiniAODv2, run2_nanoAOD_102Xv1, run2_nanoAOD_106Xv1

This need to be in compatible with the JEC analyst have in the GT, unless you plan to reserve the previous.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hello,
We want the old PUPPI tune for all the mentioned ones, except for run2_nanoAOD_106Xv1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello, I am concerned about the code crashing if the makePuppiesFromMiniAOD(process,True) is not called at all under one of the conditions. It seems to be required to obtain the puppiMETSequence
and is recommended to be included here:
https://twiki.cern.ch/twiki/bin/view/CMS/MissingETUncertaintyPrescription#Puppi_MET
even if the PUPPI tune is not to be updated.

I tried testing out the code without that the call to makePuppiesFromMiniAOD(process,True), and the code crashes with the message
'Process' object has no attribute 'puppiMETSequence'

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes so makePuppiesFromMiniAOD(process,True) is apparently needed.
Turning on/off the new PUPPI tune is done via:
process.puppiNoLep.useExistingWeights = False
process.puppi.useExistingWeights = False
This is where the modifiers should be placed from what I understand.

Copy link
Contributor

@lathomas lathomas Aug 11, 2020

Choose a reason for hiding this comment

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

After some discussion with Maria, we believe the two following lines:
process.puppiNoLep.useExistingWeights = True
process.puppi.useExistingWeights = True
should be put in the nanoAOD_customizeCommon function
Then an era modifier should be used to set this to false when needed (only for run2_nanoAOD_106Xv1 in 106X from what I understand).

Copy link
Contributor

@mariadalfonso mariadalfonso Aug 11, 2020

Choose a reason for hiding this comment

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

yes correct

I suggest to put useExistingWeights = False for EOY and UL106 basically for all these
run2_nanoAOD_94XMiniAODv1,
run2_nanoAOD_94XMiniAODv2, run2_nanoAOD_94X2016, run2_nanoAOD_102Xv1,
run2_nanoAOD_106Xv1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do I understand correctly that for the version to be merged with the master, no era modifier is required?
Then I'll just move those two lines to the nanoAOD_customizeCommon function for the version to be merged with the master, and add the era modifier later for the 106X version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I will make the changes and update

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello, updated with the latest commit, please check

@@ -195,10 +195,10 @@ def nanoAOD_recalibrateMETs(process,isData):
table.variables.muonSubtrFactor = Var("1-userFloat('muonSubtrRawPt')/(pt()*jecFactor('Uncorrected'))",float,doc="1-(muon-subtracted raw pt)/(raw pt)",precision=6)
process.metTables += process.corrT1METJetTable
# makePuppiesFromMiniAOD(process,True) # call this before in the global customizer otherwise it would reset photon IDs in VID
Copy link
Contributor

@mariadalfonso mariadalfonso Jul 29, 2020

Choose a reason for hiding this comment

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

can we clean up this call ? same lines
process.puppiNoLep.useExistingWeights = False
process.puppi.useExistingWeights = False

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 have now modified this part to explicitly set as True the useExistingWeights as per the previous comment thread

@mariadalfonso
Copy link
Contributor

@saghosh @ahinzmann @lathomas
please let me know how we proceed with this PR

@saghosh
Copy link
Contributor Author

saghosh commented Aug 4, 2020

hi @mariadalfonso , was busy with ICHEP last week, will get back to this ASAP

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@saghosh
Copy link
Contributor Author

saghosh commented Aug 11, 2020

@mariadalfonso @lathomas additional commit addresses the points mentioned in the comments above

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30922/17696

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 9, 2020

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

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

  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-c251bc/1325.61_TTbar_13_106Xv1NanoAODINPUT+TTbar_13_106Xv1NanoAODINPUT+NANOAODMC2017_106XMiniAODv1
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-c251bc/1325.81_TTbar_13_106Xv1NanoAODINPUT+TTbar_13_106Xv1NanoAODINPUT+NANOEDMMC2017_106XMiniAODv1+HARVESTNANOAODMC2017_106XMiniAODv1
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-c251bc/136.8522_RunJetHT2018A_nanoUL+RunJetHT2018A_nanoUL+NANOEDM2018_106Xv1+HARVESTNANOAOD2018_106Xv1

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 18 differences found in the comparisons
  • DQMHistoTests: Total files compared: 35
  • DQMHistoTests: Total histograms compared: 2609667
  • DQMHistoTests: Total failures: 10
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2609635
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 1.008 KiB( 34 files compared)
  • DQMHistoSizes: changed ( 1325.7 ): 1.008 KiB Physics/NanoAODDQM
  • Checked 149 log files, 22 edm output root files, 35 DQM output files

@santocch
Copy link

+1

@silviodonato
Copy link
Contributor

@cms-sw/xpog-l2 @ahinzmann @lathomas Do you have further comments?

@ahinzmann
Copy link
Contributor

ahinzmann commented Sep 16, 2020

I'd suggest to leave this PR as is and deal with re-clustering slimmedAK8Jets(Puppi v15) for the modifier run2_nanoAOD_106Xv1 in a separate PR as it is not trivial and decoupled from the bug addressed here. In this way, we are done with the bug fix part and can deal with the (more difficult) NanoAOD V8 part in a followup PR.

@mariadalfonso
Copy link
Contributor

+xpog

For new UL (reMini and run2_nanoAOD_106Xv1 ) type1 MET fixed with:

  • new v15 puppi tune used for the rawPuppiMET
  • type1MET correction done on the fly with ak4 puppi jets (still with v15) and with the appropriate jets. [Not possible to do a posteriori since the ak4 puppi jets are not stored in nano].

puppiMET for EOY not touched, only type1 corrections are reapplied.

to be followed in a new PR the handling of AK8puppi for the run2_nanoAOD_106Xv1:

  • now use the V11 puppi tune
  • JEC AK8 puppi not yet available

@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. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

@silviodonato
Copy link
Contributor

+1

@mariadalfonso
Copy link
Contributor

@saghosh
consider backporting this PR to 10_6

@saghosh
Copy link
Contributor Author

saghosh commented Sep 17, 2020

@saghosh
consider backporting this PR to 10_6

@mariadalfonso will do so.

# process.puppiNoLep.useExistingWeights = False
# process.puppi.useExistingWeights = False
# process.nanoSequenceCommon.insert(process.nanoSequenceCommon.index(jetSequence),cms.Sequence(process.puppiMETSequence+process.fullPatMetSequencePuppi))
runMetCorAndUncFromMiniAOD(process,isData=isData,metType="Puppi",postfix="Puppi",jetFlavor="AK4PFPuppi")
Copy link
Contributor

Choose a reason for hiding this comment

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

For the followup PR to switch to PUPPI v15 for run2_nanoAOD_106Xv1 consistently for MET, AK4 and AK8, please considered adding the switches "recoMetFromPFCs=True, reclusterJets=True" to runMetCorAndUncFromMiniAOD for run2_nanoAOD_106Xv1 (to recompute MET and Type-1 with v15). And test if it has the desired effect.
@mariadalfonso @alefisico

@mariadalfonso
Copy link
Contributor

@saghosh
consider backporting this PR to 10_6

@mariadalfonso will do so.

any news on this backport ?

@lathomas
Copy link
Contributor

@saghosh any news? We would need this for the upcoming JME NANOAOD production Thanks !

@saghosh
Copy link
Contributor Author

saghosh commented Oct 1, 2020

@mariadalfonso @lathomas Please see : #31638

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