[PWGCF] Add CFMultiplicitySet to support multiple auxilary multiplicities/centralities #12425
Conversation
|
O2 linter results: ❌ 39 errors, |
github-actions
bot
changed the title
| output(multiplicities); | ||
| } | ||
| } | ||
| PROCESS_SWITCH(MultiplicitySetBuilder, processEstimators, "Process auxiliary multiplicity/centrality estimators", false); |
There was a problem hiding this comment.
The workflow will hang if there are not default process function
Is it intended to always run this task?
There was a problem hiding this comment.
If it hangs, it remains to be solved. Most of the time it won't be needed - only in some specific cases we need the extra estimators (e.g. outliers, systematics). By default it should not run. I first thought about combining it with the existing MultiplicitySelector, but the output tables need to align with the accepted collisions.
There was a problem hiding this comment.
As the task is incorporated to the workflow and there is not process function assigned to it most of the times, the workflow will hang. At least it used to be that way
Have you tested locally?
There was a problem hiding this comment.
An empty process function subscribed to the collisions table will do the trick
There was a problem hiding this comment.
Not yet tested locally. I think if the hang still happens, I can probably do the empty process then as the enabled default.
There was a problem hiding this comment.
It could not be part of FilterCF simply?
There was a problem hiding this comment.
It can be, but then I need one (or more) processDataXYZ functions that subscribe to these multiplicity/centrality tables in addition to the existing one. For now it would add one, which is probably fine though.
| { | ||
| DECLARE_SOA_COLUMN(Multiplicities, multiplicities, std::vector<float>); //! List of auxiliary multiplicities | ||
| } | ||
| DECLARE_SOA_TABLE(CFMultiplicitySets, "AOD", "CFMULTSET", cfmultiplicityset::Multiplicities); //! Auxilary multiplicity set table |
There was a problem hiding this comment.
The name doesn't work like this for merging. The first and third names must be the same + s
| output(multiplicities); | ||
| } | ||
| } | ||
| PROCESS_SWITCH(MultiplicitySetBuilder, processEstimators, "Process auxiliary multiplicity/centrality estimators", false); |
There was a problem hiding this comment.
It could not be part of FilterCF simply?
|
I've moved the output to FilterCF, if that seems better. |
jgrosseo
requested review from
alibuild,
ariedel-cern,
glromane,
lauraser,
lgraczykCern,
otonvd,
prchakra,
saganatt,
shouqiye and
zchochul
as code owners
jgrosseo
left a comment
jgrosseo
left a comment
There was a problem hiding this comment.
I am ready to merge with the two doubts below. Feel free to still fix them in this PR or in the next one.
| if constexpr (std::experimental::is_detected<HasMultTables, typename T1::iterator>::value) { | ||
| multiplicities.clear(); | ||
| if (cfgEstimatorBitMask & kCentFT0C) | ||
| multiplicities.push_back(collision.centFT0C()); | ||
| if (cfgEstimatorBitMask & kMultFV0A) | ||
| multiplicities.push_back(collision.multFV0A()); | ||
| if (cfgEstimatorBitMask & kMultNTracksPV) | ||
| multiplicities.push_back(collision.multNTracksPV()); | ||
| if (cfgEstimatorBitMask & kMultNTracksGlobal) | ||
| multiplicities.push_back(collision.multNTracksGlobal()); | ||
| outputMultSets(multiplicities); | ||
| } | ||
|
|
There was a problem hiding this comment.
It does not matter much but in principle this code could be in processDataMults. Nothing should speak against filling one table there and the then one would not need the HasMultTables, or am missing something?
There was a problem hiding this comment.
What about the keepCollision in processDataT? Somehow the multiplicity tables would need to stay synced so they can then ultimately be joined with the collision output table.
| { | ||
| DECLARE_SOA_COLUMN(Multiplicities, multiplicities, std::vector<float>); //! List of auxiliary multiplicities | ||
| } | ||
| DECLARE_SOA_TABLE(CFMultiplicitySets, "AOD", "CFMULTIPLICITYSET", cfmultiplicityset::Multiplicities); //! Auxilary multiplicity set table |
There was a problem hiding this comment.
I remember there was a maximal number of characters (14?) for the third argument. Does this work in run time? And does the table in the file has the name o2cfmultiplicitysets?
There was a problem hiding this comment.
I will test this. I'm not able to test locally at the moment.
There was a problem hiding this comment.
You're right that the name is clipped to CFMULTIPLICITYS (or o2cfmultiplicitys). It's not ideal so I'll change it in the next PR while making other fixes too.
There was a problem hiding this comment.
Yes, otherwise the merging does not work
3 participants
No description provided.