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

Cleanup Geant4 sensitive detectors #19939

Merged
merged 7 commits into from Aug 11, 2017
Merged

Cleanup Geant4 sensitive detectors #19939

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a996c0d
substitute auto_ptr and remove redundant code
civanch Jul 27, 2017
24b53ba
cleanup initialisation
civanch Jul 27, 2017
11b9b63
more cleanup
civanch Jul 27, 2017
2e296d9
cosmetics
civanch Jul 31, 2017
d54d821
fixed typo
civanch Jul 31, 2017
fc6048c
fixed runMatrix error
civanch Jul 31, 2017
f26db6e
cleanup
civanch Jul 31, 2017
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
7 changes: 4 additions & 3 deletions SimG4CMS/Calo/interface/CaloSD.h
View file
Open in desktop
Expand Up @@ -54,8 +54,8 @@ class CaloSD : public SensitiveCaloDetector,
edm::ParameterSet const & p, const SimTrackManager*,
float timeSlice=1., bool ignoreTkID=false);
virtual ~CaloSD();
virtual bool ProcessHits(G4Step * step,G4TouchableHistory * tHistory);
virtual bool ProcessHits(G4GFlashSpot*aSpot,G4TouchableHistory*);
virtual bool ProcessHits(G4Step * step, G4TouchableHistory * tHistory);
virtual bool ProcessHits(G4GFlashSpot*aSpot, G4TouchableHistory*);
Copy link
Contributor

Choose a reason for hiding this comment

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

@civanch - I'd add a space before aSpot

virtual double getEnergyDeposit(G4Step* step);
virtual uint32_t setDetUnitId(G4Step* step)=0;

Expand Down Expand Up @@ -116,6 +116,7 @@ class CaloSD : public SensitiveCaloDetector,

CaloHitID currentID, previousID;
G4Track* theTrack;
G4TouchableHistory* theTouchableHistory;

G4StepPoint* preStepPoint;
float edepositEM, edepositHAD;
Expand All @@ -126,7 +127,7 @@ class CaloSD : public SensitiveCaloDetector,

const SimTrackManager* m_trackManager;
CaloG4Hit* currentHit;
// TimerProxy theHitTimer;
// TimerProxy theHitTimer;
bool runInit;

bool corrTOFBeam, suppressHeavy;
Expand Down
88 changes: 41 additions & 47 deletions SimG4CMS/Calo/src/CaloSD.cc
View file
Open in desktop
Expand Up @@ -19,21 +19,26 @@
#include "G4SystemOfUnits.hh"
#include "G4PhysicalConstants.hh"

//#define DebugLog
// #define DebugLog

