Fix hot HCAL channels in legacy 2016 #22713
Conversation
|
A new Pull Request was created by @deguio (Federico De Guio) for CMSSW_9_4_X. It involves the following packages: CommonTools/ParticleFlow @perrotta, @monttj, @cmsbuild, @slava77, @gpetruc, @arizzi can you please review it and eventually sign? Thanks. cms-bot commands are listed here
|
| #include "DataFormats/Common/interface/Association.h" | ||
| #include <iostream> | ||
|
|
||
| class PFCandidateRecalibrator : public edm::EDProducer { |
There was a problem hiding this comment.
(new) legacy modules are not allowed.
Please use stream:: or global::
| process.load("RecoVertex.AdaptiveVertexFinder.inclusiveVertexing_cff") | ||
| task.add(process.inclusiveVertexingTask) | ||
| task.add(process.inclusiveCandidateVertexingTask) | ||
| task.add(process.inclusiveCandidateVertexingCvsLTask) |
There was a problem hiding this comment.
this list looks kind of incomplete.
There was a problem hiding this comment.
for this I let @gpetruc comment
|
There should be a PR for the master branch first. complete integration of this will have to be done on top of CMSSW_9_4_MAOD_X branch. IIUC, this is needed for the re-miniAOD based on 2016 80X legacy outputs. |
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
@deguio |
|
hi @slava77 |
| edm::ESHandle<CaloGeometry> calogeom; | ||
| iSetup.get<CaloGeometryRecord>().get(calogeom); | ||
| const CaloGeometry* cgeo = calogeom.product(); | ||
| HcalGeometry* hgeom = (HcalGeometry*)(cgeo->getSubdetectorGeometry(DetId::Hcal,HcalForward)); |
There was a problem hiding this comment.
is it obvious that all of the conditions accessed above will never change during a run?
Also, if a job is processing a skim and there are multiple runs in the same IOV, it seems wasteful to recompute all values.
Better change to use ESWatchers
https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideHowToGetDataFromES#Seeing_that_a_Record_has_changed
| void PFCandidateRecalibrator::beginRun(const edm::Run &iRun, const edm::EventSetup &iSetup) | ||
| { | ||
| //Get Calib Constants from current GT | ||
| edm::ESHandle<HcalDbService> GTCond; |
There was a problem hiding this comment.
start local variables and data members with lower case letter. Capitalization is reserved for class names.
|
|
||
| void PFCandidateRecalibrator::beginRun(const edm::Run &iRun, const edm::EventSetup &iSetup) | ||
| { | ||
| //Get Calib Constants from current GT |
There was a problem hiding this comment.
I don't see a reset to badCh vectors.
A job processing multiple runs will have these vectors grow at best redundantly, but at worst it will introduce dependence on the order of processed events if two runs have different sets of bad channels
There was a problem hiding this comment.
this is done in the last commit
|
please test |
|
The tests are being triggered in jenkins. |
|
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
+1 |
|
+1 |
1 similar comment
|
+1 |
|
@slava77 the update didn't concern directly reco code, I assume you confirm your signature |
|
thanks @fabiocos |
|
+1 the reconstruction part was unchanged since the signature 4 days ago, just the GT has been updated. |
|
merge |
this PR is supposed to provide a recipe to fix the hot channels problem affecting HE and HF in the 2016 legacy rereco. the issue is described at:
https://baylor.app.box.com/s/n0yrtd0p2grzngv33jx0qtdae0tvwu8e
the hot cells are being cured at PFCandidate level and can be applied at the miniAODv2 step by adding
--customise_unsch PhysicsTools/PatAlgos/slimming/customizeMiniAOD_HcalFixLegacy2016.custo\ mizeAllwhile the HE hot cells are cured, some tuning is still needed in HF.
@gpetruc