New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed and clean-up initialisation of Geant4 #27479
Conversation
The code-checks are being triggered in jenkins. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27479/10809
|
A new Pull Request was created by @civanch (Vladimir Ivantchenko) for master. It involves the following packages: SimG4Core/Application @cmsbuild, @civanch, @mdhildreth can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
please test |
The tests are being triggered in jenkins. |
Comparison job queued. |
Comparison is ready Comparison Summary:
|
@@ -302,6 +304,7 @@ void ParametrisedEMPhysics::ConstructProcess() { | |||
ModifyTransportation(G4Positron::Positron(), nt, th1, th2); | |||
ModifyTransportation(G4Proton::Proton(), nt, th1, th2); | |||
} | |||
G4cout << "ParametrisedEMPhysics::ConstructProcess() is done" << G4endl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@civanch why inside CMSSW code do we use Geant4 messages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fabiocos , this is not 100% CMS code - it is an implementation of Geant4 derived class. G4cout is redirected to LogVerbatim, so it is equivalent. However, this may be changed.
Problem of this PR - sim history changed, this means that it is needed to understand why. I had a hope not break histories. The problem, which is fixed here is not obvious, more study required.
if (verbose > 1) { | ||
G4cout << "CMSEmStandardPhysicsLPM: HcalRegion " << aRegion << G4endl; | ||
G4cout << "CMSEmStandardPhysicsLPM: HGCalRegion " << bRegion << G4endl; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same question here, I see also other older instances...
if (verbose > 1) { | ||
G4cout << "CMSEmStandardPhysicsLPM: HcalRegion " << aRegion << G4endl; | ||
G4cout << "CMSEmStandardPhysicsLPM: HGCalRegion " << bRegion << G4endl; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fabiocos , done
The code-checks are being triggered in jenkins. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27479/10830
|
Pull request #27479 was updated. @cmsbuild, @civanch, @mdhildreth can you please check and sign again. |
please test |
The tests are being triggered in jenkins. |
Comparison job queued. |
Comparison is ready @slava77 comparisons for the following workflows were not done due to missing matrix map:
Comparison Summary:
|
+1 Fortunately, the problem does not affect SIM results, so likely back port is not needed. |
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, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@civanch thanks for following my comments, I have just a couple of questions. Even though the PR test does not show a regression, we will need to reproduce GEN-SIM in 11_0_0_pre4 and have a more extensive test with RelVals
@@ -57,27 +57,28 @@ OscarMTMasterThread::OscarMTMasterThread(const edm::ParameterSet& iConfig) | |||
runManagerMaster = std::make_shared<RunManagerMT>(iConfig); | |||
m_runManagerMaster = runManagerMaster; | |||
|
|||
edm::LogVerbatim("SimG4CoreApplication") << "OscarMTMasterThread: initialization of RunManagerMT finished"; | |||
LogDebug("SimG4CoreApplication") << "OscarMTMasterThread: initialization of RunManagerMT finished"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here the level of verbosity is raised, but I think it can be ok, and make simpler to follow the initialization behaviour in case, for normal operations we filter out this level anyway
|
||
if (!aRegion) { | ||
edm::LogVerbatim("SimG4CoreApplication") << "ParametrisedEMPhysics::ConstructProcess: " | ||
<< "EcalRegion is not defined, GFlash will not be enabled for ECAL!"; | ||
edm::LogWarning("SimG4CoreApplication") << "ParametrisedEMPhysics::ConstructProcess: " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@civanch this is quite a jump in verbosity, do we really need to treat it as a warning? As this code is devoted to GFlash, if GFlash is not activated in ECAL probably it makes sense...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fabiocos , this warning is valid: if GFlash is asked for EcalRegion but EcalRegion is not defined the warning should be seen.
pmanager->AddProcess(msc, -1, 1, 1); | ||
pmanager->AddProcess(new G4hhIonisation, -1, 2, 2); | ||
pmanager->AddProcess(new G4eBremsstrahlung, -1, -3, 3); | ||
pmanager->AddProcess(msc, -1, 1, -1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@civanch what is the expected effect of these changes in physics lists, in the process intialization? There are several of them...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fabiocos , this is an old EM configuration which these days are not used. The choice was to remove it or update to be consistent with Geant4 10.4. This EM physics may be applied for some exotic studies(difficult to predict) or will not be used any more....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@civanch thanks, my question is actually for all the changes to physics list in this PR, but I understand they fall in the same category. I will integrate this PR in next IB
+1 |
PR description:
In this PR a fix of order of initialisation is introduced and clean up/optimisation:
G4Regions and cuts should be initialized before physics list is created. This is the main fix of this PR, which provides correct initialisation of physics for special physics lists
DDWorld class should not communicate with Geant4 kernel
DDProductionCuts - memory churn is reduced (each G4Region and G4ProductionCuts is instantiated and filled only once)
EM physics constructors are cleaned up
Improved info messages and substituted G4cout by LogVerbatim
Should provide small reduction of memory churn and not change results of mainstream simulation.
PR validation:
Checked SIM hits with private test
if this PR is a backport please specify the original PR:
Before submitting your pull requests, make sure you followed this checklist: