BTL simulation: OOT effects #28433

casarsa · 2019-11-20T11:28:21Z

PR description:

This PR modifies the BTL digitization to include the effects on time resolution due to the photon tails of earlier OOT hits in the same crystal. It implements the model proposed by A. Ledovskoy (slides). (@pmeridian @parbol )

The size of MTDSimHitData had to be increased in order for the information of previous BXs
for both bar sides to be available in the BTLElectronicSim. The premixing configuration has been updated accordingly. (@makortel )

PR validation:

The new code has been tested in CMSSW_11_0_X_2019-11-18-2300 with the TTbar WF 22234 (PU200):

step 2:

MemoryReport> Peak virtual size 6280.26 Mbytes
Key events increasing vsize:
[0] run: 0 lumi: 0 event: 0 vsize = 0 deltaVsize = 0 rss = 0 delta = 0
[1] run: 1 lumi: 1 event: 1 vsize = 4296.21 deltaVsize = 0 rss = 3077.83 delta = 0
[3] run: 1 lumi: 1 event: 3 vsize = 5128.21 deltaVsize = 832 rss = 3166.95 delta = 89.1211
[88] run: 1 lumi: 1 event: 88 vsize = 6280.26 deltaVsize = 640 rss = 3146.59 delta = -20.3594
[12] run: 1 lumi: 1 event: 12 vsize = 5640.25 deltaVsize = 512.008 rss = 3204.43 delta = 37.4727
[90] run: 1 lumi: 1 event: 90 vsize = 6280.26 deltaVsize = 0 rss = 3078.7 delta = -67.8984
[89] run: 1 lumi: 1 event: 89 vsize = 6280.26 deltaVsize = 0 rss = 3203.86 delta = 57.2656
[88] run: 1 lumi: 1 event: 88 vsize = 6280.26 deltaVsize = 640 rss = 3146.59 delta = -34.625
TimeReport> Time report complete in 10911.9 seconds
Time Summary:

Min event: 80.4885
Max event: 128.658
Avg event: 107.631
Total loop: 10882.3
Total init: 29.3701
Total job: 10911.9
EventSetup Lock: 0.00013113
EventSetup Get: 83.4706
Event Throughput: 0.00918923 ev/s
CPU Summary:
Total loop: 9314.68
Total init: 13.381
Total extra: 0
Total job: 9328.3

step 3

MemoryReport> Peak virtual size 10161.6 Mbytes
Key events increasing vsize:
[8] run: 1 lumi: 1 event: 8 vsize = 9139.4 deltaVsize = 32.0586 rss = 6650.14 delta = 89.9492
[3] run: 1 lumi: 1 event: 3 vsize = 9107.34 deltaVsize = 900.121 rss = 6560.19 delta = 155.914
[42] run: 1 lumi: 1 event: 42 vsize = 10161.6 deltaVsize = 1022.13 rss = 6870.45 delta = 310.262
[0] run: 0 lumi: 0 event: 0 vsize = 0 deltaVsize = 0 rss = 0 delta = 0
[0] run: 0 lumi: 0 event: 0 vsize = 0 deltaVsize = 0 rss = 0 delta = 0
[44] run: 1 lumi: 1 event: 44 vsize = 10161.6 deltaVsize = 0 rss = 6714.96 delta = -155.488
[43] run: 1 lumi: 1 event: 43 vsize = 10161.6 deltaVsize = 0 rss = 6831.04 delta = -39.4141
[42] run: 1 lumi: 1 event: 42 vsize = 10161.6 deltaVsize = 1022.13 rss = 6870.45 delta = 68.0742
TimeReport> Time report complete in 24287.1 seconds
Time Summary:

Min event: 145.066
Max event: 412.86
Avg event: 237.186
Total loop: 24138
Total init: 138.612
Total job: 24287.1
EventSetup Lock: 0.000231981
EventSetup Get: 95.3623
Event Throughput: 0.00414284 ev/s
CPU Summary:
Total loop: 20984.7
Total init: 101.39
Total extra: 0
Total job: 21096.3

…f photon tails from earlier OOT hits

cmsbuild · 2019-11-20T11:28:51Z

The code-checks are being triggered in jenkins.

cmsbuild · 2019-11-20T11:35:02Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28433/12836

This PR adds an extra 28KB to repository

