From c3edf219c2507b069667eb04c4102213e4d0f7bf Mon Sep 17 00:00:00 2001 From: Robert Bainbridge Date: Thu, 14 Feb 2019 14:55:34 +0100 Subject: [PATCH 1/6] store Seed BDT outputs in ValueMaps keyed by GsfTrackRef instead of GsfElectronRef --- .../LowPtGsfElectronSeedValueMapsProducer.cc | 38 +++++++++---------- .../LowPtGsfElectronSeedValueMapsProducer.h | 5 +-- 2 files changed, 20 insertions(+), 23 deletions(-) diff --git a/RecoEgamma/EgammaElectronProducers/plugins/LowPtGsfElectronSeedValueMapsProducer.cc b/RecoEgamma/EgammaElectronProducers/plugins/LowPtGsfElectronSeedValueMapsProducer.cc index c688c0c4a171d..a38504e3e7e97 100644 --- a/RecoEgamma/EgammaElectronProducers/plugins/LowPtGsfElectronSeedValueMapsProducer.cc +++ b/RecoEgamma/EgammaElectronProducers/plugins/LowPtGsfElectronSeedValueMapsProducer.cc @@ -1,11 +1,10 @@ #include "DataFormats/Common/interface/Handle.h" -#include "DataFormats/EgammaCandidates/interface/GsfElectron.h" -#include "DataFormats/EgammaCandidates/interface/GsfElectronCore.h" #include "DataFormats/EgammaReco/interface/ElectronSeed.h" #include "DataFormats/EgammaReco/interface/ElectronSeedFwd.h" #include "DataFormats/GsfTrackReco/interface/GsfTrack.h" #include "DataFormats/GsfTrackReco/interface/GsfTrackExtra.h" #include "DataFormats/ParticleFlowReco/interface/PreId.h" +#include "FWCore/MessageLogger/interface/MessageLogger.h" #include "FWCore/ParameterSet/interface/ParameterSet.h" #include "FWCore/ParameterSet/interface/ParameterSetDescription.h" #include "FWCore/Utilities/interface/InputTag.h" @@ -14,7 +13,7 @@ //////////////////////////////////////////////////////////////////////////////// // LowPtGsfElectronSeedValueMapsProducer::LowPtGsfElectronSeedValueMapsProducer( const edm::ParameterSet& conf ) : - gsfElectrons_(consumes(conf.getParameter("electrons"))), + gsfTracks_(consumes(conf.getParameter("gsfTracks"))), preIdsValueMap_(consumes< edm::ValueMap >(conf.getParameter("preIdsValueMap"))), names_(conf.getParameter< std::vector >("ModelNames")) { @@ -29,33 +28,32 @@ LowPtGsfElectronSeedValueMapsProducer::~LowPtGsfElectronSeedValueMapsProducer() // void LowPtGsfElectronSeedValueMapsProducer::produce( edm::Event& event, const edm::EventSetup& setup ) { - // Retrieve GsfElectrons from Event - edm::Handle gsfElectrons; - event.getByToken(gsfElectrons_,gsfElectrons); - if ( !gsfElectrons.isValid() ) { edm::LogError("Problem with gsfElectrons handle"); } + // Retrieve GsfTracks from Event + edm::Handle gsfTracks; + event.getByToken(gsfTracks_,gsfTracks); + if ( !gsfTracks.isValid() ) { edm::LogError("Problem with gsfTracks handle"); } // Retrieve PreIds from Event edm::Handle< edm::ValueMap > preIdsValueMap; event.getByToken(preIdsValueMap_,preIdsValueMap); if ( !preIdsValueMap.isValid() ) { edm::LogError("Problem with preIdsValueMap handle"); } - // Iterate through Electrons, extract BDT output, and store result in ValueMap for each model + // Iterate through GsfTracks, extract BDT output, and store result in ValueMap for each model std::vector< std::vector > output; for ( unsigned int iname = 0; iname < names_.size(); ++iname ) { - output.push_back( std::vector(gsfElectrons->size(),-999.) ); + output.push_back( std::vector(gsfTracks->size(),-999.) ); } - for ( unsigned int iele = 0; iele < gsfElectrons->size(); iele++ ) { - reco::GsfElectronRef ele(gsfElectrons,iele); - if ( ele->core().isNonnull() && - ele->core()->gsfTrack().isNonnull() && - ele->core()->gsfTrack()->extra().isNonnull() && - ele->core()->gsfTrack()->extra()->seedRef().isNonnull() ) { - reco::ElectronSeedRef seed = ele->core()->gsfTrack()->extra()->seedRef().castTo(); + for ( unsigned int igsf = 0; igsf < gsfTracks->size(); igsf++ ) { + reco::GsfTrackRef gsf(gsfTracks,igsf); + if ( gsf.isNonnull() && + gsf->extra().isNonnull() && + gsf->extra()->seedRef().isNonnull() ) { + reco::ElectronSeedRef seed = gsf->extra()->seedRef().castTo(); if ( seed.isNonnull() && seed->ctfTrack().isNonnull() ) { const reco::PreIdRef preid = (*preIdsValueMap)[seed->ctfTrack()]; if ( preid.isNonnull() ) { for ( unsigned int iname = 0; iname < names_.size(); ++iname ) { - output[iname][iele] = preid->mva(iname); + output[iname][igsf] = preid->mva(iname); } } } @@ -66,7 +64,7 @@ void LowPtGsfElectronSeedValueMapsProducer::produce( edm::Event& event, const ed for ( unsigned int iname = 0; iname < names_.size(); ++iname ) { auto ptr = std::make_unique< edm::ValueMap >( edm::ValueMap() ); edm::ValueMap::Filler filler(*ptr); - filler.insert(gsfElectrons, output[iname].begin(), output[iname].end()); + filler.insert(gsfTracks, output[iname].begin(), output[iname].end()); filler.fill(); event.put(std::move(ptr),names_[iname]); } @@ -78,9 +76,9 @@ void LowPtGsfElectronSeedValueMapsProducer::produce( edm::Event& event, const ed void LowPtGsfElectronSeedValueMapsProducer::fillDescriptions( edm::ConfigurationDescriptions& descriptions ) { edm::ParameterSetDescription desc; - desc.add("electrons",edm::InputTag("lowPtGsfElectrons")); + desc.add("gsfTracks",edm::InputTag("lowPtGsfEleGsfTracks")); desc.add("preIdsValueMap",edm::InputTag("lowPtGsfElectronSeeds")); - desc.add< std::vector >("ModelNames",std::vector({"default"})); + desc.add< std::vector >("ModelNames",std::vector()); descriptions.add("defaultLowPtGsfElectronSeedValueMaps",desc); } diff --git a/RecoEgamma/EgammaElectronProducers/plugins/LowPtGsfElectronSeedValueMapsProducer.h b/RecoEgamma/EgammaElectronProducers/plugins/LowPtGsfElectronSeedValueMapsProducer.h index c3cc7acdbb572..28f70374e5510 100644 --- a/RecoEgamma/EgammaElectronProducers/plugins/LowPtGsfElectronSeedValueMapsProducer.h +++ b/RecoEgamma/EgammaElectronProducers/plugins/LowPtGsfElectronSeedValueMapsProducer.h @@ -2,7 +2,7 @@ #define RecoEgamma_EgammaElectronProducers_LowPtGsfElectronSeedValueMapsProducer_h #include "DataFormats/Common/interface/ValueMap.h" -#include "DataFormats/EgammaCandidates/interface/GsfElectronFwd.h" +#include "DataFormats/GsfTrackReco/interface/GsfTrackFwd.h" #include "DataFormats/ParticleFlowReco/interface/PreIdFwd.h" #include "FWCore/Framework/interface/ESHandle.h" #include "FWCore/Framework/interface/stream/EDProducer.h" @@ -24,11 +24,10 @@ class LowPtGsfElectronSeedValueMapsProducer : public edm::stream::EDProducer<> { private: - const edm::EDGetTokenT gsfElectrons_; + const edm::EDGetTokenT gsfTracks_; const edm::EDGetTokenT< edm::ValueMap > preIdsValueMap_; const std::vector names_; }; #endif // RecoEgamma_EgammaElectronProducers_LowPtGsfElectronSeedValueMapsProducer_h - From 60b856f7c536f58a54f0be8a26605417d4ceb54d Mon Sep 17 00:00:00 2001 From: Robert Bainbridge Date: Sat, 9 Feb 2019 00:22:40 +0100 Subject: [PATCH 2/6] first pass of seed BDTs trained with 10.2.X MC --- .../python/lowPtGsfElectronSeeds_cfi.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/RecoEgamma/EgammaElectronProducers/python/lowPtGsfElectronSeeds_cfi.py b/RecoEgamma/EgammaElectronProducers/python/lowPtGsfElectronSeeds_cfi.py index 3b9ea16ad0a9d..3a089e9351216 100644 --- a/RecoEgamma/EgammaElectronProducers/python/lowPtGsfElectronSeeds_cfi.py +++ b/RecoEgamma/EgammaElectronProducers/python/lowPtGsfElectronSeeds_cfi.py @@ -1,8 +1,8 @@ import FWCore.ParameterSet.Config as cms def thresholds( wp ) : - return cms.vdouble([{"L": 1.03,"M":1.75,"T":2.61}.get(wp,1.e6), # unbiased - {"L":-0.48,"M":0.76,"T":1.83}.get(wp,1.e6)]) # ptbiased + return cms.vdouble([{"L": 1.20,"M":2.02,"T":3.05}.get(wp,1.e6), # unbiased + {"L": 0.01,"M":1.29,"T":2.42}.get(wp,1.e6)]) # ptbiased lowPtGsfElectronSeeds = cms.EDProducer( "LowPtGsfElectronSeedProducer", @@ -22,8 +22,8 @@ def thresholds( wp ) : 'ptbiased', ]), ModelWeights = cms.vstring([ - 'RecoEgamma/ElectronIdentification/data/LowPtElectrons/RunII_Fall17_LowPtElectrons_unbiased.xml.gz', - 'RecoEgamma/ElectronIdentification/data/LowPtElectrons/RunII_Fall17_LowPtElectrons_displaced_pt_eta_biased.xml.gz', + 'RecoEgamma/ElectronIdentification/data/LowPtElectrons/RunII_Autumn18_LowPtElectrons_unbiased.xml.gz', + 'RecoEgamma/ElectronIdentification/data/LowPtElectrons/RunII_Autumn18_LowPtElectrons_displaced_pt_eta_biased.xml.gz', ]), ModelThresholds = thresholds("T"), PassThrough = cms.bool(False), From ad31169a1b7d7ca09b6915d6377841db4f242c9a Mon Sep 17 00:00:00 2001 From: Robert Bainbridge Date: Sat, 9 Feb 2019 11:35:33 +0100 Subject: [PATCH 3/6] updated to latest ID MDT trained with 10.2.X MC --- .../plugins/LowPtGsfElectronIDProducer.cc | 6 +++--- .../python/lowPtGsfElectronID_cff.py | 8 +++++++- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/RecoEgamma/EgammaElectronProducers/plugins/LowPtGsfElectronIDProducer.cc b/RecoEgamma/EgammaElectronProducers/plugins/LowPtGsfElectronIDProducer.cc index 14e9e92551363..f4c65dde72c11 100644 --- a/RecoEgamma/EgammaElectronProducers/plugins/LowPtGsfElectronIDProducer.cc +++ b/RecoEgamma/EgammaElectronProducers/plugins/LowPtGsfElectronIDProducer.cc @@ -72,9 +72,9 @@ void LowPtGsfElectronIDProducer::fillDescriptions( edm::ConfigurationDescription edm::ParameterSetDescription desc; desc.add("electrons",edm::InputTag("lowPtGsfElectrons")); desc.add("rho",edm::InputTag("fixedGridRhoFastjetAllTmp")); - desc.add< std::vector >("ModelNames",std::vector({""})); - desc.add< std::vector >("ModelWeights",std::vector({"RecoEgamma/ElectronIdentification/data/LowPtElectrons/RunII_Fall17_LowPtElectrons_mva_id.xml.gz"})); - desc.add< std::vector >("ModelThresholds",std::vector({-1.})); + desc.add< std::vector >("ModelNames",std::vector()); + desc.add< std::vector >("ModelWeights",std::vector()); + desc.add< std::vector >("ModelThresholds",std::vector()); desc.add("PassThrough",false); desc.add("MinPtThreshold",0.5); desc.add("MaxPtThreshold",15.); diff --git a/RecoEgamma/EgammaElectronProducers/python/lowPtGsfElectronID_cff.py b/RecoEgamma/EgammaElectronProducers/python/lowPtGsfElectronID_cff.py index 03dc4f52821c9..09f1db56328ed 100644 --- a/RecoEgamma/EgammaElectronProducers/python/lowPtGsfElectronID_cff.py +++ b/RecoEgamma/EgammaElectronProducers/python/lowPtGsfElectronID_cff.py @@ -2,4 +2,10 @@ from RecoEgamma.EgammaElectronProducers.defaultLowPtGsfElectronID_cfi import defaultLowPtGsfElectronID -lowPtGsfElectronID = defaultLowPtGsfElectronID.clone() +lowPtGsfElectronID = defaultLowPtGsfElectronID.clone( + ModelNames = cms.vstring(['']), + ModelWeights = cms.vstring([ + 'RecoEgamma/ElectronIdentification/data/LowPtElectrons/RunII_Autumn18_LowPtElectrons_mva_id.xml.gz', + ]), + ModelThresholds = cms.vdouble([-10.]) + ) From 274449d4972ab205042e963282ac7be6490ac461 Mon Sep 17 00:00:00 2001 From: Robert Bainbridge Date: Tue, 19 Feb 2019 13:37:23 +0100 Subject: [PATCH 4/6] remove duplicate edm::Ref to ECAL clusters --- .../plugins/LowPtGsfElectronSCProducer.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/RecoEgamma/EgammaElectronProducers/plugins/LowPtGsfElectronSCProducer.cc b/RecoEgamma/EgammaElectronProducers/plugins/LowPtGsfElectronSCProducer.cc index d97e8882dda44..ef00d88a59d39 100644 --- a/RecoEgamma/EgammaElectronProducers/plugins/LowPtGsfElectronSCProducer.cc +++ b/RecoEgamma/EgammaElectronProducers/plugins/LowPtGsfElectronSCProducer.cc @@ -166,7 +166,6 @@ void LowPtGsfElectronSCProducer::produce( edm::Event& event, const edm::EventSet sc.setCorrectedEnergy(energy); sc.setSeed(seed); sc.setClusters(clusters); - for ( const auto clu : clusters ) { sc.addCluster(clu); } PFClusterWidthAlgo pfwidth(barePtrs); sc.setEtaWidth(pfwidth.pflowEtaWidth()); sc.setPhiWidth(pfwidth.pflowPhiWidth()); From c8c65676e1c20803c7068c620f25aab7e2d7522b Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Sat, 16 Feb 2019 08:33:31 -0600 Subject: [PATCH 5/6] Fix memory use after delete in LowPtGsfElectronSCProducer The code was making a temporary copy of a PFBrem, then holding onto the address of internal data after the temporary was deleted. This change avoids the unnecessary copy. The problem was found by the address sanitizer. --- .../plugins/LowPtGsfElectronSCProducer.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/RecoEgamma/EgammaElectronProducers/plugins/LowPtGsfElectronSCProducer.cc b/RecoEgamma/EgammaElectronProducers/plugins/LowPtGsfElectronSCProducer.cc index ef00d88a59d39..4dd476d6cf394 100644 --- a/RecoEgamma/EgammaElectronProducers/plugins/LowPtGsfElectronSCProducer.cc +++ b/RecoEgamma/EgammaElectronProducers/plugins/LowPtGsfElectronSCProducer.cc @@ -80,7 +80,7 @@ void LowPtGsfElectronSCProducer::produce( edm::Event& event, const edm::EventSet points[itrk].reserve(trk->PFRecBrem().size()+1); points[itrk].push_back( &trk->extrapolatedPoint(reco::PFTrajectoryPoint::LayerType::ECALShowerMax) ); // Extrapolated brem trajectories - for ( auto brem : trk->PFRecBrem() ) { + for ( auto const& brem : trk->PFRecBrem() ) { points[itrk].push_back( &brem.extrapolatedPoint(reco::PFTrajectoryPoint::LayerType::ECALShowerMax) ); } // Resize containers From 71cfe2546449d28806e079e75cc2526746c23fd5 Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Sat, 16 Feb 2019 08:52:56 -0600 Subject: [PATCH 6/6] Avoid creating an unnecessary edm::Ref --- .../plugins/LowPtGsfElectronSCProducer.cc | 36 ++++++++++--------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/RecoEgamma/EgammaElectronProducers/plugins/LowPtGsfElectronSCProducer.cc b/RecoEgamma/EgammaElectronProducers/plugins/LowPtGsfElectronSCProducer.cc index 4dd476d6cf394..cf724125748ea 100644 --- a/RecoEgamma/EgammaElectronProducers/plugins/LowPtGsfElectronSCProducer.cc +++ b/RecoEgamma/EgammaElectronProducers/plugins/LowPtGsfElectronSCProducer.cc @@ -61,32 +61,34 @@ void LowPtGsfElectronSCProducer::produce( edm::Event& event, const edm::EventSet // Index[GSF track][trajectory point] for "best" CaloCluster std::vector< std::vector > cluster_idx; - cluster_idx.resize( gsfPfRecTracks->size(), std::vector() ); + cluster_idx.reserve( gsfPfRecTracks->size()); // Index[GSF track][trajectory point] for "best" PFCluster std::vector< std::vector > pfcluster_idx; - pfcluster_idx.resize( gsfPfRecTracks->size(), std::vector() ); + pfcluster_idx.reserve( gsfPfRecTracks->size()); // dr2min[GSF track][trajectory point] for "best" CaloCluster std::vector< std::vector > cluster_dr2min; - cluster_dr2min.resize( gsfPfRecTracks->size(), std::vector() ); + cluster_dr2min.reserve( gsfPfRecTracks->size()); // Construct list of trajectory points from the GSF track and electron brems std::vector< std::vector > points; - points.resize( gsfPfRecTracks->size(), std::vector() ); - for ( size_t itrk = 0; itrk < gsfPfRecTracks->size(); ++itrk ) { + points.reserve( gsfPfRecTracks->size()); + for ( auto const& trk: *gsfPfRecTracks) { // Extrapolated track - reco::GsfPFRecTrackRef trk(gsfPfRecTracks,itrk); - points[itrk].reserve(trk->PFRecBrem().size()+1); - points[itrk].push_back( &trk->extrapolatedPoint(reco::PFTrajectoryPoint::LayerType::ECALShowerMax) ); + std::vector traj; + traj.reserve(trk.PFRecBrem().size()+1); + traj.push_back( &trk.extrapolatedPoint(reco::PFTrajectoryPoint::LayerType::ECALShowerMax) ); // Extrapolated brem trajectories - for ( auto const& brem : trk->PFRecBrem() ) { - points[itrk].push_back( &brem.extrapolatedPoint(reco::PFTrajectoryPoint::LayerType::ECALShowerMax) ); + for ( auto const& brem : trk.PFRecBrem() ) { + traj.push_back( &brem.extrapolatedPoint(reco::PFTrajectoryPoint::LayerType::ECALShowerMax) ); } - // Resize containers - cluster_idx[itrk].resize(points[itrk].size(),-1); - pfcluster_idx[itrk].resize(points[itrk].size(),-1); - cluster_dr2min[itrk].resize(points[itrk].size(),1.e6); + auto size = traj.size(); + points.push_back(std::move(traj)); + // Size containers + cluster_idx.emplace_back(size,-1); + pfcluster_idx.emplace_back(size,-1); + cluster_dr2min.emplace_back(size,1.e6); } // For each cluster, find closest trajectory point ("global" arbitration at event level) @@ -153,8 +155,10 @@ void LowPtGsfElectronSCProducer::produce( edm::Event& event, const edm::EventSet X += clu->position().X() * clu->correctedEnergy(); Y += clu->position().Y() * clu->correctedEnergy(); Z += clu->position().Z() * clu->correctedEnergy(); - reco::PFClusterRef pfclu(ecalClusters,pfcluster_idx[itrk][ipoint]); - if ( pfclu.isNonnull() ) { barePtrs.push_back(&*pfclu); } + auto index = pfcluster_idx[itrk][ipoint]; + if(index < static_cast(ecalClusters->size())) { + barePtrs.push_back(&(*ecalClusters)[index]); + } } X /= energy; Y /= energy;