Skip to content
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

HCAL Simulation : reformatted HF Shower Library for Run3 #35154

Merged
merged 5 commits into from Sep 7, 2021

Conversation

abdoulline
Copy link

@abdoulline abdoulline commented Sep 5, 2021

PR description:

Much faster CPU-wise access to HF ShowerLibrary (SL) content, as suggested by Lev Kheyn:
https://indico.cern.ch/event/1071568/contributions/4510124/attachments/2303109/3917905/HF_SL_perf_lk_03_09_2021.pdf

The content per se didn't change physics-wise, just has been rearranged/reassembled differently.
Private tests show no change of GEANT history.

NB: needs availability of HFShowerLibrary_run3_v6.root for RelVal/Jenkins tests
cms-sw/cmsdist#7269

PR includes:
(1) SL assembling/production code modifications, allowing for performant splitLevel=0;
(2) SL-reading modification, back-compatible with previous SL versions ;
(3) small adjustment/reduction of HF reco energy scale by ~3% (via Digitization config);
(4) SL-production "README" recipe.

PR validation:

Single-pion gun ("CaloScan") private 2021 test yields no change in 12_1_0_pre2 :
(i) with this branch (PR) without aformentionent component (3)
wrt
(ii) actual default
https://cms-cpt-software.web.cern.ch/cms-cpt-software/General/Validation/SVSuite/HCAL/calo_scan_single_pi/12_1_X/12_1_0_pre2_reformSL_run3_vs_12_1_0_pre2_run3_SinglePi/

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 5, 2021

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35154/25082

  • This PR adds an extra 28KB to repository

  • There are other open Pull requests which might conflict with changes you have proposed:

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 5, 2021

A new Pull Request was created by @abdoulline (Salavat Abdullin) for master.

It involves the following packages:

  • Geometry/HcalSimData (geometry)
  • SimCalorimetry/HcalSimProducers (simulation)
  • SimG4CMS/Calo (simulation)
  • SimG4CMS/ShowerLibraryProducer (simulation)

@civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @cmsbuild can you please review it and eventually sign? Thanks.
@felicepantaleo, @rovere, @bsunanda, @thomreis, @cvuosalo, @simonepigazzini, @mariadalfonso, @fabiocos, @slomeo this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@smuzaffar
Copy link
Contributor

please test with cms-sw/cmsdist#7269

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 5, 2021

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d9c38c/18321/summary.html
COMMIT: 5ba9b2c
CMSSW: CMSSW_12_1_X_2021-09-05-0000/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35154/18321/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 3493 differences found in the comparisons
  • DQMHistoTests: Total files compared: 39
  • DQMHistoTests: Total histograms compared: 3000491
  • DQMHistoTests: Total failures: 7426
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2993043
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 38 files compared)
  • Checked 165 log files, 37 edm output root files, 39 DQM output files
  • TriggerResults: found differences in 3 / 38 workflows

@abdoulline
Copy link
Author

abdoulline commented Sep 5, 2021

I believe this PR does exactly what it's supposed to:

  • small differences observed only in Run3 and Phase2 wf's
  • SIM (HF SimHits) didn't change and GEANT history preserved
  • HF Digis -> HF RecHits -> dowstream consumers of HF got 2-3% energy scale reduction

Plets below come from wf 11634.0:

RecHits_Emean_HF

SET_CAloTowers_HF

@abdoulline
Copy link
Author

@smuzaffar thanks for submitting new version of ShowerLibrary to cmssdt and arranging for the PR tests
during weekend!

@civanch
Copy link
Contributor

civanch commented Sep 5, 2021

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 5, 2021

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. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

@smuzaffar
Copy link
Contributor

this should be merged with cms-sw/cmsdist#7269

@qliphy
Copy link
Contributor

qliphy commented Sep 6, 2021

@abdoulline This is still a draft, is it ready for review?

