Removing clang warning in EgammaHLTPixelMatchVarProducer #46488
Conversation
|
cms-bot internal usage |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46488/42343
|
|
A new Pull Request was created by @Sam-Harper for master. It involves the following packages:
@Martin-Grunewald, @cmsbuild, @mmusich can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@cmsbuild, please test |
RecoEgamma/EgammaHLTProducers/plugins/EgammaHLTPixelMatchVarProducer.cc
Outdated
Show resolved
Hide resolved
| int layerBit = 0x1 << layerOffset << seed.layerOrDiskNr(hitNr); | ||
|
|
||
| int layerOrDiskNr = seed.layerOrDiskNr(hitNr); | ||
| if (layerOrDiskNr == 0 || layerOrDiskNr > 4) { |
There was a problem hiding this comment.
for my education what happens in the phase-2 case? You don't foresee to make use of the full FPix ?
There was a problem hiding this comment.
ah thanks, I had understood from our brief conversation that we were still 4 layers in phase-II and of course we dont, sorry my bad.
Right now we dont take advantage of the full fpx only 1-4 and we should review this as it'll be important at high eta. But this requires a reworking of this code and more reason to have the warning in there to cause problems.
Thanks for pointing it out
There was a problem hiding this comment.
ah thanks, I had understood from our brief conversation that we were still 4 layers in phase-II and of course we dont, sorry my bad.
In phase-2 we'll still have 4 layers in BPix, but 12 disks in the endcaps:
sorry it was probably me not being clear during our conversation.
So, IIUC, you don't think this 4 should be a 12 for the time being until there's more code rework, right?
|
+1 Size: This PR adds an extra 20KB to repository Comparison SummarySummary:
|
…oducer.cc Co-authored-by: Marco Musich <[email protected]>
cmsbuild
added
tests-pending
code-checks-pending
and removed
tests-approved
code-checks-approved
labels
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46488/42347
|
|
Pull request #46488 was updated. @Martin-Grunewald, @cmsbuild, @mmusich can you please check and sign again. |
|
@cmsbuild, please test |
|
+1 Size: This PR adds an extra 20KB to repository Comparison SummarySummary:
|
PR description:
This PR fixes the clang warning reported in #46319
The warning was harmless but in looking at the code, figured would make it a bit more robust against bad data (and if we ever added a new pixel layer unlikely that it is)
So the code now explicitly verfies that the value is in range and throws an exception if caused. I also strongly suspect but did not prove 100% that numeric_limits::max is an invalid value for this specific quanity and put that as an exception as well. I'm running over a bunch of data to verify this (about 20k HLTPhysics events and will try to run on an EGamma file)
Finally it should be noted that this code is only run if productsToWrite = 2 which is only done for debugging when we wish to dump the values of this producer for studies. So it doesnt normally run in the HLT and I dont believe there is any workflow that runs this code.
So this should be a zero output change. As this code does not run normally and when it does run the data should always pass these checks.
PR validation:
Runs as expected on my local machine based on HLT timing data. Will run on E/gamma data later with productsToWrite=2 later.