cmsbuild · 2019-11-20T11:35:34Z

A new Pull Request was created by @casarsa (Massimo Casarsa) for master.

It involves the following packages:

DataFormats/FTLDigi
SimFastTiming/FastTimingCommon

@cmsbuild, @civanch, @kpedro88, @mdhildreth can you please review it and eventually sign? Thanks.
@rovere this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

kpedro88 · 2019-11-20T14:03:22Z

please test

cmsbuild · 2019-11-20T14:03:53Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/3554/console Started: 2019/11/20 15:04

cmsbuild · 2019-11-20T15:15:40Z

+1
Tested at: 298bfcf
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-54131d/3554/summary.html

cmsbuild · 2019-11-20T15:15:46Z

Comparison job queued.

makortel · 2019-11-20T16:47:12Z

The premixing configuration has been updated accordingly.

The changes themselves look fine. Did you check the impact on the pileup library disk size?

cmsbuild · 2019-11-20T17:42:58Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-54131d/3554/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 9242 differences found in the comparisons
DQMHistoTests: Total files compared: 34
DQMHistoTests: Total histograms compared: 2936360
DQMHistoTests: Total failures: 5416
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2930603
DQMHistoTests: Total skipped: 341
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
Checked 147 log files, 16 edm output root files, 34 DQM output files

SimFastTiming/FastTimingCommon/src/BTLBarDeviceSim.cc

kpedro88 · 2019-11-21T19:29:37Z

SimFastTiming/FastTimingCommon/src/BTLBarDeviceSim.cc

+    double tL = std::get<2>(hitRef) + LightCollSlopeL_ * distL;
+
+    // --- Accumulate in 15 buckets of 25ns (9 pre-samples, 1 in-time, 5 post-samples)
+    const int iBXR = std::floor(tR / bxTime_) + 9;


magic number 9 should be named constant

I've introduced a named constant

kpedro88 · 2019-11-21T19:29:44Z

SimFastTiming/FastTimingCommon/src/BTLBarDeviceSim.cc

