Skip to content

[PWGHF] Add KFParticle Quality Assurance in XicToXiPi candidate creator #10972

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

Merged
merged 14 commits into from
Apr 30, 2025

Conversation

Rrantu
Copy link
Contributor

@Rrantu Rrantu commented Apr 24, 2025

Evaluate the resolution of the particles reconstructed using the KFParticle method.

Copy link

github-actions bot commented Apr 24, 2025

O2 linter results: ❌ 9 errors, ⚠️ 0 warnings, 🔕 2 disabled

@github-actions github-actions bot changed the title Add KFParticle Quality Assurance [PWGHF] Add KFParticle Quality Assurance Apr 24, 2025
@vkucera vkucera marked this pull request as draft April 24, 2025 13:21
@vkucera
Copy link
Collaborator

vkucera commented Apr 24, 2025

Hi @Rrantu , such a huge PR deserves some comments.

  • First of all, what is your goal?
  • Second, why do you need so many variables, new structs, histograms and tables?
  • Why do you need to do further MC matching?
  • How much more memory does the extra stuff cost?
  • Last but not least, fix the issues first, only then make the PR ready.

@Rrantu
Copy link
Contributor Author

Rrantu commented Apr 24, 2025

Hi @vkucera , I'm trying to check the resolution of the particles reconstructed using the KFParticle method. For this, I need the information saved in runXic0Omegac0Creator function, Therefore, I’ve added a new tree within the runXic0Omegac0Creator function to store these variables.
Because KFParticle reconstruction starts from V0s, my MC matching also needs to begin from V0s. So, I’ve written a new process function to read the new tree and do the MC matching accordingly.
I’m not exactly sure how much memory this will consume, but it should be minimal because the new tree is relatively small.
And for the O2 linter issues, they were not introduced by my changes.

@Rrantu Rrantu marked this pull request as ready for review April 24, 2025 14:15
@zhangbiao-phy
Copy link
Collaborator

hi @Rrantu, Thanks for the implementation! I have a few comments. See the review

zhangbiao-phy
zhangbiao-phy previously approved these changes Apr 25, 2025
@zhangbiao-phy
Copy link
Collaborator

Hi @vkucera , I'm trying to check the resolution of the particles reconstructed using the KFParticle method. For this, I need the information saved in runXic0Omegac0Creator function, Therefore, I’ve added a new tree within the runXic0Omegac0Creator function to store these variables. Because KFParticle reconstruction starts from V0s, my MC matching also needs to begin from V0s. So, I’ve written a new process function to read the new tree and do the MC matching accordingly. I’m not exactly sure how much memory this will consume, but it should be minimal because the new tree is relatively small. And for the O2 linter issues, they were not introduced by my changes.

Hi @Rrantu, Thanks! Please fix the O2 linter issues next time!

@zhangbiao-phy zhangbiao-phy enabled auto-merge (squash) April 25, 2025 07:44
@zhangbiao-phy zhangbiao-phy disabled auto-merge April 25, 2025 09:55
@alibuild
Copy link
Collaborator

alibuild commented Apr 25, 2025

Error while checking build/O2Physics/o2 for 3268c45 at 2025-04-25 16:23:

