Iss1376 geant4 object pointer for comparison #1633

LiamBrennan-UCSB · 2025-02-23T04:14:28Z

Fixing Issue #1286 and Issue #1376. Adds functionality to call a Geant4 pointer to compare to another pointer within Simcore, Biasing, etc such that a pointer to pointer comparison can be made rather than completing string to string as was done previously.

What are the issues that this addresses?

Check List

I successfully compiled ldmx-sw with my developments
I ran my developments and the following shows that they are successful.

Biasing/src/Biasing/TargetBremFilter.cxx

…he elctron definition to be simpler

SimCore/src/SimCore/DetectorConstruction.cxx

SimCore/src/SimCore/XsecBiasingOperator.cxx

Biasing/include/Biasing/TargetDarkBremFilter.h

SimCore/include/SimCore/Geant4_PtrRetrieval.h

EinarElen · 2025-02-24T17:17:01Z

SimCore/include/SimCore/Geant4_PtrRetrieval.h

+      return volume;
+    }
+  }
+  return nullptr;  // Volume not found


Should we warn here?

EinarElen · 2025-02-24T17:18:27Z

Biasing/src/Biasing/TargetBremFilter.cxx

-
-        if (processName.compareTo("eBrem") == 0 &&
+        auto electron = G4Electron::Definition();
+        auto eBrem_process = Geant4_PtrRetrieval::GetProcess(electron, "eBrem");


Here we can just do pointer comparison, no need to do string comparison right?

tvami

I have several style comments, but I can point those out privately.
Bigger picture tho: why did you need two classes and not just one? Geant4_PtrRetrieval is still not a good name, there are a billion pointers in GEANT4. I honestly dont have a bullet proof suggestion, but something like a GeometryHelper, or VolumeCheckHelper would be better (and it should have the conent of the current pointer retrieval and volume test file). @tomeichlersmith @EinarElen do you have a preference, new suggestion?

LiamBrennan-UCSB · 2025-02-24T18:08:31Z

One thing of note though: I wasn't able to fix this line

      const G4BiasingProcessInterface* wrapperProcess =
          (sharedData->GetPhysicsBiasingProcessInterfaces())[iprocess];

      if (wrapperProcess->GetWrappedProcess()->GetProcessName().compareTo(
              process) == 0) {
        return true;

Because its a wrapped process the pointer it creates is not the same as the unwrapped pointer. So when I would try to do comparisons and printouts.

biasWrapper(GammaToMuPair) Looking for process with name: biasWrapper(photonNuclear) 
Checking wrapped process: 0x5576eefb0778 (GammaToMuPair) against process: 0x5576f3fcfd20 (biasWrapper(photonNuclear)) 
Checking wrapped process: 0x5576ecfb9b90 (photonNuclear) against process: 0x5576f3fcfd20 (biasWrapper(photonNuclear)) 
Checking wrapped process: 0x5576ee922950 (conv) against process: 0x5576f3fcfd20 (biasWrapper(photonNuclear)) 
Checking wrapped process: 0x5576ee920240 (compt) against process: 0x5576f3fcfd20 (biasWrapper(photonNuclear)) 
Checking wrapped process: 0x5576ee91dc20 (phot) against process: 0x5576f3fcfd20 (biasWrapper(photonNuclear)) 
[ fire ] 0 fatal: [BiasSetup] : photonNuclear is not found in list of biased processes!

Clearly photonNuclear is in there. But the pointer is different because of the wrapping. I couldn't figure out how to get rid of the wrapping without going back to just doing string to string, but more complicated. If anyone has any suggestions on how this could be fixed I would appreciate it @tomeichlersmith @EinarElen

tomeichlersmith · 2025-02-25T14:42:12Z

Could you use the pointer returned by GetWrappedProcess() for the comparison?

In any case, I think that's okay. The main goal of switching to comparing pointers rather than strings is the performance improvement. This is only called at the beginning of a run so it doesn't have as large of a performance effect.

tomeichlersmith · 2025-02-25T14:50:13Z

why did you need two classes and not just one?

@tvami What are you referring to here? The functions are just in the Geant4_PtrRetrieval namespace, there are no classes.

As far as the naming, I would maybe rename it to be more clearly just a namespace where we have used all lower case, no under scores.
Maybe simcore::g4ptr so that it is within the simcore namespace?
I also don't have a great idea for a name. While I tend to agree that Geant4_PtrRetrieval and g4ptr are too generic, the functions in those namespaces aren't really doing much else.

tvami · 2025-02-25T16:35:23Z

I meant to not have two header files (Geant4_PtrRetrieval.h and VolumeTests.h), just one.

Haha it's too generic and too specific at the same time. Can we not expect to retrieve something else than a pointer? Or then it will be another namespace? VolumeTests.h already just returns booleans, so if we merge the two then g4ptr is too specific. What about simcore::g4helper with the idea of extending this with more stuff in the future?

tomeichlersmith · 2025-02-25T17:49:20Z

We have the G4User submodule of SimCore: https://github.com/LDMX-Software/ldmx-sw/tree/trunk/SimCore/include/SimCore/G4User

Right now, it holds implementations of Geant4's user actions, but I think including a PtrRetrieval.h and VolumeTests.h headers there would make sense since we are "using" Geant4. I still think those could be their own namespace just to keep the functions organized (simcore::g4user::ptr and simcore::g4user::volumetests respectively).

(If this sounds good but you are having trouble implementing it @LiamBrennan-UCSB , let me know and I can push what I'm thinking to this branch.)

tvami · 2025-02-25T18:03:56Z

OK but then volumetests should be more like volumechecks? It's not actually testing the volumes, right?

tomeichlersmith · 2025-02-25T18:06:02Z

Yea that makes sense 👍

tvami · 2025-02-27T04:27:54Z

Biasing/include/Biasing/TargetENProcessFilter.h

@@ -56,7 +56,6 @@ class TargetENProcessFilter : public simcore::UserAction {
   * target parent volume and so it will break if the target parent volume
   * changes its name.
   */


You removed the variable but not the corresponding comment

tvami · 2025-02-27T04:31:37Z

Biasing/src/Biasing/DeepEcalProcessFilter.cxx

+  // auto volume{track->GetVolume()->GetLogicalVolume()
+  //                 ? track->GetVolume()->GetLogicalVolume()->GetName()
+  //                 : "undefined"};


Delete these, although I'm not sure what the advantage of your new lines are wrt to what is now to be deleted

tvami · 2025-02-27T04:38:17Z

SimCore/include/SimCore/G4User/PtrRetrieval.h

+      return process;
+    }
+  }
+  std::cerr << "[WARNING] Process '" << processName << "' not found.\n";


Dont use cerr use the ldmx logging

Suggested change

std::cerr << "[WARNING] Process '" << processName << "' not found.\n";

ldmx_log(warn) << "Process '" << processName << "' not found.";

Talking with Liam I understand this might not be simple, as this is a namespace. Should we not throw the warning in the user class end? i.e. if the getProcess returned a nullptr then have the warning in the respective filter? @EinarElen @tomeichlersmith

I vote to have the warning in the user class end although its not technically difficult to support logging within a namespace. The only requirement for ldmx_log to function is for a framework::logging::logger named theLog_ to be present within scope. A namespace-level variable could then support logging for all of the functions under one "channel", but I think it makes more sense to have the warning be in the user classes.

tvami · 2025-02-27T04:38:29Z

SimCore/include/SimCore/G4User/PtrRetrieval.h

+inline G4Region* getRegion(const std::string& name) {
+  G4Region* region = G4RegionStore::GetInstance()->GetRegion(name);
+  if (!region) {
+    std::cerr << "[WARNING] Region '" << name << "' not found.\n";


tvami · 2025-02-27T04:38:36Z

SimCore/include/SimCore/G4User/PtrRetrieval.h

+inline G4VPhysicalVolume* getPhysicalVolume(const std::string& name) {
+  auto* volume = G4PhysicalVolumeStore::GetInstance()->GetVolume(name);
+  if (!volume) {
+    std::cerr << "[WARNING] Volume '" << name << "' not found.\n";


tvami · 2025-02-27T04:38:42Z

SimCore/include/SimCore/G4User/PtrRetrieval.h

+inline G4LogicalVolume* getLogicalVolume(const std::string& name) {
+  auto* volume = G4LogicalVolumeStore::GetInstance()->GetVolume(name);
+  if (!volume) {
+    std::cerr << "[WARNING] Volume '" << name << "' not found.\n";


tvami · 2025-02-27T04:40:03Z

SimCore/include/SimCore/G4User/VolumeChecks.h

+ * @param[in] vol G4LogicalVolume to check
+ * @param[in] vol_to_bias UNUSED name of volume to bias
+ */
+[[maybe_unused]] inline bool isInHcal(G4LogicalVolume* vol,


I'm confused about the [[maybe_unused]] here and in the next occasions

Me too, it is certainly used within DetectorConstruction.cxx to I think these attributes should be removed to make sure that the compilation is appropriately using this single definition in multiple places.

SimCore/src/SimCore/XsecBiasingOperator.cxx

tvami · 2025-02-27T04:45:00Z

Biasing/src/Biasing/TargetBremFilter.cxx

-        if (processName.compareTo("eBrem") == 0 &&
+        auto electron = G4Electron::Definition();
+        auto ebrem_process =
+            simcore::g4user::ptrretrieval::getProcess(electron, "eBrem");


is it really the electron that's supposed to be here @tomeichlersmith @EinarElen ?

I think so... thats the particle that would do the eBrem. Are you asking if this should be a photon whose creator process is eBrem?

Yes, that was my thinking, but I guess it works as it is

…identally deleted file

….com:LDMX-Software/ldmx-sw into Iss1376-Geant4-Object-Pointer-for-Comparison

tvami · 2025-02-27T15:23:46Z

SimCore/src/SimCore/XsecBiasingOperator.cxx

@@ -6,7 +6,7 @@ namespace simcore {

 XsecBiasingOperator::XsecBiasingOperator(
    std::string name, const framework::config::Parameters& parameters)
-    : G4VBiasingOperator(name) {}
+    : G4VBiasingOperator(name), theLog_{framework::logging::makeLogger(name)} {}


I think you merged but should have rebased, bc these changes here are already merged in https://github.com/LDMX-Software/ldmx-sw/pull/1630/files

git switch trunk git fetch git pull git switch - git rebase trunk

Liam Brennan and others added 9 commits February 16, 2025 21:04

Adding a pointer to use for Geant4 Objects rather than Strings

0a6ad66

Fixed issues

30398bb

Replaced the compareto() functions

fba72fb

Added isInEcal function

5331ed8

Fixed bug 1286 and added option to check for isInEcal

eb27f63

Removed XsecBiasing

df379bf

Fixed some spacing and unnecessary printouts

b2ed776

Removed unnecessary spaces

43463bc

Apply clang-format

b88ce92

EinarElen reviewed Feb 23, 2025

View reviewed changes

Biasing/src/Biasing/TargetBremFilter.cxx Outdated Show resolved Hide resolved

Liam Brennan and others added 2 commits February 22, 2025 21:23

Marked the currently unused 'volume_tests' as maybe unused. Changed t…

d2a2f78

…he elctron definition to be simpler

Apply clang-format

3646ebb

tvami reviewed Feb 24, 2025

View reviewed changes

SimCore/src/SimCore/DetectorConstruction.cxx Outdated Show resolved Hide resolved

tvami reviewed Feb 24, 2025

View reviewed changes

SimCore/src/SimCore/XsecBiasingOperator.cxx Outdated Show resolved Hide resolved

EinarElen reviewed Feb 24, 2025

View reviewed changes

Biasing/include/Biasing/TargetDarkBremFilter.h Outdated Show resolved Hide resolved

EinarElen reviewed Feb 24, 2025

View reviewed changes

tvami requested changes Feb 24, 2025

View reviewed changes

tvami requested a review from tomeichlersmith February 24, 2025 17:21

Liam Brennan added 3 commits February 26, 2025 19:22

addressing comments

73cd81e

Replacing isInEcal with new version and addressing comments

d9ffc1e

fixed conflicts

2f10323

tvami reviewed Feb 27, 2025

View reviewed changes

tvami requested a review from EinarElen February 27, 2025 04:45

Liam Brennan and others added 7 commits February 26, 2025 20:45

restoring XsecBiasingOperator

0777f46

Apply clang-format

a001c93

deleting comments and fixing printouts

2d9cca6

Merge Conflicts

ccc5ade

Apply clang-format

3a81c52

Note: I did not author XsecBiasingOperator.cxx, just restored the acc…

44769ee

…identally deleted file

Merge branch 'Iss1376-Geant4-Object-Pointer-for-Comparison' of github…

30adc7e

….com:LDMX-Software/ldmx-sw into Iss1376-Geant4-Object-Pointer-for-Comparison

tvami reviewed Feb 27, 2025

View reviewed changes

Liam Brennan and others added 2 commits February 28, 2025 18:27

undid log(ldmx) for now until it can be solved

dd05e10

Apply clang-format

0ffccb7

LiamBrennan-UCSB marked this pull request as ready for review March 3, 2025 18:30

tomeichlersmith mentioned this pull request Mar 3, 2025

patches to ldmx-sw following compiler update #1640

Merged

21 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Iss1376 geant4 object pointer for comparison #1633

Iss1376 geant4 object pointer for comparison #1633

LiamBrennan-UCSB commented Feb 23, 2025 •

edited

Loading

EinarElen Feb 24, 2025

EinarElen Feb 24, 2025

tvami left a comment

LiamBrennan-UCSB commented Feb 24, 2025

tomeichlersmith commented Feb 25, 2025

tomeichlersmith commented Feb 25, 2025

tvami commented Feb 25, 2025

tomeichlersmith commented Feb 25, 2025 •

edited

Loading

tvami commented Feb 25, 2025 •

edited

Loading

tomeichlersmith commented Feb 25, 2025

tvami Feb 27, 2025

tvami Feb 27, 2025

tvami Feb 27, 2025

tvami Feb 27, 2025

tomeichlersmith Mar 4, 2025

tvami Feb 27, 2025

tvami Feb 27, 2025

tvami Feb 27, 2025

tvami Feb 27, 2025

tomeichlersmith Mar 4, 2025

tvami Feb 27, 2025

tomeichlersmith Mar 4, 2025

tvami Mar 4, 2025

tvami Feb 27, 2025

tvami Feb 27, 2025

	std::cerr << "[WARNING] Process '" << processName << "' not found.\n";
	ldmx_log(warn) << "Process '" << processName << "' not found.";

Iss1376 geant4 object pointer for comparison #1633

Are you sure you want to change the base?

Iss1376 geant4 object pointer for comparison #1633

Conversation

LiamBrennan-UCSB commented Feb 23, 2025 • edited Loading

What are the issues that this addresses?

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvami left a comment

Choose a reason for hiding this comment

LiamBrennan-UCSB commented Feb 24, 2025

tomeichlersmith commented Feb 25, 2025

tomeichlersmith commented Feb 25, 2025

tvami commented Feb 25, 2025

tomeichlersmith commented Feb 25, 2025 • edited Loading

tvami commented Feb 25, 2025 • edited Loading

tomeichlersmith commented Feb 25, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LiamBrennan-UCSB commented Feb 23, 2025 •

edited

Loading

tomeichlersmith commented Feb 25, 2025 •

edited

Loading

tvami commented Feb 25, 2025 •

edited

Loading