Prompt calibration loop module for SiPixelLorentzAngle #35507

wweiphy · 2021-10-01T19:14:11Z

PR description:

With this PR, we introduce the Lorentz Angle (LA) calibration PCL module for barrel Pixel detector. It includes the general workflow of PCL as well as dedicated improvements for LA measurements. The details are listed below:

SiPixelLorentzAnglePCLWorker creates and fills the 2D histograms of electron drift and its production depth as tracks pass through the barrel pixel. We applied improved selections on clusters. Tree production of tracks and clusters is also included in case of other needs and is made optional.
SiPixelLorentzAnglePCLHarvester fits the histograms and obtains LA. We have improved the fitting by moving to a higher order polynomial fit to include the nonlinear distributions in the histograms.
Pass the DetID of layer 2 new modules in SiPixelLorentzAnglePCLWorker and SiPixelLorentzAnglePCLHarvester and measure LA fo each one of them.

New module names:
    BPix_BmI_SEC7_LYR2_LDR12F_MOD1
    BPix_BmI_SEC8_LYR2_LDR14F_MOD1
    BPix_BmO_SEC3_LYR2_LDR5F_MOD1
    BPix_BmO_SEC3_LYR2_LDR5F_MOD2
    BPix_BmO_SEC3_LYR2_LDR5F_MOD3
    BPix_BpO_SEC1_LYR2_LDR1F_MOD1
    BPix_BpO_SEC1_LYR2_LDR1F_MOD2
    BPix_BpO_SEC1_LYR2_LDR1F_MOD3

PR validation:

Tested with 50000 events from /store/data/Run2018D/SingleMuon/ALCARECO/SiPixelCalSingleMuon-ForPixelALCARECO_UL2018-v1/230000/F33B7CA6-256A-B34F-9536-7594FBC6F75B.root
Results were presented in the AlCaDB meeting and also attached.
LA_AlCa.pdf

if this PR is a backport please specify the original PR and why you need to backport that PR: N/A

Before submitting your pull requests, make sure you followed this checklist:

verify that the PR is really intended for the chosen branch
verify that changes follow CMS Naming, Coding, And Style Rules
verify that the PR passes the basic test procedure suggested in the CMSSW PR instructions

cmsbuild · 2021-10-01T19:20:47Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35507/25703

This PR adds an extra 164KB to repository
Found files with invalid states:
- CalibTracker/SiPixelLorentzAngle/test/test.py:
  - Added: cb3b184
  - Deleted: 94654b5
There are other open Pull requests which might conflict with changes you have proposed:
- File Configuration/PyReleaseValidation/python/relval_steps.py modified in PR(s): Add NanoAOD DQM to RelVal wfs #35412, Use pdigi_hi_nogen for HI MC wfs #35028

cmsbuild · 2021-10-01T19:21:12Z

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

It involves the following packages:

CalibTracker/SiPixelLorentzAngle (alca)
Calibration/TkAlCaRecoProducers (alca)
Configuration/AlCa (alca)
Configuration/EventContent (operations)
Configuration/PyReleaseValidation (pdmv, upgrade)
Configuration/StandardSequences (operations)

@perrotta, @malbouis, @yuanchao, @jordan-martins, @davidlange6, @bbilin, @wajidalikhan, @cmsbuild, @AdrianoDee, @srimanob, @kskovpen, @qliphy, @francescobrivio, @fabiocos, @tvami can you please review it and eventually sign? Thanks.
@felicepantaleo, @kpedro88, @Martin-Grunewald, @OzAmram, @threus, @mmusich, @slomeo, @makortel, @JanFSchulte, @dgulhan, @missirol, @ferencek, @GiacomoSguazzoni, @rovere, @VinInn, @tocheng, @ebrondol, @mtosi, @fabiocos, @dkotlins, @lecriste, @tvami this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

mmusich · 2021-10-01T20:23:57Z

test parameters:

workflow = 1001.2

mmusich · 2021-10-01T20:25:02Z

@cmsbuild, please test

cmsbuild · 2021-10-01T23:16:41Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-86c3fd/19325/summary.html
COMMIT: bd25f72
CMSSW: CMSSW_12_1_X_2021-09-30-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35507/19325/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

The workflows 1001.0 have different files in step1_dasquery.log than the ones found in the baseline. You may want to check and retrigger the tests if necessary. You can check it in the "files" directory in the results of the comparisons

@slava77 comparisons for the following workflows were not done due to missing matrix map:

