EgammaAnalysis/ElectronTools: potential bug fix, add parens to set implied order of + and ?

[gartung]

These clang warnings imply a possible bug because + is evaluated before ?. Also clean up of test class functions.

/build/cmsbld/jenkins-workarea/workspace/build-any-ib/w/tmp/BUILDROOT/ba81249bb4729771235b98cb83b6e8d8/opt/cmssw/slc6_amd64_gcc530/cms/cmssw/CMSSW_9_0_CLANG_X_2016-12-13-1100/src/EgammaAnalysis/ElectronTools/src/SimplePhoton.cc:8:56: warning: operator '?:' has lower precedence than '+'; '+' will be evaluated first [-Wparentheses]
scEnergy_(in.superCluster()->rawEnergy() + in.isEB() ? 0 : in.superCluster()->preshowerEnergy()),
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
/build/cmsbld/jenkins-workarea/workspace/build-any-ib/w/tmp/BUILDROOT/ba81249bb4729771235b98cb83b6e8d8/opt/cmssw/slc6_amd64_gcc530/cms/cmssw/CMSSW_9_0_CLANG_X_2016-12-13-1100/src/EgammaAnalysis/ElectronTools/src/SimplePhoton.cc:8:56: note: place parentheses around the '+' expression to silence this warning
scEnergy_(in.superCluster()->rawEnergy() + in.isEB() ? 0 : in.superCluster()->preshowerEnergy()),
^
( )
/build/cmsbld/jenkins-workarea/workspace/build-any-ib/w/tmp/BUILDROOT/ba81249bb4729771235b98cb83b6e8d8/opt/cmssw/slc6_amd64_gcc530/cms/cmssw/CMSSW_9_0_CLANG_X_2016-12-13-1100/src/EgammaAnalysis/ElectronTools/src/SimplePhoton.cc:8:56: note: place parentheses around the '?:' expression to evaluate it first
scEnergy_(in.superCluster()->rawEnergy() + in.isEB() ? 0 : in.superCluster()->preshowerEnergy()),
^
( )
1 warning generated.
/build/cmsbld/jenkins-workarea/workspace/build-any-ib/w/tmp/BUILDROOT/ba81249bb4729771235b98cb83b6e8d8/opt/cmssw/slc6_amd64_gcc530/cms/cmssw/CMSSW_9_0_CLANG_X_2016-12-13-1100/src/EgammaAnalysis/ElectronTools/src/SimpleElectron.cc:7:62: warning: operator '?:' has lower precedence than '+'; '+' will be evaluated first [-Wparentheses]
scEnergy_(in.superCluster()->rawEnergy() + in.isEB() ? 0 : in.superCluster()->preshowerEnergy()),
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
/build/cmsbld/jenkins-workarea/workspace/build-any-ib/w/tmp/BUILDROOT/ba81249bb4729771235b98cb83b6e8d8/opt/cmssw/slc6_amd64_gcc530/cms/cmssw/CMSSW_9_0_CLANG_X_2016-12-13-1100/src/EgammaAnalysis/ElectronTools/src/SimpleElectron.cc:7:62: note: place parentheses around the '+' expression to silence this warning
scEnergy_(in.superCluster()->rawEnergy() + in.isEB() ? 0 : in.superCluster()->preshowerEnergy()),
^
( )
/build/cmsbld/jenkins-workarea/workspace/build-any-ib/w/tmp/BUILDROOT/ba81249bb4729771235b98cb83b6e8d8/opt/cmssw/slc6_amd64_gcc530/cms/cmssw/CMSSW_9_0_CLANG_X_2016-12-13-1100/src/EgammaAnalysis/ElectronTools/src/SimpleElectron.cc:7:62: note: place parentheses around the '?:' expression to evaluate it first
scEnergy_(in.superCluster()->rawEnergy() + in.isEB() ? 0 : in.superCluster()->preshowerEnergy()),
^
( )

[cmsbuild]

A new Pull Request was created by @gartung (Patrick Gartung) for CMSSW_9_0_X.

It involves the following packages:

EgammaAnalysis/ElectronTools

@cmsbuild, @monttj, @davidlange6 can you please review it and eventually sign? Thanks.
@Sam-Harper, @rafaellopesdesa this is something you requested to watch as well.
@slava77, @smuzaffar you are the release manager for this.

cms-bot commands are listed here #13028

[gartung]

@Dr15Jones please test

[Dr15Jones]

How about adding override?

[cmsbuild]

Pull request #17014 was updated. @cmsbuild, @monttj, @davidlange6 can you please check and sign again.

[gartung]

@Dr15Jones I added the override keyword as needed.

[monttj]

@cmsbuild please test

[cmsbuild]

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/17151/console Started: 2016/12/23 09:34

[cmsbuild]

+1
Tested at: f390de2
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-17014/17151/summary.html

[cmsbuild]

Comparison job queued.

[cmsbuild]

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

[monttj]

+1
With this PR, the condition is now clear and fixed.

[cmsbuild]

This pull request is fully signed and it will be integrated in one of the next CMSSW_9_0_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @smuzaffar

[davidlange6]

Hi @slava77 -can this bug affect the reco?

[slava77]

@davidlange6
some non-exhaustive "git grep" shows that only analysis use cases are affected by this specific code.

@rafaellopesdesa @fcouderc
please clarify if the bugfix affects the analysis recipes only (and if it is a significant effect).
Does it also affect the procedure used to derive logic for the egamma energy calibration (which can indirectly impact reco)

[rafaellopesdesa]

Hi Slava,

No, it does not affect the calibration in the RECO sequence.

We used the SimpleElectron class for the EGMSmearer (data-MC residuals in EGM calibration) in 2015/2016 but the bug does not seem to alter the output since this variable is not used (the input variable for the Ep combination after data-MC residual application is SimpleElectron::newEnergy_, which is copied from GsfElectron::correctedEcalEnergy).

I am not aware of any current use of the SimplePhoton class in RECO or in any EGM-maintained user application. Maybe someone is using privately, but then I have no way to know.

I hope this clarify, cheers.

[davidlange6]

it would be great to just delete SimplePhoton if its not used. Thanks

[davidlange6]

+1