Decoupling charge reweighting from SiPixelDigitizerAlgorithm #31622

carolinecollard · 2020-09-30T09:08:44Z

PR description:

Add a new class (SiPixelChargeReweightingAlgorithm) to extract the charge reweighting functions from SiPixelDigitizerAlgorithm.
Correct a typo in SiPixelDigitizerAlgorithm.

These are requested by the DPG Tracker conveners and the Pixel Offline conveners

PR validation:

Two tests have been run:
runTheMatrix.py -l 11834.0 --command='-n 100'
runTheMatrix.py -l 11834.99 --command='-n 100'

Results comparing the default one with the proposed ("decouple") version: there is perfect agreement.
---> https://tinyurl.com/y3gctmqs for standard mixing
---> https://tinyurl.com/y6pzmu8r for pre-mixing

…ng typo

cmsbuild · 2020-09-30T09:09:10Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-09-30T09:14:39Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31622/18691

This PR adds an extra 44KB to repository

cmsbuild · 2020-09-30T09:15:08Z

A new Pull Request was created by @carolinecollard for master.

It involves the following packages:

SimTracker/SiPixelDigitizer

@cmsbuild, @civanch, @mdhildreth can you please review it and eventually sign? Thanks.
@makortel, @mtosi, @GiacomoSguazzoni, @JanFSchulte, @rovere, @VinInn, @dkotlins, @ferencek, @ebrondol, @threus, @dgulhan, @tvami this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

mmusich

Thanks @carolinecollard!
I have just a couple of minor comments, see below.

mmusich · 2020-09-30T09:19:47Z

SimTracker/SiPixelDigitizer/plugins/SiPixelChargeReweightingAlgorithm.cc

+//class SiPixelChargeReweightingAlgorithm SimTracker/SiPixelDigitizer/src/SiPixelChargeReweightingAlgorithm.cc
+
+// Original Author Caroline Collard
+// September 2020 : Extraction of the code for cluster charge reweighting from SiPixelDigitizerAlgoithm to a new class


minor typo: "SiPixelDigitizerAlgoithm" -> "SiPixelDigitizerAlgorithm"

ok. corrected.

mmusich · 2020-09-30T09:26:33Z

SimTracker/SiPixelDigitizer/plugins/SiPixelChargeReweightingAlgorithm.h

+
+#include "SimTracker/SiPixelDigitizer/plugins/SiPixelDigitizerAlgorithm.h"
+
+typedef boost::multi_array<float, 2> array_2d;


is this needed as it's already defined at line 63?

Line 6 is not needed --> removed.
But line 4 is really needed because it is used for
typedef std::map<int, SiPixelDigitizerAlgorithm::Amplitude, std::less > signal_map_type;
on new line 42.

mmusich · 2020-09-30T09:52:31Z

@emiglior FYI

cmsbuild · 2020-09-30T10:35:07Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-09-30T10:40:59Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31622/18695

This PR adds an extra 44KB to repository

cmsbuild · 2020-09-30T10:41:21Z

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

civanch · 2020-09-30T17:54:27Z

SimTracker/SiPixelDigitizer/plugins/SiPixelChargeReweightingAlgorithm.cc

