Pixel local reco: reduce code duplication in SiPixelRawToDigi related code #33884

czangela · 2021-05-28T15:30:09Z

PR description:

This PR should resolve some duplication issues, namely:

from Open issues regarding the Pixel local reconstruction on GPU #32483 reuse existing constants in SiPixelRawToClusterGPUKernel
Avoid code duplication in SiPixelRawToDigi ErrorChecker's #26731 avoid code duplication in SiPixelRawToDigi ErrorChecker's

Accordingly, the changes made can be categorized this way:

Reusing different bit manipulation constants in all the ErrorCheckers, as well as PixelDataFormatter and SiPixelRawToClusterGPUKernel
As part of this effort I introduced a new namespace with some constexpr variables and functions named sipixelconstants and placed it in DataFormats/SiPixelDigi. It should probably have a different name, and I suppose a more appropriate place would be DataFormats/SiPixelRawToDigi, but I let that the more experienced contributors decide. I just didn't want to do the fifth rebase yet.
Move common functions from ErrorCheckerPhase0 and ErrorChecker (this is phase 1) to ErrorCheckerBase
This is really not harmful.

PR validation:

Since this PR influences parts of the code which don't contribute to DQM histograms, there was a custom validation that took place.

Basically, I was just printing the FED errors before and after the changes and comparing them.

This is the description of this validation:

Phase 0 - workflow 136.731

Run workflow, modify # of events to 4000

runTheMatrix.py -e -l 136.731 --ibeos -t 4 -j 0 --maxSteps=3

Run step2 and step3
During step3 log events with FED errors.
Select events with at least one FED error:
In repository /afs/cern.ch/work/a/aczirkos/public/errorchecker_validation/phase1
the file runlumievent.txt contains 378 entries of run:lumi:event
With edmPickEventy.py use this file to filter the events

edmPickEvents.py "SinglePhoton/Run2016B-v2/RAW" <filename> --crab

Run with crab

crab submit -c pickevents_crab.py
crab getoutput

Example result file in repository /afs/cern.ch/work/a/aczirkos/public/errorchecker_validation/phase0 the file step2.root

Run on reduced dataset step3 before and after the changes.

Use some custom LogDebug to track error collection. Make sure that no messages get dropped (indicated at the end of some generated log file).

Example of adding LogDebug entry:

