From 80bfa6ae152e1524c2e9ba17605d130951afb97e Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Fri, 24 Aug 2018 16:09:07 -0500 Subject: [PATCH 1/5] Do not use edm::Handle as member data It is best to avoid holding onto information from the Event. --- .../EcalDeadCellTriggerPrimitiveFilter.cc | 53 +++++++++++-------- 1 file changed, 30 insertions(+), 23 deletions(-) diff --git a/RecoMET/METFilters/plugins/EcalDeadCellTriggerPrimitiveFilter.cc b/RecoMET/METFilters/plugins/EcalDeadCellTriggerPrimitiveFilter.cc index 830c77443f50f..24e26dbf3cc0a 100644 --- a/RecoMET/METFilters/plugins/EcalDeadCellTriggerPrimitiveFilter.cc +++ b/RecoMET/METFilters/plugins/EcalDeadCellTriggerPrimitiveFilter.cc @@ -77,7 +77,7 @@ class EcalDeadCellTriggerPrimitiveFilter : public edm::stream::EDFilter<> { private: bool filter(edm::Event&, const edm::EventSetup&) override; void beginRun(const edm::Run&, const edm::EventSetup&) override; - virtual void envSet(const edm::EventSetup&); + void envSet(const edm::EventSetup&); // ----------member data --------------------------- @@ -92,15 +92,17 @@ class EcalDeadCellTriggerPrimitiveFilter : public edm::stream::EDFilter<> { edm::ESHandle ecalStatus; edm::ESHandle geometry; - void loadEcalDigis(edm::Event& iEvent, const edm::EventSetup& iSetup); - void loadEcalRecHits(edm::Event& iEvent, const edm::EventSetup& iSetup); + void loadEcalDigis(edm::Event& iEvent, + edm::Handle& pTPDigis +); + void loadEcalRecHits(edm::Event& iEvent, + edm::Handle& barrelReducedRecHitsHandle, + edm::Handle& endcapReducedRecHitsHandle); const edm::InputTag ebReducedRecHitCollection_; const edm::InputTag eeReducedRecHitCollection_; edm::EDGetTokenT ebReducedRecHitCollectionToken_; edm::EDGetTokenT eeReducedRecHitCollectionToken_; - edm::Handle barrelReducedRecHitsHandle; - edm::Handle endcapReducedRecHitsHandle; edm::ESHandle ttMap_; @@ -125,12 +127,11 @@ class EcalDeadCellTriggerPrimitiveFilter : public edm::stream::EDFilter<> { const edm::InputTag tpDigiCollection_; edm::EDGetTokenT tpDigiCollectionToken_; - edm::Handle pTPDigis; // chnStatus > 0, then exclusive, i.e., only consider status == chnStatus // chnStatus < 0, then inclusive, i.e., consider status >= abs(chnStatus) // Return value: + : positive zside - : negative zside - int setEvtTPstatus(const double &tpCntCut, const int &chnStatus); + int setEvtTPstatus(const EcalTrigPrimDigiCollection& , const double &tpCntCut, const int &chnStatus); const bool useTTsum_; //If set to true, the filter will compare the sum of the 5x5 tower to the provided energy threshold const bool usekTPSaturated_; //If set to true, the filter will check the kTPSaturated flag @@ -156,7 +157,9 @@ class EcalDeadCellTriggerPrimitiveFilter : public edm::stream::EDFilter<> { // To be used before a bug fix std::vector avoidDuplicateVec; - int setEvtRecHitstatus(const double &tpValCut, const int &chnStatus, const int &towerTest); + int setEvtRecHitstatus(const double &tpValCut, const int &chnStatus, const int &towerTest, + const EBRecHitCollection& HitecalEB, + const EERecHitCollection& HitecalEE); }; @@ -243,14 +246,17 @@ void EcalDeadCellTriggerPrimitiveFilter::loadEventInfoForFilter(const edm::Event } -void EcalDeadCellTriggerPrimitiveFilter::loadEcalDigis(edm::Event& iEvent, const edm::EventSetup& iSetup){ +void EcalDeadCellTriggerPrimitiveFilter::loadEcalDigis(edm::Event& iEvent, edm::Handle& pTPDigis){ iEvent.getByToken(tpDigiCollectionToken_, pTPDigis); if ( !pTPDigis.isValid() ) { edm::LogWarning("EcalDeadCellTriggerPrimitiveFilter") << "Can't get the product " << tpDigiCollection_.instance() << " with label " << tpDigiCollection_.label(); return; } } -void EcalDeadCellTriggerPrimitiveFilter::loadEcalRecHits(edm::Event& iEvent, const edm::EventSetup& iSetup){ +void EcalDeadCellTriggerPrimitiveFilter::loadEcalRecHits(edm::Event& iEvent, + edm::Handle& barrelReducedRecHitsHandle, + edm::Handle& endcapReducedRecHitsHandle) +{ iEvent.getByToken(ebReducedRecHitCollectionToken_,barrelReducedRecHitsHandle); iEvent.getByToken(eeReducedRecHitCollectionToken_,endcapReducedRecHitsHandle); @@ -290,13 +296,16 @@ bool EcalDeadCellTriggerPrimitiveFilter::filter(edm::Event& iEvent, const edm::E int evtTagged = 0; if( useTPmethod_ ){ - loadEcalDigis(iEvent, iSetup); - evtTagged = setEvtTPstatus(etValToBeFlagged_, 13); + edm::Handle pTPDigis; + loadEcalDigis(iEvent, pTPDigis); + evtTagged = setEvtTPstatus(*pTPDigis, etValToBeFlagged_, 13); } if( useHITmethod_ ){ - loadEcalRecHits(iEvent, iSetup); - evtTagged = setEvtRecHitstatus(etValToBeFlagged_, 13, 13); + edm::Handle barrelReducedRecHitsHandle; + edm::Handle endcapReducedRecHitsHandle; + loadEcalRecHits(iEvent, barrelReducedRecHitsHandle, endcapReducedRecHitsHandle); + evtTagged = setEvtRecHitstatus(etValToBeFlagged_, 13, 13, *barrelReducedRecHitsHandle, *endcapReducedRecHitsHandle); } if( evtTagged ){ pass = false; } @@ -322,7 +331,10 @@ void EcalDeadCellTriggerPrimitiveFilter::beginRun(const edm::Run &iRun, const ed return ; } -int EcalDeadCellTriggerPrimitiveFilter::setEvtRecHitstatus(const double &tpValCut, const int &chnStatus, const int &towerTest){ +int EcalDeadCellTriggerPrimitiveFilter::setEvtRecHitstatus(const double &tpValCut, const int &chnStatus, const int &towerTest, + const EBRecHitCollection& HitecalEB, + const EERecHitCollection& HitecalEE + ){ if( debug_ && verbose_ >=2) edm::LogInfo("EcalDeadCellTriggerPrimitiveFilter")<<"***begin setEvtTPstatusRecHits***"; @@ -334,9 +346,6 @@ int EcalDeadCellTriggerPrimitiveFilter::setEvtRecHitstatus(const double &tpValCu const EBRecHitCollection HitecalEB = *(barrelRecHitsHandle.product()); const EERecHitCollection HitecalEE = *(endcapRecHitsHandle.product()); */ - const EBRecHitCollection HitecalEB = *(barrelReducedRecHitsHandle.product()); - const EERecHitCollection HitecalEE = *(endcapReducedRecHitsHandle.product()); - int isPassCut =0; EBRecHitCollection::const_iterator ebrechit; @@ -524,7 +533,7 @@ int EcalDeadCellTriggerPrimitiveFilter::setEvtRecHitstatus(const double &tpValCu } -int EcalDeadCellTriggerPrimitiveFilter::setEvtTPstatus(const double &tpValCut, const int &chnStatus) { +int EcalDeadCellTriggerPrimitiveFilter::setEvtTPstatus(EcalTrigPrimDigiCollection const& tpDigis, const double &tpValCut, const int &chnStatus) { if( debug_ && verbose_ >=2) edm::LogInfo("EcalDeadCellTriggerPrimitiveFilter")<<"***begin setEvtTPstatus***"; @@ -551,10 +560,8 @@ int EcalDeadCellTriggerPrimitiveFilter::setEvtTPstatus(const double &tpValCut, c EcalTrigTowerDetId ttDetId = ttItor->second; int ttzside = ttDetId.zside(); - const EcalTrigPrimDigiCollection * tpDigis = nullptr; - tpDigis = pTPDigis.product(); - EcalTrigPrimDigiCollection::const_iterator tp = tpDigis->find( ttDetId ); - if( tp != tpDigis->end() ){ + EcalTrigPrimDigiCollection::const_iterator tp = tpDigis.find( ttDetId ); + if( tp != tpDigis.end() ){ double tpEt = ecalScale_.getTPGInGeV( tp->compressedEt(), tp->id() ); if(tpEt >= tpValCut ){ isPassCut = 1; isPassCut *= ttzside; } } From 115460b56ffea8034a87f64ba94a2a78430efd2e Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Fri, 24 Aug 2018 17:12:39 -0500 Subject: [PATCH 2/5] Remove inappropriate use of provenance Removed use of getAllStableProvenance. The consumes calls were already forcing the prefetching of the different data products. Switched to using callWhenNewProductsRegistered and only call 'consumes' if the alternate data product is actually available and the prefered product has not been seen. --- .../EcalDeadCellTriggerPrimitiveFilter.cc | 25 ++++++++----------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/RecoMET/METFilters/plugins/EcalDeadCellTriggerPrimitiveFilter.cc b/RecoMET/METFilters/plugins/EcalDeadCellTriggerPrimitiveFilter.cc index 24e26dbf3cc0a..817c7634fe4ec 100644 --- a/RecoMET/METFilters/plugins/EcalDeadCellTriggerPrimitiveFilter.cc +++ b/RecoMET/METFilters/plugins/EcalDeadCellTriggerPrimitiveFilter.cc @@ -173,8 +173,6 @@ EcalDeadCellTriggerPrimitiveFilter::EcalDeadCellTriggerPrimitiveFilter(const edm , doEEfilter_ (iConfig.getUntrackedParameter("doEEfilter") ) , ebReducedRecHitCollection_ (iConfig.getParameter("ebReducedRecHitCollection") ) , eeReducedRecHitCollection_ (iConfig.getParameter("eeReducedRecHitCollection") ) - , ebReducedRecHitCollectionToken_ (consumes(ebReducedRecHitCollection_)) - , eeReducedRecHitCollectionToken_ (consumes(eeReducedRecHitCollection_)) , maskedEcalChannelStatusThreshold_ (iConfig.getParameter("maskedEcalChannelStatusThreshold") ) , etValToBeFlagged_ (iConfig.getParameter("etValToBeFlagged") ) , tpDigiCollection_ (iConfig.getParameter("tpDigiCollection") ) @@ -187,6 +185,17 @@ EcalDeadCellTriggerPrimitiveFilter::EcalDeadCellTriggerPrimitiveFilter(const edm useTPmethod_ = true; useHITmethod_ = false; produces(); + + callWhenNewProductsRegistered([this](edm::BranchDescription const& iBranch) { + if( iBranch.moduleLabel() == tpDigiCollection_.label() ){ hastpDigiCollection_ = 1; } + if( iBranch.moduleLabel() == ebReducedRecHitCollection_.label() || iBranch.moduleLabel() == eeReducedRecHitCollection_.label() ){ + hasReducedRecHits_++; + if(hasReducedRecHits_ == 2 and hastpDigiCollection_ == 0) { + ebReducedRecHitCollectionToken_ = consumes(ebReducedRecHitCollection_); + eeReducedRecHitCollectionToken_ = consumes(eeReducedRecHitCollection_); + } + } + }); } EcalDeadCellTriggerPrimitiveFilter::~EcalDeadCellTriggerPrimitiveFilter() { @@ -194,18 +203,6 @@ EcalDeadCellTriggerPrimitiveFilter::~EcalDeadCellTriggerPrimitiveFilter() { void EcalDeadCellTriggerPrimitiveFilter::loadEventInfoForFilter(const edm::Event &iEvent){ - std::vector provenances; - iEvent.getAllStableProvenance(provenances); - const unsigned int nProvenance = provenances.size(); - for (unsigned int ip = 0; ip < nProvenance; ip++) { - const edm::StableProvenance& provenance = *( provenances[ip] ); - if( provenance.moduleLabel() == tpDigiCollection_.label() ){ hastpDigiCollection_ = 1; } - if( provenance.moduleLabel() == ebReducedRecHitCollection_.label() || provenance.moduleLabel() == eeReducedRecHitCollection_.label() ){ - hasReducedRecHits_++; - } - if( hastpDigiCollection_ && hasReducedRecHits_>=2 ){ break; } - } - if( debug_ ) edm::LogInfo("EcalDeadCellTriggerPrimitiveFilter")<<"\nhastpDigiCollection_ : "< Date: Fri, 24 Aug 2018 17:19:02 -0500 Subject: [PATCH 3/5] Use edm::Event::emplace --- .../plugins/EcalDeadCellTriggerPrimitiveFilter.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/RecoMET/METFilters/plugins/EcalDeadCellTriggerPrimitiveFilter.cc b/RecoMET/METFilters/plugins/EcalDeadCellTriggerPrimitiveFilter.cc index 817c7634fe4ec..51387f2fffa53 100644 --- a/RecoMET/METFilters/plugins/EcalDeadCellTriggerPrimitiveFilter.cc +++ b/RecoMET/METFilters/plugins/EcalDeadCellTriggerPrimitiveFilter.cc @@ -143,6 +143,8 @@ class EcalDeadCellTriggerPrimitiveFilter : public edm::stream::EDFilter<> { bool useTPmethod_, useHITmethod_; + edm::EDPutTokenT putToken_; + void loadEventInfoForFilter(const edm::Event& iEvent); // Only for EB since the dead front-end has one-to-one map to TT @@ -179,12 +181,12 @@ EcalDeadCellTriggerPrimitiveFilter::EcalDeadCellTriggerPrimitiveFilter(const edm , tpDigiCollectionToken_(consumes(tpDigiCollection_)) , useTTsum_ (iConfig.getParameter("useTTsum") ) , usekTPSaturated_ (iConfig.getParameter("usekTPSaturated") ) + , putToken_ ( produces() ) { getEventInfoForFilterOnce_ = false; hastpDigiCollection_ = 0; hasReducedRecHits_ = 0; useTPmethod_ = true; useHITmethod_ = false; - produces(); callWhenNewProductsRegistered([this](edm::BranchDescription const& iBranch) { if( iBranch.moduleLabel() == tpDigiCollection_.label() ){ hastpDigiCollection_ = 1; } @@ -312,7 +314,7 @@ bool EcalDeadCellTriggerPrimitiveFilter::filter(edm::Event& iEvent, const edm::E printf("\nrun : %8u event : %10llu lumi : %4u evtTPstatus ABS : %d 13 : % 2d\n", run, event, ls, evtstatusABS, evtTagged); } - iEvent.put(std::make_unique(pass)); + iEvent.emplace(putToken_, pass); if (taggingMode_) return true; else return pass; From 862698da529d871c892cfa185331e57d1b156e1a Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Wed, 29 Aug 2018 10:12:35 -0500 Subject: [PATCH 4/5] Made code nicer to look at --- .../METFilters/plugins/EcalDeadCellTriggerPrimitiveFilter.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/RecoMET/METFilters/plugins/EcalDeadCellTriggerPrimitiveFilter.cc b/RecoMET/METFilters/plugins/EcalDeadCellTriggerPrimitiveFilter.cc index 51387f2fffa53..faae962596dad 100644 --- a/RecoMET/METFilters/plugins/EcalDeadCellTriggerPrimitiveFilter.cc +++ b/RecoMET/METFilters/plugins/EcalDeadCellTriggerPrimitiveFilter.cc @@ -93,8 +93,7 @@ class EcalDeadCellTriggerPrimitiveFilter : public edm::stream::EDFilter<> { edm::ESHandle geometry; void loadEcalDigis(edm::Event& iEvent, - edm::Handle& pTPDigis -); + edm::Handle& pTPDigis); void loadEcalRecHits(edm::Event& iEvent, edm::Handle& barrelReducedRecHitsHandle, edm::Handle& endcapReducedRecHitsHandle); From 53fbe206ea8702712be46f226cbb03efccbb43fa Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Wed, 29 Aug 2018 10:52:40 -0500 Subject: [PATCH 5/5] Removed check on CMSSW version used to produce data being read --- .../EcalDeadCellTriggerPrimitiveFilter.cc | 77 ++++++------------- 1 file changed, 25 insertions(+), 52 deletions(-) diff --git a/RecoMET/METFilters/plugins/EcalDeadCellTriggerPrimitiveFilter.cc b/RecoMET/METFilters/plugins/EcalDeadCellTriggerPrimitiveFilter.cc index faae962596dad..c22c0a253228b 100644 --- a/RecoMET/METFilters/plugins/EcalDeadCellTriggerPrimitiveFilter.cc +++ b/RecoMET/METFilters/plugins/EcalDeadCellTriggerPrimitiveFilter.cc @@ -75,6 +75,7 @@ class EcalDeadCellTriggerPrimitiveFilter : public edm::stream::EDFilter<> { ~EcalDeadCellTriggerPrimitiveFilter() override; private: + void beginStream(edm::StreamID) override; bool filter(edm::Event&, const edm::EventSetup&) override; void beginRun(const edm::Run&, const edm::EventSetup&) override; void envSet(const edm::EventSetup&); @@ -135,17 +136,13 @@ class EcalDeadCellTriggerPrimitiveFilter : public edm::stream::EDFilter<> { const bool useTTsum_; //If set to true, the filter will compare the sum of the 5x5 tower to the provided energy threshold const bool usekTPSaturated_; //If set to true, the filter will check the kTPSaturated flag - bool getEventInfoForFilterOnce_; + int hasReducedRecHits_ = 0; - std::string releaseVersion_; - int hastpDigiCollection_, hasReducedRecHits_; - - bool useTPmethod_, useHITmethod_; + bool useTPmethod_ = false; + bool useHITmethod_ = false; edm::EDPutTokenT putToken_; - void loadEventInfoForFilter(const edm::Event& iEvent); - // Only for EB since the dead front-end has one-to-one map to TT std::map accuTTetMap; std::map accuTTchnMap; @@ -182,16 +179,21 @@ EcalDeadCellTriggerPrimitiveFilter::EcalDeadCellTriggerPrimitiveFilter(const edm , usekTPSaturated_ (iConfig.getParameter("usekTPSaturated") ) , putToken_ ( produces() ) { - getEventInfoForFilterOnce_ = false; - hastpDigiCollection_ = 0; hasReducedRecHits_ = 0; - useTPmethod_ = true; useHITmethod_ = false; - - callWhenNewProductsRegistered([this](edm::BranchDescription const& iBranch) { - if( iBranch.moduleLabel() == tpDigiCollection_.label() ){ hastpDigiCollection_ = 1; } + // If TP is available, always use TP. + // In RECO file, we always have ecalTPSkim (at least from 38X for data and 39X for MC). + // In AOD file, we can only have recovered rechits in the reduced rechits collection after 42X + // Do NOT expect end-users provide ecalTPSkim or recovered rechits themselves!! + // If they really can provide them, they must be experts to modify this code to suit their own purpose :-) + if( iBranch.moduleLabel() == tpDigiCollection_.label() ){ + useTPmethod_ = true; + //if both collections are in the job then we may already have seen the reduced collections + useHITmethod_ = false; + } if( iBranch.moduleLabel() == ebReducedRecHitCollection_.label() || iBranch.moduleLabel() == eeReducedRecHitCollection_.label() ){ hasReducedRecHits_++; - if(hasReducedRecHits_ == 2 and hastpDigiCollection_ == 0) { + if(not useTPmethod_ && hasReducedRecHits_ == 2) { + useHITmethod_ = true; ebReducedRecHitCollectionToken_ = consumes(ebReducedRecHitCollection_); eeReducedRecHitCollectionToken_ = consumes(eeReducedRecHitCollection_); } @@ -202,46 +204,19 @@ EcalDeadCellTriggerPrimitiveFilter::EcalDeadCellTriggerPrimitiveFilter(const edm EcalDeadCellTriggerPrimitiveFilter::~EcalDeadCellTriggerPrimitiveFilter() { } -void EcalDeadCellTriggerPrimitiveFilter::loadEventInfoForFilter(const edm::Event &iEvent){ - - if( debug_ ) edm::LogInfo("EcalDeadCellTriggerPrimitiveFilter")<<"\nhastpDigiCollection_ : "<At(1)->GetName()).Atoi(); - int minorV = TString(split->At(2)->GetName()).Atoi(); - - if( debug_ ) edm::LogInfo("EcalDeadCellTriggerPrimitiveFilter")<<"processName : "<=4 && minorV >=2 ){ useTPmethod_ = false; useHITmethod_ = true; } - else if( majorV >=5 || (majorV==4 && minorV >=2) ){ useTPmethod_ = false; useHITmethod_ = true; } - else{ useTPmethod_ = false; useHITmethod_ = false; - if( debug_ ){ - edm::LogInfo("EcalDeadCellTriggerPrimitiveFilter")<<"\nWARNING ... TP filter can ONLY be used in AOD after 42X"; - edm::LogInfo("EcalDeadCellTriggerPrimitiveFilter")<<" Will NOT DO ANY FILTERING !"; - } +void EcalDeadCellTriggerPrimitiveFilter::beginStream(edm::StreamID){ + + if( debug_ ) edm::LogInfo("EcalDeadCellTriggerPrimitiveFilter")<<"\nuseTPmethod_ : "<& pTPDigis){ @@ -287,8 +262,6 @@ bool EcalDeadCellTriggerPrimitiveFilter::filter(edm::Event& iEvent, const edm::E edm::EventNumber_t event = iEvent.id().event(); edm::LuminosityBlockNumber_t ls = iEvent.luminosityBlock(); - if( !getEventInfoForFilterOnce_ ){ loadEventInfoForFilter(iEvent); } - bool pass = true; int evtTagged = 0;