PWGHF: addition of D0 ML response class#3789
Conversation
Please consider the following formatting changes to #3789
|
Hello @atavirag , I don't have any comments on the header but I have a few regarding the selector, otherwise it looks good to me :)
|
|
Hi @AlexBigO, I have just added your comments. Thank you! |
| } | ||
| hfMlResponse.cacheInputFeaturesIndices(namesInputFeatures); | ||
| hfMlResponse.init(); | ||
| outputMl.assign(((std::vector<int>)cutDirMl).size(), -1.f); // dummy value for ML output |
There was a problem hiding this comment.
Everything is okay on my side. Just a minor comment, now you don't need this line (the one where you assign dummy values to the outputMl vector) anymore as the outputMl vector (defined above init) is going to be passed by reference in isSelectedMl.
atavirag
requested review from
NicoleBastid,
adubla,
alibuild,
deepathoms,
fcatalan92,
fcolamar,
fgrosa,
gtaillepied,
mfaggin,
mmazzilli,
nzardosh and
vkucera
as code owners
fgrosa
left a comment
fgrosa
left a comment
There was a problem hiding this comment.
Hi @atavirag thanks a lot for the implementation! I added few comments regarding variables to remove or add and then the other missing thing to address (not present for the D+) is the way to handle the two possible mass hypotheses for the D0/D0bar candidates.
|
Hi @fgrosa, I just made a commit changing the variables. I also added the column |
fgrosa
left a comment
fgrosa
left a comment
There was a problem hiding this comment.
Hi @atavirag thanks! I still have a couple of suggestions, then I would wait for @fcatalan92's feedback on the table with the two vectors and the decision whether to apply anyway always the ML model to both mass hypotheses (as in your implementation) or instead whether to do it only for the hypothesis which is preselected (maybe better).
|
Hi @atavirag and @fgrosa, I would apply the model only to candidates that pass the pre-selections, so that we do not perform the model computation if not needed. I would also leave the vector empty in case the model is not applied, this prevents to use a wrong information later on, since if the vector is accessed we should get a crash (the correct way should be to check the flag of the ML selection before accessing the ML output). |
fcatalan92
left a comment
fcatalan92
left a comment
There was a problem hiding this comment.
Hi @atavirag, thanks for the updates. I added a comment on the code.
Moreover, I have another remark: at the end of the part related to the ML application, you should clear the vectors, otherwise the ML output will be wrongly filled, e.g., also for candidates that do not pass the pre-selections.
Before exiting the if you can do
outputMlD0.clear();
outputMlD0bar.clear();
The vectors are currently being cleared at the beginning of the for loop, in lines 266 and 267. That is, before they get the chance to get filled again. Would it be okay to leave it like that? |
Yes, they are fine. Sorry I missed that! |
I am sorry, thank you for the fix. I will add the subscription to the description. |
* First version of addition of ML response class for D0s * Please consider the following formatting changes * Fix MegaLinter * Add Alexandre's comments * Remove dummy value of outputMl * Add Fabrizio's suggestions * Add new suggestions * add Fabio's suggestions * Always fill hfMlD0Candidate table --------- Co-authored-by: ALICE Action Bot <alibuild@cern.ch>
6 participants
Following @AlexBigO's ML response class for Dplus, I made an equivalent class for D0 mesons. I added the variables currently available in treeCreatorD0ToKPi.cxx.
The PR also introduces a subscription in the candidateSelectorD0 to the tables aod::TracksPidPiExt and aod::TracksPidKaExt.