Avoid storing empty DetSet containers in the pixel digi collection

[ferencek]

PR description:

This PR addresses the issue with excessive LogWarning printouts in the pixel clusterizer reported in #36326 (comment). This PR is an alternative to #36968 and tries to fix the problem at the source, in the RawToDigi code. This is a technical fix so no impact on physics is expected.

PR validation:

The code compiles and was tested to successfully run with the MinimumBias workflow 139.001.

@tsusa @mmusich

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37035/28466

• This PR adds an extra 16KB to repository

[cmsbuild]

A new Pull Request was created by @ferencek (Dinko F.) for master.

It involves the following packages:

• EventFilter/SiPixelRawToDigi (reconstruction)

@jpata, @cmsbuild, @clacaputo, @slava77 can you please review it and eventually sign? Thanks.
@OzAmram, @VinInn, @Martin-Grunewald, @missirol, @dkotlins, @ferencek, @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]

please test

[mmusich]

type bug-fix

[cmsbuild]

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2ab2f3/22591/summary.html
COMMIT: d951d12
CMSSW: CMSSW_12_3_X_2022-02-22-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/37035/22591/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: 28 differences found in the comparisons
• DQMHistoTests: Total files compared: 49
• DQMHistoTests: Total histograms compared: 3965143
• DQMHistoTests: Total failures: 32
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 3965089
• DQMHistoTests: Total skipped: 22
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
• Checked 204 log files, 45 edm output root files, 49 DQM output files
• TriggerResults: no differences found

[VinInn]

There was a rational behind the current behaviour: an empty detset signals that it has been decoded and it is empty. no detset signals that was not read... (or absent in the redout?)

[jpata]

DQM detects some differences in 136.731 / Pixel / Barrel:

are these expected?

[ferencek]

There was a rational behind the current behaviour: an empty detset signals that it has been decoded and it is empty. no detset signals that was not read... (or absent in the redout?)

Hi @VinInn, ok, I see, so the empty DetSets are a sort of diagnostic of corrupt data coming from certain modules. But isn't the purpose of the error collection edm::DetSetVector<SiPixelRawDataError> precisely to store this type of information. If I look at this comment https://github.com/cms-sw/cmssw/blob/CMSSW_12_2_0/EventFilter/SiPixelRawToDigi/plugins/SiPixelRawToDigi.cc#L241 right before PixelDataFormatter::interpretRawData(...) is called, I would naively guess there would be no empty DetSets stored in the digi collection.

[ferencek]

DQM detects some differences in 136.731 / Pixel / Barrel:

are these expected?

Hmm, I would say these are not expected based on the code changes. I also see some small differences in 4.53 / Pixel / Endcap. I will need to check these more closely (unless we decide to drop the PR altogether).

[mmusich]

Hmm, I would say these are not expected based on the code changes. I also see some small differences in 4.53 / Pixel / Endcap. I will need to check these more closely (unless we decide to drop the PR altogether).

somehow interesting these show only in the phase-0 wfs.

[ferencek]

I had a closer look at wf 136.731 where I see an empty DetSet appearing

Begin processing the 91st record. Run 274199, Event 40350490, LumiSection 21 on stream 0 at 23-Feb-2022 23:27:58.280 CET
%MSG-w clusterizeDetUnit():   SiPixelClusterProducer:siPixelClustersPreSplitting@cpu  23-Feb-2022 23:27:58 CET Run: 274199 Event: 40350490
No digis to clusterize
%MSG

and similarly in wf 4.53

Begin processing the 15th record. Run 194533, Event 462529546, LumiSection 329 on stream 0 at 24-Feb-2022 08:25:21.711 CET
%MSG-w clusterizeDetUnit():   SiPixelClusterProducer:siPixelClustersPreSplitting@cpu  24-Feb-2022 08:25:21 CET Run: 194533 Event: 462529546
No digis to clusterize
%MSG
Begin processing the 47th record. Run 194533, Event 462781441, LumiSection 329 on stream 0 at 24-Feb-2022 08:26:35.774 CET
%MSG-w clusterizeDetUnit():   SiPixelClusterProducer:siPixelClustersPreSplitting@cpu  24-Feb-2022 08:26:35 CET Run: 194533 Event: 462781441
No digis to clusterize
%MSG
Begin processing the 93rd record. Run 194533, Event 462831597, LumiSection 329 on stream 0 at 24-Feb-2022 08:28:28.169 CET
%MSG-w clusterizeDetUnit():   SiPixelClusterProducer:siPixelClustersPreSplitting@cpu  24-Feb-2022 08:28:28 CET Run: 194533 Event: 462831597
No digis to clusterize
%MSG

Specifically for wf 136.731, I checked that the total number of digis in each event remains unchanged before and after the fix and the difference in the SUMOFF_digis_Barrel appearing in ladder 65

disappears once Run: 274199 Event: 40350490 is skipped

So the differences appearing in the SUMOFF_digis_Barrel are an artifact of how the number of digis in a given module is counted. There are three options:

• DetId appearing with N pixel hits: N digis in the corresponding DetSet
• DetId appearing with no valid hits: Empty DetSet, 0 digis counted for that DetId
• DetId missing: Nothing done for that DetId

In other words, the differences we see is equivalent to comparing the average of (0, 5, 7) and the average of (5, 7) which will, of course, be different.

The multiplicity of higher level objects such as clusters and RecHits remains unchanged

So to conclude, this fix is indeed transparent to physics.

P.S. @mmusich made an interesting observation that differences seem to be showing up only for Phase 0 wfs. This is indeed confusing because empty DetSets were earlier observed by Marco in the Phase 1 wf 139.001. However, there are differences in the content of the DQM files for Phase 0

and Phase 1

so the ndigis plot showing differences in the Phase 0 wfs is actually not available for Phase 1.

[ferencek]

I applied Andrea's commit suggestion and amended it with a LogError printout.

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37035/28501

• This PR adds an extra 16KB to repository

[cmsbuild]

Pull request #37035 was updated. @jpata, @cmsbuild, @clacaputo, @slava77 can you please check and sign again.

[jpata]

@cmsbuild please test

[cmsbuild]

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2ab2f3/22669/summary.html
COMMIT: 8ce818f
CMSSW: CMSSW_12_3_X_2022-02-24-2300/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/37035/22669/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: 28 differences found in the comparisons
• DQMHistoTests: Total files compared: 49
• DQMHistoTests: Total histograms compared: 4001143
• DQMHistoTests: Total failures: 32
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 4001089
• DQMHistoTests: Total skipped: 22
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
• Checked 204 log files, 45 edm output root files, 49 DQM output files
• TriggerResults: no differences found

[clacaputo]

+reconstruction

• PR addresses excessive LogWarning printouts in the pixel clusterizer reported in Update Run3 offline data GT and mcRun3 GTs with final DD4HEP #36326 (comment)
• no reco changes expected or observed
• DMQ differences in SUMOFF_ndigis_Barrel explained in Avoid storing empty DetSet containers in the pixel digi collection #37035 (comment)
  • The multiplicity of higher level objects such as clusters and RecHits remains unchanged

[cmsbuild]

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)

[ferencek]

This PR and #36968 were discussed in yesterday's pixel offline meeting right after the news slides (https://indico.cern.ch/event/1119492/#b-448625-pixel-calibration-loc) and the conclusion was to merge both PRs with a small change in #36968 where LogWarning is turned into LogError instead of LogDebug.

[mmusich]

and the conclusion was to merge both PRs with a small change in #36968 where LogWarning is turned into LogError instead of LogDebug.

#36968 has been changed accordingly.

[perrotta]

+1