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

Puppi MET and MET significance fixes in the MET tool (80X) #15600

Merged
merged 17 commits into from
Sep 7, 2016

Conversation

mmarionncern
Copy link

Same as #15565 for 80X

I will be out of office from August 27th to September 17th. If comments comes during that period, please contact @mariadalfonso and @zdemirag .

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

DataFormats/METReco
DataFormats/PatCandidates
PhysicsTools/PatAlgos
PhysicsTools/PatUtils
RecoMET/METProducers

@cmsbuild, @cvuosalo, @slava77, @montjj, @davidlange6 can you please review it and eventually sign? Thanks.
@rappoccio, @imarches, @ahinzmann, @acaudron, @gpetruc, @TaiSakuma, @jdolen, @nhanvtran, @JyothsnaKomaragiri, @schoef, @ferencek, @mverzett, @mariadalfonso, @pvmulder, @cbernet 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 Aug 25, 2016

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 25, 2016

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

@cmsbuild
Copy link
Contributor

-1

Tested at: d2bccaf

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-15600/14731/summary.html

I found follow errors while testing this PR

Failed tests: RelVals

  • RelVals:

When I ran the RelVals I found an error in the following worklfows:
1306.0 step3

runTheMatrix-results/1306.0_SingleMuPt1_UP15+SingleMuPt1_UP15+DIGIUP15+RECOUP15+HARVESTUP15/step3_SingleMuPt1_UP15+SingleMuPt1_UP15+DIGIUP15+RECOUP15+HARVESTUP15.log
135.4 step3
runTheMatrix-results/135.4_ZEE_13+ZEEFS_13+HARVESTUP15FS+MINIAODMCUP15FS/step3_ZEE_13+ZEEFS_13+HARVESTUP15FS+MINIAODMCUP15FS.log
136.731 step3
runTheMatrix-results/136.731_RunSinglePh2016B+RunSinglePh2016B+HLTDR2_2016+RECODR2_2016reHLT+HARVESTDR2/step3_RunSinglePh2016B+RunSinglePh2016B+HLTDR2_2016+RECODR2_2016reHLT+HARVESTDR2.log
1330.0 step3
runTheMatrix-results/1330.0_ZMM_13+ZMM_13+DIGIUP15+RECOUP15+HARVESTUP15/step3_ZMM_13+ZMM_13+DIGIUP15+RECOUP15+HARVESTUP15.log
50202.0 step3
runTheMatrix-results/50202.0_TTbar_13+TTbar_13+DIGIUP15_PU50+RECOUP15_PU50+HARVESTUP15_PU50/step3_TTbar_13+TTbar_13+DIGIUP15_PU50+RECOUP15_PU50+HARVESTUP15_PU50.log
25202.0 step3
runTheMatrix-results/25202.0_TTbar_13+TTbar_13+DIGIUP15_PU25+RECOUP15_PU25+HARVESTUP15_PU25/step3_TTbar_13+TTbar_13+DIGIUP15_PU25+RECOUP15_PU25+HARVESTUP15_PU25.log

@slava77
Copy link
Contributor

slava77 commented Aug 25, 2016

the errors are

No data of type "JME::JetResolutionObject" with label "AK4PFPuppi_pt" in record "JetResolutionRcd"

it looks like the 80X GTs do not have the needed payloads at all; in 81X version of the PR only run2 50ns GT doesn't have them.
@mmusich @franzoni I suppose a request for updates are on the way, please post here when GT has the changes.

@mmusich
Copy link
Contributor

mmusich commented Aug 25, 2016

A bit unfortunate not having the update planned ahead the PR

@cmsbuild
Copy link
Contributor

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

@slava77
Copy link
Contributor

slava77 commented Sep 1, 2016

@mmusich @mariadalfonso
what is the status of GT in 80X regarding needs for this PR ?
At least operationally, the 81X version has now settled.

@slava77
Copy link
Contributor

slava77 commented Sep 1, 2016

backport of #15565

@mmusich
Copy link
Contributor

mmusich commented Sep 1, 2016

@slava77 there is no update relevant yet in 80x available.
It's in the pipeline with lower priority with respect to fixing #15434.
Clarify what will be the usage of this PR, for instance if there is plan to use this in the upcoming MC production and/or data reprocessing

