LDMX-Software
Use Geant4-object pointer comparisons instead of string-comparisons where possible
We have a lot of code in SimCore and Biasing which does things like
auto MyThing {track->GetThing()->GetThingName()};
if (MyThing.compare(SomeString)) {
// Do the work here
}
Where thing can be something like
- A particular process, e.g. photoNuclear
- A particular geometry region, e.g. "CalorimeterRegion"
- A particular physical/logical volume, e.g. "hadronic_calorimeter"
This has two big downsides
- It is way too easy to mess this up and introduce bugs. For example: https://github.com/LDMX-Software/ldmx-sw/blob/28fd69677fca6250588d02b182661c24156f1e60/Biasing/src/Biasing/EcalProcessFilter.cxx#L101-L111
This is a bug. There is no physical volume called hcal_PV so that condition will never trigger. The volume in question happens to be called "hadronic_calorimeter" which differs from all the others that have names like tagger_PV etc. If we are using pointers as our comparison tool, we can enforce at configuration time that these are initialized and throw otherwise (so issues like the hcal name here would be caught).
- String comparisons can be really slow, especially for anything that is called ~at every Geant4 step (biasing operators and stepping actions). I made a quick and dirty comparison and there was a ~30% performance improvement by moving from string comparisons to pointer comparisons for EcalPN samples (!!!)
I think the best way to do this would be to have a helper type in SimCore that can be used to get the various pointers people might need and make sure that things like biasing wrappers are handled correctly.
class Geant4ObjectUtility // Someone please suggest a better name
{
const G4VProcess* GetPhotonuclearProcess() const {
return GetProcess(G4Gamma::Definition(), "photonNuclear");
}
const G4VProcess* GetProcess(G4ParticleDefinition* particle, std::string processName) const {
const auto manager {particle->GetProcessManager()};
const auto processes {manager->GetProcessList()};
for (int i{0}; i < processes->size(); ++i) {
const auto process {(*processes)[i]};
const auto name {process->GetProcessName()};
if (name.contains(processName)) {
return process;
}
}
return nullptr;
}
G4Region* GetRegion(std::string name) {
return G4RegionStore::GetInstance()->GetRegion(name);
}
};
etc
I don't think we want a class since it would never hold its own data, instead maybe just a namespace? ptr_retrieval?
namespace ptr_retrieval {
const G4VProcess* GetPhotonuclearProcess() const {
return GetProcess(G4Gamma::Definition(), "photonNuclear");
}
const G4VProcess* GetProcess(G4ParticleDefinition* particle, std::string processName) const {
const auto manager {particle->GetProcessManager()};
const auto processes {manager->GetProcessList()};
for (int i{0}; i < processes->size(); ++i) {
const auto process {(*processes)[i]};
const auto name {process->GetProcessName()};
if (name.contains(processName)) {
return process;
}
}
// for processes from the particle table, I can't think of a situation where this would be useful
// maybe code-in the exception here?
return nullptr;
}
G4Region* GetRegion(std::string name) {
return G4RegionStore::GetInstance()->GetRegion(name);
}
} // namespace ptr_retrieval
and then the call-site would look readable like
auto pn = ptr_retrieval::GetPhotonuclearProcess();
So where should this go? And then have a GetProcess for all other kinda processes?
It's not clear to me how the proposed snippet gets rid of string comparisons, there is still a name.contains() in it.
But I guess if we do this, it will resolve https://github.com/LDMX-Software/ldmx-sw/issues/1286 by having the same things for the detector elements, right?
This would be utility functions available from within SimCore and then the places that know they need to do region or volume checks (e.g. in filters) would store the values of these pointers and then only use the values of the pointers during running.