Reduced radom number calls in the runSimple Method of HGCDigitizerBase for Potential Speedup. #27980
Conversation
|
The code-checks are being triggered in jenkins. |
adas1994
changed the title
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27980/11861
|
|
A new Pull Request was created by @adas1994 for master. It involves the following packages: SimCalorimetry/HGCalSimProducers @cmsbuild, @civanch, @kpedro88, @mdhildreth can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
1 similar comment
|
please test |
|
The tests are being triggered in jenkins. |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
| @@ -22,7 +22,7 @@ | |||
| #include "Geometry/HcalTowerAlgo/interface/HcalGeometry.h" | |||
|
|
|||
| #include "SimCalorimetry/HGCalSimAlgos/interface/HGCalSiNoiseMap.h" | |||
|
|
|||
| #include "TRandom.h" | |||
There was a problem hiding this comment.
@adas1994 why aren't you using the framework random number generator service?
| void HGCDigitizerBase<DFr>::GenerateGaussianNoise(const double NoiseMean, const double NoiseStd) { | ||
| unsigned int seed = 123456; | ||
| seed = seed + SeedOffset_; | ||
| TRandom trandom(seed); |
There was a problem hiding this comment.
@adas1994 why not using the framework service based on the defined engine, i.e. move here CLHEP::RandGaussQ::shoot(engine, 0.0, noiseWidth) ? Furthermore TRandom looks the generator of poorest quality in the ROOT family according to the documentation The generator provided in TRandom itself is a LCG (Linear Congruential Generator), the BSD rand generator, that it should not be used because its period is only 2**31, i.e. approximatly 2 billion events, that can be generated in just few seconds.
There was a problem hiding this comment.
( @fabiocos )We were trying to see, if we can get to make HGCal simulate similar noise using random numbers from a much smaller pool of Gaussian random numbers and not generating a whole bunch of them for every channel for every event.
Consider this, at the end of the day, it is all just a bunch of Normal/Gaussian random numbers. And we are not generating billions of them so that they could cause problem due to its limited period.
There was a problem hiding this comment.
@adas1994 that has nothing to do with which pRNG should be used. Similar comments had been made on previous versions of this PR. Please use the CMSSW random number service exclusively.
There was a problem hiding this comment.
@kpedro88 @fabiocos The problem is that what needs to happen is that the noise collection needs to be generated in the constructor, before there are any streams to refer to. Here, the noise itself is generated with a fixed seed, so it is completely reproducible. The random number service is used later to choose noise values from this fixed array, so that is thread safe.
There was a problem hiding this comment.
@mdhildreth I imagine that what you need here is to have a predefined noise collection before the first event noise is produced, which does not necessarily mean within the constructor. I understand the rationale behind this strategy, but I also see risks:
-
is this collection big enough to avoid reusing too frequently the same noise and create artificial correlations?
-
although strictly speaking your noise is reproducible reusing the same configuration, the change of random numbers among jobs in production are managed through the framework service, nobody will manually randomize the starting seed offset in your configuration, so you will end up in each single job with exactly the same noise, which I doubt is what we want. The configurable offsets will be practically useless, and you might probably structure things so as the noise array is static since every instance will end up using the same.
Provided this is really the way you want to go I can imagine a strategy to initialise the noise array within runSimple at the first function call, to be done with care in a multi-thread safe way. That would be a clean way to rely on the random number service at least, although reusing the same library.
There was a problem hiding this comment.
@fabiocos The idea is to make the library big enough so that it doesn't matter. It's just generating low-level single-channel noise, over millions of channels, using a gaussian distribution. We should make it big enough so it can't be seen in physics validation plots, of course. In terms of the random seed variation: it doesn't matter if this is randomized if the collection is big enough. Each event gets an appropriate random entry into the library in a thread-safe way. This should be sufficient.
We can look at runSimple to see if this is a better way. Thanks.
| TRandom trandom(seed); | ||
| for (size_t i = 0; i < NoiseArrayLength_; i++) { | ||
| for (size_t j = 0; j < samplesize_; j++) { | ||
| GaussianNoiseArray_[i][j] = trandom.Gaus(NoiseMean, NoiseStd); |
There was a problem hiding this comment.
in principle there is also the fireArray method that can be used
There was a problem hiding this comment.
I have stored the random numbers generated by old method and hashed by our new method right before they are being used in runShaperToT and plotted a random sample from them to visualize any possible bias. I found these :
.
|
I might have got a middle ground here, using CMS Random number generator
service to do it's job and also avoiding generating gaussian noise for
every event for every channel. I will commit my changes soon enough and see
if that works fine.
thanks,
…On Fri, Sep 13, 2019 at 5:07 AM Fabio Cossutti ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In SimCalorimetry/HGCalSimProducers/src/HGCDigitizerBase.cc
<#27980 (comment)>:
> @@ -51,6 +52,19 @@ HGCDigitizerBase<DFr>::HGCDigitizerBase(const edm::ParameterSet& ps) : scaleByDo
edm::ParameterSet feCfg = myCfg_.getParameter<edm::ParameterSet>("feCfg");
myFEelectronics_ = std::unique_ptr<HGCFEElectronics<DFr>>(new HGCFEElectronics<DFr>(feCfg));
myFEelectronics_->SetNoiseValues(noise_fC_);
+ GenerateGaussianNoise(NoiseMean_, NoiseStd_);
+}
+
+template <class DFr>
+void HGCDigitizerBase<DFr>::GenerateGaussianNoise(const double NoiseMean, const double NoiseStd) {
+ unsigned int seed = 123456;
+ seed = seed + SeedOffset_;
+ TRandom trandom(seed);
@mdhildreth <https://github.com/mdhildreth> I imagine that what you need
here is to have a predefined noise collection before the first event noise
is produced, which does not necessarily mean within the constructor. I
understand the rationale behind this strategy, but I also see risks:
-
is this collection big enough to avoid reusing too frequently the same
noise and create artificial correlations?
-
although strictly speaking your noise is reproducible reusing the same
configuration, the change of random numbers among jobs in production are
managed through the framework service, nobody will manually randomize the
starting seed offset in your configuration, so you will end up in each
single job with exactly the same noise, which I doubt is what we want. The
configurable offsets will be practically useless, and you might probably
structure things so as the noise array is static since every instance will
end up using the same.
Provided this is really the way you want to go I can imagine a strategy to
initialise the noise array within runSimple at the first function call,
to be done with care in a multi-thread safe way. That would be a clean way
to rely on the random number service at least, although reusing the same
library.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#27980?email_source=notifications&email_token=AG64M4PN3Q7QDPGLFROVRLDQJNJ57A5CNFSM4IVWYBSKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCEUPCYI#discussion_r324101449>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AG64M4MBHHSC2N6ZYF2DMA3QJNJ57ANCNFSM4IVWYBSA>
.
--
Sincerely,
Abhishek Das.
|
|
This is a commit with a python configurable switch to decide which random noise generation method to use; old or modified method. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27980/11937
|
|
Pull request #27980 was updated. @cmsbuild, @civanch, @kpedro88, @mdhildreth can you please check and sign again. |
|
please test |
|
The tests are being triggered in jenkins. |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
Hi Kevin,
I have 2 quick queries -
1. Any update/thought on this PR?
2. Also, Prof. Hildreth asked me to ask you if there are other areas on
HGCal that needs to be worked on and we could try to contribute.
thanks,
…On Thu, Sep 19, 2019 at 3:19 PM cmsbuild ***@***.***> wrote:
Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6919f9/2578/summary.html
Comparison Summary:
- No significant changes to the logs found
- Reco comparison results: 9545 differences found in the comparisons
- DQMHistoTests: Total files compared: 34
- DQMHistoTests: Total histograms compared: 2957336
- DQMHistoTests: Total failures: 9354
- DQMHistoTests: Total nulls: 0
- DQMHistoTests: Total successes: 2947641
- DQMHistoTests: Total skipped: 341
- DQMHistoTests: Total Missing objects: 0
- DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
- Checked 145 log files, 15 edm output root files, 34 DQM output files
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#27980?email_source=notifications&email_token=AG64M4IQAZ3GXDENBGX6VT3QKPGFPA5CNFSM4IVWYBSKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7ERHMI#issuecomment-533271473>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AG64M4NIH7DNQWDOHGUKS4TQKPGFPANCNFSM4IVWYBSA>
.
--
Sincerely,
Abhishek Das.
|
|
+1 |
|
+upgrade |
cmsbuild
added
fully-signed
upgrade-approved
and removed
pending-signatures
upgrade-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, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
PR description:
The runSimple member function of HGCDigitizerBase.cc contains SIMD-type instructions which runs on millions of channels, causing the whole digitization step to move slowly. I identified several conditional branches(if statements) and replaced them with boolean variables in order to limit the number of branching. This helps compiler to autovectorize the code to gain potential speedup.
PR validation:
I used workflow number 21234.0_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2026D44_GenSimHLBeamSpotFull14+DigiFullTrigger_2026D44+RecoFullGlobal_2026D44+HARVESTFullGlobal_2026D44
if this PR is a backport please specify the original PR:
Before submitting your pull requests, make sure you followed this checklist: