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

Changes for pandora PFA #419

Merged

Conversation

giovannimarchiori
Copy link
Contributor

@giovannimarchiori giovannimarchiori commented Jan 15, 2025

Started from Archi's code in https://github.com/Archil-AD/k4geo/tree/pandora, with some modifications for SimpleCylinder v02 to make it more general
BEGINRELEASENOTES

  • Use theta rather than eta grid for muon tagger readout in ALLEGRO
  • Add detector type and calorimeter data to various detectors, for pandora PFA
  • Add simple cylinder geometry v02 with calo data and possibility to use it for the endcap with both endcaps created within the same volume
  • Add simple cylinder geometry v03 which introduces longitudinal segmentation
    ENDRELEASENOTES

Tagging @BrieucF - let's look at the code (there are definitely at least a couple of things to be changed quickly, and for some others like the cell sizes for caloData we can think/see how to properly retrieve them from the segmentation and retrieve them. Will mark the PR as draft meanwhile

…and simple cylinder geometry with both endcaps in same volume
@giovannimarchiori giovannimarchiori marked this pull request as ready for review February 10, 2025 13:17
Copy link
Collaborator

@atolosadelgado atolosadelgado left a comment

Choose a reason for hiding this comment

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

Thanks for this PR! I have left some in-line comments, mainly regarding rhetorical questions and FIXME comments. Addressing these would help improve the overall clarity and maintainability of the code. Let me know your thoughts, and feel free to reach out if you’d like to discuss anything further, for instance, regarding the multisegmentation feature

@giovannimarchiori
Copy link
Contributor Author

Hello @atolosadelgado thanks for your review!
I implemented most of your comments. I replied to the others trying to explain why we would rather implement the remaining things in other PRs later (some of the changes require major changes in pandora...)
I let you resolve the conversations if you agree with the replies - please also let me know if you agree with renaming v03 to v02 for the simple cylinder detector.
Thanks, Giovanni

@giovannimarchiori
Copy link
Contributor Author

Hello @atolosadelgado
I merged SimpleCylinder v02 and v03 into v02.
The only two unresolved comments left are those on the FIXME for the cell size info used by DDMarlinPandora to fill the pandora info. I would keep them like these and fix in the future - we still need to think what's the best way of doing that and would require quite some coding.

@atolosadelgado
Copy link
Collaborator

Hello @atolosadelgado I merged SimpleCylinder v02 and v03 into v02. The only two unresolved comments left are those on the FIXME for the cell size info used by DDMarlinPandora to fill the pandora info. I would keep them like these and fix in the future - we still need to think what's the best way of doing that and would require quite some coding.

Hello @atolosadelgado I merged SimpleCylinder v02 and v03 into v02. The only two unresolved comments left are those on the FIXME for the cell size info used by DDMarlinPandora to fill the pandora info. I would keep them like these and fix in the future - we still need to think what's the best way of doing that and would require quite some coding.

Thanks for the refactoring of the generic cylinder. Before merging, may I ask if it is needed to update the Readme files?

@giovannimarchiori
Copy link
Contributor Author

I updated the ALLEGRO readme to mention the change in segmentation for the muon tagger in v03 and v04

@atolosadelgado
Copy link
Collaborator

@giovannimarchiori shall we merge?

@giovannimarchiori
Copy link
Contributor Author

@giovannimarchiori shall we merge?

Yes please!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants