Forward port of Geant4 SIMP custom physics model to master branch #28763
Conversation
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28763/13412
|
|
A new Pull Request was created by @fabiocos (Fabio Cossutti) for master. It involves the following packages: SimG4Core/CustomPhysics @cmsbuild, @civanch, @mdhildreth can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
update files modified in lowette:simps_to_customphysics_7_1, adapt SIMP model code to master, move to more standard file location
fabiocos
force-pushed
the
fc-simps-111X
branch
from
efa420a
to
70a1949
Compare
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28763/13413
|
|
Pull request #28763 was updated. @cmsbuild, @civanch, @mdhildreth can you please check and sign again. |
|
please test |
|
The tests are being triggered in jenkins. |
|
+1 |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
| theStringModel->SetFragmentationModel(theStringDecay); | ||
|
|
||
| theCascade = new G4GeneratorPrecompoundInterface; | ||
| thePreEquilib = new G4PreCompoundModel(new G4ExcitationHandler); |
There was a problem hiding this comment.
@fabiocos , lines 22-23 should be removed.
| CMSQGSPSIMPBuilder::~CMSQGSPSIMPBuilder() { | ||
| delete theStringDecay; | ||
| delete theStringModel; | ||
| delete thePreEquilib; |
There was a problem hiding this comment.
@fabiocos , lines 38-39 should be removed (delete thePreEquilib; delete theCascade)
|
|
||
| class G4ParticleDefinition; | ||
|
|
||
| class CMSSIMPInelasticProcess : public G4HadronicProcess { |
There was a problem hiding this comment.
@fabiocos , most of virtual methods must be removed - they are implemented inside G4HadronicProcess. In different Geant4 releases they may be different.
There was a problem hiding this comment.
@fabiocos , The code was working in 10.0 where we run in single threading mode, in 10.6 many things were changed except interfaces, so the code is well compiling but there may be problems, which I cannot catch promptly.
|
@fabiocos , would you agree if we merge this PR and after I will try to shape these classes to recent Geant4? I foresee in this 7_1 implementation some thread unsafe things not seen in an easy way. Suspect, that it would be ineffective if will put too many comments. |
|
+1 |
cmsbuild
added
fully-signed
simulation-approved
and removed
pending-signatures
simulation-pending
labels
|
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @davidlange6, @silviodonato, @fabiocos (and backports should be raised in the release meeting by the corresponding L2) |
|
@civanch thanks for your comment, as said mine was a basic attempt to do the porting with the minimal needed code change. I might easily implement the first comments of yours, but if more is needed, may be it is better to follow your suggestion, let @silviodonato integrate this (it will not break anyway anything as far as I can see) and let you clean the code in a second step. Just to be sure, do we have some test workflow using CustomPhysics, just to have a small cross check? |
|
+1 |
|
@fabiocos , @civanch looks like this broke GEANT4 IBs |
|
@smuzaffar , thanks! I expect problems for GEANT4 IB but they happens on dramatically high level. Will try to make next PR asap. The reason of the problem: author of this code rewrite all Geant4 base class virtual methods for physics and x-sections with old variants of 6 years ago. Because he is using inheritance, he should rewrite instead only few methods, which really required his modifications. |
|
@civanch @smuzaffar as far as I can see, the issue to be solved in the code cleaning is the redefinition of the particle performed in https://github.com/cms-sw/cmssw/blob/master/SimG4Core/CustomPhysics/src/CMSSIMPInelasticProcess.cc#L145 |
PR description:
Port of #26094 (targeting 7_1_X) to master, based on the original proposal by @lowette .
The code has been adapted to the new 11_1_X branch trying to mimic as closely as possible the original proposal. The
G4GlauberGribovCrossSectionclass, no more present in Geant4, has been replaced by the availableG4ComponentGGHadronNucleusXsc. Include files have been moved tointerface, theG4names have been turned intoCMSas done for many other models, and include guards have been updated according to the standard we adopt.PR validation:
Code compiles and passes
scram b code-checks, but no dedicated test workflow is provided to probe the real code behaviour in the original proposal. @civanch should confirm the correctness of the adopted approach, and possible tests to be performed.