Dynamic pps timing calibration fit #44235

ChrisMisan · 2024-02-27T19:03:04Z

PR description:

Extension to the existing calibration fit that calculates the parameter from the histogram, instead of relying on the pre-defined ranges. Logic works as follows:

Find the peak of counts in the TOT 1-d distribution (the peak must be located at TOT<20ns) -> this define the TOTMAX_bin
From TOTMAX_bin move to higher TOT till the counts are below THR% of max -> this define the upper_tot_range for the fit (in any case below 18 ns)
From TOTMAX_bin move to lower TOT till the counts are below THR% of max -> this define the lower_tot_range for the fit (in any case above 8ns)
Fit the 2d profile in the range (upper_tot_range, lower_tot_range)

PR validation:

Standard wfs for PPS timing calibration.

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:

not a backport

cmsbuild · 2024-02-27T19:03:33Z

cms-bot internal usage

cmsbuild · 2024-02-27T19:09:40Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44235/39243

This PR adds an extra 12KB to repository

cmsbuild · 2024-02-27T19:10:02Z

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

It involves the following packages:

CalibPPS/TimingCalibration (alca)

@cmsbuild, @saumyaphor4252, @consuegs, @perrotta can you please review it and eventually sign? Thanks.
@yuanchao, @rsreds, @fabferro, @mmusich, @tocheng this is something you requested to watch as well.
@sextonkennedy, @antoniovilela, @rappoccio you are the release manager for this.

cms-bot commands are listed here

perrotta · 2024-02-28T08:59:32Z

CalibPPS/TimingCalibration/plugins/PPSTimingCalibrationPCLHarvester.cc

+    for (int i = 0; i < hists.toT[chid]->getNbinsX(); i++) {
+      double bin_value = hists.toT[chid]->getBinContent(i);
+      int bin_x_pos = hists.toT[chid]->getTH1()->GetXaxis()->GetBinCenter(i);
+      if (bin_x_pos > 20)


Can this "20" be assigned to a named constant, with a meaningful name?

perrotta · 2024-02-28T09:00:55Z

CalibPPS/TimingCalibration/plugins/PPSTimingCalibrationPCLHarvester.cc

+    int upper_limit_pos = max_bin_pos;
+    int lower_limit_pos = max_bin_pos;
+    double threshold = threshold_percent_of_max_ * hists.toT[chid]->getBinContent(max_bin_pos);
+    while (hists.toT[chid]->getTH1()->GetXaxis()->GetBinCenter(upper_limit_pos) < 18) {


Can this "18" be assigned to a named constant, with a meaningful name? Is it the previous 20 minus 2?

perrotta · 2024-02-28T09:01:06Z

CalibPPS/TimingCalibration/plugins/PPSTimingCalibrationPCLHarvester.cc

+      if (hists.toT[chid]->getBinContent(upper_limit_pos) < threshold)
+        break;
+    }
+    while (hists.toT[chid]->getTH1()->GetXaxis()->GetBinCenter(lower_limit_pos) > 8) {


Can this "8" be assigned to a named constant, with a meaningful name?

perrotta · 2024-02-28T09:01:54Z

CalibPPS/TimingCalibration/plugins/PPSTimingCalibrationPCLHarvester.cc

+    double upper_tot_range = hists.toT[chid]->getTH1()->GetXaxis()->GetBinCenter(upper_limit_pos);
+    double lower_tot_range = hists.toT[chid]->getTH1()->GetXaxis()->GetBinCenter(lower_limit_pos);
+
+    //const double upper_tot_range = hists.toT[chid]->getMean() + 2.5;


Is this commented out line needed? Add another comment line with an explanation if so, otherwise just remove

ChrisMisan · 2024-02-28T18:15:44Z

@perrotta thanks, I addressed all of the comments.

cmsbuild · 2024-02-28T18:22:36Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44235/39272

This PR adds an extra 16KB to repository

cmsbuild · 2024-02-28T18:23:03Z

Pull request #44235 was updated. @perrotta, @cmsbuild, @saumyaphor4252, @consuegs can you please check and sign again.

perrotta · 2024-02-29T08:50:47Z

CalibPPS/TimingCalibration/plugins/PPSTimingCalibrationPCLHarvester.cc

@@ -42,6 +42,10 @@ class PPSTimingCalibrationPCLHarvester : public DQMEDHarvester {
  const std::string dqmDir_;
  const std::string formula_;
  const unsigned int min_entries_;
+  const double threshold_fraction_of_max_;
+  static constexpr double upper_limit_max_search_ = 20;
+  static constexpr double upper_limit_range_search_ = 20;


This was 18 before the last update: please check and either confirm or fix

Change was intentional, after discussing it internally we decided to switch it to 20.

grzanka · 2024-02-29T09:47:09Z

@ChrisMisan As for tests, am I correct we need to run it with WFs 1043-1045 ?

ChrisMisan · 2024-02-29T10:59:11Z

@ChrisMisan As for tests, am I correct we need to run it with WFs 1043-1045 ?

@grzanka I think only 1044 is relevant for this pr.

perrotta · 2024-02-29T13:29:54Z

test parameters:

workflow = 1043.0,1044.0,1045.0

perrotta · 2024-02-29T13:30:19Z

please test
(let test all three extra workflow anyhow)

cmsbuild · 2024-02-29T19:03:07Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6a3155/37812/summary.html
COMMIT: 9cf5b07
CMSSW: CMSSW_14_1_X_2024-02-29-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/44235/37812/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially added 26 lines to the logs
Reco comparison results: 45 differences found in the comparisons
DQMHistoTests: Total files compared: 52
DQMHistoTests: Total histograms compared: 3339890
DQMHistoTests: Total failures: 0
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3339868
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 51 files compared)
Checked 217 log files, 169 edm output root files, 52 DQM output files
TriggerResults: no differences found

perrotta · 2024-03-01T10:11:43Z

Effect on wf 1044 is only in the step4 (harvesting) step, as expected (last three of the four fit results are affected):

162 Lines only in /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/CMSSW_14_1_X_2024-02-29-1100/1044.0_RunRawPPS2022B/step4_RunRawPPS2022B.log
****************************************
Minimizer is Minuit2 / Migrad
   Chi2                      =      19.6154
   NDf                       =           11
   Edm                       =  1.09752e-06
   NCalls                    =          385
   p0                        =      3.58848   +/-   0.189209
   p1                        =       13.603   +/-   1.24546      	 (limited)
   p2                        =          2.5   +/-   1.93094      	 (limited)
   p3                        =      2.37315   +/-   0.4864
****************************************
Minimizer is Minuit2 / Migrad
   Chi2                      =      19.9551
   Edm                       =  2.00645e-06
   NCalls                    =          203
   p0                        =     0.693048   +/-   0.22637
   p1                        =      13.1288   +/-   0.272828     	 (limited)
   p2                        =     0.319507   +/-   0.192729     	 (limited)
   p3                        =      4.10712   +/-   0.110669
****************************************
Minimizer is Minuit2 / Migrad
   Chi2                      =      11.6601
   Edm                       =  3.90676e-06
   NCalls                    =          207
   p0                        =     0.550487   +/-   0.0468207
   p1                        =      12.9663   +/-   0.0823703    	 (limited)

164 Lines only in /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-6a3155/1044.0_RunRawPPS2022B/step4_RunRawPPS2022B.log
****************************************
Minimizer is Minuit2 / Migrad
   Chi2                      =      18.7617
   Edm                       =  5.59906e-06
   NCalls                    =          669
   p0                        =      3.57753   +/-   0.207017
   p1                        =      13.6579   +/-   1.60594      	 (limited)
   p2                        =      2.49999   +/-   1.9103       	 (limited)
   p3                        =      2.35879   +/-   0.621526
****************************************
Minimizer is Minuit2 / Migrad
   Chi2                      =      19.9071
   Edm                       =  7.03391e-06
   NCalls                    =          202
   p0                        =     0.732759   +/-   0.35828
   p1                        =      13.0974   +/-   0.335658     	 (limited)
   p2                        =     0.346668   +/-   0.270142     	 (limited)
   p3                        =      4.09894   +/-   0.134283
****************************************
Minimizer is Minuit2 / Migrad
   Chi2                      =       10.576
   Edm                       =   4.0813e-06
   NCalls                    =          263
   p0                        =     0.464763   +/-   0.152014
   p1                        =      13.0262   +/-   0.1353       	 (limited)
   p2                        =          0.2   +/-   0.0344622    	 (limited)

perrotta · 2024-03-01T10:11:54Z

+1

cmsbuild · 2024-03-01T10:12:01Z

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

antoniovilela · 2024-03-01T11:37:25Z

+1

cmsbuild added this to the CMSSW_14_1_X milestone Feb 27, 2024

cmsbuild added alca-pending pending-signatures tests-pending orp-pending code-checks-pending labels Feb 27, 2024

cmsbuild added code-checks-approved and removed code-checks-pending labels Feb 27, 2024

perrotta reviewed Feb 28, 2024

View reviewed changes

Dynamic pps timing calibration fit

9cf5b07

ChrisMisan force-pushed the pps-timing-calibration-new-fit branch from 86cf244 to 9cf5b07 Compare February 28, 2024 18:14

cmsbuild added code-checks-pending and removed code-checks-approved labels Feb 28, 2024

cmsbuild added code-checks-approved and removed code-checks-pending labels Feb 28, 2024

perrotta reviewed Feb 29, 2024

View reviewed changes

cmsbuild added tests-started and removed tests-pending labels Feb 29, 2024

cmsbuild removed the tests-started label Feb 29, 2024

cmsbuild added the tests-approved label Feb 29, 2024

cmsbuild added alca-approved fully-signed and removed alca-pending pending-signatures labels Mar 1, 2024

cmsbuild added orp-approved and removed orp-pending labels Mar 1, 2024

cmsbuild merged commit d493461 into cms-sw:master Mar 1, 2024
12 checks passed

grzanka mentioned this pull request Mar 3, 2024

Dynamic PPS timing calibration fit [backport of 44235] #44289

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dynamic pps timing calibration fit #44235

Dynamic pps timing calibration fit #44235

ChrisMisan commented Feb 27, 2024 •

edited

Loading

cmsbuild commented Feb 27, 2024 •

edited

Loading

cmsbuild commented Feb 27, 2024

cmsbuild commented Feb 27, 2024

perrotta Feb 28, 2024

perrotta Feb 28, 2024

perrotta Feb 28, 2024

perrotta Feb 28, 2024

ChrisMisan commented Feb 28, 2024

cmsbuild commented Feb 28, 2024

cmsbuild commented Feb 28, 2024

perrotta Feb 29, 2024

ChrisMisan Feb 29, 2024

grzanka commented Feb 29, 2024

ChrisMisan commented Feb 29, 2024

perrotta commented Feb 29, 2024

perrotta commented Feb 29, 2024

cmsbuild commented Feb 29, 2024

perrotta commented Mar 1, 2024

perrotta commented Mar 1, 2024

cmsbuild commented Mar 1, 2024

antoniovilela commented Mar 1, 2024

Dynamic pps timing calibration fit #44235

Dynamic pps timing calibration fit #44235

Conversation

ChrisMisan commented Feb 27, 2024 • edited Loading

PR description:

PR validation:

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:

cmsbuild commented Feb 27, 2024 • edited Loading

cmsbuild commented Feb 27, 2024

cmsbuild commented Feb 27, 2024

perrotta Feb 28, 2024

Choose a reason for hiding this comment

perrotta Feb 28, 2024

Choose a reason for hiding this comment

perrotta Feb 28, 2024

Choose a reason for hiding this comment

perrotta Feb 28, 2024

Choose a reason for hiding this comment

ChrisMisan commented Feb 28, 2024

cmsbuild commented Feb 28, 2024

cmsbuild commented Feb 28, 2024

perrotta Feb 29, 2024

Choose a reason for hiding this comment

ChrisMisan Feb 29, 2024

Choose a reason for hiding this comment

grzanka commented Feb 29, 2024

ChrisMisan commented Feb 29, 2024

perrotta commented Feb 29, 2024

perrotta commented Feb 29, 2024

cmsbuild commented Feb 29, 2024

Comparison Summary

perrotta commented Mar 1, 2024

perrotta commented Mar 1, 2024

cmsbuild commented Mar 1, 2024

antoniovilela commented Mar 1, 2024

ChrisMisan commented Feb 27, 2024 •

edited

Loading

cmsbuild commented Feb 27, 2024 •

edited

Loading