CTP: update of the check class #2372
Conversation
lhusova
requested review from
Barthelemy,
knopers8 and
justonedev1
as code owners
| @@ -57,6 +57,7 @@ class CTPRawDataReaderTask final : public TaskInterface | |||
| std::unique_ptr<TH1D> mHistoMTVXBC = nullptr; // histogram of BC positions to check LHC filling scheme | |||
| int mRunNumber; | |||
| long int mTimestamp; | |||
| TString classNames[65]; | |||
There was a problem hiding this comment.
Are you sure this number will never change? Could the number of class be extended elsewhere and cause an out-of-bound access in your code?
Since you use 65 elsewhere, why not make it a constant?
There was a problem hiding this comment.
Also, I would recommend using std::string unless you need a ROOT-specific feature (just because standard library is better tested than ROOT) and std::array instead of c-style array (which is also safer to use).
There was a problem hiding this comment.
the constant and std::string are implemented
| double val = mHist->GetBinContent(i); | ||
| double valPrev = mHistPrev->GetBinContent(i); | ||
| double relDiff = (valPrev != 0) ? (val - valPrev) / TMath::Sqrt(val) : 0; |
There was a problem hiding this comment.
Can val become 0? It may cause a division by zero here.
| for (size_t i = 1; i < mHist->GetXaxis()->GetNbins() + 1; i++) { // Check how many inputs/classes changed more than a threshold value | ||
| double val = mHist->GetBinContent(i); | ||
| double valPrev = mHistPrev->GetBinContent(i); | ||
| double relDiff = (valPrev != 0) ? (val - valPrev) / valPrev : 0; | ||
| double relDiff = (val != 0) ? (val - valPrev) / (val * TMath::Sqrt(1 / mHistAbs->GetBinContent(3) + 1 / mHistAbs->GetBinContent(i))) : 0; |
There was a problem hiding this comment.
I must say I am completely lost why bin 3 is used here in particular. Just to be sure, it's not a typo?
There was a problem hiding this comment.
Also, I hope mHistAbs->GetBinContent(3) is never -1 and mHistAbs->GetBinContent(i) is never 0. Are you sure it should never happen?
There was a problem hiding this comment.
Indeed it is true only for inputs, that we need the bin 3, so I did it in a more general way now.
Checking the change in nSigma instead of percentage