Fix bit value checks in HcalNoiseInfoProducer

[makortel]

PR description:

Comparison like (value & (1<<shift)) == 1 for shift > 1 will always return false. Assuming that the intention was to check with == 1 that the corresponding bit was set, and with == 0 that it was not set, this PR suggests to replace the explicit bit operations with calls to HcalChannelStatus::isBitSet(). These were found by GCC 9.

PR validation:

The code compiles without warnings with GCC 9.

[cmsbuild]

The code-checks are being triggered in jenkins.

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26556/9456

• This PR adds an extra 24KB to repository

[cmsbuild]

A new Pull Request was created by @makortel (Matti Kortelainen) for master.

It involves the following packages:

RecoMET/METProducers

@perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@TaiSakuma, @gouskos, @ahinzmann, @mmarionncern, @jdamgov, @jdolen, @nhanvtran, @gkasieczka, @clelange, @schoef, @mariadalfonso, @seemasharmafnal this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

[makortel]

@cmsbuild, please test

[cmsbuild]

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/34378/console Started: 2019/04/26 23:44

[cmsbuild]

+1
Tested at: f74875f
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-26556/34378/summary.html

[cmsbuild]

Comparison job queued.

[cmsbuild]

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

Comparison Summary:

• No significant changes to the logs found
• Reco comparison results: 3 differences found in the comparisons
• DQMHistoTests: Total files compared: 33
• DQMHistoTests: Total histograms compared: 3211964
• DQMHistoTests: Total failures: 1
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 3211759
• DQMHistoTests: Total skipped: 204
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 32 files compared)
• Checked 137 log files, 14 edm output root files, 33 DQM output files

[perrotta]

@jkunkle @igv4321 @abdoulline @deguio @mariadalfonso @eioannou : could any of you please check and confirm?

[abdoulline]

@perrotta

Andrea, just to let you know - Josh (jkunkle) and Federico (deguio) aren't involved (at least directly) in HCAL business anymore.
Yesterday I've asked Jae (@jaehyeok) and new HCAL Noise group convener Chandiprasad Kar
to evaluate possible effect of this fix...

[perrotta]

@perrotta

Andrea, just to let you know - Josh (jkunkle) and Federico (deguio) aren't involved (at least directly) in HCAL business anymore.
Yesterday I've asked Jae (@jaehyeok) and new HCAL Noise group convener Chandiprasad Kar
to evaluate possible effect of this fix...

Thank you Salavat! I'm waiting for some feedback from the group then, just to confirm that the original intention of the authors are correctly mirrored in this PR

[igv4321]

Just by eyeballing the changes in the code, this fix definitely looks like a step in the right direction. But, of course, the HCAL Noise group will provide more informative feedback...

[perrotta]

type bugfix

[perrotta]

@abdoulline @jaehyeok : did you already evaluate the possible effects of this fix? As the code changes look quite reasonable, it is just a matter of getting confirmation from your side that there are no unwanted side effects, and then this PR can be accepted for the integration: it wold be nice doing it before the pre5 deadline of tomorrow.

[jaehyeok]

Hi @perrotta,

Let me check with the guy who has been working on it. I will get back to you soon.

[cpkar]

Hi @abdoulline and @jaehyeok ,
I have made a comparison plot of the noise variables before and after the code fix.
Initially, I have taken 2500 events from MET data set[1] and looked at
the noise variables before and after the fix in the code, the plots are
matching well with each other[2]. similar conclusion is also drawn from the
SingleMuon dataset.

Thanks

[1] file:/eos/cms/store/data/Run2018A/MET/RAW/v1/000/316/995/00000/CC6B1013-4462-E811-B884-FA163EE3010D.root
[2] hbhe.pdf

[perrotta]

Hi @abdoulline and @jaehyeok ,
I have made a comparison plot of the noise variables before and after the code fix.
Initially, I have taken 2500 events from MET data set[1] and looked at
the noise variables before and after the fix in the code, the plots are
matching well with each other[2]. similar conclusion is also drawn from the
SingleMuon dataset.

Thanks

[1] file:/eos/cms/store/data/Run2018A/MET/RAW/v1/000/316/995/00000/CC6B1013-4462-E811-B884-FA163EE3010D.root
[2] hbhe.pdf

Thank you @cpkar
Are you meaning that the fix is uneffective, then?

[abdoulline]

(better late than never...) Sorry for the late reply.
Now, after discussing with Jae in HCAL thread :

as far as I can see, the fix is applied to the HCAL Channel status bits, which aren't set in DB.
I.e. these bits are reserved/enumerated
https://cmssdt.cern.ch/lxr/source/CondFormats/HcalObjects/interface/HcalChannelStatus.h#0019
but not actually used, e.g. in the last 2018 IOV of HcalCannelQuality (DB dump)
https://twiki.cern.ch/twiki/pub/CMS/HcalChannelQualityTags2011/HcalChannelQuality_2018_v4.1_data.txt
there are only dead cells explicitly listed (as non-0).

That's why there is no difference in Chandi's test.

[perrotta]

+1

• A ill formed set of instructions in the code of HcalNoiseInfoProducer is fixed here
• According to the result of the investigations done by the HCAL experts (Fix bit value checks in HcalNoiseInfoProducer #26556 (comment)), that part of code is not actually used: that's why no difference is visible in the tests outputs
• It is worth to fix the code anyhow

[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. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2)

[fabiocos]

+1