Protect default PAT jet configuration (80X) #13605

ferencek · 2016-03-04T22:26:48Z

This PR addresses the problem reported in https://hypernews.cern.ch/HyperNews/CMS/get/edmFramework/3631.html by protecting the default PAT jet configuration from being modified inadvertently.

Update (Mar. 17, 2016): Rebased to b981663 on the CMSSW_8_0_X branch with

process.patJetsPuppi.userData.userInts.src = cms.VInputTag(cms.InputTag(""))

(introduced in #13641 but now redundant) removed from PhysicsTools/PatAlgos/python/slimming/miniAOD_tools.py.

cmsbuild · 2016-03-04T22:27:08Z

A new Pull Request was created by @ferencek (Dinko Ferenček) for CMSSW_8_0_X.

It involves the following packages:

PhysicsTools/PatAlgos

@cvuosalo, @monttj, @cmsbuild, @slava77, @vadler, @davidlange6 can you please review it and eventually sign? Thanks.
@TaiSakuma, @imarches, @ahinzmann, @acaudron, @mmarionncern, @rappoccio, @jdolen, @nhanvtran, @schoef, @ferencek, @gpetruc, @mariadalfonso, @pvmulder 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

slava77 · 2016-03-04T23:49:24Z

@cmsbuild please test
@ferencek please make a PR for 81X

cmsbuild · 2016-03-04T23:49:48Z

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

ferencek · 2016-03-05T00:00:55Z

@slava77, 81X PR submitted (#13606)

cmsbuild · 2016-03-05T03:46:55Z

+1
Tested at: ab0002c
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-13605/11724/summary.html

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
17700b7
02f36fb
fcb7c2e
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-13605/11724/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-13605/11724/git-merge-result

cmsbuild · 2016-03-05T04:42:42Z

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

cvuosalo · 2016-03-08T20:08:50Z

For workflow 134.911_RunSinglePh2015D, a comparison against baseline CMSSW_8_0_1 shows that the effect of this PR on the config of the PAT/RECO step is only four small changes to patJetsPuppi parameters:

addTagInfos = cms.bool(True),  --> addTagInfos = cms.bool(False),
userFloats = cms.PSet(
src = cms.VInputTag(cms.InputTag("")) -->  src = cms.VInputTag("")
),

and then these two parameters are set to null:

userFunctionLabels = cms.vstring('vtxMass', 'vtxNtracks', 'vtx3DVal', 'vtx3DSig', 'vtxPx', 'vtxPy', 'vtxPz', 'vtxPosX', 'vtxPosY', 'vtxPosZ'),
userFunctions = cms.vstring('?(tagInfoCandSecondaryVertex("pfSecondaryVertex").nVertices()>0)?(tagInfoCandSecondaryVertex("pfSecondaryVertex").secondaryVertex(0).p4.M):(0)', '?(tagInfoCandSecondaryVertex("pfSecondaryVertex").nVertices()>0)?(tagInfoCandSecondaryVertex("pfSecondaryVertex").secondaryVertex(0).numberOfSourceCandidatePtrs):(0)', '?(tagInfoCandSecondaryVertex("pfSecondaryVertex").nVertices()>0)?(tagInfoCandSecondaryVertex("pfSecondaryVertex").flightDistance(0).value):(0)', '?(tagInfoCandSecondaryVertex("pfSecondaryVertex").nVertices()>0)?(tagInfoCandSecondaryVertex("pfSecondaryVertex").flightDistance(0).significance):(0)', '?(tagInfoCandSecondaryVertex("pfSecondaryVertex").nVertices()>0)?(tagInfoCandSecondaryVertex("pfSecondaryVertex").secondaryVertex(0).p4.x):(0)', '?(tagInfoCandSecondaryVertex("pfSecondaryVertex").nVertices()>0)?(tagInfoCandSecondaryVertex("pfSecondaryVertex").secondaryVertex(0).p4.y):(0)', '?(tagInfoCandSecondaryVertex("pfSecondaryVertex").nVertices()>0)?(tagInfoCandSecondaryVertex("pfSecondaryVertex").secondaryVertex(0).p4.z):(0)', '?(tagInfoCandSecondaryVertex("pfSecondaryVertex").nVertices()>0)?(tagInfoCandSecondaryVertex("pfSecondaryVertex").secondaryVertex(0).vertex.x):(0)', '?(tagInfoCandSecondaryVertex("pfSecondaryVertex").nVertices()>0)?(tagInfoCandSecondaryVertex("pfSecondaryVertex").secondaryVertex(0).vertex.y):(0)', '?(tagInfoCandSecondaryVertex("pfSecondaryVertex").nVertices()>0)?(tagInfoCandSecondaryVertex("pfSecondaryVertex").secondaryVertex(0).vertex.z):(0)'),

becoming

userFunctionLabels = cms.vstring(),
userFunctions = cms.vstring(),

ferencek · 2016-03-08T20:34:09Z

Hi Carl,

Thanks for the summary. These changes are expected. Also, there should be
some changes in MiniAOD where puppi jets would no longer store extra info
effectively reset by this PR.

-Dinko

(Sent from my phone. Please excuse the brevity, spelling, and punctuation.)
On Mar 8, 2016 9:09 PM, "Carl Vuosalo" notifications@github.com wrote:

For workflow 134.911_RunSinglePh2015D, a comparison against baseline
CMSSW_8_0_1 shows that the effect of this PR on the config of the PAT/RECO
step is only four small changes to patJetsPuppi parameters:

addTagInfos = cms.bool(True), --> addTagInfos = cms.bool(False),
src = cms.VInputTag(cms.InputTag("")) --> src = cms.VInputTag("")

and then these two parameters are set to null:

userFunctionLabels = cms.vstring('vtxMass', 'vtxNtracks', 'vtx3DVal',
'vtx3DSig', 'vtxPx',
'vtxPy', 'vtxPz', 'vtxPosX', 'vtxPosY', 'vtxPosZ'),
userFunctions =
cms.vstring('?(tagInfoCandSecondaryVertex("pfSecondaryVertex").nVertices()>0)?(tagInfoCandSecondaryVertex("pfSecondaryVertex").secondaryVertex(0).p4.M):(0)',

'?(tagInfoCandSecondaryVertex("pfSecondaryVertex").nVertices()>0)?(tagInfoCandSecondaryVertex("pfSecondaryVertex").secondaryVertex(0).numberOfSourceCandidatePtrs):(0)',

'?(tagInfoCandSecondaryVertex("pfSecondaryVertex").nVertices()>0)?(tagInfoCandSecondaryVertex("pfSecondaryVertex").flightDistance(0).value):(0)',

'?(tagInfoCandSecondaryVertex("pfSecondaryVertex").nVertices()>0)?(tagInfoCandSecondaryVertex("pfSecondaryVertex").flightDistance(0).significance):(0)',

'?(tagInfoCandSecondaryVertex("pfSecondaryVertex").nVertices()>0)?(tagInfoCandSecondaryVertex("pfSecondaryVertex").secondaryVertex(0).p4.x):(0)',

'?(tagInfoCandSecondaryVertex("pfSecondaryVertex").nVertices()>0)?(tagInfoCandSecondaryVertex("pfSecondaryVertex").secondaryVertex(0).p4.y):(0)',

'?(tagInfoCandSecondaryVertex("pfSecondaryVertex").nVertices()>0)?(tagInfoCandSecondaryVertex("pfSecondaryVertex").secondaryVertex(0).p4.z):(0)',

'?(tagInfoCandSecondaryVertex("pfSecondaryVertex").nVertices()>0)?(tagInfoCandSecondaryVertex("pfSecondaryVertex").secondaryVertex(0).vertex.x):(0)',

'?(tagInfoCandSecondaryVertex("pfSecondaryVertex").nVertices()>0)?(tagInfoCandSecondaryVertex("pfSecondaryVertex").secondaryVertex(0).vertex.y):(0)',

'?(tagInfoCandSecondaryVertex("pfSecondaryVertex").nVertices()>0)?(tagInfoCandSecondaryVertex("pfSecondaryVertex").secondaryVertex(0).vertex.z):(0)'),

becoming

userFunctionLabels = cms.vstring(),
userFunctions = cms.vstring(),

—
Reply to this email directly or view it on GitHub
#13605 (comment).

cvuosalo · 2016-03-08T20:40:01Z

@ahinzmann, @schoef: Could you please check the results of this PR (see above)? We see that this PR causes patJetsPuppi to have its addTagInfos parameter set to False even though its tagInfoSources parameter is still set with several values, which appears to be an inconsistency. What are the correct parameter values for patJetsPuppi?

ferencek · 2016-03-08T23:42:46Z

@cvuosalo, regarding addTagInfos, this is the standard PAT behaviour. Whenever b-tag discriminators are added to jets, corresponding TagInfos are added to the list of tagInfoSources but addTagInfos remains set to False since TagInfos are a bit heavy to store. What is enabled by default is the storage of b-tag discriminator through setting of addBTagInfo and addDiscriminators to True.

slava77 · 2016-03-09T01:12:49Z

@ferencek
it sounds like the tagInfoSources are added indiscriminately.
it would be better to not set the tagInfoSources if they are not needed.

ferencek · 2016-03-09T08:11:14Z

Hi Slava,

This goes beyond the scope of this PR.

While I get your point, this is how PAT, more specifically jetTools.py,
used to behave since its inception. The point is that jetTools.py adds all
the TagInfos associated with the added b-tag discriminators and it is then
left to the user to decide whether to actually add the TagInfos or not.
Figuring out later what TagInfos need to be added to have a consistent set
of TagInfos and discriminators would be a bit complicated. Note that the
fact that addTagInfos is not one of the input arguments to the
switchJetCollection and addJetCollection functions is not an exception. All
of the userData is also left to the user to be configured outside these
functions.

Best,
Dinko

(Sent from my phone. Please excuse the brevity, spelling, and punctuation.)
On Mar 9, 2016 2:13 AM, "Slava Krutelyov" notifications@github.com wrote:

@ferencek https://github.com/ferencek
it sounds like the tagInfoSources are added indiscriminately.
it would be better to not set the tagInfoSources if they are not needed.

—
Reply to this email directly or view it on GitHub
#13605 (comment).

cvuosalo · 2016-03-09T19:25:26Z

+1

For #13605 ab0002c

Protecting PAT jet configuration from undesired modification.

The code change is satisfactory, and Jenkins tests against baseline CMSSW_8_0_X_2016-03-04-1100 show no significant differences, as expected, except for the differences caused by the additional PRs Jenkins included in the tests. A check of the RECO config shows the expected changes due to this PR, as discussed above.

cvuosalo · 2016-03-09T20:01:47Z

Addendum: An extended test of workflow 134.911_RunSinglePh2015D with 70 events against baseline CMSSW_8_0_1 shows no significant differences.

…vertently

cmsbuild · 2016-03-17T10:14:48Z

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

davidlange6 · 2016-03-17T11:07:23Z

please test

cmsbuild · 2016-03-17T11:07:42Z

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

…SSW_8_0_1 Protect default PAT jet configuration (80X)

cmsbuild · 2016-03-17T14:39:25Z

+1
Tested at: bdd7e4e
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-13605/11941/summary.html

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
b981663
67db1aa
5437ff7
7315223
e750bd8
5fdb535
1425e8e
d933b1d
1bf6d30
1c5e04e
6c4b6cd
5f85d4a
5b42169
ddf0521
bceed6c
d26718c
e140519
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-13605/11941/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-13605/11941/git-merge-result

cmsbuild · 2016-03-17T16:01:14Z

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

cmsbuild added this to the Next CMSSW_8_0_X milestone Mar 4, 2016

cmsbuild added reconstruction-pending analysis-pending pending-signatures tests-pending orp-pending comparison-pending labels Mar 4, 2016

ferencek mentioned this pull request Mar 4, 2016

MET PAT tool update + update of MET significance #13115

Merged

ferencek changed the title ~~Protect default PAT jet configurations~~ Protect default PAT jet configuration Mar 4, 2016

cmsbuild added tests-started and removed tests-pending labels Mar 4, 2016

ferencek changed the title ~~Protect default PAT jet configuration~~ Protect default PAT jet configuration (80X) Mar 4, 2016

cmsbuild added tests-approved and removed tests-started labels Mar 5, 2016

cmsbuild added comparison-available and removed comparison-pending labels Mar 5, 2016

This was referenced Mar 8, 2016

Add PU id working points to MiniAOD #13641

Merged

Add PU id working points to MiniAOD 8_1_X #13642

Merged

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Mar 9, 2016

cvuosalo mentioned this pull request Mar 9, 2016

Protect default PAT jet configuration (81X) #13606

Merged

ferencek mentioned this pull request Mar 11, 2016

PAT btag update (80X) #13686

Merged

ferencek added 2 commits March 17, 2016 11:13

protecting the default pat jet configuration from being modified inad…

c911ad5

…vertently

removing redundant lines

bdd7e4e

ferencek force-pushed the ProtectPATJetDefaults_from-CMSSW_8_0_1 branch from ab0002c to bdd7e4e Compare March 17, 2016 10:14

cmsbuild added reconstruction-pending tests-pending comparison-pending and removed comparison-available reconstruction-approved tests-approved labels Mar 17, 2016

cmsbuild added tests-started and removed tests-pending labels Mar 17, 2016

davidlange6 added a commit that referenced this pull request Mar 17, 2016

Merge pull request #13605 from ferencek/ProtectPATJetDefaults_from-CM…

bd2081e

…SSW_8_0_1 Protect default PAT jet configuration (80X)

davidlange6 merged commit bd2081e into cms-sw:CMSSW_8_0_X Mar 17, 2016

ferencek deleted the ProtectPATJetDefaults_from-CMSSW_8_0_1 branch March 17, 2016 11:13

cmsbuild added tests-approved and removed tests-started labels Mar 17, 2016

cmsbuild added comparison-available and removed comparison-pending labels Mar 17, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Protect default PAT jet configuration (80X) #13605

Protect default PAT jet configuration (80X) #13605

ferencek commented Mar 4, 2016

cmsbuild commented Mar 4, 2016

slava77 commented Mar 4, 2016

cmsbuild commented Mar 4, 2016

ferencek commented Mar 5, 2016

cmsbuild commented Mar 5, 2016

cmsbuild commented Mar 5, 2016

cvuosalo commented Mar 8, 2016

ferencek commented Mar 8, 2016

cvuosalo commented Mar 8, 2016

ferencek commented Mar 8, 2016

slava77 commented Mar 9, 2016

ferencek commented Mar 9, 2016

cvuosalo commented Mar 9, 2016

cvuosalo commented Mar 9, 2016

cmsbuild commented Mar 17, 2016

davidlange6 commented Mar 17, 2016

cmsbuild commented Mar 17, 2016

cmsbuild commented Mar 17, 2016

cmsbuild commented Mar 17, 2016

Protect default PAT jet configuration (80X) #13605

Protect default PAT jet configuration (80X) #13605

Conversation

ferencek commented Mar 4, 2016

cmsbuild commented Mar 4, 2016

slava77 commented Mar 4, 2016

cmsbuild commented Mar 4, 2016

ferencek commented Mar 5, 2016

cmsbuild commented Mar 5, 2016

cmsbuild commented Mar 5, 2016

cvuosalo commented Mar 8, 2016

ferencek commented Mar 8, 2016

cvuosalo commented Mar 8, 2016

ferencek commented Mar 8, 2016

slava77 commented Mar 9, 2016

ferencek commented Mar 9, 2016

cvuosalo commented Mar 9, 2016

cvuosalo commented Mar 9, 2016

cmsbuild commented Mar 17, 2016

davidlange6 commented Mar 17, 2016

cmsbuild commented Mar 17, 2016

cmsbuild commented Mar 17, 2016

cmsbuild commented Mar 17, 2016