PPS reco patch for bad pot #38553

fabferro · 2022-06-29T13:35:47Z

PR description:

This PR provides a patch to be able to produce tracks inside PPS roman pot 45-220-fr that has only 2 planes working out of 6 (as to the current date). The code creates tracks using two hits (one per plane) drawing a straight line between them. The track is required to be isolated and horizontal. The goal is to recover at least some proton tracks for alignment/debugging purposes.
The patch is enabled by means of a runtime flag. Run2 processes are protected via Era mechanism.

PR validation:

Run2 data show no difference. Preliminary run3 data show recovered tracks as expected.

This PR will have to be backported to 12_4 and 12_3

@jpata @grzanka @AndreaBellora

cmsbuild · 2022-06-29T13:42:51Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38553/30790

This PR adds an extra 28KB to repository

cmsbuild · 2022-06-29T13:43:20Z

A new Pull Request was created by @fabferro (Fabrizio Ferro) for master.

It involves the following packages:

RecoPPS/Local (reconstruction)

@jpata, @cmsbuild, @clacaputo can you please review it and eventually sign? Thanks.
@grzanka 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

jpata · 2022-06-30T07:25:28Z

@cmsbuild please test

cmsbuild · 2022-06-30T11:11:52Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4812b5/25906/summary.html
COMMIT: effefa3
CMSSW: CMSSW_12_5_X_2022-06-29-2300/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/38553/25906/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 8 differences found in the comparisons
DQMHistoTests: Total files compared: 50
DQMHistoTests: Total histograms compared: 3654586
DQMHistoTests: Total failures: 14
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3654550
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
Checked 208 log files, 45 edm output root files, 50 DQM output files
TriggerResults: no differences found

jpata · 2022-06-30T11:17:46Z

@cmsbuild enable profiling

jpata · 2022-06-30T11:17:56Z

@cmsbuild please test

cmsbuild · 2022-06-30T15:01:03Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4812b5/25913/summary.html
COMMIT: effefa3
CMSSW: CMSSW_12_5_X_2022-06-29-2300/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/38553/25913/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 8 differences found in the comparisons
DQMHistoTests: Total files compared: 50
DQMHistoTests: Total histograms compared: 3654586
DQMHistoTests: Total failures: 13
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 3654550
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.004 KiB( 49 files compared)
DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
Checked 208 log files, 45 edm output root files, 50 DQM output files
TriggerResults: no differences found

jpata · 2022-07-01T07:33:15Z

Profiling seems not to have run. Does it have something to do with the PR still being in draft mode? Perhaps it can be removed?

cmsbuild · 2022-07-12T14:42:41Z

Pull request #38553 was updated. @emanueleusai, @ahmad3213, @cmsbuild, @jfernan2, @clacaputo, @jpata, @pmandrik, @micsucmed, @rvenditti can you please check and sign again.

malbouis · 2022-07-12T16:09:22Z

process.GlobalTag.toGet = cms.VPSet(
cms.PSet(record = cms.string("CTPPSPixelAnalysisMaskRcd"),
tag = cms.string("CTPPSPixelAnalysisMask_2022_45-220-pl1roc45pl2pl3pl4roc45"),
connect = cms.string("frontier://FrontierPrep/CMS_CONDITIONS")
)
)
then any workflow can be used. I would suggest 136.793 (2017 run2 data). I ran it manually and it worked fine.

@fabferro , in general, the best practice is to not customize the code to read a specific tag as we might run into trouble in the future.

The best way to proceed with this analysis mask tag would be to:
1 - upload it to production DataBase (with a meaningful name, if possible);
2 - queue it to the Queues it should be added to

then we can include it in the GTs.

fabferro · 2022-07-12T20:44:26Z

Can you suggest us the best way to profile a workflow ? Do you mean to run some workflow with and without this PR or extend the current workflow sets to cover all cases of bad pot PR ?

The instruction for running profiling using igprof can be found in RECO Twiki. We already profile 11834.21 for each pre-releases, so it would be enough for you to profile the workflow with the current PR in and with CTPPS enabled

We have a limited experience with that kind of machinery.
Should we add new steps in the https://github.com/cms-sw/cmssw/blob/CMSSW_12_5_X/Configuration/PyReleaseValidation/python/relval_steps.py#L2171 file ?

I would say so

We would also need to add some data for Run3 with PPS data, how this is usually handled ? If I remember correctly, we cannot do it ourselves.

I need to check about it, I don't know whether we have worflows using run3 data as input

Where can we find definition of the workflow 11834.21 ? I went through the files in https://github.com/cms-sw/cmssw/tree/CMSSW_12_5_X/Configuration/PyReleaseValidation/python and haven't found what exactly this workflow is doing.

