cmssw icon indicating copy to clipboard operation
cmssw copied to clipboard

[nT2] rechits reconstruction from digis

Open forthommel opened this issue 3 years ago • 23 comments

PR description:

In this PR we introduce the Totem (for now, new T2-only) geometry-DetId mapping condition object, and the construction of a T2 rechits collection from the digis collection recently introduced in #38886. In order to avoid yet another duplication of pre-existing digi-rechits conversion code, the algorithmic part of this latter was transformed into a TimingRecHitProducerAlgorithm base class from which the pre-existing CTPPSDiamondRecHitProducerAlgorithm, TotemTimingRecHitProducerAlgorithm, and the newly-introduced TotemT2RecHitProducerAlgorithm inheritate. Most of the development history has been stashed in a few minimally-working commits, and follow-up developments were rebased on top of this branch.

PR validation:

Code compiles and runs on top of a digi-level sample. Matrix tests succede.

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: N/A

forthommel avatar Aug 07 '22 10:08 forthommel

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38986/31440

  • This PR adds an extra 76KB to repository

cmsbuild avatar Aug 07 '22 10:08 cmsbuild

A new Pull Request was created by @forthommel (Laurent Forthomme) for master.

It involves the following packages:

  • DataFormats/TotemReco (reconstruction)
  • Geometry/ForwardGeometry (geometry)
  • Geometry/Records (geometry)
  • RecoPPS/Local (reconstruction)

@civanch, @Dr15Jones, @makortel, @ianna, @clacaputo, @cmsbuild, @bsunanda, @mdhildreth, @jpata can you please review it and eventually sign? Thanks. @bsunanda, @rovere, @fabiocos, @grzanka this is something you requested to watch as well. @perrotta, @dpiparo, @qliphy, @rappoccio you are the release manager for this.

cms-bot commands are listed here

cmsbuild avatar Aug 07 '22 10:08 cmsbuild

please test

civanch avatar Aug 08 '22 07:08 civanch

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5000cf/26688/summary.html COMMIT: a4dc9da37399593fc432403a013473a1489809c4 CMSSW: CMSSW_12_5_X_2022-08-07-2300/el8_amd64_gcc10 User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/38986/26688/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: 51
  • DQMHistoTests: Total histograms compared: 3691612
  • DQMHistoTests: Total failures: 8
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3691582
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 50 files compared)
  • Checked 212 log files, 49 edm output root files, 51 DQM output files
  • TriggerResults: no differences found

cmsbuild avatar Aug 08 '22 11:08 cmsbuild

Hi @forthommel , is this PR planned to be backported into 12_4_X?

clacaputo avatar Aug 08 '22 12:08 clacaputo

Hi @clacaputo ! No, it will only used under specific pp data-taking conditions in ~october-november, and for HI runs later on. So, 12_5_X is all fine.

forthommel avatar Aug 08 '22 12:08 forthommel

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38986/31512

  • This PR adds an extra 32KB to repository

cmsbuild avatar Aug 11 '22 08:08 cmsbuild

Pull request #38986 was updated. @civanch, @Dr15Jones, @makortel, @ianna, @clacaputo, @cmsbuild, @bsunanda, @mdhildreth, @jpata can you please check and sign again.

cmsbuild avatar Aug 11 '22 08:08 cmsbuild

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38986/31513

  • This PR adds an extra 36KB to repository

cmsbuild avatar Aug 11 '22 09:08 cmsbuild

Pull request #38986 was updated. @civanch, @Dr15Jones, @makortel, @ianna, @clacaputo, @cmsbuild, @bsunanda, @mdhildreth, @jpata can you please check and sign again.

cmsbuild avatar Aug 11 '22 09:08 cmsbuild

Ready for another batch of reviews and/or a please test.

forthommel avatar Aug 11 '22 10:08 forthommel

@cmsbuild please test

clacaputo avatar Aug 11 '22 10:08 clacaputo

Thanks, @clacaputo ! Although the tests can now be aborted, I found an obvious problem with the new TimingRecHitsProducerAlgorithm which is about to be solved in a follow-up push.

forthommel avatar Aug 11 '22 11:08 forthommel

-1

Failed Tests: RelVals RelVals-INPUT AddOn Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5000cf/26753/summary.html COMMIT: 2e0936e8befe0652fbd391719826fb83be2b3946 CMSSW: CMSSW_12_5_X_2022-08-10-2300/el8_amd64_gcc10 User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/38986/26753/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals

