Cleanup Geant4 sensitive detectors #19939
Conversation
|
A new Pull Request was created by @civanch (Vladimir Ivantchenko) for master. It involves the following packages: SimG4CMS/Calo @cmsbuild, @civanch, @mdhildreth, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
|
The tests are being triggered in jenkins. |
|
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
Comparison job queued. |
|
Pull request #19939 was updated. @cmsbuild, @civanch, @mdhildreth, @davidlange6 can you please check and sign again. |
|
please test |
|
The tests are being triggered in jenkins. |
|
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
Comparison job queued. |
|
please test |
|
The tests are being triggered in jenkins. |
|
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
hi @civanch - is there a reason to think all the ecal barrel changes in dqm are not related? |
|
Hi @davidlange6 , similar differences are in comparison of the recent #20083 - in that case definitely SIM is not touched, but Ecal and DQM differences remain. |
|
@davidlange6 , the recent comparisons for #20106 (RECO PR) show the same differences in SIM. |
|
+1 |
|
This PR description has
In the jenkins summary I see
you can see in the comparisons that simulation in phase-1 workflows has changed (e.g. https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_9_3_X_2017-08-08-2300+19939/21860/validateJR/all_OldVSNew_TTbar13TeV2017wf10024p0/ ; Is there a simple explanation for the changes? this probably means that the next relval should have a resim for phase-1 |
|
actually we will need a resim anyways due to the incoming magnetic field changes. |
|
@slava77 , at that day, similar differences were with other even very simple PRs. I have no explanation for myself. The regeneration was needed due to recent Sunanda fix of Ecal SIM. |
|
On 8/16/17 10:25 AM, Vladimir Ivantchenko wrote:
@slava77 <https://github.com/slava77> , at that day, similar differences
were with other even very simple PRs. I have no explanation for myself.
The regeneration was needed due to recent Sunanda fix of Ecal SIM.
I tried to follow links to the "similar" differences on that day and I
did not find any similarity before sending my last message.
Changes in phase-1 MC from this PR were unique.
E.g. in wf 10024.0 we
- In this PR [on top of CMSSW_9_3_X_2017-08-08-2300] we have 1008 reco
differences clearly indicative of a change in GEN-SIM output
- #19939 (comment)
mentions #20106 which was tested on top of CMSSW_9_3_X_2017-08-09-1100
and has only 4 reco differences all related to changes made in that reco
PR (as expected)
- #19939 (comment)
mentions #20083 which was tested on top of CMSSW_9_3_X_2017-08-09-1100
and has 0 differences in reco quantities (as expected)
Perhaps I have missed something else with actually similar differences
to this PR.
Please clarify.
So far, it looks like this PR has introduced changes in the SIM step.
…
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#19939 (comment)>, or
mute the thread
<https://github.com/notifications/unsubscribe-auth/AEdcbl0u2YdZ77RuK3_SX-_e4kHPTAlRks5sYyYOgaJpZM4OlcaJ>.
|
|
@slava77 , I have re-checked all modifications in the code and it is all only c++ clean-ups. How this can change sim histories - not clear, because these classes related to saving calo hits, not to G4 tracking. Likely histories should be the same. Why calo hits are different is not clear also. |
|
On 8/16/17 10:53 AM, Vladimir Ivantchenko wrote:
@slava77 <https://github.com/slava77> , I have re-checked all
modifications in the code and it is all only c++ clean-ups. How this can
change sim histories - not clear, because these classes related to
saving calo hits, not to G4 tracking. Likely histories should be the
same. Why calo hits are different is not clear also.
from a cursory look, the changes look OK,
but then it's not obvious if
- some of the added initializations may matter [if they do, supposedly
the new version is more correct]
- some math changes are not obviously bit-wise identical [if they are,
it is probably OK to within numerical precision]
Thank you for checking in more detail.
…
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#19939 (comment)>, or
mute the thread
<https://github.com/notifications/unsubscribe-auth/AEdcbtDkHRUAuPqb0ojszDu_szppY9Gvks5sYyyngaJpZM4OlcaJ>.
|
|
hi @civanch - please make a PR Friday reverting these changes so that these changes can be studied more carefully Thanks. |
|
I don't recall spotting anything extreme here when I looked at the diffs, but then maybe it was not a careful look 17 days ago. |
Substituted remaining auto_ptr by unique_ptr.
Proper initialised CaloSD with nullptr and all local members.
Removed not necessary computations in the beginning of produce(..) method, simplified computations in SensitiveDetector class by using directly G4 methods.
Should bring a small CPU performance improvement and no change in results.