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

Update to MVA MET: Fullfills "consumes" call requirement, Bugfix for the calculation of the covariance matrix, updated path to weight files #13086

Merged
merged 6 commits into from
Feb 9, 2016

Conversation

rfriese
Copy link
Contributor

@rfriese rfriese commented Jan 27, 2016

Covariance matrix change: Was a bug not to use power of 2. See these plots done with the fix:
metpfpullx
metpfpully
Without the fix the pulls are just way off.

All other things do not change the performance at all but are necessary to make the plugin work due to missing consumes calls before.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @rfriese (Raphael Friese) for CMSSW_7_6_X.

It involves the following packages:

RecoMET/METPUSubtraction

@cmsbuild, @cvuosalo, @davidlange6, @slava77 can you please review it and eventually sign? Thanks.
@TaiSakuma, @ahinzmann, @mmarionncern, @jdolen, @nhanvtran, @schoef, @mariadalfonso this is something you requested to watch as well.
@slava77, @Degano, @smuzaffar you are the release manager for this.

cms-bot commands are list here #13028

@@ -16,12 +16,13 @@ PFMETProducerMVA::PFMETProducerMVA(const edm::ParameterSet& cfg)
srcPFCandidatesView_ = consumes<reco::CandidateView>(cfg.getParameter<edm::InputTag>("srcPFCandidates"));
srcVertices_ = consumes<reco::VertexCollection>(cfg.getParameter<edm::InputTag>("srcVertices"));
vInputTag srcLeptonsTags = cfg.getParameter<vInputTag>("srcLeptons");
edm::InputTag srcRho = ( cfg.exists("srcRho") ) ? cfg.getParameter<edm::InputTag>("srcRho") : (edm::InputTag("fixedGridRhoFastjetAll"));
Copy link
Contributor

Choose a reason for hiding this comment

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

ParameterSet::exists can be used only in exceptional cases.
Please define a fillDescriptions method if you need a default value
https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideConfigurationValidationAndHelp

@slava77
Copy link
Contributor

slava77 commented Jan 27, 2016

@rfriese
please edit the PR description (the first comment block #13086 (comment), which is empty now) and provide some details on changes in performance (plots or a link to a presentation in a POG meeting)

@slava77
Copy link
Contributor

slava77 commented Jan 27, 2016

@cmsbuild please test

@cmsbuild
Copy link
Contributor

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

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

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

src = cms.InputTag('ak4PFJets'),
correctors = cms.vstring("ak4PFL1FastL2L3") # NOTE: use "ak5PFL1FastL2L3" for MC / "ak5PFL1FastL2L3Residual" for Data
correctors = cms.VInputTag("ak4PFL1FastL2L3Corrector") # NOTE: use "ak5PFL1FastL2L3" for MC / "ak5PFL1FastL2L3Residual" for Data
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the comment should be updated. What are the correct tags for MC and data?

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 1, 2016

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

@cvuosalo
Copy link
Contributor

cvuosalo commented Feb 1, 2016

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 1, 2016

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

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 1, 2016

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 1, 2016

@cvuosalo
Copy link
Contributor

cvuosalo commented Feb 1, 2016

+1

For #13086 151e776

Bug fix for MVA MET covariance matrix calculation. #13088 is the 80X version of this PR.

The code changes are satisfactory, and Jenkins tests against baseline CMSSW_7_6_X_2016-01-31-2300 show no significant differences.

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 1, 2016

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_6_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @Degano, @smuzaffar

@davidlange6
Copy link
Contributor

+1

cmsbuild added a commit that referenced this pull request Feb 9, 2016
Update to MVA MET: Fullfills "consumes" call requirement, Bugfix for the calculation of the covariance matrix, updated path to weight files
@cmsbuild cmsbuild merged commit 603847d into cms-sw:CMSSW_7_6_X Feb 9, 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.

5 participants