Strip clusterizer speedup: technical changes from #28304 #30046
Conversation
|
The code-checks are being triggered in jenkins. |
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30046/15751
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
cmsbuild
added
code-checks-rejected
code-checks-pending
and removed
code-checks-pending
code-checks-rejected
labels
|
The code-checks are being triggered in jenkins. |
this apparently depends only on values in Cond or Calib Formats (SiStripGain, SiStripNoises, SiStripQuality). I'm thinking that it's more practical to place the object in CalibFormats/SiStripObjects. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30046/15754
|
|
A new Pull Request was created by @pieterdavid (Pieter David) for master. It involves the following packages: Alignment/APEEstimation @perrotta, @andrius-k, @kmaeshima, @christopheralanwest, @tlampen, @schneiml, @tocheng, @cmsbuild, @jfernan2, @fioriNTU, @slava77, @ggovi, @pohsun can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
|
-1 Tested at: 32cb20d CMSSW: CMSSW_11_2_X_2020-05-29-1100 I found follow errors while testing this PR Failed tests: RelVals AddOn
When I ran the RelVals I found an error in the following workflows: runTheMatrix-results/158.0_HydjetQ_B12_5020GeV_2018_ppReco+HydjetQ_B12_5020GeV_2018_ppReco+DIGIHI2018PPRECO+RECOHI2018PPRECO+ALCARECOHI2018PPRECO+HARVESTHI2018PPRECO/step2_HydjetQ_B12_5020GeV_2018_ppReco+HydjetQ_B12_5020GeV_2018_ppReco+DIGIHI2018PPRECO+RECOHI2018PPRECO+ALCARECOHI2018PPRECO+HARVESTHI2018PPRECO.log11634.0 step2 runTheMatrix-results/11634.0_TTbar_14TeV+TTbar_14TeV_TuneCP5_2021_GenSimFull+DigiFull_2021+RecoFull_2021+HARVESTFull_2021+ALCAFull_2021/step2_TTbar_14TeV+TTbar_14TeV_TuneCP5_2021_GenSimFull+DigiFull_2021+RecoFull_2021+HARVESTFull_2021+ALCAFull_2021.log12434.0 step2 runTheMatrix-results/12434.0_TTbar_14TeV+TTbar_14TeV_TuneCP5_2023_GenSimFull+DigiFull_2023+RecoFull_2023+HARVESTFull_2023+ALCAFull_2023/step2_TTbar_14TeV+TTbar_14TeV_TuneCP5_2023_GenSimFull+DigiFull_2023+RecoFull_2023+HARVESTFull_2023+ALCAFull_2023.log
I found errors in the following addon tests: cmsRun /cvmfs/cms-ib.cern.ch/week0/slc7_amd64_gcc820/cms/cmssw/CMSSW_11_2_X_2020-05-29-1100/src/HLTrigger/Configuration/test/OnLine_HLT_2018.py realData=False globalTag=@ inputFiles=@ : FAILED - time: date Fri May 29 21:34:55 2020-date Fri May 29 21:21:40 2020 s - exit: 16640 |
|
Comparison not run due to runTheMatrix errors (RelVals and Igprof tests were also skipped) |
|
+1 |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
+1 |
|
+1 |
|
+1 |
|
+1 |
cmsbuild
added
db-approved
fully-signed
and removed
db-pending
pending-signatures
labels
|
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. @silviodonato, @dpiparo (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
PR description:
This PR contains the technical changes from #28304 by @VinInn , i.e. those other than the new unpacker+clusterizer module and the use of the averaged noise per APV (rather than per strip), which need some more study and validation still (I did keep the changes to make the code that uses SiStripCluster independent of the amplitudes vector type because it makes future changes easier).
I also reorganised the
StripClusterizerclass a bit: the version currently in CMSSW caches indices of the quality, noise, and gain ranges etc. in a few vectors, and creates a helper object with all the information for one module when needed. #28304 added caching of all those per-module objects and pre-calculation of 1/gain (such that the gain calibration is a multiplication instead of a division) and the average noise per module (not included here) - I kept this, but then removed the vectors fromStripClusterizersince they are not needed anymore, and moved all of this to the EventSetup to avoid having a copy for every stream.This is where I have a few questions: is
SiStripClusterizerConditionsa reasonable name for that object? I kept the header inRecoLocalTracker/SiStripClusterizerand put the record inRecoLocalTracker/Recordsby analogy with the CPE, but I'm not sure these are the recommended places. Another option is to move the whole clusterizer tool to the EventSetup (like the CPE).On the performance gain: in local tests in a rather unrealistic setup (only running strip local reco, so the limitation is IO and (de)compression) the full clusterizer seems a bit faster (that's also expected), but the numbers are not very reproducible (if there are instructions or tools to do proper benchmarking please let me know). Allocation and deallocation per event are a bit higher, but the difference is the same, so I suppose this is because of the
reservecalls (the number of allocations should be smaller).PR validation:
I dumped all reconstructed clusters in 5 data events (279076 in total) and found three digis that were different by 1 (probably due to numerical precision and truncation/rounding after applying the gain), charge/CM and noise squared (cuts in the clusterizer) are identical for all.
CC: @VinInn @vischia @robervalwalsh