----- Begin Fatal Exception 11-Aug-2022 12:31:16 CEST-----------------------
An exception of category 'Configuration' occurred while
   [0] Constructing the EventProcessor
   [1] Constructing module: class=TotemTimingRecHitProducer label='totemTimingRecHits'
Exception Message:
MissingParameter: Parameter 'timeSliceNs' not found.
----- End Fatal Exception -------------------------------------------------
----- Begin Fatal Exception 11-Aug-2022 12:32:55 CEST-----------------------
An exception of category 'Configuration' occurred while
   [0] Constructing the EventProcessor
   [1] Constructing module: class=TotemTimingRecHitProducer label='totemTimingRecHits'
Exception Message:
MissingParameter: Parameter 'timeSliceNs' not found.
----- End Fatal Exception -------------------------------------------------
----- Begin Fatal Exception 11-Aug-2022 12:33:17 CEST-----------------------
An exception of category 'Configuration' occurred while
   [0] Constructing the EventProcessor
   [1] Constructing module: class=TotemTimingRecHitProducer label='totemTimingRecHits'
Exception Message:
MissingParameter: Parameter 'timeSliceNs' not found.
----- End Fatal Exception -------------------------------------------------
Expand to see more relval errors ...

RelVals-INPUT

  • 138.4138.4_PromptCollisions+RunMinimumBias2021+ALCARECOPROMPTR3+HARVESTDPROMPTR3/step2_PromptCollisions+RunMinimumBias2021+ALCARECOPROMPTR3+HARVESTDPROMPTR3.log
  • 136.9136.9_RunDoubleMuon2016C+RunDoubleMuon2016C+RAWRECOTE16+RAWRECOLHECLEANTE16+EMBEDHADTE16+EMBEDMINIAOD16/step2_RunDoubleMuon2016C+RunDoubleMuon2016C+RAWRECOTE16+RAWRECOLHECLEANTE16+EMBEDHADTE16+EMBEDMINIAOD16.log
  • 138.5138.5_ExpressCollisions+RunMinimumBias2021+TIER0EXPRUN3+ALCARECOEXPR3+HARVESTDEXPR3/step2_ExpressCollisions+RunMinimumBias2021+TIER0EXPRUN3+ALCARECOEXPR3+HARVESTDEXPR3.log
Expand to see more relval errors ...

AddOn Tests

----- Begin Fatal Exception 11-Aug-2022 12:34:57 CEST-----------------------
An exception of category 'Configuration' occurred while
   [0] Constructing the EventProcessor
   [1] Constructing module: class=TotemTimingRecHitProducer label='totemTimingRecHits'
Exception Message:
MissingParameter: Parameter 'timeSliceNs' not found.
----- End Fatal Exception -------------------------------------------------
----- Begin Fatal Exception 11-Aug-2022 12:36:58 CEST-----------------------
An exception of category 'Configuration' occurred while
   [0] Constructing the EventProcessor
   [1] Constructing module: class=TotemTimingRecHitProducer label='totemTimingRecHits'
Exception Message:
MissingParameter: Parameter 'timeSliceNs' not found.
----- End Fatal Exception -------------------------------------------------
----- Begin Fatal Exception 11-Aug-2022 12:35:57 CEST-----------------------
An exception of category 'Configuration' occurred while
   [0] Constructing the EventProcessor
   [1] Constructing module: class=TotemTimingRecHitProducer label='totemTimingRecHits'
Exception Message:
MissingParameter: Parameter 'timeSliceNs' not found.
----- End Fatal Exception -------------------------------------------------
Expand to see more addon errors ...

cmsbuild avatar Aug 11 '22 11:08 cmsbuild

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38986/31517

  • This PR adds an extra 96KB to repository

cmsbuild avatar Aug 11 '22 11:08 cmsbuild

Pull request #38986 was updated. @civanch, @Dr15Jones, @makortel, @ianna, @clacaputo, @cmsbuild, @bsunanda, @mdhildreth, @jpata can you please check and sign again.

cmsbuild avatar Aug 11 '22 11:08 cmsbuild

@cmsbuild please test