/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-86c3fd/1001.2_RunZeroBias2017F+RunZeroBias2017F+TIER0EXPRUN2+ALCAEXPRUN2+ALCAHARVDSIPIXELCAL

Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 40
DQMHistoTests: Total histograms compared: 3210975
DQMHistoTests: Total failures: 0
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3210953
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 38 files compared)
Checked 160 log files, 37 edm output root files, 40 DQM output files
TriggerResults: no differences found

mmusich

thanks @wweiphy!
Find below a first pass of review comments.
Also please change the title in order to contain the word SiPixel in it.
Finally please add the new PCL workflow to the testPCLAlCaHarvesting unit test at

cmssw/Calibration/TkAlCaRecoProducers/test/testPCLAlCaHarvesting.py

Line 83 in c9e62f9

    
           process.PoolDBOutputService.toPut.append(process.ALCAHARVESTSiPixelAli_dbOutput)

mmusich · 2021-10-02T07:57:09Z

CalibTracker/SiPixelLorentzAngle/src/SiPixelLorentzAnglePCLHarvester.cc

+      // if the fit quality is OK
+      if (prob > fitProbCut_) {
+        for (const auto& id : detIdsToFill) {
+          bPixLorentzAnglePerTesla_ = p1 / 3.8f;


please take the magnetic field value from the ES.

mmusich · 2021-10-02T08:05:09Z

CalibTracker/SiPixelLorentzAngle/src/SiPixelLorentzAnglePCLHarvester.cc

+      edm::LogError("SiPixelLorentzAngleDB") << er.what() << std::endl;
+    } catch (const std::exception& er) {
+      edm::LogError("SiPixelLorentzAngleDB") << "caught std::exception " << er.what() << std::endl;
+    } catch (...) {


This triggers a warning in the static analyzer.

Should catch (...) be replaced?

yes, I think it should be removed.

@wweiphy see 7.11 in https://cms-sw.github.io/cms_coding_rules.html

mmusich · 2021-10-02T08:06:34Z

CalibTracker/SiPixelLorentzAngle/src/SiPixelLorentzAnglePCLHarvester.cc

+  MonitorElement* h_drift_depth_adc_slice_ =
+      iBooker.book1D("h_drift_depth_adc_slice", "slice of adc histogram", hist_drift_, min_drift_, max_drift_);
+
+  edm::LogPrint("LorentzAngle") << "module"


this is not very readable, please break into multiple lines.

mmusich · 2021-10-02T08:10:17Z

CalibTracker/SiPixelLorentzAngle/src/SiPixelLorentzAnglePCLWorker.cc

+
+// tree branches barrel
+int run_;
+long int event_;


All these variables are giving static analyzer warnings "non-const global static variable":
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-86c3fd/19325/llvm-analysis/

@mmusich Any recommendation to fix this? I think we need the global variables for Tree production.

Can't these be private members of the class?

Moving those variables into private class result in errors such as:
assignment of member 'SiPixelLorentzAnglePCLWorker::panelF_' in read-only object
because function dqmAnalyze is set const.

Perhaps another DQM class interface is better suited:
https://github.com/cms-sw/cmssw/tree/master/DQMServices/Core

mmusich · 2021-10-02T08:11:20Z

CalibTracker/SiPixelLorentzAngle/src/SiPixelLorentzAnglePCLWorker.cc

+  }
+}
+
+SiPixelLorentzAnglePCLWorker::~SiPixelLorentzAnglePCLWorker() {


If nothing is done here, then set it as default

mmusich · 2021-10-02T10:07:05Z

CalibTracker/SiPixelLorentzAngle/interface/SiPixelLorentzAngleCalibrationStruct.h

+  std::vector<int> FPixnewDetIds_;
+  std::vector<int> FPixnewDisk_;
+  std::vector<int> FPixnewBlade_;
+  std::map<uint32_t, std::vector<uint32_t> > detIdsList;


can be unordered?

mmusich · 2021-10-02T10:07:37Z

CalibTracker/SiPixelLorentzAngle/src/SiPixelLorentzAnglePCLHarvester.cc

+  }
+  */
+
+  for (int i_layer = 1; i_layer <= hists.nlay; i_layer++) {


range based loop?

mmusich · 2021-10-02T10:07:54Z

CalibTracker/SiPixelLorentzAngle/src/SiPixelLorentzAnglePCLHarvester.cc

+    }
+  }
+
+  for (int i = 0; i < (int)hists.BPixnewDetIds_.size(); i++) {


range based loop?

mmusich · 2021-10-02T10:09:28Z

CalibTracker/SiPixelLorentzAngle/src/SiPixelLorentzAnglePCLHarvester.cc

+                                << "\t"
+                                << "newDetId" << std::endl;
+
+  SiPixelLorentzAngle* LorentzAngle = new SiPixelLorentzAngle();


where is this deleted?

mmusich · 2021-10-02T10:13:05Z

CalibTracker/SiPixelLorentzAngle/src/SiPixelLorentzAnglePCLWorker.cc

+          bool large_pix = false;
+          for (int j = 0; j < pixinfo_.npix; j++) {
+            int colpos = static_cast<int>(pixinfo_.col[j]);
+            if (pixinfo_.row[j] == 0 || pixinfo_.row[j] == 79 || pixinfo_.row[j] == 80 || pixinfo_.row[j] == 159 ||


maybe this interface can be re-used?

cmssw/Geometry/TrackerGeometryBuilder/interface/phase1PixelTopology.h

Line 139 in c9e62f9

constexpr inline bool isBigPixX(uint16_t px) { return (px == 79) | (px == 80); }

cmssw/Geometry/TrackerGeometryBuilder/interface/phase1PixelTopology.h

Line 141 in c9e62f9

constexpr inline bool isBigPixY(uint16_t py) {

@mmusich there are extra terms pixinfo_.row[j] == 0 and pixinfo_.row[j] == 159 , which are not included in the function phase1PixelTopology::isBigPixX. Should I keep or remove them?

In other words:

if (pixinfo_.row[j] == 0 || phase1PixelTopology::isBigPixX(pixinfo_.row[j]) || pixinfo_.row[j] == 159 || phase1PixelTopology::isBigPixY(pixinfo_.col[j]))

or

if (phase1PixelTopology::isBigPixX(pixinfo_.row[j]) || phase1PixelTopology::isBigPixY(pixinfo_.col[j])){

@tsusa please clarify why the previous usage.

@tsusa please clarify, so that we can proceed with this PR

@perotta, @mmusich, @wweiphy The cut removes the hits which are at the ROC edges. In order to keep the compatibility with the code used to derive LA values that enter DB, I would keep that cut as it is.

tvami · 2021-10-02T16:22:44Z

CalibTracker/SiPixelLorentzAngle/src/SiPixelLorentzAnglePCLHarvester.cc

+#include "Geometry/TrackerGeometryBuilder/interface/TrackerGeometry.h"
+
+#include "Geometry/Records/interface/TrackerDigiGeometryRecord.h"
+#include "../interface/SiPixelLorentzAngleCalibrationStruct.h"


I think it's preferred to have absolute path instead of the relative path

tvami · 2021-10-02T16:24:07Z

CalibTracker/SiPixelLorentzAngle/src/SiPixelLorentzAnglePCLHarvester.cc

+  /*
+  for(const auto& [index,histo] : hists.h_drift_depth_adc_){
+    std::cout << index << " => " << histo->getName();
+  }
+  */


this can either go or become LogDebug

tvami · 2021-10-02T16:26:57Z

CalibTracker/SiPixelLorentzAngle/src/SiPixelLorentzAnglePCLWorker.cc

+#include "Geometry/TrackerGeometryBuilder/interface/TrackerGeometry.h"
+
+#include "FWCore/ParameterSet/interface/ParameterSet.h"
+#include "../interface/SiPixelLorentzAngleCalibrationStruct.h"


Please use absolute path

tvami · 2021-10-02T16:27:34Z

CalibTracker/SiPixelLorentzAngle/src/SiPixelLorentzAnglePCLWorker.cc

+  double max_depth_;
+  double min_drift_;
+  double max_drift_;
+  int bufsize;


Suggested change

int bufsize;

int bufsize_;

Hmm I guess one can argue that this should stay bufsize, let's see what others think

Yes, all class members should follow the same conventions for naming, therefore the trailing underscore should be added here.
On the other hand, bufsize is only used inside the constructor, where it is also initialized. Then I don't see why it should remain a class member, instead of becoming a local variable instead (without the trailing underscore in that case).

tvami · 2021-10-02T17:20:51Z

Anyway, the new PromptCalibProdSiPixelLorentzAngle.root tree is being created here
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-86c3fd/19325/runTheMatrix-results/1001.2_RunZeroBias2017F+RunZeroBias2017F+TIER0EXPRUN2+ALCAEXPRUN2+ALCAHARVDSIPIXELCAL/

cmsbuild · 2021-10-13T22:53:15Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-86c3fd/19612/summary.html
COMMIT: a9c114b
CMSSW: CMSSW_12_1_X_2021-10-13-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35507/19612/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

The workflows 1001.0, 140.53 have different files in step1_dasquery.log than the ones found in the baseline. You may want to check and retrigger the tests if necessary. You can check it in the "files" directory in the results of the comparisons

@slava77 comparisons for the following workflows were not done due to missing matrix map:

/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-86c3fd/1001.2_RunZeroBias2017F+RunZeroBias2017F+TIER0EXPRUN2+ALCAEXPRUN2+ALCAHARVDSIPIXELCAL

Summary:

No significant changes to the logs found
Reco comparison results: 1000 differences found in the comparisons
DQMHistoTests: Total files compared: 40
DQMHistoTests: Total histograms compared: 2796686
DQMHistoTests: Total failures: 2568
DQMHistoTests: Total nulls: 62
DQMHistoTests: Total successes: 2794034
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 65.355 KiB( 38 files compared)
DQMHistoSizes: changed ( 140.53 ): 63.680 KiB Hcal/DigiRunHarvesting
DQMHistoSizes: changed ( 140.53 ): 1.676 KiB RPC/DCSInfo
Checked 160 log files, 37 edm output root files, 40 DQM output files
TriggerResults: no differences found

tvami · 2021-10-14T00:34:32Z

+alca

srimanob · 2021-10-14T11:55:19Z

+Upgrade

The PR introduces a new PCL module as described in the PR description. Changes in Configuration/PyReleaseValidation is configuration as expected.

bbilin · 2021-10-14T12:24:37Z

+1

tvami · 2021-10-14T12:32:17Z

@perrotta @qliphy this is again essentially fully signed, and we have another PR in the pipeline that right now changes some of the files here, so please sign it asap, thanks!

perrotta · 2021-10-14T12:40:43Z

+operations

cmsbuild · 2021-10-14T12:41:07Z

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

perrotta · 2021-10-14T12:41:22Z

+1

cmsbuild added this to the CMSSW_12_1_X milestone Oct 1, 2021

cmsbuild added alca-pending code-checks-pending operations-pending orp-pending pdmv-pending pending-signatures tests-pending upgrade-pending labels Oct 1, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 1, 2021

cmsbuild added tests-started and removed tests-pending labels Oct 1, 2021

cmsbuild added tests-approved and removed tests-started labels Oct 1, 2021

mmusich reviewed Oct 2, 2021

View reviewed changes

cmsbuild mentioned this pull request Oct 2, 2021

Add NanoAOD DQM to RelVal wfs #35412

Merged

tvami reviewed Oct 2, 2021

View reviewed changes

cmsbuild added code-checks-pending and removed tests-approved code-checks-approved labels Oct 5, 2021

cmsbuild added tests-started and removed tests-pending labels Oct 13, 2021

cmsbuild added tests-approved and removed tests-started labels Oct 13, 2021

cmsbuild added alca-approved and removed alca-pending labels Oct 14, 2021

cmsbuild added upgrade-approved and removed upgrade-pending labels Oct 14, 2021

cmsbuild added pdmv-approved and removed pdmv-pending labels Oct 14, 2021

cmsbuild added fully-signed operations-approved and removed operations-pending pending-signatures labels Oct 14, 2021

cmsbuild added orp-approved and removed orp-pending labels Oct 14, 2021

cmsbuild merged commit 2a970a2 into cms-sw:master Oct 14, 2021

This was referenced Oct 14, 2021

PPS bug fix of Run2 pixel topology #35671

Merged

Add DQM Matrix to RelVals #35678

Merged

Change to the new method of using writeOneIOV in PPSAssociationCuts #35669

Merged

DAQ fix spurious luminosityBlockAuxiliary assertion in HLT (12_1_X) #35682

Merged

mmusich mentioned this pull request Dec 17, 2021

Add SiPixelLorentzAnglePCLRenderPlugin dmwm/deployment#1118

Merged

Prompt calibration loop module for SiPixelLorentzAngle #35507

Prompt calibration loop module for SiPixelLorentzAngle #35507

Conversation

wweiphy commented Oct 1, 2021

PR description:

PR validation:

if this PR is a backport please specify the original PR and why you need to backport that PR: N/A

cmsbuild commented Oct 1, 2021

cmsbuild commented Oct 1, 2021

mmusich commented Oct 1, 2021 • edited

mmusich commented Oct 1, 2021

cmsbuild commented Oct 1, 2021

Comparison Summary

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

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

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

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

tsusa Oct 13, 2021 • edited

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

Choose a reason for hiding this comment

tvami commented Oct 2, 2021

cmsbuild commented Oct 13, 2021

Comparison Summary

tvami commented Oct 14, 2021

srimanob commented Oct 14, 2021

bbilin commented Oct 14, 2021

tvami commented Oct 14, 2021

perrotta commented Oct 14, 2021

cmsbuild commented Oct 14, 2021

perrotta commented Oct 14, 2021

mmusich commented Oct 1, 2021 •

edited

tsusa Oct 13, 2021 •

edited