FastTimerService: removed explicit calls to MonitorElement::getTH1

[missirol]

PR description:

This PR is a simple follow-up to #32343 (see this comment): now that the MonitorElement::setStatOverflows implementation has been updated in #32364, it is possible to remove explicit calls to getTH1() in the FastTimerService, in compliance with DQM recommendations.

No changes expected (this is only a change in syntax).

FYI: @jfernan2

PR validation:

Code compiles.

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32726/20867

• 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]

please test

[cmsbuild]

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f06192/12500/summary.html
COMMIT: 9c4cbf0
CMSSW: CMSSW_11_3_X_2021-01-24-0000/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/32726/12500/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: 37
• DQMHistoTests: Total histograms compared: 2716961
• DQMHistoTests: Total failures: 7
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 2716932
• DQMHistoTests: Total skipped: 22
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 36 files compared)
• Checked 156 log files, 37 edm output root files, 37 DQM output files

[jfernan2]

Thanks @missirol !

[jfernan2]

+1

[Martin-Grunewald]

(1) seems like a magic number, how about (kTrue) as used in other PRs?

[missirol]

Good point, Martin. I thought it might be better to use the same type as required by the function (unsigned int)

cmssw/DQMServices/Core/src/MonitorElement.cc

Line 907 in dc7bcc5

void MonitorElement::setStatOverflows(unsigned int value) {

It's not clear to me why MonitorElement::setStatOverflows internally resorts to kTRUE. Naively, I would have expect something like (pseudocode)

  void MonitorElement::setStatOverflows(unsigned int value) {
    auto access = this->accessMut();
    if (value == 0)
      access.value.object_->SetStatOverflows(TH1::kIgnore);
    else if (value == 1)
      access.value.object_->SetStatOverflows(TH1::kConsider);
    else if (value == 2)
      access.value.object_->SetStatOverflows(TH1::kNeutral);
    else
      WARNING
  }

(see: https://root.cern.ch/doc/v622/classTH1.html#a476f6c171ecaad7e811fd615e8a2468a)

@jfernan2 do you have any suggestions?

Anyways, I can certainly change to kTRUE; it will give the same result.

[jfernan2]

IMHO, kNeutral option is misleading and we should avoid it since the global flag in CMSSW is unclear: behaviour should be to consider overflows or not.

I sticked to kTrue since it implied the least number of modifications around:
https://github.com/cms-sw/cmssw/search?q=setStatOverflows

but I am open to suggestions and to move everything to numbers. I don't have a strong preference

[missirol]

Hi @jfernan2 , thanks for your comment. I don't have a strong opinion either, here are just my 2 cents:

• if the idea is to enforce "overflows or not", as you mentioned, then it might be clearer to change the argument of MonitorElement::setStatOverflows to bool instead of unsigned int

• an alternative would be to just let MonitorElement::setStatOverflows(unsigned int) be a direct interface to TH1::SetStatOverflows (leaving it up to the user to call it properly), along the lines of the pseudocode written above. In this second case, existing code using kTRUE will still work as intended, even though it would become clearer to call myME.setStatOverflows(1).

The only weak spot I see in the current impl is that if someone mistakenly calls myME.setStatOverflows(10) (random number), that will silently translate to kIgnore.

My 2 cents aside, unless there are other comments on this by you or others, later today I will change this to kTRUE in this PR, just to be consistent with how the function is used elsewhere in CMSSW.

[fwyzard]

* if the idea is to enforce "overflows or not", as you mentioned, then it might be clearer to change the argument of `MonitorElement::setStatOverflows` to `bool` instead of `unsigned int`

This is what seems to make the most sense to me.

[fwyzard]

please test

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32726/20883

• This PR adds an extra 24KB to repository

[cmsbuild]

Pull request #32726 was updated. @Martin-Grunewald, @fwyzard can you please check and sign again.

[jfernan2]

I have created:
#32737
To set bool the function parameter as discussed.
Thanks

[cmsbuild]

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f06192/12519/summary.html
COMMIT: 2c22465
CMSSW: CMSSW_11_3_X_2021-01-24-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/32726/12519/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: 8 differences found in the comparisons
• DQMHistoTests: Total files compared: 37
• DQMHistoTests: Total histograms compared: 2716961
• DQMHistoTests: Total failures: 12
• DQMHistoTests: Total nulls: 1
• DQMHistoTests: Total successes: 2716926
• 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 156 log files, 37 edm output root files, 37 DQM output files

[fwyzard]

+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)

[qliphy]

+1