LDMX-Software / ldmx-sw

The Light Dark Matter eXperiment simulation and reconstruction framework.
https://ldmx-software.github.io
GNU General Public License v3.0
21 stars 19 forks source link

Use Geant4-object pointer comparisons instead of string-comparisons where possible #1376

Open EinarElen opened 1 month ago

EinarElen commented 1 month ago

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

This has two big downsides

  1. 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).

  1. 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

tomeichlersmith commented 1 month ago

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();
EinarElen commented 1 month ago

Yeah, that seems sensible :)