Ph2 TK Digitizer software updated for RandomNumberGenerator service #22836

suchandradutta · 2018-04-04T10:18:48Z

The usage of edm::RandomNumberGenerator reviewed following the comment in [1]. We are now generating random numbers in each event independently. The code is accordingly updated.
@makortel, @boudoul, @emiglior

[1] #22710 (comment)

cmsbuild · 2018-04-04T10:19:06Z

The code-checks are being triggered in jenkins.

cmsbuild · 2018-04-04T10:21:20Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22836/4223

cmsbuild · 2018-04-04T10:21:39Z

A new Pull Request was created by @suchandradutta (Suchandra Dutta) for master.

It involves the following packages:

SimTracker/SiPhase2Digitizer

@cmsbuild, @civanch, @kpedro88, @mdhildreth can you please review it and eventually sign? Thanks.
@makortel, @GiacomoSguazzoni, @rovere, @VinInn, @ebrondol, @threus, @dgulhan 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

boudoul · 2018-04-04T10:33:21Z

Thanks @suchandradutta , Could you please edit the title mentioning that this is a fix in the random generator usage in the phase2 tracker digitizer to be a bit more explicit ? Also could you please backport to 93X ? Thank you again

suchandradutta · 2018-04-04T11:13:15Z

@boudoul yes I shall back port it for 93X once the PR is reviewed and accepted. I hope you agree.

boudoul · 2018-04-04T11:16:47Z

sure, thanks

civanch · 2018-04-04T11:20:45Z

SimTracker/SiPhase2Digitizer/plugins/Phase2TrackerDigitizerAlgorithm.cc

