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

Manage PluginFactory plugins with unique_ptr in RecoLocalMuon #25899

Merged
merged 2 commits into from Feb 11, 2019
Merged

Manage PluginFactory plugins with unique_ptr in RecoLocalMuon #25899

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
06febc2
Manage PluginFactory plugins with unique_ptr in RecoLocalMuon
makortel Jan 16, 2019
2a951b3
Use unique_ptr constructor for PluginFactory plugin in RPCRecHitProducer
makortel Jan 17, 2019
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
13 changes: 3 additions & 10 deletions RecoLocalMuon/CSCSegment/src/CSCSegmentBuilder.cc
View file
Open in desktop
Expand Up @@ -51,20 +51,13 @@ CSCSegmentBuilder::CSCSegmentBuilder(const edm::ParameterSet& ps) : geom_(nullpt
// Ask factory to build this algorithm, giving it appropriate ParameterSet

for (size_t j=0; j<chType.size(); ++j) {
algoMap[chType[j]] = CSCSegmentBuilderPluginFactory::get()->
create(algoName, segAlgoPSet[algoToType[j]-1]);
algoMap.emplace(chType[j], CSCSegmentBuilderPluginFactory::get()->
create(algoName, segAlgoPSet[algoToType[j]-1]));
edm::LogVerbatim("CSCSegment|CSC")<< "using algorithm #" << algoToType[j] << " for chamber type " << chType[j];
}
}

CSCSegmentBuilder::~CSCSegmentBuilder() {
//
// loop on algomap and delete them
//
for (std::map<std::string, CSCSegmentAlgorithm*>::iterator it = algoMap.begin();it != algoMap.end(); it++){
delete ((*it).second);
}
}
CSCSegmentBuilder::~CSCSegmentBuilder() = default;

