[Common/PWGHF]: store PID info in 2-prong candidate table and propagate it to analyses.#7805

Merged

ddobrigk merged 18 commits intoAliceO2Group:masterfrom

mfaggin:nsigma_pid_candcreator

Oct 18, 2024

Collaborator

mfaggin commented Sep 26, 2024 •

edited

Loading

@fgrosa here a possible development for the 2-prong only. 3-prong case not yet addressed

alibuild mentioned this pull request

Please consider the following formatting changes to #7805 mfaggin/O2Physics#35

Merged

mfaggin added a commit to mfaggin/O2Physics that referenced this pull request


          Merge pull request #35 from alibuild/alibot-cleanup-7805

fc2ee3b

Please consider the following formatting changes to AliceO2Group#7805

mfaggin force-pushed the nsigma_pid_candcreator branch from fa42dfc to 1464526 Compare

September 26, 2024 16:33

fgrosa requested changes

View reviewed changes

Collaborator

fgrosa left a comment

Hi @mfaggin thanks for this implementation!
I started with the two major comments, I will check the minor details after the first revision maybe (waiting also @vkucera's feedback about the TrackSelectorPID.h)

PWGHF/DataModel/CandidateReconstructionTables.h Outdated Show resolved Hide resolved

PWGHF/TableProducer/pidCreator.cxx Outdated Show resolved Hide resolved

Collaborator Author

mfaggin commented Oct 3, 2024

thanks @fgrosa , I've implemented the 2 major comments of yours. I've also reverted the pidCreator.cxx

vkucera requested changes

View reviewed changes

PWGHF/DataModel/CandidateReconstructionTables.h Outdated Show resolved Hide resolved

Common/Core/TrackSelectorPID.h Outdated Show resolved Hide resolved

Common/Core/TrackSelectorPID.h Outdated Show resolved Hide resolved

PWGHF/DataModel/CandidateReconstructionTables.h Outdated Show resolved Hide resolved

PWGHF/DataModel/CandidateReconstructionTables.h Show resolved Hide resolved

PWGHF/DataModel/CandidateReconstructionTables.h Outdated Show resolved Hide resolved

PWGHF/TableProducer/candidateCreator2Prong.cxx Outdated Show resolved Hide resolved

PWGHF/DataModel/CandidateReconstructionTables.h Outdated

Comment on lines 702 to 705

+                                hf_cand::NSigTpcPi0, hf_cand::NSigTpcKa0,
+                                hf_cand::NSigTpcPi1, hf_cand::NSigTpcKa1,
+                                hf_cand::NSigTofPi0, hf_cand::NSigTofKa0,
+                                hf_cand::NSigTofPi1, hf_cand::NSigTofKa1,

Collaborator

vkucera Oct 9, 2024

In terms of generalisation of the common HF PID code, it would be better to have one table per prong index and per species, e.g. HfPidProng0Pi, HfPidProng0Ka, HfPidProng1Pi,...

Collaborator Author

mfaggin Oct 15, 2024

addressed

PWGHF/DataModel/CandidateReconstructionTables.h Show resolved Hide resolved

PWGHF/TableProducer/candidateSelectorD0.cxx Outdated Show resolved Hide resolved

Collaborator Author

mfaggin commented Oct 15, 2024 •

edited

Loading

Thanks a lot @vkucera for your suggestions. I've implemented them in the new commits.
A couple of comments:

I've decided not to rename neither NSigTpcPi0 etc. nor TpcTofNSigmaPi0 etc. because they are largely used in many selectors/ML inference class/analysis tasks also for the 3-prong analyses
as discussed yesterday, by mistake after your review I've done git pull --rebase upstream master instead of git merge the master branch. This changed my history locally and prevented me to smoothly push here again. I managed to push here after git pull --rebase origin <this_branch> but as you see now the history is destroyed (in particular: this put in time order my commits, mixing them up with pre-existing ones instead of putting all mine on top). I find it super dirty, and I'd propose to force-push from a backup branch that I have aligned with the upstream master. This will delete your review here I'm afraid... let me know what you think.

Collaborator

vkucera commented Oct 15, 2024 •

edited

Loading

Hi @mfaggin , I agree, in this case it is cleaner to force-push a history with only your work.

Mattia Faggin and others added 13 commits

October 15, 2024 15:17


          Add nSigma TPC and TOF to cand. creator 2-prong table.

0fa3ee5


          Propagate 2-prong cand. daughter PID usage up to candidateSelectorD0.

c91c79c

 - Needed tables added
 - TrackSelectorPID patch added to force the usage of a custom PID nSigma value (from 2-prong candidate table)
 - Add process functions via macro for combined TPC-TOF PID for 2-prong candidates
 - ML response class updated accordingly

TODO: propagate it to the next pieces of the chain.
Files to be adjusted:
 - []  TableProducer/derivedDataCreatorD0ToKPi.cxx
 - []  TableProducer/treeCreatorD0ToKPi.cxx
 - []  HFC/TableProducer/correlatorDMesonPairs.cxx
 - []  D2H/Tasks/taskd0.cxx
 - []  D2H/Tasks/taskBplus.cxx


          Remove unused tables, and add process checkers.

53dbced


          Fix Clang format.

09d40f6


          Please consider the following formatting changes

9caed76


          Propagate 2-prong PID info to PWGHF/TableProducer/derivedDataCreatorD…

f081883

…0ToKPi.cxx.


          Propagate 2-prong PID info to PWGHF/TableProducer/treeCreatorD0ToKPi.…

87958eb

…cxx.


          Propagate 2-prong PID info to PWGHF/HFC/TableProducer/correlatorDMeso…

3a89466

…nPairs.cxx.


          Implement suggestions from Fabrizio

 - put the PID nsigma columns in another table, to be joined in analysis
 - use dynamic columns for nsigma combined


          Readapt TrackSelectorPID adding optional args. to functions instead o…

4466ba8

…f track-related data members.


          Move combineNSigma in a dedicated HF PID header.

387b590


          Re-add tiny case.

d178d6b


          Define PID tables per species and prong ID.

7bec368

mfaggin force-pushed the nsigma_pid_candcreator branch from ef70d15 to 7bec368 Compare

October 15, 2024 13:25

vkucera requested changes

View reviewed changes

Collaborator

vkucera left a comment

Thanks a lot @mfaggin for the great improvement. Please see some minor suggestions.

PWGHF/Utils/utilsPid.h Outdated Show resolved Hide resolved

PWGHF/Utils/utilsPid.h Outdated Show resolved Hide resolved

PWGHF/Utils/utilsPid.h Outdated Show resolved Hide resolved

Mattia Faggin added 2 commits

October 15, 2024 18:29


          Change enum name and fix a couple of descriptions.

74f8143


          Fix MegaLinter problem.

a0f1e98

alibuild mentioned this pull request

Please consider the following formatting changes to #7805 mfaggin/O2Physics#37

Closed


          Fix Clang format.

46d892e

Collaborator Author

mfaggin commented Oct 15, 2024

Thanks @vkucera , I've just addressed also your last comments.
I've also managed to test the code locally for the standard D0 analysis, and everything worked for me.


          Fix MegaLinter problem n.2 .

a765708

Collaborator Author

mfaggin commented Oct 16, 2024 •

edited

Loading

Hi @fgrosa @vkucera , I've just tried running the analysis aligning O2Physics to the master branch (blue) as well as with the development branch of this PR (red) and I checked the first 3 plots in the taskD0 output as an example. They all look identical (the weird shapes are just because I asked the configuration of a random train by Mingyu, where ML selections are applied with different values vs. pt).

I attach also the full files used for the tests, for completeness.
input_data.txt
configuration.json
run.txt

mfaggin mentioned this pull request

PWGHF: add THn axis for prompt BDT score in D0 task #8029

Merged

fgrosa previously approved these changes

View reviewed changes

mfaggin marked this pull request as ready for review

October 16, 2024 16:14

mfaggin requested review from a team, NicoleBastid, alibuild, apalasciano, ddobrigk, deepathoms, fcatalan92, fcolamar, hahassan7, iarsene, jgrosseo, jpxrk, ktf and mmazzilli as code owners

October 16, 2024 16:14

Collaborator Author

mfaggin commented Oct 16, 2024

Hi @vkucera , @fgrosa I moved the PR from "draft" to "ready for review"

vkucera requested changes

View reviewed changes

Collaborator

vkucera left a comment •

edited

Loading

@mfaggin thanks for the thorough validation.
Please see a few small suggestions that would be good to implement before merging.

PWGHF/Utils/utilsPid.h Outdated Show resolved Hide resolved

PWGHF/Utils/utilsPid.h Outdated Show resolved Hide resolved

PWGHF/Utils/utilsPid.h Show resolved Hide resolved


          Minor fixes.

4d73770

mfaggin dismissed fgrosa’s stale review via

4d73770

October 17, 2024 11:12

Collaborator Author

mfaggin commented Oct 17, 2024

tagging also @ddobrigk since a file in Common is touched

vkucera approved these changes

View reviewed changes

ddobrigk approved these changes

View reviewed changes

ddobrigk merged commit d71797e into AliceO2Group:master

vkucera mentioned this pull request

[PWGHF] store PID info in 3-prong candidate table and propagate it to analyses. #10984

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

vkucera vkucera approved these changes

ddobrigk ddobrigk approved these changes

fgrosa fgrosa left review comments

alibuild Awaiting requested review from alibuild alibuild is a code owner

fcolamar Awaiting requested review from fcolamar fcolamar is a code owner

fcatalan92 Awaiting requested review from fcatalan92 fcatalan92 is a code owner

mmazzilli Awaiting requested review from mmazzilli mmazzilli is a code owner

deepathoms Awaiting requested review from deepathoms deepathoms is a code owner

NicoleBastid Awaiting requested review from NicoleBastid NicoleBastid is a code owner

hahassan7 Awaiting requested review from hahassan7 hahassan7 is a code owner

jpxrk Awaiting requested review from jpxrk jpxrk is a code owner

apalasciano Awaiting requested review from apalasciano apalasciano is a code owner

jgrosseo Awaiting requested review from jgrosseo jgrosseo is a code owner

iarsene Awaiting requested review from iarsene iarsene is a code owner

ktf Awaiting requested review from ktf ktf is a code owner

Labels

None yet