[PWGCF] : Flow Event Plane #13550
[PWGCF] : Flow Event Plane #13550
Conversation
Updated histogram types and added CCDB object structure to consume less memory
yashpatley
requested review from
alibuild,
ariedel-cern,
glromane,
lauraser,
lgraczykCern,
otonvd,
prchakra,
shouqiye,
victor-gonzalez and
zchochul
as code owners
|
O2 linter results: ❌ 0 errors, |
| struct CcdbObjects { | ||
| TList* ccdbList; | ||
| TObject* obj; | ||
| TProfile* hp; | ||
| THnSparseF* hn; | ||
| } ccdbObjects; |
There was a problem hiding this comment.
Why is a full global structure needed to store entities that are used locally and temporarily?
There was a problem hiding this comment.
No it wasn't necessary, it was for the bookkeeping. But I will change it.
| ccdbObjects.hp = reinterpret_cast<TProfile*>(ccdbObjects.obj->Clone()); | ||
| vAvgOutput[cntrx] += ccdbObjects.hp->GetBinContent(ccdbObjects.hp->GetXaxis()->FindBin(vCollParam[cntry])); | ||
| delete ccdbObjects.hp; |
There was a problem hiding this comment.
I would not recommend allocating and deallocating objects on a per collision level
Is it really needed to clone the CCDB objects?
There was a problem hiding this comment.
I can't think of any other way other then fetching histograms from CCDB on per collision level, let me know if there is another way of getting them. I am not sure about the cloning at the moment, but I'll try without that, I think its possible.
| for (auto const& x : vHistNames) { | ||
| int cntry = 0; | ||
| for (auto const& y : x) { | ||
| TObject* obj = reinterpret_cast<TObject*>(ccdbObject->FindObject(y.c_str())); | ||
| ccdbObjects.obj = reinterpret_cast<TObject*>(ccdbObject->FindObject(y.c_str())); | ||
| if (corrType == kFineCorr) { | ||
| TProfile* hp = reinterpret_cast<TProfile*>(obj->Clone()); | ||
| vAvgOutput[cntrx] += hp->GetBinContent(hp->GetXaxis()->FindBin(vCollParam[cntry])); | ||
| ccdbObjects.hp = reinterpret_cast<TProfile*>(ccdbObjects.obj->Clone()); | ||
| vAvgOutput[cntrx] += ccdbObjects.hp->GetBinContent(ccdbObjects.hp->GetXaxis()->FindBin(vCollParam[cntry])); | ||
| delete ccdbObjects.hp; | ||
| } else { | ||
| THnF* hn = reinterpret_cast<THnF*>(obj->Clone()); | ||
| ccdbObjects.hn = reinterpret_cast<THnSparseF*>(ccdbObjects.obj->Clone()); | ||
| for (int i = 0; i < static_cast<int>(vHistNames.size()); ++i) { | ||
| binarray[i] = hn->GetAxis(i)->FindBin(vCollParam[i]); | ||
| binarray[i] = ccdbObjects.hn->GetAxis(i)->FindBin(vCollParam[i]); | ||
| } | ||
| vAvgOutput[cntrx] += hn->GetBinContent(hn->GetBin(binarray)); | ||
| vAvgOutput[cntrx] += ccdbObjects.hn->GetBinContent(ccdbObjects.hn->GetBin(binarray)); | ||
| delete ccdbObjects.hn; | ||
| } | ||
| ++cntry; |
There was a problem hiding this comment.
This code is cryptic
Cryptic code is really hard to maintain and to evolve
I would recommend code that is readable with meaningful variable names and no magic numbers
There was a problem hiding this comment.
For the moment it may seem like "cryptic", but please give me a few iteration, I will try to modify it.
victor-gonzalez
left a comment
victor-gonzalez
left a comment
There was a problem hiding this comment.
I will approve it for the time being but please have a look at my comments
Programming languages provide powerful syntax to do whatever one wants to do
One must choose the constructions that are easier to maintain and to evolve. Of course, one is free for choosing the worst constructions but this will have side effects one has to be ready to confront
2 participants
Updated histogram types and added CCDB object structure to consume less memory