You can find it running runTheMatrix.py -l11834.21 -n -e
[1]: cmsDriver.py TTbar_14TeV_TuneCP5_cfi  -s GEN,SIM -n 10 --conditions auto:phase1_2021_realistic --beamspot Run3RoundOptics25ns13TeVLowSigmaZ --datatier GEN-SIM --eventcontent FEVTDEBUG --geometry DB:Extended --era Run3 --relval 9000,100
[2]: cmsDriver.py step2  -s DIGI,L1,DIGI2RAW,HLT:@relval2021 --conditions auto:phase1_2021_realistic --datatier GEN-SIM-RAW -n 10 --eventcontent RAWSIM --geometry DB:Extended --era Run3 --pileup Run3_Flat55To75_PoissonOOTPU --pileup_input das:/RelValMinBias_14TeV/CMSSW_12_0_0_pre4-120X_mcRun3_2021_realistic_v2-v1/GEN-SIM
[3]: cmsDriver.py step3  -s RAW2DIGI,L1Reco,RECO,RECOSIM --conditions auto:phase1_2021_realistic --datatier AODSIM -n 10 --eventcontent AODSIM --geometry DB:Extended --era Run3 --pileup Run3_Flat55To75_PoissonOOTPU --pileup_input das:/RelValMinBias_14TeV/CMSSW_12_0_0_pre4-120X_mcRun3_2021_realistic_v2-v1/GEN-SIM
[4]: cmsDriver.py step4  -s PAT --conditions auto:phase1_2021_realistic --datatier MINIAODSIM -n 10 --eventcontent MINIAODSIM --geometry DB:Extended --era Run3 --pileup Run3_Flat55To75_PoissonOOTPU --pileup_input das:/RelValMinBias_14TeV/CMSSW_12_0_0_pre4-120X_mcRun3_2021_realistic_v2-v1/GEN-SIM
[5]: cmsDriver.py step5  -s NANO --conditions auto:phase1_2021_realistic --datatier NANOAODSIM -n 10 --eventcontent NANOEDMAODSIM --filein file:step4.root --geometry DB:Extended --era Run3 --pileup Run3_Flat55To75_PoissonOOTPU --pileup_input das:/RelValMinBias_14TeV/CMSSW_12_0_0_pre4-120X_mcRun3_2021_realistic_v2-v1/GEN-SIM

@clacaputo I profiled workflow 136.793 with the PR in and PPS code enabled. Following the instructions in the twiki page I produced the results for cpu and memory usage. They are /afs/cern.ch/work/f/fabferro/public/badPot/profiling/newCode/ig.sql3 and /afs/cern.ch/work/f/fabferro/public/badPot/profiling/newCode/ig.1.sql3. I assume you can read the info inside.
It would be great if we could not miss 12_4_3 deadline.

clacaputo · 2022-07-13T10:34:01Z

@cmsbuild please test

clacaputo · 2022-07-13T10:39:23Z

@clacaputo I profiled workflow 136.793 with the PR in and PPS code enabled. Following the instructions in the twiki page I produced the results for cpu and memory usage. They are /afs/cern.ch/work/f/fabferro/public/badPot/profiling/newCode/ig.sql3 and /afs/cern.ch/work/f/fabferro/public/badPot/profiling/newCode/ig.1.sql3. I assume you can read the info inside. It would be great if we could not miss 12_4_3 deadline.

@fabferro thanks for the profiling files; I've found nothing suspicious. As soon as the test ends, I can sing the pr

tvami

The PR is not under alca, but I wanted to make sure that the record is consumed correctly when it will be provided from the GT. So by reading the code I have these comments, given the urgency of the PR, maybe these could be addressed in a follow-up PR?

tvami · 2022-07-13T13:40:39Z

RecoPPS/Local/plugins/CTPPSPixelLocalTrackProducer.cc