+      UseReweighting(conf.getParameter<bool>("UseReweighting")),
+      PrintClusters(conf.getParameter<bool>("PrintClusters")),
+      PrintTemplates(conf.getParameter<bool>("PrintTemplates")) {
+  LogInfo("PixelDigitizer ") << "SiPixelChargeReweightingAlgorithm constructed"


@carolinecollard , better to use edm::LogVerbatim

ok. I am changing it. I will update on git in a few minutes.

civanch · 2020-09-30T18:03:27Z

SimTracker/SiPixelDigitizer/plugins/SiPixelChargeReweightingAlgorithm.cc

+  }
+
+  if (PrintClusters) {
+    std::cout << "Cluster after reweighting: " << std::endl;


use #ifdef TP_DEBUG or LogDEbug

@civanch I disagree with this proposal. The purpose of the "PrintClusters" bool on line 303 (and 326 on your next comment) is to activate specifically this print out. By default this variable is set to false in the python, so there is no risk to get print out messages in standard running. It was coded like that by the original author when it was included in SiPixelDigitizerAlgorithm.cc. I prefer not to change.

civanch · 2020-09-30T18:03:51Z

SimTracker/SiPixelDigitizer/plugins/SiPixelChargeReweightingAlgorithm.cc

+  }
+
+  if (PrintClusters) {
+    std::cout << std::endl;


the same cout

see previous reply.

cmsbuild · 2020-10-02T07:57:37Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-10-02T08:03:39Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31622/18739

This PR adds an extra 48KB to repository

cmsbuild · 2020-10-02T08:04:06Z

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

civanch · 2020-10-02T08:29:08Z

please test

cmsbuild · 2020-10-02T08:29:34Z

The tests are being triggered in jenkins.

CMSSW_11_2_X_2020-10-01-2300/slc7_amd64_gcc820: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/9702/console Started: 2020/10/02 10:30

cmsbuild · 2020-10-02T09:40:51Z

+1
Tested at: 74cf769
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-663c26/9702/summary.html
CMSSW: CMSSW_11_2_X_2020-10-01-2300
SCRAM_ARCH: slc7_amd64_gcc820

cmsbuild · 2020-10-02T09:40:53Z

Comparison job queued.

cmsbuild · 2020-10-02T11:10:00Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 35
DQMHistoTests: Total histograms compared: 2542225
DQMHistoTests: Total failures: 7
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2542196
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
Checked 149 log files, 22 edm output root files, 35 DQM output files

civanch · 2020-10-02T13:55:23Z

+1

cmsbuild · 2020-10-02T13:55:44Z

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

silviodonato · 2020-10-02T15:18:38Z

@carolinecollard I know cout was already in the code before this PR. Howver, would you mind to move from cout to edm::LogInfo or something like that (see https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideMessageLogger ) ? std::cout is not thread safe, so we should move away from it (see cms-sw/cms-sw.github.io#94)

carolinecollard · 2020-10-03T15:37:05Z

@silviodonato Ok, I understand. Unfortunately, I will have no time to make any change before October 12.

silviodonato · 2020-10-03T15:53:11Z

silviodonato · 2020-10-03T15:53:20Z

+1

carolinecollard added 2 commits September 30, 2020 10:11

Decoupling Charge Reweighting from SiPixelDigitizerAlgorithm and Fixi…

4ad5e4b

…ng typo

code-checks & code-format

0e4e035

cmsbuild added this to the CMSSW_11_2_X milestone Sep 30, 2020

cmsbuild added code-checks-pending comparison-pending orp-pending pending-signatures simulation-pending tests-pending labels Sep 30, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 30, 2020

mmusich reviewed Sep 30, 2020

View reviewed changes

correcting typo and removing duplicated definition

ec36bc7

cmsbuild added code-checks-pending and removed code-checks-approved labels Sep 30, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 30, 2020

civanch reviewed Sep 30, 2020

View reviewed changes

changing LogInfo by LogVerbatim

f7f4a6f

cmsbuild added code-checks-pending and removed code-checks-approved labels Oct 2, 2020

cmsbuild removed the code-checks-pending label Oct 2, 2020

cmsbuild added the code-checks-approved label Oct 2, 2020

cmsbuild added tests-started and removed tests-pending labels Oct 2, 2020

cmsbuild added tests-approved and removed tests-started labels Oct 2, 2020

cmsbuild added comparison-available and removed comparison-pending labels Oct 2, 2020

cmsbuild added fully-signed simulation-approved and removed pending-signatures simulation-pending labels Oct 2, 2020

cmsbuild added orp-approved and removed orp-pending labels Oct 3, 2020

cmsbuild merged commit 92e2e87 into cms-sw:master Oct 3, 2020

silviodonato mentioned this pull request Oct 22, 2020

Changes in Pixel 2D template making code cfg for Phase-2 + adding a unit test #31796

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decoupling charge reweighting from SiPixelDigitizerAlgorithm #31622

Decoupling charge reweighting from SiPixelDigitizerAlgorithm #31622

carolinecollard commented Sep 30, 2020

cmsbuild commented Sep 30, 2020

cmsbuild commented Sep 30, 2020

cmsbuild commented Sep 30, 2020

mmusich left a comment

mmusich Sep 30, 2020

carolinecollard Sep 30, 2020

mmusich Sep 30, 2020

carolinecollard Sep 30, 2020

mmusich commented Sep 30, 2020

cmsbuild commented Sep 30, 2020

cmsbuild commented Sep 30, 2020

cmsbuild commented Sep 30, 2020

civanch Sep 30, 2020

carolinecollard Oct 2, 2020

civanch Sep 30, 2020

carolinecollard Oct 2, 2020

civanch Sep 30, 2020

carolinecollard Oct 2, 2020

cmsbuild commented Oct 2, 2020

cmsbuild commented Oct 2, 2020

cmsbuild commented Oct 2, 2020

civanch commented Oct 2, 2020

cmsbuild commented Oct 2, 2020 •

edited

Loading

cmsbuild commented Oct 2, 2020

cmsbuild commented Oct 2, 2020

cmsbuild commented Oct 2, 2020

civanch commented Oct 2, 2020

cmsbuild commented Oct 2, 2020

silviodonato commented Oct 2, 2020

carolinecollard commented Oct 3, 2020

silviodonato commented Oct 3, 2020

silviodonato commented Oct 3, 2020


		#include "SimTracker/SiPixelDigitizer/plugins/SiPixelDigitizerAlgorithm.h"

		typedef boost::multi_array<float, 2> array_2d;

Decoupling charge reweighting from SiPixelDigitizerAlgorithm #31622

Decoupling charge reweighting from SiPixelDigitizerAlgorithm #31622

Conversation

carolinecollard commented Sep 30, 2020

PR description:

PR validation:

cmsbuild commented Sep 30, 2020

cmsbuild commented Sep 30, 2020

cmsbuild commented Sep 30, 2020

mmusich left a 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

mmusich commented Sep 30, 2020

cmsbuild commented Sep 30, 2020

cmsbuild commented Sep 30, 2020

cmsbuild commented Sep 30, 2020

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 Oct 2, 2020

cmsbuild commented Oct 2, 2020

cmsbuild commented Oct 2, 2020

civanch commented Oct 2, 2020

cmsbuild commented Oct 2, 2020 • edited Loading

cmsbuild commented Oct 2, 2020

cmsbuild commented Oct 2, 2020

cmsbuild commented Oct 2, 2020

civanch commented Oct 2, 2020

cmsbuild commented Oct 2, 2020

silviodonato commented Oct 2, 2020

carolinecollard commented Oct 3, 2020

silviodonato commented Oct 3, 2020

silviodonato commented Oct 3, 2020

cmsbuild commented Oct 2, 2020 •

edited

Loading