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

Implimentation of Dual-Slope signal scaling for the Inner Pixel #24325

Merged

Conversation

suchandradutta
Copy link
Contributor

Updating Phase2TrackerDigitizerAlgorithm with the option of dual slope scaling of signal to be used for the inner pixel detector. A new monitoring histogram added in one of the analyzers in the validation suite (Phase2TrackerMonitorDigi).

The update was present in the Phase2 tracker simulation meeting
https://indico.cern.ch/event/688845/contributions/3026531/attachments/1660515/2660028/DualSlopeCorrection_May30.pdf

NB: There will be a similar PR for the Phase2 clusterizer both these PRs should be included together

@emiglior @avartak @boudoul

Suchandra added 2 commits August 9, 2018 13:40
…e scaling of signal to be used for the inner pixel detector. A new monitoinghistogram added in the analyzer Phase2TrackerMonitorDigi
…hreshold subtraction and keeping the TDR option with a specific value of the flag
@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

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

@civanch
Copy link
Contributor

civanch commented Aug 20, 2018

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 20, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/29929/console Started: 2018/08/20 16:52

@cmsbuild
Copy link
Contributor

-1

Tested at: 702fda5

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-24325/29929/summary.html

I found follow errors while testing this PR

Failed tests: RelVals

  • RelVals:

When I ran the RelVals I found an error in the following worklfows:
20034.0 step2

runTheMatrix-results/20034.0_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2023D17_GenSimHLBeamSpotFull14+DigiFullTrigger_2023D17+RecoFullGlobal_2023D17+HARVESTFullGlobal_2023D17/step2_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2023D17_GenSimHLBeamSpotFull14+DigiFullTrigger_2023D17+RecoFullGlobal_2023D17+HARVESTFullGlobal_2023D17.log

20434.0 step2
runTheMatrix-results/20434.0_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2023D19_GenSimHLBeamSpotFull14+DigiFullTrigger_2023D19+RecoFullGlobal_2023D19+HARVESTFullGlobal_2023D19/step2_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2023D19_GenSimHLBeamSpotFull14+DigiFullTrigger_2023D19+RecoFullGlobal_2023D19+HARVESTFullGlobal_2023D19.log

21234.0 step2
runTheMatrix-results/21234.0_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2023D21_GenSimHLBeamSpotFull14+DigiFullTrigger_2023D21+RecoFullGlobal_2023D21+HARVESTFullGlobal_2023D21/step2_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2023D21_GenSimHLBeamSpotFull14+DigiFullTrigger_2023D21+RecoFullGlobal_2023D21+HARVESTFullGlobal_2023D21.log

@cmsbuild
Copy link
Contributor

Comparison not run due to runTheMatrix errors (RelVals and Igprof tests were also skipped)

@kpedro88
Copy link
Contributor

20-Aug-2018 17:56:03 CEST  Initiating request to open file file:step1.root
20-Aug-2018 17:56:04 CEST  Successfully opened file file:step1.root
20-Aug-2018 17:56:06 CEST  Closed file file:step1.root
----- Begin Fatal Exception 20-Aug-2018 17:56:06 CEST-----------------------
An exception of category 'Configuration' occurred while
   [0] Constructing the EventProcessor
   [1] Constructing module: class=MixingModule label='mix'
Exception Message:
MissingParameter: Parameter 'ReadoutMode' not found.
----- End Fatal Exception -------------------------------------------------

temp_signal = std::floor(signal_in_elec / theElectronPerADC) + 1;
} else {
// calculate the kink point and the slope
const int dualslope_param = std::min(abs(thePhase2ReadoutMode), 10);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make a named constant for magic number 10

@@ -116,7 +116,7 @@ class Phase2TrackerDigitizerAlgorithm {
const float ClusterWidth; // Gaussian charge cutoff width in sigma units

//-- make_digis
const bool doDigitalReadout; // Flag to decide analog or digital readout
const int thePhase2ReadoutMode; // Flag to decide readout mode (digital/Analong dual slope etc.)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

analong -> analog

} else {
position /= width;
local_mes.ClusterWidth->Fill(width);
local_mes.ClusterPosition->Fill(position);
for (std::vector<int>::iterator ic = charges.begin(); ic != charges.end(); ic++) local_mes.ChargeOfDigisVsWidth->Fill((*ic), width);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use range-based loop

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 6, 2018

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

@emiglior
Copy link
Contributor

emiglior commented Sep 6, 2018

please test

1 similar comment
@civanch
Copy link
Contributor

civanch commented Sep 6, 2018

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 6, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/30286/console Started: 2018/09/06 19:26

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 6, 2018

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 6, 2018

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 6, 2018

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 1418 differences found in the comparisons
  • DQMHistoTests: Total files compared: 32
  • DQMHistoTests: Total histograms compared: 3143975
  • DQMHistoTests: Total failures: 1141
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3142637
  • DQMHistoTests: Total skipped: 197
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 31 files compared)
  • Checked 133 log files, 14 edm output root files, 32 DQM output files

@emiglior
Copy link
Contributor

emiglior commented Sep 6, 2018

Screenshots of a dEdx distribution in the previous version of the PR [1] and in the new version of the PR [2]. In the new PR the differences wrt baseline are much reduced a part from small (expected) differences in the tail (we have slightly modified the max value of one parameter).

[1] workflow20034-dedxdqmharm2po-originalpr
[2] workflow20034-dedxdqmharm2po-newpr

@kpedro88
Copy link
Contributor

kpedro88 commented Sep 6, 2018

+upgrade

@civanch
Copy link
Contributor

civanch commented Sep 7, 2018

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 7, 2018

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
Copy link
Contributor

fabiocos commented Sep 8, 2018

+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

6 participants