Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PWGHF] KFParticle reconstruction of 3-prong decay #9211

Merged
merged 31 commits into from
Jan 21, 2025

Conversation

lubynets
Copy link
Contributor

@lubynets lubynets commented Jan 7, 2025

  1. In this PR the usage of Kalman Filter Particle algorithm is implemented for 3-body decay reconstruction (by way of analogy with 2-body decay).
  2. A table with KF-specific variables (mainly topological) is added.
  3. Changes related to item 1 are introduced into selection and treeCreator tasks.

fgrosa
fgrosa previously approved these changes Jan 16, 2025
@fgrosa
Copy link
Collaborator

fgrosa commented Jan 16, 2025

Hi @fgrosa, thanks for your feedback and proposed changes, I implemented them. What about stability of the DCAFitter part of the code and memory usage - I checked both. I have run reconstruction of Lc using DCAfitter both in my version of the code, proposed in this PR, and in master version. The results are identical, see distributions of invariant mass and azimuthal angle - they are on top of each other (master and KF). Also I looked through other distributions - their number of entries, means and std devs agree in all digits that allows me to conclude that the results are identical. minv phi What about memory usage I also built both for master and KF versions the proportionalSetSize graphs (if I decoded PSS in your comment correctly) - they also look similar in my opinion. Are other values in performance graphs also worth to be compared? pss master pss KF I placed generated AnalysisResults.root files and performanceMetrics.json files by this link https://cernbox.cern.ch/s/XUukbYeVeJtb8Ja

Thanks! It looks good to me. Approved now, even if we cannot merge yet because an issue in the CI with macOS-arm (will do as soon as solved)

@vkucera
Copy link
Collaborator

vkucera commented Jan 16, 2025

@lubynets Please fix the MegaLinter errors.

@lubynets
Copy link
Contributor Author

@lubynets Please fix the MegaLinter errors.

Done

Please consider the following formatting changes to AliceO2Group#9211
@vkucera
Copy link
Collaborator

vkucera commented Jan 17, 2025

@lubynets Thanks for addressing my comments.
Not strictly required for this PR, but there is a lot of duplication between fillTablesData and fillTablesMc. It would be worth exploring the possibility to reduce it with refactoring that code.

@fgrosa
Copy link
Collaborator

fgrosa commented Jan 19, 2025

@lubynets Thanks for addressing my comments. Not strictly required for this PR, but there is a lot of duplication between fillTablesData and fillTablesMc. It would be worth exploring the possibility to reduce it with refactoring that code.

I agree on this, but I also agree that it can be done in a second PR (@lubynets please consider it) in order to move on with this one. I cannot merge however because the utils header was included in Tools, for which I have no approval rights (maybe @ktf can merge? Thanks!)

@fgrosa fgrosa enabled auto-merge (squash) January 19, 2025 15:14
@fgrosa fgrosa disabled auto-merge January 19, 2025 17:28
@ddobrigk ddobrigk merged commit 3a0382b into AliceO2Group:master Jan 21, 2025
14 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

5 participants