@abdoulline abdoulline marked this pull request as ready for review September 6, 2021 04:44
@abdoulline
Copy link
Author

abdoulline commented Sep 6, 2021

@qliphy it was draft because it couldn't be tested without cms-sw/cmsdist#7269 and, as Shahzad mentioned earlier, should be merged simultaneously with it. I've removed the draft flag, now that all the tests passes successfully. Should have done it yesterday...

@perrotta
Copy link
Contributor

perrotta commented Sep 6, 2021

@abdoulline

  • HF Digis -> HF RecHits -> dowstream consumers of HF got 2-3% energy scale reduction

Would this imply the need for some retuning of PF algos?

@perrotta
Copy link
Contributor

perrotta commented Sep 6, 2021

@abdoulline which is the impact on memory due to this change. This can be probably derived from page 3 of https://indico.cern.ch/event/1071568/contributions/4510124/attachments/2303109/3917905/HF_SL_perf_lk_03_09_2021.pdf, but it is not evident to me how to translate it into memory usage of benchmark run3 workflows

@abdoulline
Copy link
Author

abdoulline commented Sep 6, 2021

@perrotta

(replying from vacation...)

(1) PF has no HF clustering calibration, so HF can be modified in 12_1_X, in principle. While HB/HE are frozen in 12_0_0.
More so, there was an agreement yet in June between PPD and HCAL DPG that HF will be modified in 12_1_X, as for 12_0_X it was very tight. But most of the modifications went to 12_0_X, this one is a residual scale corrections after new SL implementation, so that HF scale would now be very similar to 11_3_X one, albeit not exactly the same, as new SL is qualitatively somewhat different form the previous one, because em and had particles have slighly different responses wrt previous SL ones.

(2) I suppose this particular PR has virtually no impact on the memory. But SIM group monitors (runs profiling) the performance changes quite often (each pre-release, I suppose)...

@abdoulline
Copy link
Author

abdoulline commented Sep 7, 2021

@perrotta

I've run wf 11634.0 wf (TTbar 2021, GENSIM step, 100 ev) 2 jobs simultaneously on the same lxplus machine:

(1) without this PR, actual default ------------------------------------------

Event Throughput: 0.0399849 ev/s
CPU Summary:

  • Total loop: 2447.64
  • Total init: 12.353
    MemoryReport> Peak virtual size 1925.07 Mbytes
    Key events increasing vsize:
    [12] run: 1 lumi: 1 event: 12 vsize = 1757.07 deltaVsize = 20 rss = 623.168 delta = 7.24219
    [23] run: 1 lumi: 1 event: 23 vsize = 1821.07 deltaVsize = 64 rss = 673.621 delta = 50.4531
    [39] run: 1 lumi: 1 event: 39 vsize = 1901.07 deltaVsize = 80 rss = 711.824 delta = 38.2031
    [96] run: 1 lumi: 1 event: 96 vsize = 1925.07 deltaVsize = 24 rss = 724.434 delta = 12.6094

(2) with this PR ------------------------------------------

Event Throughput: 0.0405794 ev/s
CPU Summary:

  • Total loop: 2391.46
  • Total init: 12.3854
    MemoryReport> Peak virtual size 1880.07 Mbytes
    Key events increasing vsize:
    [24] run: 1 lumi: 1 event: 24 vsize = 1800.07 deltaVsize = 20 rss = 674.023 delta = 6.31641
    [23] run: 1 lumi: 1 event: 23 vsize = 1780.07 deltaVsize = 64 rss = 667.707 delta = -110.023
    [39] run: 1 lumi: 1 event: 39 vsize = 1880.07 deltaVsize = 80 rss = 704.273 delta = 36.5664

@perrotta
Copy link
Contributor

perrotta commented Sep 7, 2021

Thank you @abdoulline for running jobs from vacations!

@perrotta
Copy link
Contributor

perrotta commented Sep 7, 2021

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants