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

Fixing the calculation of MET Uncertainties by using PFMuons, PFElectrons, PFTaus, and PFPhotons #26302

Merged

Conversation

eioannou
Copy link
Contributor

This PR is submitted in order to fix a bug in the calculation of MET Uncertainties. MET Uncertainties must be calculated by using PF Electrons, PF Photons, PF Taus, and PF Muons only.

The additional snippet of the code finds PF objects and stores them into the collections and then, those collections are used for the calculations of MET uncertainties

This PR is related to the issue #25751

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26302/8996

  • This PR adds an extra 32KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @eioannou (Emilios Ioannou) for master.

It involves the following packages:

PhysicsTools/PatUtils

@cmsbuild, @perrotta, @santocch, @slava77 can you please review it and eventually sign? Thanks.
@TaiSakuma, @gouskos, @rappoccio, @HeinerTholen, @seemasharmafnal, @mmarionncern, @ahinzmann, @smoortga, @acaudron, @jdolen, @drkovalskyi, @ferencek, @jdamgov, @nhanvtran, @gkasieczka, @schoef, @clelange, @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

@perrotta
Copy link
Contributor

please test
(thank you @eioannou )

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 31, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/33865/console Started: 2019/03/31 18:03

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

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

Comparison Summary:

  • No significant changes to the logs found
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 605 differences found in the comparisons
  • DQMHistoTests: Total files compared: 32
  • DQMHistoTests: Total histograms compared: 3134059
  • DQMHistoTests: Total failures: 115
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3133747
  • DQMHistoTests: Total skipped: 197
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 31 files compared)
  • Checked 133 log files, 14 edm output root files, 32 DQM output files

@perrotta
Copy link
Contributor

perrotta commented Apr 3, 2019

@Sam-Harper , FYI

@perrotta
Copy link
Contributor

perrotta commented Apr 9, 2019

@eioannou

Here you re-define pfElectrons, pfPhotons, pfMuons, pfTaus, and then you use them to calculate the MET uncertainties.

Isn't it quite error prone doing so? What happen if any of those objects gets re-defined for the MET calculation sometime in the future? Still you will end up with having decoupled the calculation of the MET and the one of its its error, as it was the origin of the issue pointed out in #25751.

The original python configurations are quite convoluted and I can hardly reconnect all pieces... However, I would say that the real fix would be to reuse the very same objects that entered the calculation of the MET for the calculation of the MET uncertainties, which means import'ing for the errors the same configuration which are used (elsewhere) for the MET. Is there any technical hindrance in doing so?

@eioannou
Copy link
Contributor Author

eioannou commented Apr 9, 2019

If I change the name of the collections (pfElectrons, pfPhotons, pfMuons and pfTaus) which will be used to the met uncertainties calculation, will it be okay?
If I give a different name of collections which include only the PF objects, I will not have any problem in the future, right?

@perrotta
Copy link
Contributor

perrotta commented Apr 9, 2019

If I change the name of the collections (pfElectrons, pfPhotons, pfMuons and pfTaus) which will be used to the met uncertainties calculation, will it be okay?
If I give a different name of collections which include only the PF objects, I will not have any problem in the future, right?

Sorry, my point was kind of the opposite: why cannot you import the very same collections which were used (elsewhere, I guess) to compute the MET? As such, you wouldn't need to modify them here by hand if some of those collections will get re-defined for the MET calculation (e.g. different thresholds, etc.).
By the way, could you point me to the config which steer that MET calculation, instead of its error?

@ahinzmann
Copy link
Contributor

I guess the issue here is that PF candidates and packed candidates have slightly different interfaces for the cuts, so one needs to somewhere define a selection for packed candidates:
https://github.com/cms-sw/cmssw/blob/02d4198c0b6615287fd88e9a8ff650aea994412e/CommonTools/ParticleFlow/python/pfElectrons_cff.py

@eioannou
Copy link
Contributor Author

eioannou commented Apr 9, 2019

If I am correct, the MET is computed in this config file

