HBHE: Fix uninitialized vectors (to avoid crash) #15695
Conversation
|
A new Pull Request was created by @mariadalfonso for CMSSW_8_1_X. It involves the following packages: RecoLocalCalo/HcalRecAlgos @cmsbuild, @cvuosalo, @slava77, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are list here #13028 |
| if(channelData.id().subdet() == HcalSubdetector::HcalBarrel) noiseADCArr[ip] = psfPtr_->sigmaHPDQIE8(chargeArr[ip]); | ||
| if(channelData.id().subdet() == HcalSubdetector::HcalEndcap) noiseADCArr[ip] = psfPtr_->sigmaSiPMQIE10(chargeArr[ip]); | ||
| if(!channelData.hasTimeInfo()) noiseADCArr[ip] = psfPtr_->sigmaHPDQIE8(chargeArr[ip]); | ||
| if(channelData.hasTimeInfo()) noiseADCArr[ip] = psfPtr_->sigmaSiPMQIE10(chargeArr[ip]); |
There was a problem hiding this comment.
this is technically a new feature.
The code looks more correct for the completed phase-1 (HB as well).
... isn't it going to be QIE11, actually? Consider changing the name of the methods to either QIExx neutral or simply add the same method with QIE11 in the name if they are identical anyways (but may need to be different at some point for some reason)
There was a problem hiding this comment.
[posted too soon]
this is technically a new feature. [... ]
Please make sure to note this in the PR description.
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
| @@ -340,6 +341,13 @@ void PulseShapeFitOOTPileupCorrection::apply(const CaloSamples & cs, | |||
| fitParsVec.push_back(0.); | |||
| fitParsVec.push_back(999.); | |||
| fitParsVec.push_back(false); | |||
| } else { | |||
There was a problem hiding this comment.
It looks like the old behavior is restored this way: old version (pre10) passed a vector of parameters of size 1 on output and in this case the default settings (time = -9999 and ampl = 0) were used. OK.
Note that this method is called with ts4Min == 0 and it looks like we end up in this last "else" only if tsTOTen == 0.
Is this case singled out for a reason or was it an old bug in the method?
There was a problem hiding this comment.
@slava77 This extra else was in practice done inside the producer where the size of the returned vector was checked.
https://github.com/cms-sw/cmssw/blob/CMSSW_8_0_X/RecoLocalCalo/HcalRecAlgos/src/HcalSimpleRecAlgo.cc#L366-L369
The initial parameters I pick (time = -9999 and ampl = 0) are basically this one
https://github.com/cms-sw/cmssw/blob/CMSSW_8_0_X/RecoLocalCalo/HcalRecAlgos/src/HcalSimpleRecAlgo.cc#L322-L326
For now I just restored the old behavior, there is no need to single out this case.
I prefer to fix this workflow crash now. I can came back to this later, but prefer to have new feature in later PR.
There was a problem hiding this comment.
@mariadalfonso
Thanks for confirming that tsTOTen == 0 doesn't need to be singled out.
It can indeed be addressed later.
|
This pull request is fully signed and it will be integrated in one of the next CMSSW_8_1_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @smuzaffar |
cmsbuild
added
fully-signed
reconstruction-approved
and removed
pending-signatures
reconstruction-pending
labels
@kpedro88 @slava77 @igv4321
Fixed crash due to uninitialized vectors, due to #15499
Pass the runTheMatrix.py -l 140.53