clacaputo avatar Aug 11 '22 12:08 clacaputo

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5000cf/26757/summary.html COMMIT: a064d03e3052ac80c8698779ef196847b86110cf CMSSW: CMSSW_12_5_X_2022-08-11-1100/el8_amd64_gcc10 User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/38986/26757/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: 2 differences found in the comparisons
  • DQMHistoTests: Total files compared: 51
  • DQMHistoTests: Total histograms compared: 3692476
  • DQMHistoTests: Total failures: 8
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3692446
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 50 files compared)
  • Checked 212 log files, 49 edm output root files, 51 DQM output files
  • TriggerResults: no differences found

cmsbuild avatar Aug 11 '22 17:08 cmsbuild

@cms-sw/reconstruction-l2, @cms-sw/geometry-l2, any plan to tackle this PR in the forthcoming days?

forthommel avatar Aug 15 '22 10:08 forthommel

Vacations affect our review turnaround time during summer, but I think it's on track for the next release.

  • Which workflows does the new code run in?
  • Are any reco changes expected?
  • What is the CPU/mem profiling performance of the new code?

jpata avatar Aug 15 '22 10:08 jpata

Many thanks for your fast reaction, @jpata! And indeed I know the Summer can certainly affect the various workflows, hence my kind "ping" :) No workflow currently contain this added reconstruction scheme, as the detector is not yet integrated into cDAQ and we can only rely on simulated frames. Consequently, no reco changes are expected for standard WFs, and CPU/memory profiling is relatively difficult to evaluate outside thumbrule/scaling (only 64 readout channels are to be unpacked for low-PU end-of-year conditions). However, we can surely provide a recipe to steer the newly-introduced rechits producer algorithm, and we are more than open to discuss the rechits data format if necessary.

forthommel avatar Aug 15 '22 11:08 forthommel

No workflow currently contain this added reconstruction scheme, as the detector is not yet integrated into cDAQ and we can only rely on simulated frames.

It might be useful to consider some kind of regular testing (e.g. a sim-only workflow) for this.

Can you assess the impact on the data size?

jpata avatar Aug 15 '22 12:08 jpata

@forthommel , you will also nee to squash commits at the end.

civanch avatar Aug 15 '22 15:08 civanch

@civanch I would suggest to edit the FAQ to match with the current release managers mindset where the squashing, effectively destroying the development history of a full project, is enforced. For this particular feature development, we already collapsed 50+ commits into about 10 self-contained ones before review, but it seems that your strategy remains a mystery (at least for me).

forthommel avatar Aug 16 '22 12:08 forthommel

@forthommel , I did not wrote FAQ and would not change there anything myself. What is important - initialize class members at construction as @jpata suggested.

I also guess nT2 data is relatively small, even if all scintillators will be fired.

civanch avatar Aug 19 '22 08:08 civanch

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38986/31644

  • This PR adds an extra 88KB to repository

cmsbuild avatar Aug 19 '22 09:08 cmsbuild

Pull request #38986 was updated. @civanch, @Dr15Jones, @makortel, @ianna, @clacaputo, @cmsbuild, @bsunanda, @mdhildreth, @jpata, @mandrenguyen can you please check and sign again.

cmsbuild avatar Aug 19 '22 09:08 cmsbuild

Thanks! I just zero-initialised the rechits members in the last push.

For the rate, based on the fact the nT2 unpacking will be made under low-intensity (PU << 1) conditions, each event will deliver at most 8 (planes) * 4 (channels) * 2 (arms) = 64 rechits, with 3 (GlobalPoint) + 3 (time, time uncertainty, tot) = 6 floats/rechit. Hence the maximal raw (uncompressed, prior to L1/HLT) is 64 * 6 * 4 (bytes/float on 64-bit) = 1536 bytes/BX = 1.5 kB/BX.

forthommel avatar Aug 19 '22 09:08 forthommel

@cmsbuild please test

jpata avatar Aug 19 '22 09:08 jpata

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5000cf/26922/summary.html COMMIT: 65b6a5e64a3c12485440d682d129693f2f863feb CMSSW: CMSSW_12_5_X_2022-08-18-2300/el8_amd64_gcc10 User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/38986/26922/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: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 51
  • DQMHistoTests: Total histograms compared: 3693040
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3693016
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 50 files compared)
  • Checked 212 log files, 49 edm output root files, 51 DQM output files
  • TriggerResults: no differences found

cmsbuild avatar Aug 19 '22 14:08 cmsbuild