-    if ((simHitIt->second).hit_info[1][0] == 0 || tR < (simHitIt->second).hit_info[1][0])
-      (simHitIt->second).hit_info[1][0] = tR;
+    // --- Right side
+    if (iBXR > 0 && iBXR < 15) {


magic number 15 should be named constant

I've replaced 15 with a named constant

kpedro88 · 2019-11-21T19:30:12Z

SimFastTiming/FastTimingCommon/src/BTLElectronicsSim.cc

+      if (smearTimeForOOTtails_) {
+        float rate_oot = 0.;
+        // Loop on earlier OOT hits
+        for (unsigned int ibx = 0; ibx < 9; ++ibx) {


magic number 9

9 has been replaced with a named constant

kpedro88 · 2019-11-21T19:31:34Z

SimFastTiming/FastTimingCommon/src/BTLElectronicsSim.cc

+        for (unsigned int ibx = 0; ibx < 9; ++ibx) {
+          if ((it->second).hit_info[2 * iside][ibx] > 0.) {
+            float npe_oot = CLHEP::RandPoissonQ::shoot(hre, (it->second).hit_info[2 * iside][ibx]);
+            rate_oot += npe_oot * exp((it->second).hit_info[1 + 2 * iside][ibx] / ScintillatorDecayTime_) /


can slightly speed up the math here by storing ScintillatorDecayTimeInv_ = 1./ScintillatorDecayTime_ once and then multiplying by ScintillatorDecayTimeInv_ (rather than dividing, which is slower)

kpedro88 · 2019-11-21T19:32:06Z

SimFastTiming/FastTimingCommon/src/BTLTileDeviceSim.cc

@@ -84,14 +84,16 @@ void BTLTileDeviceSim::getHitsResponse(const std::vector<std::tuple<int, uint32_
    if (smearLightCollTime_ > 0.)
      toa += CLHEP::RandGaussQ::shoot(hre, 0., smearLightCollTime_);

-    if (toa > bxTime_ || toa < 0)  //just consider BX==0
+    // Accumulate in 15 buckets of 25ns (9 pre-samples, 1 in-time, 5 post-samples)
+    const int iBX = std::floor(toa / bxTime_) + 9;


magic number 9

Introduced named constant.

kpedro88 · 2019-11-21T19:32:16Z

SimFastTiming/FastTimingCommon/src/BTLTileDeviceSim.cc

-    if (toa > bxTime_ || toa < 0)  //just consider BX==0
+    // Accumulate in 15 buckets of 25ns (9 pre-samples, 1 in-time, 5 post-samples)
+    const int iBX = std::floor(toa / bxTime_) + 9;
+    if (iBX < 0 || iBX > 14)


magic number 14 = 15 - 1?

Rewritten with a named constant

cmsbuild · 2019-11-25T18:44:36Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-54131d/3620/summary.html

@slava77 comparisons for the following workflows were not done due to missing matrix map:

/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/results/JR-comparison/PR-54131d/20634.21_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2026D41PU_GenSimHLBeamSpotFull14+DigiFullTriggerPU_2026D41PU+RecoFullGlobalPU_ProdLike_2026D41PU+MiniAODFullGlobalPU_ProdLike_2026D41PU

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 9171 differences found in the comparisons
DQMHistoTests: Total files compared: 34
DQMHistoTests: Total histograms compared: 2787584
DQMHistoTests: Total failures: 5422
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2781821
DQMHistoTests: Total skipped: 341
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
Checked 147 log files, 16 edm output root files, 34 DQM output files

casarsa · 2019-11-26T11:32:57Z

From 100 events of WF 22261.97 with PU 200 I get:
Branch Name | Average Uncompressed Size (Bytes/Event) | Average Compressed Size (Bytes/Event)
PMTDSimAccumulator_mix_FTLBarrel_DIGI. 2.96946e+06 2.39309e+06
PMTDSimAccumulator_mix_FTLEndcap_DIGI. 1.33048e+06 775614

Thanks, so in total 3 MB/event (compressed). Could you also check the sizes without this PR? My previous estimate from 10_4_0_mtd5 is 1.1 MB/event, but that could be already out of date.

@makortel The estimate on 100 TTbar PU200 events with 11_0_X_2019-11-18-2300 w/o this PR is consistent with what you got:
PMTDSimAccumulator_mix_FTLEndcap_DIGI. 1.33048e+06 796149
PMTDSimAccumulator_mix_FTLBarrel_DIGI. 640898 393317

fabiocos · 2019-11-26T13:18:26Z

@pmeridian @parbol comments about this update? If we want it for the HLT TDR it should enter now.
In case there is the possibility to switch it off keeping the updated code.

kpedro88 · 2019-11-26T15:34:02Z

Is it possible to tune the size of the premixed library? Without MTD and without truth information, the size was previously estimated at 12.7 MB/evt (source). This change from 1 MB to 3 MB for MTD amounts to a ~15% increase in the overall library size, which is substantial.

makortel · 2019-11-26T15:36:23Z

From 100 events of WF 22261.97 with PU 200 I get:
Branch Name | Average Uncompressed Size (Bytes/Event) | Average Compressed Size (Bytes/Event)
PMTDSimAccumulator_mix_FTLBarrel_DIGI. 2.96946e+06 2.39309e+06
PMTDSimAccumulator_mix_FTLEndcap_DIGI. 1.33048e+06 775614

Thanks, so in total 3 MB/event (compressed). Could you also check the sizes without this PR? My previous estimate from 10_4_0_mtd5 is 1.1 MB/event, but that could be already out of date.

The estimate on 100 TTbar PU200 events with 11_0_X_2019-11-18-2300 w/o this PR is consistent with what you got:
PMTDSimAccumulator_mix_FTLEndcap_DIGI. 1.33048e+06 796149
PMTDSimAccumulator_mix_FTLBarrel_DIGI. 640898 393317

@casarsa Thanks! Increasing the compressed event size on disk from 1.1 MB to 3 MB is probably fine now. The total now is 45 MB, dominated by MC truth whose inclusion should become unnecessary. At that point the 3 MB/event could become something to be optimized (to compare, in 10_4_0_mtd5 HGCal consumed 8.5 MB/event (to be optimized as well), and tracker 2.8 MB/event. Run2 premixing at PU50 is 3.2 MB/event).

Ok, I see @kpedro88 already asked about reducing the size.

kpedro88 · 2019-11-26T17:27:04Z

+upgrade
size reduction in the premix library should be attempted in a followup PR

fabiocos · 2019-11-27T07:45:10Z

@civanch any further comment?

civanch · 2019-11-27T21:53:58Z

+1

cmsbuild · 2019-11-27T21:54:33Z

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)

fabiocos · 2019-11-28T08:44:36Z

+1

fabiocos · 2019-11-28T09:13:50Z

@casarsa @kpedro88 @makortel strictly speaking this PR causes the premix library to be backward incompatible as far as MTD is concerned. I doubt this is a real practical issue, especially when moving from 10_6_X to 11_0_X, but it is important to have it clear

kpedro88 · 2019-11-28T15:55:41Z

@fabiocos indeed, that should be fine because phase 2 premix libraries have not been centrally generated yet

fabiocos · 2019-11-28T16:10:22Z

@kpedro88 moving to a completely new release should be fine as well in my view, at the end of the day it is not a final data format but an intermediate technical one, to be discussed at least. Anyway I am afraid we have an issue with this PR and premix, as it is again crashing in the IB, I am making a cross check.

fabiocos · 2019-11-28T19:16:49Z

@casarsa @kpedro88 This is causing a crash in premixing workflow 20634.99 whose solution does not look entirely trivial. At this point, in order not to hold anymore the pre13 validation, I revert this PR to be possibly considered in a fix round after further careful validation.

casarsa added 3 commits November 19, 2019 11:42

Added a Gaussian smearing to the BTL time to account for the effect o…

6de9507

…f photon tails from earlier OOT hits

Fixed the premixer configuration

390d3f6

Removed commend

298bfcf

cmsbuild added this to the CMSSW_11_0_X milestone Nov 20, 2019

cmsbuild added code-checks-pending comparison-pending orp-pending pending-signatures simulation-pending tests-pending upgrade-pending labels Nov 20, 2019

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 20, 2019

cmsbuild added tests-started and removed tests-pending labels Nov 20, 2019

cmsbuild added tests-approved and removed tests-started labels Nov 20, 2019

cmsbuild added comparison-available and removed comparison-pending labels Nov 20, 2019

kpedro88 reviewed Nov 21, 2019

View reviewed changes

fabiocos mentioned this pull request Nov 22, 2019

Finalization of CMSSW_11_0_X open development #28455

Closed

9 tasks

cmsbuild added comparison-available and removed comparison-pending labels Nov 25, 2019

cmsbuild added upgrade-approved and removed upgrade-pending labels Nov 26, 2019

cmsbuild added fully-signed simulation-approved and removed pending-signatures simulation-pending labels Nov 27, 2019

cmsbuild added orp-approved and removed orp-pending labels Nov 28, 2019

cmsbuild merged commit d6ac250 into cms-sw:master Nov 28, 2019

casarsa deleted the mc-simBTL-OOT branch November 28, 2019 09:30

fabiocos mentioned this pull request Nov 28, 2019

Revert "BTL simulation: OOT effects" #28504

Merged

casarsa restored the mc-simBTL-OOT branch November 29, 2019 09:01

casarsa mentioned this pull request Jan 17, 2020

BTL simulation: OOT effects + premix fix #28757

Merged

casarsa deleted the mc-simBTL-OOT branch February 6, 2020 14:24

BTL simulation: OOT effects #28433

BTL simulation: OOT effects #28433

Conversation

casarsa commented Nov 20, 2019

PR description:

PR validation:

cmsbuild commented Nov 20, 2019

cmsbuild commented Nov 20, 2019

cmsbuild commented Nov 20, 2019

kpedro88 commented Nov 20, 2019

cmsbuild commented Nov 20, 2019 • edited

cmsbuild commented Nov 20, 2019

cmsbuild commented Nov 20, 2019

makortel commented Nov 20, 2019

cmsbuild commented Nov 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented Nov 25, 2019

casarsa commented Nov 26, 2019

fabiocos commented Nov 26, 2019

kpedro88 commented Nov 26, 2019

makortel commented Nov 26, 2019

kpedro88 commented Nov 26, 2019

fabiocos commented Nov 27, 2019

civanch commented Nov 27, 2019

cmsbuild commented Nov 27, 2019

fabiocos commented Nov 28, 2019

fabiocos commented Nov 28, 2019

kpedro88 commented Nov 28, 2019

fabiocos commented Nov 28, 2019

fabiocos commented Nov 28, 2019

cmsbuild commented Nov 20, 2019 •

edited