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.

Sign up for GitHub

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

Jump to bottom

Tackled majority of RecoEgamma code quality issues #23743

Merged
merged 8 commits into from Jul 27, 2018
Merged

Tackled majority of RecoEgamma code quality issues #23743

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
82fe330
Tackled majority of RecoEgamma code quality issues
guitargeek Jul 4, 2018
5778e46
Removed obsolete NeuralNet class and moved estimators to GBRForest Tools
guitargeek Jul 13, 2018
f8d2900
Removed non-functional unused function
guitargeek Jul 13, 2018
86ce909
Cleaned remnants of ElectronNeuralNet and cleaned MVANtuplizer
guitargeek Jul 13, 2018
11892b2
Removing MVAObjectCache
guitargeek Jul 13, 2018
3a42566
Don't allow pointers to non-const objects as global producer members
guitargeek Jul 19, 2018
a272dd4
Missed two modules which still had pointers to non-const objects
guitargeek Jul 19, 2018
8755bb3
Replaced std::cout with edm::LogError
guitargeek Jul 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions RecoEgamma/EgammaElectronProducers/plugins/SiStripElectronSeedProducer.h
View file
Open in desktop
Expand Up @@ -2,7 +2,7 @@
#define SiStripElectronSeedProducer_h


#include "FWCore/Framework/interface/EDProducer.h"
#include "FWCore/Framework/interface/stream/EDProducer.h"
#include "FWCore/Framework/interface/Event.h"
#include "DataFormats/Common/interface/Handle.h"
#include "FWCore/Framework/interface/EventSetup.h"
Expand All @@ -12,7 +12,7 @@

class SiStripElectronSeedGenerator;