CaloSD::CaloSD(G4String name, const DDCompactView & cpv,
const SensitiveDetectorCatalog & clg,
edm::ParameterSet const & p, const SimTrackManager* manager,
float timeSliceUnit, bool ignoreTkID) :
SensitiveCaloDetector(name, cpv, clg, p),
G4VGFlashSensitiveDetector(), theTrack(0), preStepPoint(0), eminHit(0),
eminHitD(0), m_trackManager(manager), currentHit(0), runInit(false),
timeSlice(timeSliceUnit), ignoreTrackID(ignoreTkID), hcID(-1), theHC(0),
meanResponse(0) {
//Add Hcal Sentitive Detector Names
G4VGFlashSensitiveDetector(), theTrack(nullptr), theTouchableHistory(nullptr),
preStepPoint(nullptr), eminHit(0.0), eminHitD(0.0), m_trackManager(manager),
currentHit(nullptr), runInit(false), timeSlice(timeSliceUnit),
ignoreTrackID(ignoreTkID), hcID(-1), theHC(nullptr), meanResponse(nullptr) {

//Add Hcal Sentitive Detector Names
collectionName.insert(name);

// initialisation
incidentEnergy = edepositEM = edepositHAD = 0.0f;
primIDSaved = -99;
emPDG = epPDG = gammaPDG = 0;

//Parameters
edm::ParameterSet m_CaloSD = p.getParameter<edm::ParameterSet>("CaloSD");
energyCut = m_CaloSD.getParameter<double>("EminTrack")*GeV;
Expand Down Expand Up @@ -111,43 +116,38 @@ CaloSD::CaloSD(G4String name, const DDCompactView & cpv,
}

CaloSD::~CaloSD() {
if (slave) delete slave;
if (theHC) delete theHC;
if (meanResponse) delete meanResponse;
delete slave;
delete theHC;
delete meanResponse;
}

bool CaloSD::ProcessHits(G4Step * aStep, G4TouchableHistory * ) {
bool CaloSD::ProcessHits(G4Step * aStep, G4TouchableHistory * tHistory) {

NaNTrap( aStep ) ;

if (aStep == NULL) {
return true;
} else {
if (getStepInfo(aStep)) {
if (hitExists() == false && edepositEM+edepositHAD>0.)
currentHit = createNewHit();
}
NaNTrap(aStep);
theTouchableHistory = tHistory;
if(getStepInfo(aStep) && !hitExists() && edepositEM+edepositHAD>0.f) {
Copy link
Contributor

Choose a reason for hiding this comment

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

hi @civanch - so no need to protect against aStep=nullptr now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was never needed and likely is left in many SD classes due to historical reasons.

There is also NaNTrap - I am not sure how it is useful, I personally prefer if FPE arithmetic checks are enabled on compiler level in jenkins tests and RelVal tests, home made NaNTrap looks strange, however, in this PR it is simplified, so cost much less than before.

currentHit = createNewHit();
}
return true;
}

bool CaloSD::ProcessHits(G4GFlashSpot* aSpot, G4TouchableHistory*) {

if (aSpot != NULL) {
if (aSpot != nullptr) {
theTrack = const_cast<G4Track *>(aSpot->GetOriginatorTrack()->GetPrimaryTrack());
G4int particleCode = theTrack->GetDefinition()->GetPDGEncoding();

if (particleCode == emPDG ||
particleCode == epPDG ||
particleCode == gammaPDG ) {
edepositEM = aSpot->GetEnergySpot()->GetEnergy();
edepositHAD = 0.;
edepositHAD = 0.f;
} else {
edepositEM = 0.;
edepositHAD = 0.;
edepositEM = 0.f;
edepositHAD = 0.f;
}

if (edepositEM>0.) {
if (edepositEM>0.f) {
G4Step * fFakeStep = new G4Step();
preStepPoint = fFakeStep->GetPreStepPoint();
G4StepPoint * fFakePostStepPoint = fFakeStep->GetPostStepPoint();
Expand Down Expand Up @@ -224,10 +224,9 @@ void CaloSD::EndOfEvent(G4HCofThisEvent* ) {

cleanHitCollection();

#ifdef DebugLog
edm::LogInfo("CaloSim") << "CaloSD: EndofEvent entered with " << theHC->entries()
Copy link
Contributor

Choose a reason for hiding this comment

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

@civanch - it may bring some extra output which was turned off with #ifdef DebugLog

<< " entries";
#endif

// TimeMe("CaloSD:sortAndMergeHits",false);
}

Expand All @@ -252,30 +251,27 @@ bool CaloSD::getStepInfo(G4Step* aStep) {
preStepPoint = aStep->GetPreStepPoint();
theTrack = aStep->GetTrack();

double time = (aStep->GetPostStepPoint()->GetGlobalTime())/nanosecond;
unsigned int unitID= setDetUnitId(aStep);
uint16_t depth = getDepth(aStep);
int primaryID = getTrackID(theTrack);

bool flag = (unitID > 0);
if (flag) {
double time = theTrack->GetGlobalTime()/CLHEP::nanosecond;
uint16_t depth = getDepth(aStep);
int primaryID = getTrackID(theTrack);
currentID.setID(unitID, time, primaryID, depth);

#ifdef DebugLog
G4TouchableHistory* touch =(G4TouchableHistory*)(theTrack->GetTouchable());
edm::LogInfo("CaloSim") << "CaloSD:: GetStepInfo for"
<< " PV " << touch->GetVolume(0)->GetName()
<< " PVid = " << touch->GetReplicaNumber(0)
<< " MVid = " << touch->GetReplicaNumber(1)
<< " PV " << theTouchableHistory->GetVolume(0)->GetName()
<< " PVid = " << theTouchableHistory->GetReplicaNumber(0)
<< " MVid = " << theTouchableHistory->GetReplicaNumber(1)
<< " Unit " << currentID.unitID()
<< " Edeposit = " << edepositEM << " " << edepositHAD;
<< " Edeposit = " << aStep->GetTotalEnergyDeposit();
} else {
G4TouchableHistory* touch =(G4TouchableHistory*)(theTrack->GetTouchable());
edm::LogInfo("CaloSim") << "CaloSD:: GetStepInfo for"
<< " PV " << touch->GetVolume(0)->GetName()
<< " PVid = " << touch->GetReplicaNumber(0)
<< " MVid = " << touch->GetReplicaNumber(1)
<< " Unit " << std::hex << unitID << std::dec
<< " Edeposit = " << edepositEM << " " << edepositHAD;
<< " PV " << theTouchableHistory->GetVolume(0)->GetName()
<< " PVid = " << theTouchableHistory->GetReplicaNumber(0)
<< " MVid = " << theTouchableHistory->GetReplicaNumber(1)
<< " Unit " << std::hex << unitID << std::dec;
#endif
}

Expand All @@ -284,9 +280,9 @@ bool CaloSD::getStepInfo(G4Step* aStep) {
particleCode == epPDG ||
particleCode == gammaPDG ) {
edepositEM = getEnergyDeposit(aStep);
edepositHAD = 0.;
edepositHAD = 0.f;
} else {
edepositEM = 0.;
edepositEM = 0.f;
edepositHAD = getEnergyDeposit(aStep);
}

Expand Down Expand Up @@ -433,7 +429,7 @@ CaloG4Hit* CaloSD::createNewHit() {
}
}
primIDSaved = currentID.trackID();
if (useMap) totalHits++;
if (useMap) ++totalHits;
return aHit;
}

Expand Down Expand Up @@ -490,10 +486,8 @@ void CaloSD::update(const BeginOfRun *) {
emPDG = theParticleTable->FindParticle(particleName="e-")->GetPDGEncoding();
epPDG = theParticleTable->FindParticle(particleName="e+")->GetPDGEncoding();
gammaPDG = theParticleTable->FindParticle(particleName="gamma")->GetPDGEncoding();
#ifdef DebugLog
edm::LogInfo("CaloSim") << "CaloSD: Particle code for e- = " << emPDG
<< " for e+ = " << epPDG << " for gamma = " << gammaPDG;
#endif
initRun();
runInit = true;
}
Expand Down Expand Up @@ -615,7 +609,7 @@ double CaloSD::getResponseWt(G4Track* aTrack) {

void CaloSD::storeHit(CaloG4Hit* hit) {
if (previousID.trackID()<0) return;
if (hit == 0) {
if (hit == nullptr) {
edm::LogWarning("CaloSim") << "CaloSD: hit to be stored is NULL !!";
return;
}
Expand Down
2 changes: 1 addition & 1 deletion SimG4CMS/CherenkovAnalysis/interface/DreamSD.h
View file
Open in desktop
Expand Up @@ -63,7 +63,7 @@ class DreamSD : public CaloSD {
int side;

/// Table of Cherenkov angle integrals vs photon momentum
std::auto_ptr<G4PhysicsOrderedFreeVector> chAngleIntegrals_;
std::unique_ptr<G4PhysicsOrderedFreeVector> chAngleIntegrals_;
G4MaterialPropertiesTable* materialPropertiesTable;
// Histogramming
TTree* ntuple_;
Expand Down
2 changes: 1 addition & 1 deletion SimG4CMS/CherenkovAnalysis/src/DreamSD.cc
View file
Open in desktop
Expand Up @@ -522,7 +522,7 @@ bool DreamSD::setPbWO2MaterialProperties_( G4Material* aMaterial ) {
// Calculate Cherenkov angle integrals:
// This is an ad-hoc solution (we hold it in the class, not in the material)
chAngleIntegrals_ =
std::auto_ptr<G4PhysicsOrderedFreeVector>( new G4PhysicsOrderedFreeVector() );
std::unique_ptr<G4PhysicsOrderedFreeVector>( new G4PhysicsOrderedFreeVector() );
Copy link
Contributor

Choose a reason for hiding this comment

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

@civanch - it is better to use std::make_unique


int index = 0;
double currentRI = RefractiveIndex[index];
Expand Down
2 changes: 1 addition & 1 deletion SimG4Core/Geometry/src/DDDWorld.cc
View file
Open in desktop
Expand Up @@ -15,7 +15,7 @@ DDDWorld::DDDWorld(const DDCompactView* cpv,
SensitiveDetectorCatalog & catalog,
bool check) {

std::auto_ptr<DDG4Builder> theBuilder(new DDG4Builder(cpv, check));
std::unique_ptr<DDG4Builder> theBuilder(new DDG4Builder(cpv, check));
Copy link
Contributor

Choose a reason for hiding this comment

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

auto theBuilder = std::make_unique(cpv, check);


DDGeometryReturnType ret = theBuilder->BuildGeometry();
G4LogicalVolume * world = ret.logicalVolume();
Expand Down
4 changes: 2 additions & 2 deletions SimG4Core/GeometryProducer/interface/GeometryProducer.h
View file
Open in desktop
Expand Up @@ -53,8 +53,8 @@ class GeometryProducer : public edm::one::EDProducer<edm::one::SharedResources,
SimActivityRegistry m_registry;
std::vector<std::shared_ptr<SimWatcher> > m_watchers;
std::vector<std::shared_ptr<SimProducer> > m_producers;
std::auto_ptr<sim::FieldBuilder> m_fieldBuilder;
std::auto_ptr<SimTrackManager> m_trackManager;
std::unique_ptr<sim::FieldBuilder> m_fieldBuilder;
std::unique_ptr<SimTrackManager> m_trackManager;
AttachSD * m_attach;
Copy link
Contributor

Choose a reason for hiding this comment

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

why this is a raw pointer?

std::vector<SensitiveTkDetector*> m_sensTkDets;
std::vector<SensitiveCaloDetector*> m_sensCaloDets;
Expand Down
4 changes: 2 additions & 2 deletions SimG4Core/GeometryProducer/src/GeometryProducer.cc
View file
Open in desktop
Expand Up @@ -134,8 +134,8 @@ void GeometryProducer::produce(edm::Event & e, const edm::EventSetup & es)
{
edm::LogInfo("GeometryProducer") << " instantiating sensitive detectors ";
// instantiate and attach the sensitive detectors
m_trackManager = std::auto_ptr<SimTrackManager>(new SimTrackManager);
if (m_attach==0) m_attach = new AttachSD;
m_trackManager = std::unique_ptr<SimTrackManager>(new SimTrackManager);
Copy link
Contributor

Choose a reason for hiding this comment

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

std::make_unique

if (m_attach == nullptr) m_attach = new AttachSD;
{
std::pair< std::vector<SensitiveTkDetector*>,
std::vector<SensitiveCaloDetector*> >
Expand Down
15 changes: 9 additions & 6 deletions SimG4Core/SensitiveDetector/interface/SensitiveDetector.h
View file
Open in desktop
Expand Up @@ -34,22 +34,25 @@ class SensitiveDetector : public G4VSensitiveDetector
virtual void AssignSD(const std::string & vname);
virtual void EndOfEvent(G4HCofThisEvent * eventHC);
enum coordinates {WorldCoordinates, LocalCoordinates};
Local3DPoint InitialStepPosition(G4Step * s, coordinates);
Local3DPoint FinalStepPosition(G4Step * s, coordinates);
Local3DPoint ConvertToLocal3DPoint(const G4ThreeVector& point);
std::string nameOfSD() { return name; }
Local3DPoint InitialStepPosition(G4Step * step, coordinates);
Local3DPoint FinalStepPosition(G4Step * step, coordinates);
inline Local3DPoint ConvertToLocal3DPoint(const G4ThreeVector& point)
{
Local3DPoint res(point.x(),point.y(),point.z());
return std::move(res);
}
inline std::string& nameOfSD() { return name; }
virtual std::vector<std::string> getNames()
{
std::vector<std::string> temp;
temp.push_back(nameOfSD());
temp.push_back(name);
return temp;
Copy link
Contributor

Choose a reason for hiding this comment

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

std::vectorstd::string getNames() { return std::vectorstd::string{name}; };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the second iteration likely this method should be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moreover, this methods are not used... Will be removed in the next iteration.

}

void NaNTrap( G4Step* step ) ;

private:
std::string name;
G4Step * currentStep;
};

#endif
6 changes: 3 additions & 3 deletions SimG4Core/SensitiveDetector/interface/SensitiveDetectorMaker.h
View file
Open in desktop
Expand Up @@ -42,10 +42,10 @@ class SensitiveDetectorMaker : public SensitiveDetectorMakerBase
const edm::ParameterSet& p,
const SimTrackManager* m,
SimActivityRegistry& reg,
std::auto_ptr<SensitiveTkDetector>& oTK,
std::auto_ptr<SensitiveCaloDetector>& oCalo) const
std::unique_ptr<SensitiveTkDetector>& oTK,
std::unique_ptr<SensitiveCaloDetector>& oCalo) const
{
std::auto_ptr<T> returnValue(new T(iname, cpv, clg, p, m));
std::unique_ptr<T> returnValue(new T(iname, cpv, clg, p, m));
Copy link
Contributor

Choose a reason for hiding this comment

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

std::make_unique

SimActivityRegistryEnroller::enroll(reg, returnValue.get());

this->convertTo(returnValue.get(), oTK,oCalo);
Expand Down
16 changes: 8 additions & 8 deletions SimG4Core/SensitiveDetector/interface/SensitiveDetectorMakerBase.h
View file
Open in desktop
Expand Up @@ -47,8 +47,8 @@ class SensitiveDetectorMakerBase
const edm::ParameterSet& p,
const SimTrackManager* m,
SimActivityRegistry& reg,
std::auto_ptr<SensitiveTkDetector>& oTK,
std::auto_ptr<SensitiveCaloDetector>& oCalo) const =0;
std::unique_ptr<SensitiveTkDetector>& oTK,
std::unique_ptr<SensitiveCaloDetector>& oCalo) const =0;
Copy link
Contributor

Choose a reason for hiding this comment

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

passing a reference to a unique pointer without transferring ownership may cause trouble if an owner decides to reset it. Would a shared_ptr be safer here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This class is used only at initialisation and it is its only function.

Copy link
Contributor

Choose a reason for hiding this comment

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

i have the same concern - so these functions do not keep this pointer around internally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This class does not create unique_ptr but only use them as input to check, if the SD class is tracker or calo.


// ---------- static member functions --------------------

Expand All @@ -57,14 +57,14 @@ class SensitiveDetectorMakerBase
protected:
//used to identify which type of Sensitive Detector we have
void convertTo( SensitiveTkDetector* iFrom,
std::auto_ptr<SensitiveTkDetector>& oTo,
std::auto_ptr<SensitiveCaloDetector>&) const{
oTo= std::auto_ptr<SensitiveTkDetector>(iFrom);
std::unique_ptr<SensitiveTkDetector>& oTo,
std::unique_ptr<SensitiveCaloDetector>&) const{
oTo= std::unique_ptr<SensitiveTkDetector>(iFrom);
}
void convertTo( SensitiveCaloDetector* iFrom,
std::auto_ptr<SensitiveTkDetector>&,
std::auto_ptr<SensitiveCaloDetector>& oTo) const{
oTo=std::auto_ptr<SensitiveCaloDetector>(iFrom);
std::unique_ptr<SensitiveTkDetector>&,
std::unique_ptr<SensitiveCaloDetector>& oTo) const{
oTo=std::unique_ptr<SensitiveCaloDetector>(iFrom);
}

private:
Expand Down
20 changes: 7 additions & 13 deletions SimG4Core/SensitiveDetector/src/AttachSD.cc
View file
Open in desktop
Expand Up @@ -7,12 +7,10 @@

#include "FWCore/MessageLogger/interface/MessageLogger.h"

using std::vector;
using std::string;
using std::cout;
using std::endl;

// #define DEBUG
//using std::vector;
//using std::string;
//using std::cout;
//using std::endl;

AttachSD::AttachSD() {}

Expand All @@ -29,20 +27,18 @@ AttachSD::create(const DDDWorld & w,
{
std::pair< std::vector<SensitiveTkDetector *>,
std::vector<SensitiveCaloDetector*> > detList;
//#ifdef DEBUG
//cout << " Initializing AttachSD " << endl;
LogDebug("SimG4CoreSensitiveDetector") << " AttachSD: Initializing" ;
//#endif
const std::vector<std::string>& rouNames = clg.readoutNames();
for (std::vector<std::string>::const_iterator it = rouNames.begin();
it != rouNames.end(); it++) {
std::string className = clg.className(*it);
//std::cout<<" trying to find something for "<<className<<" " <<*it<<std::endl;
edm::LogInfo("SimG4CoreSensitiveDetector") << " AttachSD: trying to find something for " << className << " " << *it ;
std::auto_ptr<SensitiveDetectorMakerBase> temp(
std::unique_ptr<SensitiveDetectorMakerBase> temp(
SensitiveDetectorPluginFactory::get()->create(className) );
std::auto_ptr<SensitiveTkDetector> tkDet;
std::auto_ptr<SensitiveCaloDetector> caloDet;
std::unique_ptr<SensitiveTkDetector> tkDet;
std::unique_ptr<SensitiveCaloDetector> caloDet;
temp->make(*it,cpv,clg,p,m,reg,tkDet,caloDet);
if(tkDet.get()){
detList.first.push_back(tkDet.get());
Expand All @@ -52,10 +48,8 @@ AttachSD::create(const DDDWorld & w,
detList.second.push_back(caloDet.get());
caloDet.release();
}
//#ifdef DEBUG
// cout << " AttachSD: created a " << className << " with name " << *it << endl;
LogDebug("SimG4CoreSensitiveDetector") << " AttachSD: created a " << className << " with name " << *it ;
//#endif
}
return detList;
}
Expand Down