@slava77
Copy link
Contributor

slava77 commented Sep 1, 2016

On 9/1/16 5:27 AM, Marco Musich wrote:

@slava77 https://github.com/slava77 there is no update relevant yet in
80x available.
It's in the pipeline with lower priority with respect to fixing #15434
#15434.
Clarify what will be the usage of this PR, for instance if there is plan
to use this in the upcoming MC production and/or data reprocessing

This PR ca go into 80X only with the upcoming data rereco /MC remake.

Maria should clarify how essential it is for rereco.

If it's essential, then GT update is holding this PR and, transitively,
rereco as well.


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

@mmusich
Copy link
Contributor

mmusich commented Sep 1, 2016

@slava77 I am not saying the needed update won't be perfomed at all, just to understand how fast you need it (if the code changes are OK in the 81x version, I would assume the GT update is needed mostly to pass the tests).

@slava77
Copy link
Contributor

slava77 commented Sep 1, 2016

@mmusich
OK if it's coming soon (this week?).
It would help to have fewer balls in the air (PRs waiting for some "external" factors to settle).

@mmusich
Copy link
Contributor

mmusich commented Sep 1, 2016

Hi @slava77 the issue is that the GT queues are already ahead of the current 80X HEAD (because of #15434 - not merged). It will require a bit of time and work to reproduce the state of the current HEAD and apply on top of that the missing JME conditions. Hopefully can do it by tomorrow.

@mariadalfonso
Copy link
Contributor

@mmusich @slava77 @cvuosalo
Hi Slava we need this PR.
I will check for update from @mmusich tomorrow morning
This likely need to be rebased after the #15616� is merged (locally I need to resolve conflicts), I will check is in synch with the 81X #15565

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 5, 2016

Pull request #15600 was updated. @ghellwig, @cvuosalo, @cerminar, @monttj, @cmsbuild, @franzoni, @slava77, @mmusich, @davidlange6 can you please check and sign again.

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 5, 2016

Pull request #15600 was updated. @ghellwig, @cvuosalo, @cerminar, @monttj, @cmsbuild, @franzoni, @slava77, @mmusich, @davidlange6 can you please check and sign again.

@slava77
Copy link
Contributor

slava77 commented Sep 5, 2016

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 5, 2016

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

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 5, 2016

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 6, 2016

@slava77
Copy link
Contributor

slava77 commented Sep 6, 2016

+1

for #15600 eae5dbb

  • backport of MET features from 81X: this is a combination of Puppi MET and MET significance fixes in the MET tool #15565 and older features merged in 81X much earlier. Most non-trivial edits are in PhysicsTools/PatUtils/python/tools/runMETCorrectionsAndUncertainties.py: the version of this file from this PR becomes the same as 81X (after Puppi MET and MET significance fixes in the MET tool #15565)
  • jenkins tests pass and comparisons with baseline show differences consistent with 81X: in puppiMET (monitored in DQM).
  • local tests confirm behavior of jenkins tests and also show now filled MET significance. The technical performance changes are similar to 81X (roughly the same total CPU and small increase in miniAOD size).

@mmusich
Copy link
Contributor

mmusich commented Sep 6, 2016

+1

@slava77
Copy link
Contributor

slava77 commented Sep 6, 2016

hold

a formal hold to indicate that changes should go in to the rereco release, not sooner.

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 6, 2016

Pull request has been put on hold by @slava77
They need to issue an unhold command to remove the hold state or L1 can unhold it for all

@cmsbuild cmsbuild added the hold label Sep 6, 2016
@davidlange6 davidlange6 merged commit 929f031 into cms-sw:CMSSW_8_0_X Sep 7, 2016
@slava77
Copy link
Contributor

slava77 commented Sep 8, 2016

@davidlange6
just as head up: this PR broke data processing configs.
I'll investigate and see if I can come up with a quick fix.

@slava77
Copy link
Contributor

slava77 commented Sep 9, 2016

#15791 fixes the issue introduced by this PR in DataProcessing configs

Sam-Harper pushed a commit to Sam-Harper/cmssw that referenced this pull request Nov 25, 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