void CSCSegmentBuilder::build(const CSCRecHit2DCollection* recHits, CSCSegmentCollection& oc) {

Expand Down
2 changes: 1 addition & 1 deletion RecoLocalMuon/CSCSegment/src/CSCSegmentBuilder.h
View file
Open in desktop
Expand Up @@ -45,7 +45,7 @@ class CSCSegmentBuilder {
private:

const CSCGeometry* geom_;
std::map<std::string, CSCSegmentAlgorithm*> algoMap;
std::map<std::string, std::unique_ptr<CSCSegmentAlgorithm>> algoMap;
};

#endif
2 changes: 1 addition & 1 deletion RecoLocalMuon/DTRecHit/interface/DTRecHitBaseAlgo.h
View file
Open in desktop
Expand Up @@ -89,7 +89,7 @@ class DTRecHitBaseAlgo {

protected:
// The module to be used for digi time synchronization
DTTTrigBaseSync *theSync;
std::unique_ptr<DTTTrigBaseSync> theSync;

};
#endif
Expand Down
11 changes: 4 additions & 7 deletions RecoLocalMuon/DTRecHit/plugins/DTRecHitProducer.cc
View file
Open in desktop
Expand Up @@ -32,25 +32,22 @@ using namespace std;

DTRecHitProducer::DTRecHitProducer(const ParameterSet& config) :
// Set verbose output
debug(config.getUntrackedParameter<bool>("debug", false))
debug(config.getUntrackedParameter<bool>("debug", false)),
// Get the concrete reconstruction algo from the factory
theAlgo{DTRecHitAlgoFactory::get()->create(config.getParameter<string>("recAlgo"),
config.getParameter<ParameterSet>("recAlgoConfig"))}
{
if(debug)
cout << "[DTRecHitProducer] Constructor called" << endl;

produces<DTRecHitCollection>();

DTDigiToken_ = consumes<DTDigiCollection>(config.getParameter<InputTag>("dtDigiLabel"));

// Get the concrete reconstruction algo from the factory
string theAlgoName = config.getParameter<string>("recAlgo");
theAlgo = DTRecHitAlgoFactory::get()->create(theAlgoName,
config.getParameter<ParameterSet>("recAlgoConfig"));
}

DTRecHitProducer::~DTRecHitProducer(){
if(debug)
cout << "[DTRecHitProducer] Destructor called" << endl;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this printout still relevant? (if not, the destructor could be defined as = default)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say that none of these debug printouts are relevant, at least not the ones in the c'tor and d'tor, but maybe also the one in the main method which just counts the number of hits in an unspecified DT layer: could it be simplified further by just removing the "debug" parameter and all its calls in this producer?

@namapane @fcavallo please let us know (answering if you still need the debug printout in the destructor at first, and all the other ones as second)

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree the printouts can be removed; I don't think the debug option has ever been activated in the last 10 years at least.
Whether you want to remove all printouts or only those in the dtor is your choice, I generally prefer not to touch working code unless a fix is needed, but if you prefer to clean up I don't have any objection.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@perrotta Which way do you prefer?

Copy link
Contributor

Choose a reason for hiding this comment

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

if this does not resolve any code check/quality analysis issue, perhaps we just leave it as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if this does not resolve any code check/quality analysis issue, perhaps we just leave it as is.

I don't think we flag couts, so it indeed would not. I'm fine with leaving this as they are as well.

delete theAlgo;
}


Expand Down
4 changes: 1 addition & 3 deletions RecoLocalMuon/DTRecHit/plugins/DTRecHitProducer.h
View file
Open in desktop
Expand Up @@ -38,9 +38,7 @@ class DTRecHitProducer : public edm::stream::EDProducer<> {
// The label to be used to retrieve DT digis from the event
edm::EDGetTokenT<DTDigiCollection> DTDigiToken_;
// The reconstruction algorithm
DTRecHitBaseAlgo *theAlgo;
// static string theAlgoName;

std::unique_ptr<DTRecHitBaseAlgo> theAlgo;
};
#endif

13 changes: 7 additions & 6 deletions RecoLocalMuon/DTRecHit/src/DTRecHitBaseAlgo.cc
View file
Open in desktop
Expand Up @@ -9,17 +9,18 @@
#include "RecoLocalMuon/DTRecHit/interface/DTRecHitBaseAlgo.h"

#include "Geometry/DTGeometry/interface/DTLayer.h"
#include "CalibMuon/DTDigiSync/interface/DTTTrigBaseSync.h"
#include "CalibMuon/DTDigiSync/interface/DTTTrigSyncFactory.h"
#include "FWCore/ParameterSet/interface/ParameterSet.h"

using namespace std;
using namespace edm;


DTRecHitBaseAlgo::DTRecHitBaseAlgo(const ParameterSet& config) {
theSync = DTTTrigSyncFactory::get()->create(config.getParameter<string>("tTrigMode"),
config.getParameter<ParameterSet>("tTrigModeConfig"));
}
DTRecHitBaseAlgo::DTRecHitBaseAlgo(const ParameterSet& config):
theSync{DTTTrigSyncFactory::get()->create(config.getParameter<string>("tTrigMode"),
config.getParameter<ParameterSet>("tTrigModeConfig"))}
{}

DTRecHitBaseAlgo::~DTRecHitBaseAlgo(){}

Expand All @@ -44,13 +45,13 @@ OwnVector<DTRecHit1DPair> DTRecHitBaseAlgo::reconstruct(const DTLayer* layer,
if (!OK) continue;

// Build a new pair of 1D rechit
DTRecHit1DPair* recHitPair = new DTRecHit1DPair(wireId, *digi);
auto recHitPair = std::make_unique<DTRecHit1DPair>(wireId, *digi);

// Set the position and the error of the 1D rechits
recHitPair->setPositionAndError(DTEnums::Left, lpoint, tmpErr);
recHitPair->setPositionAndError(DTEnums::Right, rpoint, tmpErr);

result.push_back(recHitPair);
result.push_back(std::move(recHitPair));
}
return result;
}
Expand Down
11 changes: 5 additions & 6 deletions RecoLocalMuon/DTSegment/src/DTRecSegment2DProducer.cc
View file
Open in desktop
Expand Up @@ -33,7 +33,10 @@ using namespace std;
/* ====================================================================== */

