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

Fix for Puppi MET and MET significance compatibility between AOD and miniAOD #16140

Merged
merged 12 commits into from Oct 31, 2016

Conversation

mmarionncern
Copy link

@mmarionncern mmarionncern commented Oct 7, 2016

This PR improves the coherence between what is done for Puppi MET and the MET significance calculation at the miniAOD production level and recomputation on top of miniAODs.

Packages touched :

  • PhysicsTools/PatAlgos
  • PhysicsTools/PatUtils
  • RecoMET/METAlgorithms
  • RecoMET/METProducers

Changes :

  • fix scheduled mode (do not affect unscheduled mode)
  • change the constructor of MET in the RecoMET extractor to copy properly the sumET from the slimmedMET collection when updating a miniAOD
  • MET significance computation at the AOD level updated to match the miniAOD level
  • Update of the test cfg files
  • fix the puppi JEC and the use of slimmedJetPuppi when recomputing the puppiMET from miniAOD
  • modify the MET signifiance code to match by reference the pfCandidates to the jets/lepton constituents + use of an extra recovery by delta"4Vector" matching when reference matching is failing

Expected changes in data file contents :

  • small changes in the mET signifiance when producing miniAOD
  • puppi MET values when rerunning on miniAOD
  • storage of the sumEt when recomputing MET on top of miniAOD

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 7, 2016

A new Pull Request was created by @mmarionncern for CMSSW_8_1_X.

It involves the following packages:

PhysicsTools/PatAlgos
PhysicsTools/PatUtils
RecoMET/METAlgorithms
RecoMET/METProducers

@cmsbuild, @cvuosalo, @slava77, @monttj, @davidlange6 can you please review it and eventually sign? Thanks.
@TaiSakuma, @gouskos, @imarches, @ahinzmann, @acaudron, @gpetruc, @rappoccio, @jdolen, @nhanvtran, @JyothsnaKomaragiri, @gkasieczka, @schoef, @ferencek, @mverzett, @mariadalfonso, @pvmulder this is something you requested to watch as well.
@slava77, @smuzaffar you are the release manager for this.

cms-bot commands are list here #13028

@slava77
Copy link
Contributor

slava77 commented Oct 7, 2016

@mmarionncern please change the title of the PR to describe what it does
"Mm fix schedule 230916" is not acceptable

@slava77
Copy link
Contributor

slava77 commented Oct 7, 2016

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 7, 2016

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

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 7, 2016

@mmarionncern mmarionncern changed the title Mm fix schedule 230916 Fix for Puppi MET and MET significance compatibility between AOD and miniAOD Oct 7, 2016
@mmarionncern
Copy link
Author

@slava77 sorry for the title, I did not noticed it, corrected

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 7, 2016

@cmsbuild
Copy link
Contributor

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

@mmarionncern
Copy link
Author

@slava77 the puppiMET is impacted when re-running the algorithm on miniAOD, I will clarify it in the description.

@mmarionncern
Copy link
Author

@slava77 MET significance should increase a bit when producing miniAODs

@cmsbuild
Copy link
Contributor

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

@slava77
Copy link
Contributor

slava77 commented Oct 18, 2016

On 10/18/16 3:05 AM, mmarionncern wrote:

@slava77 https://github.com/slava77 MET significance should increase a
bit when producing miniAODs

Is it because of statistical redefinition of significance in this PR?
If MET is the same, what are the components that lead to increase of
significance?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#16140 (comment), or
mute the thread
https://github.com/notifications/unsubscribe-auth/AEdcbpMd735jhrR7xZhyPI90tgHT1rjUks5q1JnWgaJpZM4KRBLm.

@cmsbuild
Copy link
Contributor

Pull request #16140 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/15800/console

@slava77
Copy link
Contributor

slava77 commented Oct 18, 2016

There is a visible increase in CPU time in MET modules with significance computation enabled:

relModule         relJob           baseline         ->      PR
   +0.789247      +0.10%         1.68 ms/ev ->         3.87 ms/ev patPFMetPuppi
   +0.053434      +0.02%         7.34 ms/ev ->         7.75 ms/ev patPFMet
   +0.647359      +0.08%         1.91 ms/ev ->         3.74 ms/ev patMETsPuppi
   +0.177489      +0.06%         6.58 ms/ev ->         7.87 ms/ev patMETs

this increase is small enough to be tolerated

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16140/15800/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

@slava77
Copy link
Contributor

slava77 commented Oct 18, 2016

+1

for #16140 5dabd0d

  • code changes are in line with the description. A large fraction of the code changes is at analysis level (running on miniAOD inputs)
  • jenkins tests pass and comparisons show slight changes only in MET significance
  • local tests with 1K events using PAT step with ZEE ( 25200) and TTbar (25202) confirm observed differences in jenkins tests with higher precision. There are small variations in significance compared to Fix for Puppi MET and MET significance compatibility between AOD and miniAOD #16140 (comment) (there were some small logic changes in the last commits):
    all_sign781-patvsorig-pat_zeepuwf25200p0c_patmets_slimmedmets__dqm_obj_significance
    all_sign781-patvsorig-pat_zeepuwf25200p0c_patmets_slimmedmetspuppi__dqm_obj_significance

@@ -49,7 +49,7 @@
computeMETSignificance = cms.bool(False),
# significance computation parameters, not used
# if the significance is not computed
srcJets = cms.InputTag("selectedPatJets"),
srcJets = cms.InputTag("cleanedPatJets"),
Copy link
Contributor

Choose a reason for hiding this comment

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

@mmarionncern

I am wondering what would be the motivation to switch to "cleanedPatJets" from "selectedPatJets"
and it dose not need to be the same jet collection as we save at the end?

Regards,
Taejeong

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @mmarionncern, @monttj - is this issue resolved?

Copy link
Author

Choose a reason for hiding this comment

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

@davidlange6 that change is validated for the MET group, waiting for @monttj red/green light on the explanation provided

@mmarionncern
Copy link
Author

@slava77 >> Is it because of statistical redefinition of significance in this PR? If MET is the same, what are the components that lead to increase of significance?

This comes from the way the event components used in the covariance matrix computation (lepton, jets, unclustered particles) are disentangled. Compared to the previous version, there is a recovery of some soft particles associated to the leptons that are now correctly taken out of the unclsutered component.

@monttj >> I am wondering what would be the motivation to switch to "cleanedPatJets" from "selectedPatJets"
and it dose not need to be the same jet collection as we save at the end?

The cleanPatJet collection is the collection used in all MET related computation (type1 correction, etc), as those jets are cleaned from the lepton overlaps and are consistent with the kinematic selection made for the type1MET jet selection. For consistency, one should use that collection for the MET significance computation.

@slava77
Copy link
Contributor

slava77 commented Oct 24, 2016

@monttj
are there remaining issues to address?
your signature is needed here.

@davidlange6
Copy link
Contributor

seems we should move forward on this one.

@davidlange6 davidlange6 merged commit a244399 into cms-sw:CMSSW_8_1_X Oct 31, 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

7 participants