class SiStripElectronSeedProducer : public edm::EDProducer
class SiStripElectronSeedProducer : public edm::stream::EDProducer<>
{
public:

Expand Down
6 changes: 2 additions & 4 deletions RecoEgamma/EgammaHFProducers/plugins/HFRecoEcalCandidateAlgo.cc
View file
Open in desktop
Expand Up @@ -25,7 +25,6 @@ HFRecoEcalCandidateAlgo::HFRecoEcalCandidateAlgo(bool correct,double e9e25Cut,do
const std::vector<double>& eSeLCut,
const reco::HFValueStruct hfvv
) :

m_correct(correct),
m_e9e25Cut(e9e25Cut),
m_intercept2DCut(intercept2DCut),
Expand All @@ -39,10 +38,9 @@ HFRecoEcalCandidateAlgo::HFRecoEcalCandidateAlgo(bool correct,double e9e25Cut,do
m_era(4),
m_hfvv(hfvv)
{

}

RecoEcalCandidate HFRecoEcalCandidateAlgo::correctEPosition(const SuperCluster& original , const HFEMClusterShape& shape,int nvtx) {
RecoEcalCandidate HFRecoEcalCandidateAlgo::correctEPosition(const SuperCluster& original , const HFEMClusterShape& shape,int nvtx) const {
double corEta=original.eta();
//piece-wise log energy correction to eta
double logel=log(shape.eLong3x3()/100.0);
Expand Down Expand Up @@ -108,7 +106,7 @@ RecoEcalCandidate HFRecoEcalCandidateAlgo::correctEPosition(const SuperCluster&
void HFRecoEcalCandidateAlgo::produce(const edm::Handle<SuperClusterCollection>& SuperClusters,
const HFEMClusterShapeAssociationCollection& AssocShapes,
RecoEcalCandidateCollection& RecoECand,
int nvtx) {
int nvtx) const {



Expand Down
30 changes: 15 additions & 15 deletions RecoEgamma/EgammaHFProducers/plugins/HFRecoEcalCandidateAlgo.h
View file
Open in desktop
Expand Up @@ -35,24 +35,24 @@ class HFRecoEcalCandidateAlgo {
void produce(const edm::Handle<reco::SuperClusterCollection>& SuperClusters,
const reco::HFEMClusterShapeAssociationCollection& AssocShapes,
reco::RecoEcalCandidateCollection& RecoECand,
int nvtx);
int nvtx) const;


private:
reco::RecoEcalCandidate correctEPosition(const reco::SuperCluster& original, const reco::HFEMClusterShape& shape, int nvtx);
bool m_correct;
double m_e9e25Cut;
double m_intercept2DCut;
double m_intercept2DSlope;
double m_e1e9Cuthi;
double m_eCOREe9Cuthi;
double m_eSeLCuthi;
double m_e1e9Cutlo;
double m_eCOREe9Cutlo;
double m_eSeLCutlo;
int m_era;
bool m_correctForPileup;
reco::HFValueStruct m_hfvv;
reco::RecoEcalCandidate correctEPosition(const reco::SuperCluster& original, const reco::HFEMClusterShape& shape, int nvtx) const;

const bool m_correct;
const double m_e9e25Cut;
const double m_intercept2DCut;
const double m_intercept2DSlope;
const double m_e1e9Cuthi;
const double m_eCOREe9Cuthi;
const double m_eSeLCuthi;
const double m_e1e9Cutlo;
const double m_eCOREe9Cutlo;
const double m_eSeLCutlo;
const int m_era;
const reco::HFValueStruct m_hfvv;

};

Expand Down
2 changes: 1 addition & 1 deletion RecoEgamma/EgammaHFProducers/plugins/HLTHFRecoEcalCandidateProducer.cc
View file
Open in desktop
Expand Up @@ -49,7 +49,7 @@ HLTHFRecoEcalCandidateProducer::HLTHFRecoEcalCandidateProducer(edm::ParameterSet

}

void HLTHFRecoEcalCandidateProducer::produce(edm::Event & e, edm::EventSetup const& iSetup) {
void HLTHFRecoEcalCandidateProducer::produce(edm::StreamID sid, edm::Event & e, edm::EventSetup const& iSetup) const {


edm::Handle<reco::SuperClusterCollection> super_clus;
Expand Down
21 changes: 10 additions & 11 deletions RecoEgamma/EgammaHFProducers/plugins/HLTHFRecoEcalCandidateProducer.h
View file
Open in desktop
Expand Up @@ -13,27 +13,26 @@
//
//

#include "FWCore/Framework/interface/EDProducer.h"
#include "FWCore/Framework/interface/global/EDProducer.h"
#include "FWCore/ParameterSet/interface/ParameterSet.h"
#include "FWCore/Framework/interface/Event.h"
#include "FWCore/Framework/interface/EventSetup.h"
#include "FWCore/Framework/interface/ESHandle.h"
#include "HFRecoEcalCandidateAlgo.h"
#include "HFValueStruct.h"

class HLTHFRecoEcalCandidateProducer : public edm::EDProducer {
class HLTHFRecoEcalCandidateProducer : public edm::global::EDProducer<> {
public:
explicit HLTHFRecoEcalCandidateProducer(edm::ParameterSet const& conf);
void produce(edm::Event& e, edm::EventSetup const& iSetup) override;
void produce(edm::StreamID, edm::Event&, edm::EventSetup const&) const override;
private:
edm::InputTag hfclusters_,vertices_;
int HFDBversion_;
std::vector<double> HFDBvector_;
bool doPU_;
double Cut2D_;
double defaultSlope2D_;
reco::HFValueStruct hfvars_;
HFRecoEcalCandidateAlgo algo_;
const edm::InputTag hfclusters_,vertices_;
const int HFDBversion_;
const std::vector<double> HFDBvector_;
const double Cut2D_;
const double defaultSlope2D_;
const reco::HFValueStruct hfvars_;
const HFRecoEcalCandidateAlgo algo_;
};

#endif
6 changes: 3 additions & 3 deletions RecoEgamma/EgammaHLTAlgos/interface/EgammaHLTEcalIsolation.h
View file
Open in desktop
Expand Up @@ -42,12 +42,12 @@ class EgammaHLTEcalIsolation

float isolPtSum(const reco::RecoCandidate *recocandidate,
const std::vector<const reco::SuperCluster*>& sclusters,
const std::vector<const reco::BasicCluster*>& bclusters);
const std::vector<const reco::BasicCluster*>& bclusters) const;

/// Get Et cut for ecal hits
float getetMin() { return etMin; }
float getetMin() const { return etMin; }
/// Get isolation cone size.
float getConeSize() { return conesize; }
float getConeSize() const { return conesize; }

private:

Expand Down
14 changes: 7 additions & 7 deletions RecoEgamma/EgammaHLTAlgos/interface/EgammaHLTHcalIsolationDoubleCone.h
View file
Open in desktop
Expand Up @@ -50,23 +50,23 @@ class EgammaHLTHcalIsolationDoubleCone
}


float isolPtSum(const reco::RecoCandidate* recocandidate, const HBHERecHitCollection* hbhe, const HFRecHitCollection* hf, const CaloGeometry* geometry);
float isolPtSum(const reco::RecoCandidate* recocandidate, const HBHERecHitCollection* hbhe, const HFRecHitCollection* hf, const CaloGeometry* geometry) const;


/// Get pt cut for hcal hits
float getptMin() { return ptMin; }
float getptMin() const { return ptMin; }
/// Get isolation cone size.
float getConeSize() { return conesize; }
float getConeSize() const { return conesize; }
/// Get exclusion region
float getExclusion() { return exclusion; }
float getExclusion() const { return exclusion; }

private:

// ---------- member data --------------------------------
// Parameters of isolation cone geometry.
float ptMin;
float conesize;
float exclusion;
const float ptMin;
const float conesize;
const float exclusion;

};

Expand Down
2 changes: 1 addition & 1 deletion RecoEgamma/EgammaHLTAlgos/src/EgammaHLTEcalIsolation.cc
View file
Open in desktop
Expand Up @@ -21,7 +21,7 @@

float EgammaHLTEcalIsolation::isolPtSum(const reco::RecoCandidate* recocandidate,
const std::vector<const reco::SuperCluster*>& sclusters,
const std::vector<const reco::BasicCluster*>& bclusters){
const std::vector<const reco::BasicCluster*>& bclusters) const {

float ecalIsol=0.;

Expand Down
2 changes: 1 addition & 1 deletion RecoEgamma/EgammaHLTAlgos/src/EgammaHLTHcalIsolationDoubleCone.cc
View file
Open in desktop
Expand Up @@ -21,7 +21,7 @@



float EgammaHLTHcalIsolationDoubleCone::isolPtSum(const reco::RecoCandidate* recocandidate, const HBHERecHitCollection* hbhe, const HFRecHitCollection* hf, const CaloGeometry* geometry){
float EgammaHLTHcalIsolationDoubleCone::isolPtSum(const reco::RecoCandidate* recocandidate, const HBHERecHitCollection* hbhe, const HFRecHitCollection* hf, const CaloGeometry* geometry) const {

float hcalIsol=0.;

Expand Down
6 changes: 2 additions & 4 deletions RecoEgamma/EgammaHLTProducers/interface/ESListOfFEDSProducer.h
View file
Open in desktop
Expand Up @@ -6,7 +6,7 @@

#include <DataFormats/Common/interface/Handle.h>
#include <FWCore/Framework/interface/Event.h>
#include <FWCore/Framework/interface/EDProducer.h>
#include <FWCore/Framework/interface/stream/EDProducer.h>
#include <FWCore/MessageLogger/interface/MessageLogger.h>
#include <FWCore/ParameterSet/interface/ParameterSet.h>

Expand All @@ -28,14 +28,12 @@ namespace edm {
class ConfigurationDescriptions;
}

class ESListOfFEDSProducer : public edm::EDProducer {
class ESListOfFEDSProducer : public edm::stream::EDProducer<> {

public:
ESListOfFEDSProducer(const edm::ParameterSet& pset);
~ESListOfFEDSProducer() override;
void produce(edm::Event & e, const edm::EventSetup& c) override;
void beginJob(void) override;
void endJob(void) override;
void Egamma(edm::Event& e, const edm::EventSetup& es, std::vector<int>& done, std::vector<int>& FEDs);
void Muon(edm::Event& e, const edm::EventSetup& es, std::vector<int>& done, std::vector<int>& FEDs);
void Jets(edm::Event& e, const edm::EventSetup& es, std::vector<int>& done, std::vector<int>& FEDs);
Expand Down
6 changes: 3 additions & 3 deletions RecoEgamma/EgammaHLTProducers/interface/ESRecHitsMerger.h
View file
Open in desktop
Expand Up @@ -2,7 +2,7 @@
#define EventFilter_ESRecHitsMerger_H

#include <FWCore/Framework/interface/MakerMacros.h>
#include <FWCore/Framework/interface/EDProducer.h>
#include <FWCore/Framework/interface/global/EDProducer.h>

#include <DataFormats/Common/interface/Handle.h>
#include <FWCore/Framework/interface/Event.h>
Expand All @@ -18,12 +18,12 @@ namespace edm {
class ConfigurationDescriptions;
}

class ESRecHitsMerger : public edm::EDProducer {
class ESRecHitsMerger : public edm::global::EDProducer<> {

public:
ESRecHitsMerger(const edm::ParameterSet& pset);
~ESRecHitsMerger() override;
void produce(edm::Event & e, const edm::EventSetup& c) override;
void produce(edm::StreamID sid, edm::Event & e, const edm::EventSetup& c) const override;
void beginJob(void) override;
void endJob(void) override;
static void fillDescriptions(edm::ConfigurationDescriptions& descriptions);
Expand Down
7 changes: 3 additions & 4 deletions RecoEgamma/EgammaHLTProducers/interface/EcalListOfFEDSProducer.h
View file
Open in desktop
Expand Up @@ -6,7 +6,7 @@

#include <DataFormats/Common/interface/Handle.h>
#include <FWCore/Framework/interface/Event.h>
#include <FWCore/Framework/interface/EDProducer.h>
#include <FWCore/Framework/interface/stream/EDProducer.h>
#include <FWCore/MessageLogger/interface/MessageLogger.h>
#include <FWCore/ParameterSet/interface/ParameterSet.h>

Expand All @@ -27,14 +27,13 @@ namespace edm {
class ConfigurationDescriptions;
}

class EcalListOfFEDSProducer : public edm::EDProducer {
class EcalListOfFEDSProducer : public edm::stream::EDProducer<> {

public:
EcalListOfFEDSProducer(const edm::ParameterSet& pset);
~EcalListOfFEDSProducer() override;
void produce(edm::Event & e, const edm::EventSetup& c) override;
void beginJob(void) override;
void endJob(void) override;

void Egamma(edm::Event& e, const edm::EventSetup& es, std::vector<int>& done, std::vector<int>& FEDs);
void Muon(edm::Event& e, const edm::EventSetup& es, std::vector<int>& done, std::vector<int>& FEDs);
void Jets(edm::Event& e, const edm::EventSetup& es, std::vector<int>& done, std::vector<int>& FEDs);
Expand Down
6 changes: 3 additions & 3 deletions RecoEgamma/EgammaHLTProducers/interface/EcalRecHitsMerger.h
View file
Open in desktop
Expand Up @@ -2,7 +2,7 @@
#define EventFilter_EcalRecHitsMerger_H

#include <FWCore/Framework/interface/MakerMacros.h>
#include <FWCore/Framework/interface/EDProducer.h>
#include <FWCore/Framework/interface/global/EDProducer.h>

#include <DataFormats/Common/interface/Handle.h>
#include <FWCore/Framework/interface/Event.h>
Expand All @@ -18,12 +18,12 @@ namespace edm {
class ConfigurationDescriptions;
}

class EcalRecHitsMerger : public edm::EDProducer {
class EcalRecHitsMerger : public edm::global::EDProducer<> {

public:
EcalRecHitsMerger(const edm::ParameterSet& pset);
~EcalRecHitsMerger() override;
void produce(edm::Event & e, const edm::EventSetup& c) override;
void produce(edm::StreamID sid, edm::Event & e, const edm::EventSetup& c) const override;
void beginJob(void) override;
void endJob(void) override;
static void fillDescriptions(edm::ConfigurationDescriptions& descriptions);
Expand Down
8 changes: 4 additions & 4 deletions RecoEgamma/EgammaHLTProducers/interface/EgammaHLTCombinedIsolationProducer.h
View file
Open in desktop
Expand Up @@ -13,7 +13,7 @@

// user include files
#include "FWCore/Framework/interface/Frameworkfwd.h"
#include "FWCore/Framework/interface/EDProducer.h"
#include "FWCore/Framework/interface/global/EDProducer.h"
#include "FWCore/Framework/interface/Event.h"
#include "FWCore/Framework/interface/MakerMacros.h"
#include "FWCore/ParameterSet/interface/ParameterSet.h"
Expand All @@ -27,20 +27,20 @@ namespace edm {
class ConfigurationDescriptions;
}

class EgammaHLTCombinedIsolationProducer : public edm::EDProducer {
class EgammaHLTCombinedIsolationProducer : public edm::global::EDProducer<> {
public:
explicit EgammaHLTCombinedIsolationProducer(const edm::ParameterSet&);
~EgammaHLTCombinedIsolationProducer() override;

static void fillDescriptions(edm::ConfigurationDescriptions& descriptions);
void produce(edm::Event&, const edm::EventSetup&) override;
void produce(edm::StreamID, edm::Event&, const edm::EventSetup&) const override;
private:
// ----------member data ---------------------------

edm::EDGetTokenT<reco::RecoEcalCandidateCollection> recoEcalCandidateProducer_;
std::vector<edm::EDGetTokenT<reco::RecoEcalCandidateIsolationMap> > IsolTag_;
std::vector<double> IsolWeight_;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now I was also worried about vectors of non-const types, but fortunately this is not an issue. Calling a non-const member function of a vector element disregards the const qualifier of produce. Here an example which doesn't compile:

#include <vector>

class C {
  public:
    C() : c_(0) {}
    void increment() { ++c_; }
  private:
    int c_;
};

class A {
  public:
    A() {
        v_.push_back(C{});
    }
    void produce () const { v_[0].increment(); }
  private:
    std::vector<C> v_;
};

int main() {
    A a{};
    a.produce();
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@guitargeek , the point is that

   matcher_->setupES(iSetup);

can modify the object pointed by matcher_ independently in all threads, something which is not allowed for a global module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I understand, I just try to understand how to prevent these kind of problems in an algorithmic way without having to read all the source code of the classes of the members.

edm::ParameterSet conf_;
const edm::ParameterSet conf_;

};

26 changes: 13 additions & 13 deletions RecoEgamma/EgammaHLTProducers/interface/EgammaHLTEcalIsolationProducersRegional.h
View file
Open in desktop
Expand Up @@ -23,7 +23,7 @@

// user include files
#include "FWCore/Framework/interface/Frameworkfwd.h"
#include "FWCore/Framework/interface/EDProducer.h"
#include "FWCore/Framework/interface/global/EDProducer.h"
#include "FWCore/Framework/interface/Event.h"
#include "FWCore/Framework/interface/MakerMacros.h"
#include "FWCore/ParameterSet/interface/ParameterSet.h"
Expand All @@ -42,25 +42,25 @@ namespace edm {
class ConfigurationDescriptions;
}

class EgammaHLTEcalIsolationProducersRegional : public edm::EDProducer {
class EgammaHLTEcalIsolationProducersRegional : public edm::global::EDProducer<> {
public:
explicit EgammaHLTEcalIsolationProducersRegional(const edm::ParameterSet&);
~EgammaHLTEcalIsolationProducersRegional() override;
void produce(edm::Event&, const edm::EventSetup&) override;
void produce(edm::StreamID sid, edm::Event&, const edm::EventSetup&) const override;
static void fillDescriptions(edm::ConfigurationDescriptions& descriptions);

private:
edm::EDGetTokenT<reco::RecoEcalCandidateCollection> recoEcalCandidateProducer_;
edm::EDGetTokenT<reco::BasicClusterCollection> bcBarrelProducer_;
edm::EDGetTokenT<reco::BasicClusterCollection> bcEndcapProducer_;
edm::EDGetTokenT<reco::SuperClusterCollection> scIslandBarrelProducer_;
edm::EDGetTokenT<reco::SuperClusterCollection> scIslandEndcapProducer_;
const edm::ParameterSet conf_;

edm::ParameterSet conf_;
const edm::EDGetTokenT<reco::RecoEcalCandidateCollection> recoEcalCandidateProducer_;
const edm::EDGetTokenT<reco::BasicClusterCollection> bcBarrelProducer_;
const edm::EDGetTokenT<reco::BasicClusterCollection> bcEndcapProducer_;
const edm::EDGetTokenT<reco::SuperClusterCollection> scIslandBarrelProducer_;
const edm::EDGetTokenT<reco::SuperClusterCollection> scIslandEndcapProducer_;

double egEcalIsoEtMin_;
double egEcalIsoConeSize_;
int algoType_;
EgammaHLTEcalIsolation* test_;
const double egEcalIsoEtMin_;
const double egEcalIsoConeSize_;
const int algoType_;
EgammaHLTEcalIsolation const * const test_;
};