/// Constructor
DTRecSegment2DProducer::DTRecSegment2DProducer(const edm::ParameterSet& pset) {
DTRecSegment2DProducer::DTRecSegment2DProducer(const edm::ParameterSet& pset):
theAlgo{DTRecSegment2DAlgoFactory::get()->create(pset.getParameter<string>("Reco2DAlgoName"),
pset.getParameter<ParameterSet>("Reco2DAlgoConfig"))}
{
// Set verbose output
debug = pset.getUntrackedParameter<bool>("debug");

Expand All @@ -46,17 +49,13 @@ DTRecSegment2DProducer::DTRecSegment2DProducer(const edm::ParameterSet& pset) {
produces<DTRecSegment2DCollection>();

// Get the concrete reconstruction algo from the factory
string theAlgoName = pset.getParameter<string>("Reco2DAlgoName");
if(debug) cout << "the Reco2D AlgoName is " << theAlgoName << endl;
theAlgo = DTRecSegment2DAlgoFactory::get()->create(theAlgoName,
pset.getParameter<ParameterSet>("Reco2DAlgoConfig"));
if(debug) cout << "the Reco2D AlgoName is " << pset.getParameter<string>("Reco2DAlgoName") << endl;
}

/// Destructor
DTRecSegment2DProducer::~DTRecSegment2DProducer() {
if(debug)
cout << "[DTRecSegment2DProducer] Destructor called" << endl;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this printout still relevant? (if not, the destructor could be defined as = default)

Copy link
Contributor

Choose a reason for hiding this comment

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

@namapane @fcavallo : please answer

delete theAlgo;
}

/* Operations */
Expand Down
2 changes: 1 addition & 1 deletion RecoLocalMuon/DTSegment/src/DTRecSegment2DProducer.h
View file
Open in desktop
Expand Up @@ -50,7 +50,7 @@ class DTRecSegment2DProducer : public edm::stream::EDProducer<> {
bool debug;

// The 2D-segments reconstruction algorithm
DTRecSegment2DBaseAlgo* theAlgo;
std::unique_ptr<DTRecSegment2DBaseAlgo> theAlgo;

//static std::string theAlgoName;
edm::EDGetTokenT<DTRecHitCollection> recHits1DToken_;
Expand Down
13 changes: 6 additions & 7 deletions RecoLocalMuon/DTSegment/src/DTRecSegment4DProducer.cc
View file
Open in desktop
Expand Up @@ -23,7 +23,11 @@
using namespace edm;
using namespace std;

DTRecSegment4DProducer::DTRecSegment4DProducer(const ParameterSet& pset){
DTRecSegment4DProducer::DTRecSegment4DProducer(const ParameterSet& pset):
// Get the concrete 4D-segments reconstruction algo from the factory
the4DAlgo{DTRecSegment4DAlgoFactory::get()->create(pset.getParameter<string>("Reco4DAlgoName"),
pset.getParameter<ParameterSet>("Reco4DAlgoConfig"))}
{
produces<DTRecSegment4DCollection>();

// debug parameter
Expand All @@ -38,18 +42,13 @@ DTRecSegment4DProducer::DTRecSegment4DProducer(const ParameterSet& pset){
// the name of the 2D rec hits collection
recHits2DToken_ = consumes<DTRecSegment2DCollection>(pset.getParameter<InputTag>("recHits2DLabel"));

// Get the concrete 4D-segments reconstruction algo from the factory
string theReco4DAlgoName = pset.getParameter<string>("Reco4DAlgoName");
if(debug) cout << "the Reco4D AlgoName is " << theReco4DAlgoName << endl;
the4DAlgo = DTRecSegment4DAlgoFactory::get()->create(theReco4DAlgoName,
pset.getParameter<ParameterSet>("Reco4DAlgoConfig"));
if(debug) cout << "the Reco4D AlgoName is " << pset.getParameter<string>("Reco4DAlgoName") << endl;
}

/// Destructor
DTRecSegment4DProducer::~DTRecSegment4DProducer(){
if(debug)
cout << "[DTRecSegment4DProducer] Destructor called" << endl;
delete the4DAlgo;
}

void DTRecSegment4DProducer::produce(Event& event, const EventSetup& setup){
Expand Down
3 changes: 1 addition & 2 deletions RecoLocalMuon/DTSegment/src/DTRecSegment4DProducer.h
View file
Open in desktop
Expand Up @@ -40,10 +40,9 @@ class DTRecSegment4DProducer: public edm::stream::EDProducer<> {
bool debug;

edm::EDGetTokenT<DTRecHitCollection> recHits1DToken_;
//static std::string theAlgoName;
edm::EDGetTokenT<DTRecSegment2DCollection> recHits2DToken_;
// The 4D-segments reconstruction algorithm
DTRecSegment4DBaseAlgo* the4DAlgo;
std::unique_ptr<DTRecSegment4DBaseAlgo> the4DAlgo;
};
#endif

Expand Down
11 changes: 4 additions & 7 deletions RecoLocalMuon/DTSegment/src/DTSegmentUpdator.cc
View file
Open in desktop
Expand Up @@ -45,15 +45,14 @@ using namespace edm;

/// Constructor
DTSegmentUpdator::DTSegmentUpdator(const ParameterSet& config) :
theFitter(new DTLinearFit()) ,
theFitter{std::make_unique<DTLinearFit>()},
theAlgo{DTRecHitAlgoFactory::get()->create(config.getParameter<string>("recAlgo"),
config.getParameter<ParameterSet>("recAlgoConfig"))},
vdrift_4parfit(config.getParameter<bool>("performT0_vdriftSegCorrection")),
T0_hit_resolution(config.getParameter<double>("hit_afterT0_resolution")),
perform_delta_rejecting(config.getParameter<bool>("perform_delta_rejecting")),
debug(config.getUntrackedParameter<bool>("debug",false))
{
string theAlgoName = config.getParameter<string>("recAlgo");
theAlgo = DTRecHitAlgoFactory::get()->create(theAlgoName,
config.getParameter<ParameterSet>("recAlgoConfig"));
intime_cut=20.;
if (config.exists("intime_cut"))
intime_cut = config.getParameter<double>("intime_cut");
Expand All @@ -63,9 +62,7 @@ DTSegmentUpdator::DTSegmentUpdator(const ParameterSet& config) :
}

/// Destructor
DTSegmentUpdator::~DTSegmentUpdator() {
delete theFitter;
}
DTSegmentUpdator::~DTSegmentUpdator() = default;

/* Operations */

Expand Down
4 changes: 2 additions & 2 deletions RecoLocalMuon/DTSegment/src/DTSegmentUpdator.h
View file
Open in desktop
Expand Up @@ -79,8 +79,8 @@ class DTSegmentUpdator{
protected:

private:
DTLinearFit* theFitter; // the linear fitter
DTRecHitBaseAlgo* theAlgo; // the algo for hit reconstruction
std::unique_ptr<DTLinearFit> theFitter; // the linear fitter
std::unique_ptr<DTRecHitBaseAlgo> theAlgo; // the algo for hit reconstruction
edm::ESHandle<DTGeometry> theGeom; // the geometry

void updateHits(DTRecSegment2D* seg,
Expand Down
17 changes: 8 additions & 9 deletions RecoLocalMuon/DTSegment/test/DTAnalyzerDetailed.cc
View file
Open in desktop
Expand Up @@ -60,11 +60,10 @@ using namespace std;
/* ====================================================================== */

/* Constructor */
DTAnalyzerDetailed::DTAnalyzerDetailed(const ParameterSet& pset) : _ev(0){

theSync =
DTTTrigSyncFactory::get()->create(pset.getUntrackedParameter<string>("tTrigMode"),
pset.getUntrackedParameter<ParameterSet>("tTrigModeConfig"));
DTAnalyzerDetailed::DTAnalyzerDetailed(const ParameterSet& pset) : _ev(0),
theSync{DTTTrigSyncFactory::get()->create(pset.getUntrackedParameter<string>("tTrigMode"),
pset.getUntrackedParameter<ParameterSet>("tTrigModeConfig"))}
{
// Get the debug parameter for verbose output
debug = pset.getUntrackedParameter<bool>("debug");
theRootFileName = pset.getUntrackedParameter<string>("rootFileName");
Expand Down Expand Up @@ -347,7 +346,7 @@ void DTAnalyzerDetailed::analyzeDTHits(const Event & event,
DTSuperLayerId slid = (*sl)->id();

DTMeanTimer meanTimer(dtGeom->superLayer(slid), dtRecHits, eventSetup,
theSync);
theSync.get());
vector<double> tMaxs=meanTimer.run();
for (vector<double>::const_iterator tMax=tMaxs.begin() ; tMax!=tMaxs.end();
++tMax) {
Expand Down Expand Up @@ -414,7 +413,7 @@ void DTAnalyzerDetailed::analyzeDTSegments(const Event & event,

DTSuperLayerId slid1(phiSeg->chamberId(),1);
/// Mean timer analysis
DTMeanTimer meanTimer1(dtGeom->superLayer(slid1), phiHits, eventSetup, theSync);
DTMeanTimer meanTimer1(dtGeom->superLayer(slid1), phiHits, eventSetup, theSync.get());
vector<double> tMaxs1=meanTimer1.run();
for (vector<double>::const_iterator tMax=tMaxs1.begin() ; tMax!=tMaxs1.end();
++tMax) {
Expand All @@ -431,7 +430,7 @@ void DTAnalyzerDetailed::analyzeDTSegments(const Event & event,

DTSuperLayerId slid3(phiSeg->chamberId(),3);
/// Mean timer analysis
DTMeanTimer meanTimer3(dtGeom->superLayer(slid3), phiHits, eventSetup, theSync);
DTMeanTimer meanTimer3(dtGeom->superLayer(slid3), phiHits, eventSetup, theSync.get());
vector<double> tMaxs3=meanTimer3.run();
for (vector<double>::const_iterator tMax=tMaxs3.begin() ; tMax!=tMaxs3.end();
++tMax) {
Expand All @@ -455,7 +454,7 @@ void DTAnalyzerDetailed::analyzeDTSegments(const Event & event,
zedHits= zedSeg->specificRecHits();
DTSuperLayerId slid(zedSeg->superLayerId());
/// Mean timer analysis
DTMeanTimer meanTimer(dtGeom->superLayer(slid), zedHits, eventSetup, theSync);
DTMeanTimer meanTimer(dtGeom->superLayer(slid), zedHits, eventSetup, theSync.get());
vector<double> tMaxs=meanTimer.run();
for (vector<double>::const_iterator tMax=tMaxs.begin() ; tMax!=tMaxs.end();
++tMax) {
Expand Down
5 changes: 1 addition & 4 deletions RecoLocalMuon/DTSegment/test/DTAnalyzerDetailed.h
View file
Open in desktop
Expand Up @@ -91,10 +91,7 @@ class DTAnalyzerDetailed : public edm::EDAnalyzer {
bool doHits;
bool doSegs;

DTTTrigBaseSync *theSync;

protected:

std::unique_ptr<DTTTrigBaseSync> theSync;
};
#endif // DTANALYZER_H

17 changes: 8 additions & 9 deletions RecoLocalMuon/DTSegment/test/DTSegAnalyzer.cc
View file
Open in desktop
Expand Up @@ -60,11 +60,10 @@ using namespace std;
/* ====================================================================== */

/* Constructor */
DTSegAnalyzer::DTSegAnalyzer(const ParameterSet& pset) : _ev(0){

theSync =
DTTTrigSyncFactory::get()->create(pset.getUntrackedParameter<string>("tTrigMode"),
pset.getUntrackedParameter<ParameterSet>("tTrigModeConfig"));
DTSegAnalyzer::DTSegAnalyzer(const ParameterSet& pset) : _ev(0),
theSync{DTTTrigSyncFactory::get()->create(pset.getUntrackedParameter<string>("tTrigMode"),
pset.getUntrackedParameter<ParameterSet>("tTrigModeConfig"))}
{
// Get the debug parameter for verbose output
debug = pset.getUntrackedParameter<bool>("debug");
theRootFileName = pset.getUntrackedParameter<string>("rootFileName");
Expand Down Expand Up @@ -214,7 +213,7 @@ void DTSegAnalyzer::analyzeDTHits(const Event & event,
DTSuperLayerId slid = (*sl)->id();

DTMeanTimer meanTimer(dtGeom->superLayer(slid), dtRecHits, eventSetup,
theSync);
theSync.get());
vector<double> tMaxs=meanTimer.run();
for (vector<double>::const_iterator tMax=tMaxs.begin() ; tMax!=tMaxs.end();
++tMax) {
Expand Down Expand Up @@ -278,7 +277,7 @@ void DTSegAnalyzer::analyzeDTSegments(const Event & event,

DTSuperLayerId slid1(phiSeg->chamberId(),1);
/// Mean timer analysis
DTMeanTimer meanTimer1(dtGeom->superLayer(slid1), phiHits, eventSetup, theSync);
DTMeanTimer meanTimer1(dtGeom->superLayer(slid1), phiHits, eventSetup, theSync.get());
vector<double> tMaxs1=meanTimer1.run();
for (vector<double>::const_iterator tMax=tMaxs1.begin() ; tMax!=tMaxs1.end();
++tMax) {
Expand All @@ -292,7 +291,7 @@ void DTSegAnalyzer::analyzeDTSegments(const Event & event,

DTSuperLayerId slid3(phiSeg->chamberId(),3);
/// Mean timer analysis
DTMeanTimer meanTimer3(dtGeom->superLayer(slid3), phiHits, eventSetup, theSync);
DTMeanTimer meanTimer3(dtGeom->superLayer(slid3), phiHits, eventSetup, theSync.get());
vector<double> tMaxs3=meanTimer3.run();
for (vector<double>::const_iterator tMax=tMaxs3.begin() ; tMax!=tMaxs3.end();
++tMax) {
Expand All @@ -313,7 +312,7 @@ void DTSegAnalyzer::analyzeDTSegments(const Event & event,
zedHits= zedSeg->specificRecHits();
DTSuperLayerId slid(zedSeg->superLayerId());
/// Mean timer analysis
DTMeanTimer meanTimer(dtGeom->superLayer(slid), zedHits, eventSetup, theSync);
DTMeanTimer meanTimer(dtGeom->superLayer(slid), zedHits, eventSetup, theSync.get());
vector<double> tMaxs=meanTimer.run();
for (vector<double>::const_iterator tMax=tMaxs.begin() ; tMax!=tMaxs.end();
++tMax) {
Expand Down
5 changes: 1 addition & 4 deletions RecoLocalMuon/DTSegment/test/DTSegAnalyzer.h
View file
Open in desktop
Expand Up @@ -91,10 +91,7 @@ class DTSegAnalyzer : public edm::EDAnalyzer {
bool doHits;
bool doSegs;

DTTTrigBaseSync *theSync;

protected:

std::unique_ptr<DTTTrigBaseSync> theSync;
};
#endif // DTANALYZER_H