import FWCore.ParameterSet.Config as cms
##____________________________________________________________________________||
pfMet = cms.EDProducer(
"PFMETProducer",
src = cms.InputTag("particleFlow"),
alias = cms.string('pfMet'),
globalThreshold = cms.double(0.0),
calculateSignificance = cms.bool(False),
)
##____________________________________________________________________________||

So, the collection which is used to compute the MET is the "particleFlow". However, if I try to compute the MET uncertainties by using only the particleFlow, I guess, I have to modify the whole code which calculates the met uncertainties in the MET tool. So, an idea was, before the calculation of the met uncertainties, to create new collections of PF Objects only.

The current code uses all objects (Muons, Electrons, Taus, Photons) to compute the met uncertainties which it is wrong. So, my idea was to create collections which include only PF objects before the calculation of met uncertainties.

@perrotta
Copy link
Contributor

@eioannou this PR modifies in a significant way the errors on MET.
If this is the result of a bug fix, as it should, the modified errors must replace the previous, bugged ones.
Given the significant differences, however, I would like the new errors get endorsed by JetMET POG: please post a presentation that you made (or will make) at the POG which can ceritfy that the new errors have to be preferred.

@eioannou
Copy link
Contributor Author

I will prepare a presentation and will include the link here when it is ready to be presented at MET meeting.

@perrotta
Copy link
Contributor

@eioannou
This PR is ready to get accepted, and enter the UL 10_6 release, so that the amended MET errors can replace the bugged ones.
Please just send us a link to that presentation (just one or a few slides of comparisons can be enough), so that a reference of the improved uncertainities will remain here: then I will sign it for reco.

@perrotta
Copy link
Contributor

perrotta commented May 2, 2019

ping @eioannou

@eioannou
Copy link
Contributor Author

eioannou commented May 2, 2019

Hi @perrotta, I am trying to prepare the presentation about this PR, by performing some validation tests, and I am going to present the results in the next JETMET meeting. Sorry for the delay.

@perrotta
Copy link
Contributor

perrotta commented May 2, 2019

type bugfix

@perrotta
Copy link
Contributor

perrotta commented May 6, 2019

@eioannou it would be nice to include this bugfix in 10_6. I don't think there will be any JetMET meeting by tomorrow, the deadline set for the the latest open pre of 10_6: if you have any validation ready, and if you are confident enough that it does correspond to what the MET uncertainties must be, please post it here.

@eioannou
Copy link
Contributor Author

eioannou commented May 7, 2019

@eioannou it would be nice to include this bugfix in 10_6. I don't think there will be any JetMET meeting by tomorrow, the deadline set for the the latest open pre of 10_6: if you have any validation ready, and if you are confident enough that it does correspond to what the MET uncertainties must be, please post it here.

I will try to post the plots here today.

@fabiocos
Copy link
Contributor

fabiocos commented May 7, 2019

@eioannou as @perrotta says this is a fix needed for UL, and we should try to converge asap on it

@eioannou
Copy link
Contributor Author

eioannou commented May 8, 2019

I have performed validation tests for MET uncertainties of Type1 MET. The results are shown below. The green lines correspond to MET Uncertainties which are calculated by using PF particles only and the red lines stand for the MET uncertainties which are calculated by using all particles (as it is done until now). The plots show that the changes are minor between of two calculations, however, the correct method to compute the MET uncertainties is to use only PF particles in the calculations.

Validation tests were performed by using a DY sample of 50K events (DYJetsToLL_M-50_TuneCP5_13TeV-amcatnloFXFX-pythia8).

DMET_ElectronEnDown
DMET_ElectronEnUp
DMET_MuonEnDown
DMET_MuonEnUp
DMET_PhotonEnDown
DMET_PhotonEnUp
DMET_TauEnDown
DMET_TauEnUp

@perrotta
Copy link
Contributor

perrotta commented May 9, 2019

+1

@fabiocos
Copy link
Contributor

fabiocos commented May 9, 2019

+1

@fabiocos
Copy link
Contributor

fabiocos commented May 9, 2019

merge

@cmsbuild cmsbuild merged commit 6e67838 into cms-sw:master May 9, 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 be automatically merged.

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