diff --git a/EventFilter/SiPixelRawToDigi/src/PixelDataFormatter.cc b/EventFilter/SiPixelRawToDigi/src/PixelDataFormatter.cc
index 5557f806144..02a177018b5 100644
--- a/EventFilter/SiPixelRawToDigi/src/PixelDataFormatter.cc
+++ b/EventFilter/SiPixelRawToDigi/src/PixelDataFormatter.cc
@@ -417,6 +417,11 @@ void PixelDataFormatter::unpackFEDErrors(PixelDataFormatter::Errors const& error
                                          DetErrors& nodeterrors) {
   const uint32_t dummyDetId = 0xffffffff;
   for (const auto& [errorDetId, rawErrorsVec] : errors) {
+	for (auto const& aPixelError : rawErrorsVec) {
+        LogDebug("FedErrors") << "DETID: " << errorDetId << "; FED: " << aPixelError.getFedId()
+                              << ", TYPE: " << aPixelError.getType() << ", WORD: " << aPixelError.getWord32()
+                              << ", MSG: " << aPixelError.getMessage();
+      }
     if (errorDetId == dummyDetId) {  // errors given dummy detId must be sorted by Fed
       nodeterrors.insert(nodeterrors.end(), rawErrorsVec.begin(), rawErrorsVec.end());
     } else {

Example MessageLogger entry for cmsDriver.py python config file:

# Output info to file detailedInfo.log
process.MessageLogger = cms.Service("MessageLogger",
    destinations = cms.untracked.vstring('detailedInfo'),
    categories         = cms.untracked.vstring('FedErrors'),
    debugModules  = cms.untracked.vstring('*'),
        detailedInfo          = cms.untracked.PSet(
        threshold =  cms.untracked.string('DEBUG'),
        default    =  cms.untracked.PSet(limit = cms.untracked.int32(0)),
        INFO       =  cms.untracked.PSet(limit = cms.untracked.int32(0)),
        DEBUG   = cms.untracked.PSet(limit = cms.untracked.int32(0)),
        FedErrors = cms.untracked.PSet(limit = cms.untracked.int32(10000000))
    )
)

Compare the two log files

Extract lines with FED errors

grep DETID detailedInfo.log > errors01.log

Results

Number # of FED errors by type in the validation data

      1 TYPE:30
     15 TYPE:31
    148 TYPE:37
    249 TYPE:40

In the repository /afs/cern.ch/work/a/aczirkos/public/errorchecker_validation/phase0 there are the log files, root file and python config of the validation:

ls -halt /afs/cern.ch/work/a/aczirkos/public/errorchecker_validation/phase0 
total 238M
drwxr-xr-x. 2 aczirkos zh 2,0K máj  28 15.38 .
-rw-r--r--. 1 aczirkos zh  62K máj  28 15.38 step3.py
drwxr-xr-x. 4 aczirkos zh 2,0K máj  28 15.31 ..
-rw-r--r--. 1 aczirkos zh  36K máj  25 16.01 errors_01.log
-rw-r--r--. 1 aczirkos zh  36K máj  25 16.01 errors_02.log
-rw-r--r--. 1 aczirkos zh 7,1K máj  25 16.01 select_events.txt
-rw-r--r--. 1 aczirkos zh 237M máj  25 14.37 step2.root

One can compare the errors (in bash shell) such as

diff <(sort errors_01.log) <(sort errors_02.log)

In this example errors_01.log is before and errors_02.log is after changes.
They should have no differences.

Phase 1 workflow 136.816

Run workflow, only single-threaded

runTheMatrix.py -e -l 136.816 --ibeos -t 1 -j 0 --maxSteps=3

Since FED errors are more common in the phase 1 data, running on 100 events will suffice.

Basically do the same as with phase 0 just without steps 2-5. (since almost all events have errors)

Results

Number # of FED errors by type in the validation data

  21500 TYPE:25
     93 TYPE:30
     55 TYPE:31
   6683 TYPE:37

Now from the repository /afs/cern.ch/work/a/aczirkos/public/errorchecker_validation/phase1

ls -halt /afs/cern.ch/work/a/aczirkos/public/errorchecker_validation/phase1/
total 882M
drwxr-xr-x. 2 aczirkos zh 2,0K máj  28 17.26 .
-rw-r--r--. 1 aczirkos zh 2,7M máj  28 17.25 errors_02.log
-rw-r--r--. 1 aczirkos zh 2,7M máj  28 16.49 errors_01.log
-rw-r--r--. 1 aczirkos zh  20K máj  28 15.38 step3.py
-rw-r--r--. 1 aczirkos zh 876M máj  28 15.36 step2.root
drwxr-xr-x. 4 aczirkos zh 2,0K máj  28 15.31 ..

And comparing the results (again, errors_01.log is before and errors_02.log is after changes)

diff <(sort errors_01.log) <(sort errors_02.log)

Again, no differences should appear.

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

it is not

…orCheckerBase - common functions: setErrorStatus, checkCRC, checkHeader, checkHeader, checkTrailer - copy bit manipulation constants to ErrorCheckerBase

…eckerPhase0 to SiPixelDigiConstants - remove duplicated typedefs for Word32 and Word64

…elDigiConstants - special bits and shifts for phase 1 layer 1 modules -> inline namespace - transfer comments from PixelDataFormatter

…it manipulation - constexpr functions are also callable from device if compiled with the right flag

- SiPixelRawToClusterGPUKernel.cu

- separate function to determine errorType and to issue the log message - add to error collection if the returned errorType is non-zero and includeErrors is true

- avoid multiple return statements

- includeErrors -> includeErrors_

- more specifically the default constructors in ErrorChecker and ErrorCheckerPhase0

…iple files - changed in ErrorChecker, ErrorCheckerBase and ErrorCheckerPhase0

- use in checkCRC member function

- between SiPixelDigiContants.h and SiPixelRawToClusterGPUKernel.h

- in SiPixelRawToClusterGPUKernel.cu

- except for setErrorStatus

- <name> -> <name>_

- change inheritance to protected in ErrorChecker*

cmsbuild · 2021-05-29T19:56:56Z

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

It involves the following packages:

DataFormats/SiPixelDigi
EventFilter/SiPixelRawToDigi
RecoLocalTracker/SiPixelClusterizer

@perrotta, @civanch, @mdhildreth, @cmsbuild, @slava77, @jpata can you please review it and eventually sign? Thanks.
@mtosi, @OzAmram, @makortel, @felicepantaleo, @GiacomoSguazzoni, @JanFSchulte, @rovere, @VinInn, @Martin-Grunewald, @ferencek, @dkotlins, @gpetruc, @threus, @tvami this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

mmusich · 2021-05-30T09:12:16Z

@cmsbuild, please test

cmsbuild · 2021-05-30T13:48:38Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-91f7e5/15427/summary.html
COMMIT: 4911f50
CMSSW: CMSSW_12_0_X_2021-05-29-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/33884/15427/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: 7 differences found in the comparisons
DQMHistoTests: Total files compared: 37
DQMHistoTests: Total histograms compared: 2650486
DQMHistoTests: Total failures: 12
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 2650451
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.004 KiB( 36 files compared)
DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
Checked 155 log files, 37 edm output root files, 37 DQM output files
TriggerResults: no differences found

civanch · 2021-06-02T20:23:32Z

+1

slava77 · 2021-06-03T03:25:39Z

enable gpu

slava77 · 2021-06-03T03:25:47Z

@cmsbuild please test

cmsbuild · 2021-06-03T10:17:38Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-91f7e5/15585/summary.html
COMMIT: 4911f50
CMSSW: CMSSW_12_0_X_2021-06-02-2300/slc7_amd64_gcc900
Additional Tests: GPU
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/33884/15585/install.sh to create a dev area with all the needed externals and cmssw changes.

GPU Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 4
DQMHistoTests: Total histograms compared: 9533
DQMHistoTests: Total failures: 0
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 9533
DQMHistoTests: Total skipped: 0
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 3 files compared)
Checked 12 log files, 9 edm output root files, 4 DQM output files
TriggerResults: no differences found

Comparison Summary

Summary:

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

jpata · 2021-06-04T11:37:50Z

+reconstruction

technical, no reco changes expected or observed neither in standard or GPU workflows

cmsbuild · 2021-06-04T11:38:13Z

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

qliphy · 2021-06-05T01:28:55Z

+1

czangela added 23 commits May 5, 2021 14:12

move common functions from ErrorChecker and ErrorCheckerPhase0 to Err…

c5409d1

…orCheckerBase - common functions: setErrorStatus, checkCRC, checkHeader, checkHeader, checkTrailer - copy bit manipulation constants to ErrorCheckerBase

move common constants from ErrorCheckerBase, ErrorChecker and ErrorCh…

9b53b07

…eckerPhase0 to SiPixelDigiConstants - remove duplicated typedefs for Word32 and Word64

make common ErrorCheckerBase functions non-virtual

c240102

apply naming convention to include guards

c4face3

add extra bit manipulation constants from PixelDataFormatter to SiPix…

8c8e8cb

…elDigiConstants - special bits and shifts for phase 1 layer 1 modules -> inline namespace - transfer comments from PixelDataFormatter

add (inline constexpr) getter functions to SiPixelDigiConstants for b…

e234671

…it manipulation - constexpr functions are also callable from device if compiled with the right flag

use sipixeldigicontants in PixelDataFormatter

75bc960

use newly added bit manipulation functions in PixelDataFormatter

75604ec

use newly added bit manipulation functions in GPU code

1346062

- SiPixelRawToClusterGPUKernel.cu

refactor ErrorCheckerBase::conversionError

6d59523

- separate function to determine errorType and to issue the log message - add to error collection if the returned errorType is non-zero and includeErrors is true

refactor ErrorCheckerBase::checkCRC member function

e181995

- avoid multiple return statements

Apply naming convention to member variable includeErrors

d85e5bd

- includeErrors -> includeErrors_

delegate default constructors in derived error checkers to base

ed30d4e

- more specifically the default constructors in ErrorChecker and ErrorCheckerPhase0

include SiPixelFormatterErrors instead of type aliasing Error in mult…

59c40b9

…iple files - changed in ErrorChecker, ErrorCheckerBase and ErrorCheckerPhase0

update type aliases in PixelDataFormatter

1e21a16

add new member function addErrorToCollectionDummy to ErrorCheckerBase

d011578

- use in checkCRC member function

refactor ErrorCheckerBase::checkHeader with new member function

eb6b264

refactor ErrorCheckerBase::checkTrailer with new member function

53ead58

reduce duplication of (sipixel) raw data constants (bits, shift, etc.)

e701f34

- between SiPixelDigiContants.h and SiPixelRawToClusterGPUKernel.h

use bit manipulation functions in GPU code (again)

fa6c6a2

- in SiPixelRawToClusterGPUKernel.cu

make ErroChecker* member functions const

0cb380d

- except for setErrorStatus

apply naming convention to PixelDataFormatter members

c616c19

- <name> -> <name>_

fix redeclaring the includeErrors_ member in derived classes

266bde5

- change inheritance to protected in ErrorChecker*

cmsbuild added this to the CMSSW_12_0_X milestone May 28, 2021

cmsbuild added code-checks-pending orp-pending pending-signatures reconstruction-pending simulation-pending tests-pending labels May 28, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels May 29, 2021

cmsbuild added tests-started and removed tests-pending labels May 30, 2021

cmsbuild added tests-approved and removed tests-started labels May 30, 2021

cmsbuild added simulation-approved and removed simulation-pending labels Jun 2, 2021

cmsbuild added tests-started and removed tests-approved labels Jun 3, 2021

cmsbuild added tests-approved and removed tests-started labels Jun 3, 2021

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels Jun 4, 2021

cmsbuild added orp-approved and removed orp-pending labels Jun 5, 2021

cmsbuild merged commit a0ba0d9 into cms-sw:master Jun 5, 2021

slava77 mentioned this pull request Jul 22, 2021

Avoid code duplication in SiPixelRawToDigi ErrorChecker's #26731

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pixel local reco: reduce code duplication in SiPixelRawToDigi related code #33884

Pixel local reco: reduce code duplication in SiPixelRawToDigi related code #33884

czangela commented May 28, 2021

cmsbuild commented May 29, 2021

mmusich commented May 30, 2021

cmsbuild commented May 30, 2021

civanch commented Jun 2, 2021

slava77 commented Jun 3, 2021

slava77 commented Jun 3, 2021

cmsbuild commented Jun 3, 2021

jpata commented Jun 4, 2021

cmsbuild commented Jun 4, 2021

qliphy commented Jun 5, 2021

Pixel local reco: reduce code duplication in SiPixelRawToDigi related code #33884

Pixel local reco: reduce code duplication in SiPixelRawToDigi related code #33884

Conversation

czangela commented May 28, 2021

PR description:

PR validation:

This is the description of this validation:

Phase 0 - workflow 136.731

Results

Number # of FED errors by type in the validation data

Phase 1 workflow 136.816

Results

Number # of FED errors by type in the validation data

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

cmsbuild commented May 29, 2021

mmusich commented May 30, 2021

cmsbuild commented May 30, 2021

Comparison Summary

civanch commented Jun 2, 2021

slava77 commented Jun 3, 2021

slava77 commented Jun 3, 2021

cmsbuild commented Jun 3, 2021

GPU Comparison Summary

Comparison Summary

jpata commented Jun 4, 2021

cmsbuild commented Jun 4, 2021

qliphy commented Jun 5, 2021