## sw/BUILD/O2Physics-latest/log
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
/sw/SOURCES/O2Physics/10972-slc9_x86-64/0/PWGHF/TableProducer/candidateCreatorXic0Omegac0.cxx:2579:86: error: 'const struct o2::soa::Table<o2::aod::Hash<3036674320>, o2::aod::Hash<295670893>, o2::aod::Hash<2286545062> >::TableIteratorBase<o2::soa::DefaultIndexPolicy, o2::soa::Table<o2::aod::Hash<3036674320>, o2::aod::Hash<295670893>, o2::aod::Hash<2286545062> > >' has no member named 'invMassV0Sig'; did you mean 'invMassV0Err'?
/sw/SOURCES/O2Physics/10972-slc9_x86-64/0/PWGHF/TableProducer/candidateCreatorXic0Omegac0.cxx:2660:89: error: 'const struct o2::soa::Table<o2::aod::Hash<3036674320>, o2::aod::Hash<295670893>, o2::aod::Hash<2286545062> >::TableIteratorBase<o2::soa::DefaultIndexPolicy, o2::soa::Table<o2::aod::Hash<3036674320>, o2::aod::Hash<295670893>, o2::aod::Hash<2286545062> > >' has no member named 'invMassXiSig'; did you mean 'invMassXiErr'?
/sw/SOURCES/O2Physics/10972-slc9_x86-64/0/PWGHF/TableProducer/candidateCreatorXic0Omegac0.cxx:2727:94: error: 'const struct o2::soa::Table<o2::aod::Hash<3036674320>, o2::aod::Hash<295670893>, o2::aod::Hash<2286545062> >::TableIteratorBase<o2::soa::DefaultIndexPolicy, o2::soa::Table<o2::aod::Hash<3036674320>, o2::aod::Hash<295670893>, o2::aod::Hash<2286545062> > >' has no member named 'invMassXic0Sig'; did you mean 'invMassXic0Err'?
ninja: build stopped: subcommand failed.

Full log here.

@Rrantu Rrantu requested a review from fgrosa April 25, 2025 15:10
Comment on lines 2003 to 2005
registry.add("hV0DauPosXPullVsPt", "x_{PULL} vs. p_{T}", HistType::kTH2D, {{20, 0., 20.}, {4000, -20., 20.}});
registry.add("hV0DauPosYPullVsPt", "y_{PULL} vs. p_{T}", HistType::kTH2D, {{20, 0., 20.}, {4000, -20., 20.}});
registry.add("hV0DauPosZPullVsPt", "z_{PULL} vs. p_{T}", HistType::kTH2D, {{20, 0., 20.}, {4000, -20., 20.}});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a lot of bins. Please evaluate the extra RAM consumption when running.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback. I have reduced the number of bins.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's see if it runs fine, but please check the extra memory consumption. It's still a lot of bins.

@Rrantu Rrantu requested a review from vkucera April 25, 2025 17:56
Copy link
Collaborator

@fgrosa fgrosa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is OK for me, once the comments of @vkucera are also resolved it can be merged

@Rrantu Rrantu requested a review from vkucera April 28, 2025 08:53
@fgrosa
Copy link
Collaborator

fgrosa commented Apr 30, 2025

@vkucera @Rrantu have all the comments been implemented? Can we merge this?

@Rrantu
Copy link
Contributor Author

Rrantu commented Apr 30, 2025

@vkucera @Rrantu have all the comments been implemented? Can we merge this?
I believe I’ve addressed all the comments. Please let me know if anything else is needed before merging.

@vkucera vkucera changed the title [PWGHF] Add KFParticle Quality Assurance [PWGHF] Add KFParticle Quality Assurance in XicToXiPi candidate creator Apr 30, 2025
@vkucera vkucera merged commit c7aff21 into AliceO2Group:master Apr 30, 2025
18 of 20 checks passed
mfaggin pushed a commit to mfaggin/O2Physics that referenced this pull request Apr 30, 2025
bghanley1995 pushed a commit to bghanley1995/O2Physics that referenced this pull request May 5, 2025
jinhyunni pushed a commit to jinhyunni/O2Physics that referenced this pull request May 11, 2025
jinhyunni pushed a commit to jinhyunni/O2Physics that referenced this pull request May 11, 2025
prottayCMT pushed a commit to prottayCMT/O2Physics2024 that referenced this pull request May 17, 2025
ariedel-cern pushed a commit to ariedel-cern/O2Physics that referenced this pull request May 23, 2025
ddobrigk pushed a commit to ddobrigk/O2Physics that referenced this pull request Jun 14, 2025
smaff92 pushed a commit to smaff92/O2Physics that referenced this pull request Jun 17, 2025
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