+void Phase2TrackerDigitizerAlgorithm::initializeEvent(CLHEP::HepRandomEngine* eng) {
+  if (addNoise || AddPixelInefficiency || fluctuateCharge || addThresholdSmearing) {
+
+    flatDistribution_ = std::unique_ptr<CLHEP::RandFlat>(new CLHEP::RandFlat(*eng, 0., 1.));


@suchandradutta , it is recommended to use a constructor CLHEP::RandFlat(eng,0.,1.), however, this is also an overhead. It is enough to use use "rengine_->flat(); flatDistribution_ is not needed.

civanch · 2018-04-04T11:23:11Z

SimTracker/SiPhase2Digitizer/plugins/Phase2TrackerDigitizerAlgorithm.h

@@ -200,12 +198,12 @@ class Phase2TrackerDigitizerAlgorithm  {
  const SubdetEfficiencies subdetEfficiencies_;

  // For random numbers
-  const std::unique_ptr<CLHEP::RandFlat> flatDistribution_;
-  const std::unique_ptr<CLHEP::RandGaussQ> gaussDistribution_;
+  std::unique_ptr<CLHEP::RandFlat> flatDistribution_;


this is not needed to keep flatDistribution_

makortel

@suchandradutta Thanks! I gave some suggestions for further improvements (all not necessarily for this PR).

makortel · 2018-04-04T11:21:57Z

SimTracker/SiPhase2Digitizer/plugins/Phase2TrackerDigitizer.cc

+    algomap_[AlgorithmType::InnerPixel] = std::unique_ptr<Phase2TrackerDigitizerAlgorithm>(new PixelDigitizerAlgorithm(iconfig_));
+    algomap_[AlgorithmType::PixelinPS]  = std::unique_ptr<Phase2TrackerDigitizerAlgorithm>(new PSPDigitizerAlgorithm(iconfig_));
+    algomap_[AlgorithmType::StripinPS]  = std::unique_ptr<Phase2TrackerDigitizerAlgorithm>(new PSSDigitizerAlgorithm(iconfig_));
+    algomap_[AlgorithmType::TwoStrip]   = std::unique_ptr<Phase2TrackerDigitizerAlgorithm>(new SSDigitizerAlgorithm(iconfig_));


Right-hand side could be written more shortly as std::make_unique<PixelDigitizerAlgorithm>(iConfig); etc.

On a more general note (possibly going beyond this PR)

algomap_ could be implemented as std::vector (consumes less memory, faster access)

iconfig_ could now be removed if isOTreadoutAnalog parameter would be read to member variable

@suchandradutta Could you actually remove the iconfig_ in this PR? It will have to be either removed or changed to a regular member (not reference, so consumes memory) for premixing. Thanks.

makortel · 2018-04-04T11:23:12Z

SimTracker/SiPhase2Digitizer/plugins/Phase2TrackerDigitizer.cc

  void Phase2TrackerDigitizer::endLuminosityBlock(edm::LuminosityBlock const& lumi, edm::EventSetup const& iSetup) {
-    algomap_.clear();
+    edm::LogInfo("Phase2TrackerDigitizer") << "End Luminosity Block";


Please remove this function as it does nothing useful anymore.

makortel · 2018-04-04T11:24:55Z

SimTracker/SiPhase2Digitizer/plugins/Phase2TrackerDigitizer.cc

  Phase2TrackerDigitizer::~Phase2TrackerDigitizer() {  
    edm::LogInfo("Phase2TrackerDigitizer") << "Destroying the Digitizer";
+    algomap_.clear();


Calling clear() is unnecessary (algomap_'s destructor will clean up all its contents automatically).

makortel · 2018-04-04T11:27:47Z

SimTracker/SiPhase2Digitizer/plugins/Phase2TrackerDigitizerAlgorithm.cc

+  flatDistribution_.reset(nullptr);
+  gaussDistribution_.reset(nullptr);
+  smearedThreshold_Endcap_.reset(nullptr);
+  smearedThreshold_Barrel_.reset(nullptr);  


These are not needed, unique_ptr default is nullptr.

makortel · 2018-04-04T11:29:07Z

SimTracker/SiPhase2Digitizer/plugins/Phase2TrackerDigitizerAlgorithm.cc

+  // Threshold smearing with gaussian distribution:
+  if (addThresholdSmearing) {
+    smearedThreshold_Endcap_ =  std::unique_ptr<CLHEP::RandGaussQ> (new CLHEP::RandGaussQ(eng, theThresholdInE_Endcap , theThresholdSmearing_Endcap));
+    smearedThreshold_Barrel_ = std::unique_ptr<CLHEP::RandGaussQ> (new CLHEP::RandGaussQ(eng, theThresholdInE_Barrel , theThresholdSmearing_Barrel));


std::make_unique<T>() could be used here as well (if these are still needed, ref. @civanch's comment #22836 (comment)).

kpedro88 · 2018-04-04T12:57:39Z

@boudoul is there a specific need for this in 93X?

boudoul · 2018-04-04T14:54:47Z

hi @kpedro88 , I do not have any particularly need. However when I do see in the description of the issue #22710 (adressed by this PR) how not safe it is (with many details explained in this issue) and knowing that 93X is intensively used for MC prod, I would feel better if this fix is also propagated there.

kpedro88 · 2018-04-04T14:58:35Z

Okay, I hadn't read #22710. At some point we plan to leave 9_3_X behind, so unless UPSG is going to stop current production to wait for this, I'm not sure if a backport will get merged. Well, we can still make the backport PR so we have it if needed.

civanch · 2018-04-04T17:47:34Z

Is there the same issue in run-2 Digi, or only Phase2?

makortel · 2018-04-04T17:59:32Z

@civanch No, this issue is specific to phase2.

…izer

cmsbuild · 2018-04-07T02:50:13Z

The code-checks are being triggered in jenkins.

cmsbuild · 2018-04-11T17:03:16Z

Pull request #22836 was updated. @cmsbuild, @civanch, @kpedro88, @mdhildreth can you please check and sign again.

kpedro88 · 2018-04-11T17:25:20Z

please test

cmsbuild · 2018-04-11T17:25:40Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/27442/console Started: 2018/04/11 19:27

cmsbuild · 2018-04-11T18:30:12Z

+1
Tested at: 451bbca
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-22836/27442/summary.html

cmsbuild · 2018-04-11T18:30:17Z

Comparison job queued.

cmsbuild · 2018-04-11T19:18:15Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 6263 differences found in the comparisons
DQMHistoTests: Total files compared: 29
DQMHistoTests: Total histograms compared: 2505375
DQMHistoTests: Total failures: 4920
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2500279
DQMHistoTests: Total skipped: 176
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.500000000131 KiB( 23 files compared)
Checked 119 log files, 9 edm output root files, 29 DQM output files

kpedro88 · 2018-04-11T20:57:28Z

Changes throughout Phase 2 workflows due to difference in random number usage, consistent with statistical fluctuations

kpedro88 · 2018-04-11T20:57:38Z

+1

civanch · 2018-04-12T16:34:14Z

+1

cmsbuild · 2018-04-12T16:34:36Z

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 · 2018-04-13T09:34:00Z

@suchandradutta @makortel as far as I can see this PR is complementary to #22874 and not conflicting, right?

makortel · 2018-04-13T09:36:36Z

@fabiocos Yes, they are independent.

fabiocos · 2018-04-13T17:13:48Z

+1

Updating Phase2Digitizer code with HepRandomEngine in per-event mode

4e577e6

cmsbuild added this to the CMSSW_10_2_X milestone Apr 4, 2018

cmsbuild added code-checks-pending comparison-pending orp-pending pending-signatures simulation-pending tests-pending upgrade-pending labels Apr 4, 2018

cmsbuild added code-checks-approved and removed code-checks-pending labels Apr 4, 2018

suchandradutta changed the title ~~Phase2 Tracker Digitizer software updated~~ Ph2 TK Digitizer software updated for RandomNumberGenerator service Apr 4, 2018

civanch reviewed Apr 4, 2018

View reviewed changes

makortel reviewed Apr 4, 2018

View reviewed changes

ParameterSet (iconfig_) no longer a data member of Phase2TrackerDigit…

8666ae7

…izer

cmsbuild added code-checks-pending and removed code-checks-approved labels Apr 7, 2018

cmsbuild added the code-checks-approved label Apr 11, 2018

cmsbuild added tests-started and removed tests-pending labels Apr 11, 2018

cmsbuild added tests-approved and removed tests-started labels Apr 11, 2018

cmsbuild added comparison-available and removed comparison-pending labels Apr 11, 2018

cmsbuild added upgrade-approved and removed upgrade-pending labels Apr 11, 2018

cmsbuild added fully-signed simulation-approved and removed pending-signatures simulation-pending labels Apr 12, 2018

cmsbuild added orp-approved and removed orp-pending labels Apr 13, 2018

cmsbuild merged commit b9fc2c5 into cms-sw:master Apr 13, 2018

makortel mentioned this pull request Apr 16, 2018

Use of edm::RandomNumberGenerator in Phase2TrackerDigitizer is not safe #22710

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ph2 TK Digitizer software updated for RandomNumberGenerator service #22836

Ph2 TK Digitizer software updated for RandomNumberGenerator service #22836

suchandradutta commented Apr 4, 2018

cmsbuild commented Apr 4, 2018

cmsbuild commented Apr 4, 2018

cmsbuild commented Apr 4, 2018

boudoul commented Apr 4, 2018

suchandradutta commented Apr 4, 2018

boudoul commented Apr 4, 2018

civanch Apr 4, 2018 •

edited

civanch Apr 4, 2018

makortel left a comment

makortel Apr 4, 2018

makortel Apr 6, 2018

makortel Apr 4, 2018

makortel Apr 4, 2018

makortel Apr 4, 2018

makortel Apr 4, 2018

kpedro88 commented Apr 4, 2018

boudoul commented Apr 4, 2018

kpedro88 commented Apr 4, 2018

civanch commented Apr 4, 2018

makortel commented Apr 4, 2018

cmsbuild commented Apr 7, 2018

cmsbuild commented Apr 11, 2018

kpedro88 commented Apr 11, 2018

cmsbuild commented Apr 11, 2018 •

edited

cmsbuild commented Apr 11, 2018

cmsbuild commented Apr 11, 2018

cmsbuild commented Apr 11, 2018

kpedro88 commented Apr 11, 2018

kpedro88 commented Apr 11, 2018

civanch commented Apr 12, 2018

cmsbuild commented Apr 12, 2018

fabiocos commented Apr 13, 2018

makortel commented Apr 13, 2018

fabiocos commented Apr 13, 2018

Ph2 TK Digitizer software updated for RandomNumberGenerator service #22836

Ph2 TK Digitizer software updated for RandomNumberGenerator service #22836

Conversation

suchandradutta commented Apr 4, 2018

cmsbuild commented Apr 4, 2018

cmsbuild commented Apr 4, 2018

cmsbuild commented Apr 4, 2018

boudoul commented Apr 4, 2018

suchandradutta commented Apr 4, 2018

boudoul commented Apr 4, 2018

civanch Apr 4, 2018 • edited

Choose a reason for hiding this comment

civanch Apr 4, 2018

Choose a reason for hiding this comment

makortel left a comment

Choose a reason for hiding this comment

makortel Apr 4, 2018

Choose a reason for hiding this comment

makortel Apr 6, 2018

Choose a reason for hiding this comment

makortel Apr 4, 2018

Choose a reason for hiding this comment

makortel Apr 4, 2018

Choose a reason for hiding this comment

makortel Apr 4, 2018

Choose a reason for hiding this comment

makortel Apr 4, 2018

Choose a reason for hiding this comment

kpedro88 commented Apr 4, 2018

boudoul commented Apr 4, 2018

kpedro88 commented Apr 4, 2018

civanch commented Apr 4, 2018

makortel commented Apr 4, 2018

cmsbuild commented Apr 7, 2018

cmsbuild commented Apr 11, 2018

kpedro88 commented Apr 11, 2018

cmsbuild commented Apr 11, 2018 • edited

cmsbuild commented Apr 11, 2018

cmsbuild commented Apr 11, 2018

cmsbuild commented Apr 11, 2018

kpedro88 commented Apr 11, 2018

kpedro88 commented Apr 11, 2018

civanch commented Apr 12, 2018

cmsbuild commented Apr 12, 2018

fabiocos commented Apr 13, 2018

makortel commented Apr 13, 2018

fabiocos commented Apr 13, 2018

civanch Apr 4, 2018 •

edited

cmsbuild commented Apr 11, 2018 •

edited