apt-sim / AdePT

Accelerated demonstrator of electromagnetic Particle Transport
Apache License 2.0
25 stars 34 forks source link

some cleanup and switching on energy fluctuations #302

Closed WitekPokorski closed 2 months ago

WitekPokorski commented 2 months ago

This PR enables energy loss fluctuations which are needed to properly reproduce the cell energy distribution. It also cleans one leftover printf. It removes one delete which causes a crash in G4.10.7 (not sure why it works in G4.11, but I guess, there is no problem removing it anyway).

phsft-bot commented 2 months ago

Can one of the admins verify this patch?

hahnjo commented 2 months ago

Note that energy loss fluctuations were previously measured to be significantly slower on the GPU due to added branch divergence, that's why I turned them off by default.

WitekPokorski commented 2 months ago

Indeed, I observe ~25% slow down with them on. It would probably make sense to have the possibility to switch them on/off at run time like in Geant4.

JuanGonzalezCaminero commented 2 months ago

If that's the case, I would make it a compile-time option for now, rather than leaving it always enabled.

I'd do the same for delete fTrackingManager;, it could stay behind some "Legacy compatibility mode", even if it would switch on just this line for now

agheata commented 2 months ago

Yes, please use a configuration option, default OFF for this. Why do you want to protect the deletion of the tracking manager? Do you understand why it crashes with Geant4.10.7?

WitekPokorski commented 2 months ago

Configuration option is not ideal either, because users will need to recompile to compare the two options. We need to be able to switch it on/off at run time, but I guess that requires a change in G4HepEm. I need to reinvestigate the problem with the delete in 10.7. Can't remember anymore where it was coming from. Some difference in the way vector of processes was deleted, or similar.

WitekPokorski commented 2 months ago

OK, I have added the configuration option to switch ON energy loss fluctuations (OFF by default). As for the delete fTrackingManager I haven't found out why it's crashing in 10.7 and not in 11, but I looked at g4hepem/apps/examples/TestEm3/PhysListG4EmTracking.hh and there, there is no deletion of the custom tracking manager. It seems that after doing

G4Electron::Definition()->SetTrackingManager(trackingManager);
G4Positron::Definition()->SetTrackingManager(trackingManager);
G4Gamma::Definition()->SetTrackingManager(trackingManager);

the ownership is transferred to Geant4. Again, I don 't know why it does not crash in G4.11, but it seems that not calling the delete in our physics constructor is the right approach.

WitekPokorski commented 2 months ago

It seems to be that the original design was that the custom tracking manager were deleted by G4VUserPhysicsList::RemoveTrackingManager() method. This works fine in 10.7 and can be seen in the following back trace:

#0  AdePTTrackingManager::~AdePTTrackingManager (this=0x7f3ed027c510, __in_chrg=<optimized out>) at /home/witoldp/lhcb-sim11-g4+cuda/AdePT/src/AdePTTrackingManager.cc:22
#1  0x00007f3f447e3155 in G4VUserPhysicsList::RemoveTrackingManager() () from /home/witoldp/lhcb-sim11-g4+cuda/Geant4/InstallArea/x86_64_v3-el9-gcc12+cuda12_1-opt/lib/Geant4-10.7.3.1/../libG4run.so
#2  0x00007f3f447e3336 in G4VUserPhysicsList::TerminateWorker() () from /home/witoldp/lhcb-sim11-g4+cuda/Geant4/InstallArea/x86_64_v3-el9-gcc12+cuda12_1-opt/lib/Geant4-10.7.3.1/../libG4run.so
#3  0x00007f3f447c2d78 in G4WorkerRunManager::~G4WorkerRunManager() () from /home/witoldp/lhcb-sim11-g4+cuda/Geant4/InstallArea/x86_64_v3-el9-gcc12+cuda12_1-opt/lib/Geant4-10.7.3.1/../libG4run.so

This is why when we try to delete our tracking manager (and we should not) it crashes. However, in G4.11 this line:

physicsList->TerminateWorker();

has disappeared from G4WorkerRunManager::~G4WorkerRunManager(), so G4VUserPhysicsList::RemoveTrackingManager() is not called anymore and when we call the delete it does not crash. I believe, however, that the behaviour in 10.7 was the correct one (hence, we should not call the delete of the tracking manager ourselves), so I am not sure why it was changed in G4.11. @hahnjo any comment on this?

hahnjo commented 3 weeks ago

It seems to be that the original design was that the custom tracking manager were deleted by G4VUserPhysicsList::RemoveTrackingManager() method. [...]

This is why when we try to delete our tracking manager (and we should not) it crashes. However, in G4.11 this line:

physicsList->TerminateWorker();

has disappeared from G4WorkerRunManager::~G4WorkerRunManager(), so G4VUserPhysicsList::RemoveTrackingManager() is not called anymore and when we call the delete it does not crash. I believe, however, that the behaviour in 10.7 was the correct one (hence, we should not call the delete of the tracking manager ourselves), so I am not sure why it was changed in G4.11. @hahnjo any comment on this?

Thanks for pinging and sorry for the delay in coming back to this with a proper fix after discussing offline. The call to TerminateWorker() was indeed removed in Geant4 11.0 to fix "termination issues" when making the tasking mode the default. The fix was a bit more involved because it required first solving that, but there's now a merge request that will hopefully make it into the December release.