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

TagInfo collection name update #13700

Merged

Conversation

ferencek
Copy link
Contributor

PAT jets store TagInfo names by stripping everything starting from 'TagInfos'. Hence, internally it is not possible to distinguish pfImpactParameterTagInfosAK8 from pfImpactParameterTagInfos. So to avoid possible ambiguities, TagInfo collection names need to end with 'TagInfos'.

This PR does not change MiniAOD in 76X and it also does not have any impact on reading older 76X MiniAOD files.

Backport of a single commit from #13685.

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

PhysicsTools/PatAlgos
RecoBTag/ImpactParameter
RecoBTag/SecondaryVertex

@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

@cvuosalo
Copy link
Contributor

@cmsbuild please test

@cmsbuild
Copy link
Contributor

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

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@cvuosalo
Copy link
Contributor

+1

For #13700 056c7db

Renaming TagInfos collections to avoid confusion.

The code changes are satisfactory, and Jenkins tests against baseline CMSSW_7_6_X_2016-03-08-2300 show no significant differences. An extended test of workflow 25202.0_TTbar_13 with 70 events against baseline CMSSW_7_6_4 also shows no significant differences, no change in RECO or Mini-AOD event content, and no significant change in CPU time or memory usage. The renaming's only effect is that two modules run with new names but take the same amount of CPU time:

  delta/mean delta/orJob     original                   new       module name
  ---------- ------------     --------                  ----       ------------
     removed      -0.00%         0.34 ms/ev ->         0.00 ms/ev pfInclusiveSecondaryVertexFinderTagInfosAK8AK8
       added      +0.00%         0.00 ms/ev ->         0.33 ms/ev pfInclusiveSecondaryVertexFinderAK8TagInfosAK8
     removed      -0.00%         2.10 ms/ev ->         0.00 ms/ev pfImpactParameterTagInfosAK8AK8
       added      +0.00%         0.00 ms/ev ->         2.11 ms/ev pfImpactParameterAK8TagInfosAK8

@slava77
Copy link
Contributor

slava77 commented Mar 16, 2016

       added      +0.00%         0.00 ms/ev ->         0.33 ms/ev pfInclusiveSecondaryVertexFinderAK8TagInfosAK8

This is strange. I thought the goal was to have "TagInfos" at the end of the name. Did the addJetCollection postfix add it anyways
@ferencek please clarify

@ferencek
Copy link
Contributor Author

@slava77, yes, addJetCollection adds a user-defined label (in the above case AK8) at the end of the module name. However, what matters for adding TagInfos to PAT jets is the string up to "TagInfos" (see https://github.com/cms-sw/cmssw/blob/CMSSW_7_6_4/DataFormats/PatCandidates/src/Jet.cc#L406-L416). So to be able to distinguish pfImpactParameterTagInfosAK8 from pfImpactParameterTagInfos among the added TagInfos, it is necessary to move AK8 in front of TagInfos. Additional AK8, which could be any other string, at the end of the name in the above example is not relevant and is dropped from the internally stored TagInfo name.

@davidlange6 davidlange6 merged commit 10f92d6 into cms-sw:CMSSW_7_6_X Apr 12, 2016
@ferencek ferencek deleted the TagInfoNameUpdate_from-CMSSW_7_6_4 branch April 12, 2016 16:18
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

5 participants