CTP: update of the check class#2372
Conversation
knopers8
left a comment
There was a problem hiding this comment.
Thank you Lucia! I have some suggestions that you could consider.
| 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.
| 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