removal of TH1::StatOverflows calls in FastTimerService?

[missirol]

PR description:

I stumbled upon the fact that the calls to the static member function TH1::StatOverflows in the FastTimerService change some of the global TH1 settings, and can thus affect histograms produced by unrelated modules in the same job.

The PR suggests a change to avoid this, while keeping the outputs of the FastTimerService unchanged.

PR validation:

Verified that the mean/RMS of some of the FastTimerService outputs still include underflows and overflows after these updates (admittedly, a very thorough validation was not done).

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32343/20173

• This PR adds an extra 24KB to repository

[cmsbuild]

A new Pull Request was created by @missirol (Marino Missiroli) for master.

It involves the following packages:

HLTrigger/Timer

@Martin-Grunewald, @cmsbuild, @fwyzard can you please review it and eventually sign? Thanks.
@Martin-Grunewald 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

[fwyzard]

Thanks Marino, glad to see ROOT has finally implemented a per-object flag. I'll have to double check, but in principle looks good.

[fwyzard]

please test

[cmsbuild]

The tests are being triggered in jenkins.

• CMSSW_11_3_X_2020-11-30-1100/slc7_amd64_gcc900: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/11184/console Started: 2020/11/30 21:27

[cmsbuild]

+1
Tested at: 8767f28
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-da4659/11184/summary.html
CMSSW: CMSSW_11_3_X_2020-11-30-1100
SCRAM_ARCH: slc7_amd64_gcc900

[cmsbuild]

Comparison job queued.

[cmsbuild]

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-da4659/11184/summary.html

Comparison Summary:

• No significant changes to the logs found
• Reco comparison results: 13 differences found in the comparisons
• DQMHistoTests: Total files compared: 35
• DQMHistoTests: Total histograms compared: 2529593
• DQMHistoTests: Total failures: 37
• DQMHistoTests: Total nulls: 1
• DQMHistoTests: Total successes: 2529533
• DQMHistoTests: Total skipped: 22
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.004 KiB( 34 files compared)
• DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
• Checked 148 log files, 34 edm output root files, 35 DQM output files

[Martin-Grunewald]

+1

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

[cmsbuild]

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-da4659/11230/summary.html

Comparison Summary:

• No significant changes to the logs found
• Reco comparison results: 8 differences found in the comparisons
• DQMHistoTests: Total files compared: 35
• DQMHistoTests: Total histograms compared: 2529593
• DQMHistoTests: Total failures: 33
• DQMHistoTests: Total nulls: 1
• DQMHistoTests: Total successes: 2529537
• DQMHistoTests: Total skipped: 22
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: -0.004 KiB( 34 files compared)
• DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
• Checked 148 log files, 37 edm output root files, 35 DQM output files

[missirol]

The differences in 23234.0, 23434.999 and 28234.0 might indeed be caused by this update, since the bins of the relevant histograms seem to be filled with mean/RMS values (and the FastTimerService is used in those workflows).

One note: one might wonder why changes don't show up in other wfs as well (assuming FastTimerService is also run in some other wfs). One possibility (but this is speculation) is that such wfs include at least one module in which MonitorElement::setStatOverflows is called; even though that is a "per element" call, it still uses the static function TH1::StatOverflows, which affects other TH1s in the job; something not obvious that I thought was worth mentioning (maybe worth mentioning to DQM?).

[fwyzard]

One note: one might wonder why changes don't show up in other wfs as well (assuming FastTimerService is also run in some other wfs). One possibility (but this is speculation) is that such wfs include at least one module in which MonitorElement::setStatOverflows is called; even though that is a "per element" call, it still uses the static function TH1::StatOverflows, which affects other TH1s in the job; something not obvious that I thought was worth mentioning (maybe worth mentioning to DQM?).

setStatOverflows is used by

• DQM/SiStripMonitorCluster/src/SiStripMonitorCluster.cc
• DQM/SiStripMonitorTrack/src/SiStripMonitorTrack.cc
• DQM/SiStripMonitorDigi/src/SiStripMonitorDigi.cc
• DQMOffline/EGamma/src/ElectronDqmAnalyzerBase.cc

while TH1::StatOverflows is also used by

• DPGAnalysis/SiStripTools/plugins/APVShotsAnalyzer.cc
• DQMOffline/EGamma/src/ElectronDqmHarvesterBase.cc

Few enought that it should be simple to make a change in the central DQM code and replace the call to TH1::StatOverflows in the same was as done in this PR everywhere else - and definitely something the DQM people might look into.

[fwyzard]

@cms-sw/dqm-l2 FYI

[jfernan2]

Thanks @fwyzard
I will seek for this kind of calls in DQM packages. I see that some error messages and warnings registered in Messagelogger by DQM modules are being reduced with this PR too

[silviodonato]

assign dqm

[cmsbuild]

New categories assigned: dqm

@jfernan2,@andrius-k,@fioriNTU,@kmaeshima,@ErnestaP you have been requested to review this Pull request/Issue and eventually sign? Thanks

[jfernan2]

There are several changes
eg.
https://tinyurl.com/y3mansv3
Muons/Tests/trackResidualsTest/MeanTest_phi

I believe the changes are not affecting much the final result in this case, at least with 10 event stats of the workflows, it seems in this case that under/overflows are moving the residual mean closer to 0 in the y-axis
It is difficult to know which one is correct

[silviodonato]

merge
(discussed today at the ORP meeting)

[jfernan2]

+1
For the records

[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 be automatically merged.