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

Added updateJetCollection() function to jetTools.py and related changes to the PATJetUpdater and the pat::Jet class (76X) #12890

Merged

Conversation

ferencek
Copy link
Contributor

@ferencek ferencek commented Jan 7, 2016

Partial backport of #12863 (updates to the PATJetSlimmer excluded since they would affect the MiniAOD event content by changing the internal content of slimmed jets).

Update (Jan. 14, 2016): Added backport of #12953

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 7, 2016

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

It involves the following packages:

DataFormats/PatCandidates
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.

Following commands in first line of a comment are recognized

  • +1|approve[d]|sign[ed]: L1/L2's to approve it
  • -1|reject[ed]: L1/L2's to reject it
  • assign <category>[,<category>[,...]]: L1/L2's to request signatures from other categories
  • unassign <category>[,<category>[,...]]: L1/L2's to remove signatures from other categories
  • hold: L1/all L2's/release manager to mark it as on hold
  • unhold: L1/user who put this PR on hold
  • merge: L1/release managers to merge this request
  • [@cmsbuild,] please test: L1/L2 and selected users to start jenkins tests
  • [@cmsbuild,] please test with cms-sw/cmsdist#<PR>: L1/L2 and selected users to start jenkins tests using externals from cmsdist

@slava77
Copy link
Contributor

slava77 commented Jan 7, 2016

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 7, 2016

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

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 8, 2016

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 8, 2016

@slava77
Copy link
Contributor

slava77 commented Jan 11, 2016

+1

for #12890 2e95cd6

@ferencek
Copy link
Contributor Author

No, it does not. This is an optional feature not enabled in the MiniAOD production.

@slava77
Copy link
Contributor

slava77 commented Jan 14, 2016

@cmsbuild please test

@cmsbuild
Copy link
Contributor

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

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Jan 15, 2016

+1

for #12890 5f36e28

  • compared to 2e95cd6, the last update includes "if svClustering .. setupSVClustering" calls for somewhat recent btaggers that are not running in regular workflows, the same calls as already present for regularly running similar taggers since a while (quick check points to late 2014). There is a separate PR with just the last commit in 80X.
  • jenkins tests pass and comparisons with the baseline show no differences

davidlange6 added a commit that referenced this pull request Jan 24, 2016
…_7_6_3

Added updateJetCollection() function to jetTools.py and related changes to the PATJetUpdater and the pat::Jet class (76X)
@davidlange6 davidlange6 merged commit 61e2189 into cms-sw:CMSSW_7_6_X Jan 24, 2016
cmsbuild added a commit that referenced this pull request Jan 24, 2016
@ferencek ferencek mentioned this pull request Jan 26, 2016
@ferencek ferencek deleted the UpdatedJetTools_from-CMSSW_7_6_3 branch March 9, 2016 21:36
@@ -3,13 +3,13 @@
from PhysicsTools.PatAlgos.recoLayer0.jetCorrections_cff import *
from PhysicsTools.PatAlgos.producersLayer1.jetUpdater_cfi import *

patJetCorrFactorsUpdated = patJetCorrFactors.clone(
updatedPatJetCorrFactors = patJetCorrFactors.clone(
Copy link
Contributor

Choose a reason for hiding this comment

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

why this name is changed? which doesn't seem to be necessary since the contents of this module is not changed.. This change of module name will require all physics analyses that need to re-applying the new 76x JEC using this module to do some unnecessary change of their analysis codes.
For example, for our H->ZZ->4l analysis framework, the following line need to be changed if we want to change from 763patch2 to 764:
https://github.com/CJLST/ZZAnalysis/blob/miniAOD_76X/AnalysisStep/test/MasterPy/ZZ4lAnalysis.py#L1007

but this doesn't seem to be necessary...

@ferencek
Copy link
Contributor Author

@hengne, have you seen https://twiki.cern.ch/twiki/bin/view/CMSPublic/WorkBookJetEnergyCorrections#CorrPatJets, in particular https://github.com/cms-sw/cmssw/blob/CMSSW_7_6_X/PhysicsTools/PatAlgos/test/patTuple_updateJets_fromMiniAOD_cfg.py Now you can use the updateJetCollection function to re-correct jets which takes care of everything for you.

@hengne
Copy link
Contributor

hengne commented Mar 18, 2016

@ferencek
ok, good, you also updated the recipe in the JEC twiki page for 764. So, as far as this is change is distributed to analyses, i think i am fine with it. but still, it doesn't seem to be a necessary change to me....

@ferencek
Copy link
Contributor Author

@hengne, I see your point. The main idea here was to simplify things for users and introduce the updateJetCollection function that is similar in spirit to the existing switchJetCollection and addJetCollection functions. So instead of sticking with the original recipe in the TWiki, which I find a bit too hacky for my taste, the idea was for users to switch to this newly introduced function which allows them to not only re-correct jets but to also re-run b-taggers without having to recluster jets.

@hengne
Copy link
Contributor

hengne commented Mar 18, 2016

@ferencek OK, i see. Maybe a HN announcement of this change would be useful? The 764 is just built a couple of days ago, I think more and more analyses will try 764 in the following days. They might meet the same crash as we seen in H4l. A HN announcement may save some debugging time for many analyses. Thanks!

@ferencek
Copy link
Contributor Author

@hengne, done, here it is https://hypernews.cern.ch/HyperNews/CMS/get/physTools/3457.html Thanks for the suggestion.

@hengne
Copy link
Contributor

hengne commented Mar 19, 2016

@ferencek Thanks!

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