@@ -58,13 +67,16 @@ class CTPPSPixelLocalTrackProducer : public edm::stream::EDProducer<> {
  edm::EDGetTokenT<edm::DetSetVector<CTPPSPixelRecHit>> tokenCTPPSPixelRecHit_;
  edm::ESGetToken<CTPPSGeometry, VeryForwardRealGeometryRecord> tokenCTPPSGeometry_;
  edm::ESWatcher<VeryForwardRealGeometryRecord> geometryWatcher_;
+
+  edm::ESGetToken<CTPPSPixelAnalysisMask, CTPPSPixelAnalysisMaskRcd> tokenCTPPSPixelAnalysisMask_;


This could be const

tvami · 2022-07-13T13:40:49Z

RecoPPS/Local/plugins/CTPPSPixelLocalTrackProducer.cc

  uint32_t numberOfPlanesPerPot_;
  std::vector<uint32_t> listOfAllPlanes_;

  std::unique_ptr<RPixDetPatternFinder> patternFinder_;
  std::unique_ptr<RPixDetTrackFinder> trackFinder_;

-  void run(const edm::DetSetVector<CTPPSPixelRecHit> &input, edm::DetSetVector<CTPPSPixelLocalTrack> &output);
+  //  void run(const edm::DetSetVector<CTPPSPixelRecHit> &input, edm::DetSetVector<CTPPSPixelLocalTrack> &output);


Delete unused code

tvami · 2022-07-13T13:41:05Z

RecoPPS/Local/plugins/CTPPSPixelLocalTrackProducer.cc

  tokenCTPPSPixelRecHit_ = consumes<edm::DetSetVector<CTPPSPixelRecHit>>(inputTag_);
  tokenCTPPSGeometry_ = esConsumes<CTPPSGeometry, VeryForwardRealGeometryRecord>();
+  tokenCTPPSPixelAnalysisMask_ = esConsumes<CTPPSPixelAnalysisMask, CTPPSPixelAnalysisMaskRcd>();


all these should be moved to the constructor

tvami · 2022-07-13T13:41:10Z

RecoPPS/Local/plugins/CTPPSPixelLocalTrackProducer.cc

@@ -143,6 +156,9 @@ void CTPPSPixelLocalTrackProducer::fillDescriptions(edm::ConfigurationDescriptio
  desc.add<double>("roadRadius", 1.0)->setComment("radius of pattern search window");
  desc.add<int>("minRoadSize", 3)->setComment("minimum number of points in a pattern");
  desc.add<int>("maxRoadSize", 20)->setComment("maximum number of points in a pattern");
+  //parameters for bad pot reconstruction patch 45-220-fr 2022
+  desc.add<double>("roadRadiusBadPot", 0.5)->setComment("radius of pattern search window for bad Pot");
+  //  desc.add<bool>("isBadPot", true)->setComment("flag to enable road search for bad pot");


tvami · 2022-07-13T13:41:21Z

RecoPPS/Local/python/ctppsPixelLocalReconstruction_cff.py

+#from Configuration.Eras.Modifier_ctpps_2016_cff import ctpps_2016
+#from Configuration.Eras.Modifier_ctpps_2017_cff import ctpps_2017
+#from Configuration.Eras.Modifier_ctpps_2018_cff import ctpps_2018
+#(ctpps_2016 | ctpps_2017 | ctpps_2018).toModify(ctppsPixelLocalTracks, isBadPot = cms.bool(False))
+


tvami · 2022-07-13T13:41:33Z

RecoPPS/Local/src/RPixRoadFinder.cc

@@ -31,6 +20,8 @@ RPixRoadFinder::RPixRoadFinder(edm::ParameterSet const& parameterSet) : RPixDetP
  roadRadius_ = parameterSet.getParameter<double>("roadRadius");
  minRoadSize_ = parameterSet.getParameter<int>("minRoadSize");
  maxRoadSize_ = parameterSet.getParameter<int>("maxRoadSize");
+  roadRadiusBadPot_ = parameterSet.getParameter<double>("roadRadiusBadPot");
+  //  isBadPot_ = parameterSet.getParameter<bool>("isBadPot");


tvami · 2022-07-13T13:42:27Z

Validation/CTPPS/test/simu/template_cfg.py

+process.PoolDBESSource = cms.ESSource("PoolDBESSource",
+    process.CondDB,
+    toGet = cms.VPSet(cms.PSet(
+        record = cms.string('CTPPSPixelAnalysisMaskRcd'),
+        tag = cms.string("CTPPSPixelAnalysisMask_Run3_v1_hlt")
+    ))
+)


this is fine for a test, but in the end the record will be provided from the GT, so this will be unnecessary

fabferro · 2022-07-13T14:37:23Z

@tvami I perfectly agree with your comments, I left the code as it is to clarify the change of strategy during the discussion with the reviewers. As you say I hope this PR can go in as it is and the changes you suggest be implemented right after.

cmsbuild · 2022-07-13T19:13:01Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4812b5/26199/summary.html
COMMIT: a34b07e
CMSSW: CMSSW_12_5_X_2022-07-12-2300/el8_amd64_gcc10
Additional Tests: PROFILING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/38553/26199/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 50
DQMHistoTests: Total histograms compared: 3653966
DQMHistoTests: Total failures: 7
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 3653936
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.004 KiB( 49 files compared)
DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
Checked 208 log files, 45 edm output root files, 50 DQM output files
TriggerResults: no differences found

clacaputo · 2022-07-13T22:00:00Z

+reconstruction

temporary patch to be able to produce tracks inside PPS roman pot 45-220-fr
To enable the code a new detector mask can be loaded from the GT PPS reco patch for bad pot #38553 (comment)
reco profiling performed on wf 136.793 doesn't show anything suspicious
Considering the urgency, review items still pending PPS reco patch for bad pot #38553 (review) can be addressed in future PRs

perrotta · 2022-07-14T04:50:20Z

RecoPPS/Local/interface/RPixRoadFinder.h


 private:
  int verbosity_;
  double roadRadius_;
  unsigned int minRoadSize_;
  unsigned int maxRoadSize_;
+  double roadRadiusBadPot_;
+  //  bool isBadPot_;


This also should be deleted, if not needed

perrotta · 2022-07-14T04:56:03Z

+1

@fabferro please provide a PR in the master with all the cleanups suggested during the review

perrotta · 2022-07-14T04:59:29Z

I did not notice it still misses the @cms-sw/dqm-l2 signature
Please @emanueleusai have a look and provide either your signature or comments at the earliest

emanueleusai · 2022-07-14T05:28:44Z

+dqm

good from dqm side

cmsbuild · 2022-07-14T05:29: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 be automatically merged.

fabferro added 4 commits June 14, 2022 16:51

treating bad pot 45 220

08fa0d2

cleaning cous

9805a7e

era changes to safeguard Run2

a17bb82

clean up

effefa3

cmsbuild added this to the CMSSW_12_5_X milestone Jun 29, 2022

cmsbuild added code-checks-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Jun 29, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Jun 29, 2022

cmsbuild added tests-started and removed tests-pending labels Jun 30, 2022

cmsbuild added tests-approved and removed tests-started labels Jun 30, 2022

cmsbuild added tests-started and removed tests-approved labels Jun 30, 2022

cmsbuild added tests-approved and removed tests-started labels Jun 30, 2022

adding localX>0 fiducial cut

f65b862

cmsbuild removed the tests-approved label Jul 1, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Jul 12, 2022

cmsbuild added tests-started and removed tests-pending labels Jul 13, 2022

tvami reviewed Jul 13, 2022

View reviewed changes

cmsbuild added tests-approved and removed tests-started labels Jul 13, 2022

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Jul 13, 2022

perrotta reviewed Jul 14, 2022

View reviewed changes

cmsbuild added orp-approved and removed orp-pending labels Jul 14, 2022

cmsbuild added dqm-approved fully-signed and removed dqm-pending pending-signatures labels Jul 14, 2022

cmsbuild merged commit ec43ecd into cms-sw:master Jul 14, 2022

fabferro mentioned this pull request Jul 14, 2022

PPS clean code for bad pot #38740

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PPS reco patch for bad pot #38553

PPS reco patch for bad pot #38553

fabferro commented Jun 29, 2022

cmsbuild commented Jun 29, 2022

cmsbuild commented Jun 29, 2022

jpata commented Jun 30, 2022

cmsbuild commented Jun 30, 2022

jpata commented Jun 30, 2022

jpata commented Jun 30, 2022

cmsbuild commented Jun 30, 2022

jpata commented Jul 1, 2022

cmsbuild commented Jul 12, 2022

malbouis commented Jul 12, 2022

fabferro commented Jul 12, 2022 •

edited

clacaputo commented Jul 13, 2022

clacaputo commented Jul 13, 2022

tvami left a comment

tvami Jul 13, 2022

tvami Jul 13, 2022

tvami Jul 13, 2022

tvami Jul 13, 2022

tvami Jul 13, 2022

tvami Jul 13, 2022

tvami Jul 13, 2022

fabferro commented Jul 13, 2022

cmsbuild commented Jul 13, 2022

clacaputo commented Jul 13, 2022

perrotta Jul 14, 2022

perrotta commented Jul 14, 2022

perrotta commented Jul 14, 2022

emanueleusai commented Jul 14, 2022

cmsbuild commented Jul 14, 2022

PPS reco patch for bad pot #38553

PPS reco patch for bad pot #38553

Conversation

fabferro commented Jun 29, 2022

PR description:

PR validation:

cmsbuild commented Jun 29, 2022

cmsbuild commented Jun 29, 2022

jpata commented Jun 30, 2022

cmsbuild commented Jun 30, 2022

Comparison Summary

jpata commented Jun 30, 2022

jpata commented Jun 30, 2022

cmsbuild commented Jun 30, 2022

Comparison Summary

jpata commented Jul 1, 2022

cmsbuild commented Jul 12, 2022

malbouis commented Jul 12, 2022

fabferro commented Jul 12, 2022 • edited

clacaputo commented Jul 13, 2022

clacaputo commented Jul 13, 2022

tvami 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

fabferro commented Jul 13, 2022

cmsbuild commented Jul 13, 2022

Comparison Summary

clacaputo commented Jul 13, 2022

Choose a reason for hiding this comment

perrotta commented Jul 14, 2022

perrotta commented Jul 14, 2022

emanueleusai commented Jul 14, 2022

cmsbuild commented Jul 14, 2022

fabferro commented Jul 12, 2022 •

edited