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 to run pandora on tracks created from generator-level particles #43

Merged

Conversation

giovannimarchiori
Copy link
Contributor

@giovannimarchiori giovannimarchiori commented Jan 15, 2025

Started from Archil's implementation in https://github.com/Archil-AD/k4RecTracker/tree/pandora but

  1. changed name of algorithm, make it live in parallel to the transformed based one
  2. added the possibility to pass more than one collection of sim tracker hits in input, so that first hit can be calculated from vtx and last hit from wrapper
  3. add more debug information
  4. do not keep tracks without associated simtrackerhits
  5. retrieve B field and detector size automatically
  6. implement also extrapolation to calo endcaps

BEGINRELEASENOTES

  • Add a Gaudi::Algorithm to create tracks from generator-level particles with charge!=0 and number of simtrackerhits>0 (temporary work around needed because one can not use the edm4hep <--> lcio converters with IOSvc and functionals)
  • Track states at IP, first tracker hit, last tracker hit and extrapolation to calorimeter are calculated and saved by the algorithm
    ENDRELEASENOTES

Tagging @BrieucF - let's have a look at the code not just in terms of style but also at what it does so that we do things right. For instance might be things that need to be done a bit differently for tracks that point towards the ECAL endcap rather than the barrel. I will make it a draft while we iterate over it

@BrieucF
Copy link
Collaborator

BrieucF commented Jan 17, 2025

Hi Giovanni, can you add to the release note that this is a work around needed because currently one can not us the edm4hep <--> lcio converters when using functionals?

@giovannimarchiori
Copy link
Contributor Author

Done

Copy link
Collaborator

@BrieucF BrieucF left a comment

Choose a reason for hiding this comment

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

Thanks Giovanni! Here some comments

Tracking/components/TracksFromGenParticlesAlg.cpp Outdated Show resolved Hide resolved
Tracking/components/TracksFromGenParticlesAlg.cpp Outdated Show resolved Hide resolved
Tracking/components/TracksFromGenParticlesAlg.cpp Outdated Show resolved Hide resolved
Tracking/components/TracksFromGenParticlesAlg.cpp Outdated Show resolved Hide resolved
Tracking/components/TracksFromGenParticlesAlg.cpp Outdated Show resolved Hide resolved
Tracking/components/TracksFromGenParticlesAlg.cpp Outdated Show resolved Hide resolved
Tracking/components/TracksFromGenParticlesAlg.cpp Outdated Show resolved Hide resolved
Tracking/components/TracksFromGenParticlesAlg.cpp Outdated Show resolved Hide resolved
Tracking/components/TracksFromGenParticlesAlg.cpp Outdated Show resolved Hide resolved
@giovannimarchiori
Copy link
Contributor Author

@BrieucF I think I implemented all your comments. I also made extrapolation to endcap work as well as to barrel. It should be OK for a final review round.

@giovannimarchiori giovannimarchiori marked this pull request as ready for review January 28, 2025 15:15
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 confirm it compiles.

@BrieucF do you think it would be worth to provide a test for CI, which can also be used as example for potential users?

Tracking/components/TracksFromGenParticlesAlg.cpp Outdated Show resolved Hide resolved
Tracking/CMakeLists.txt Show resolved Hide resolved
@BrieucF
Copy link
Collaborator

BrieucF commented Jan 29, 2025

@BrieucF do you think it would be worth to provide a test for CI, which can also be used as example for potential users?

Yes, I think it is worth adding a test

@giovannimarchiori
Copy link
Contributor Author

Test added ;)

@giovannimarchiori
Copy link
Contributor Author

The test fails because for the extrapolation to the calorimeter it requires the calorimeter data extension to be filled, as it will be possible when key4hep/k4geo#419 will be merged. Should we (1) wait for that PR to be merged, (2) merge anyway with a test failing, or (3) add a protection to the algorithm that when the extension is missing the extrapolation to the calo is not performed? (3) seems the best way to me but I will be busy all day...

@atolosadelgado
Copy link
Collaborator

The test fails because for the extrapolation to the calorimeter it requires the calorimeter data extension to be filled, as it will be possible when key4hep/k4geo#419 will be merged. Should we (1) wait for that PR to be merged, (2) merge anyway with a test failing, or (3) add a protection to the algorithm that when the extension is missing the extrapolation to the calo is not performed? (3) seems the best way to me but I will be busy all day...

Hi @giovannimarchiori

Thanks for adding the test! I just realized that this algorithm relies on the data extension of a specific calorimeter, making it quite specialized and less reusable. Would you consider either renaming it to better reflect its specificity or adapting it to be more general? For example, it could read the size variables [link] from input instead of depending on the data extension.

Let me know what you think!

@giovannimarchiori
Copy link
Contributor Author

Hi @atolosadelgado
the algorithm looks for extension data for calorimeters with standard types
dd4hep::DetType::CALORIMETER | dd4hep::DetType::ELECTROMAGNETIC | dd4hep::DetType::BARREL
dd4hep::DetType::CALORIMETER | dd4hep::DetType::ELECTROMAGNETIC | dd4hep::DetType::ENDCAP
so it is agnostic with respect to the detector model - all detectors should have an EM barrel and endcap. Being this said, I added a protection that if the extensions for these calorimeters are not found, a warning is printed and the trackAtCalo state is not filled. Note that trackAtCalo is needed for p-flow as the impact position at the inner face at the ecal so this is what we really need here - I would not customise the algorithm further. And we prefer to read the positions automatically from the compact file rather than hardcoding them in the scripts to avoid mistakes when copying numbers from one place to another.

@atolosadelgado
Copy link
Collaborator

Hi @atolosadelgado the algorithm looks for extension data for calorimeters with standard types dd4hep::DetType::CALORIMETER | dd4hep::DetType::ELECTROMAGNETIC | dd4hep::DetType::BARREL dd4hep::DetType::CALORIMETER | dd4hep::DetType::ELECTROMAGNETIC | dd4hep::DetType::ENDCAP so it is agnostic with respect to the detector model - all detectors should have an EM barrel and endcap. Being this said, I added a protection that if the extensions for these calorimeters are not found, a warning is printed and the trackAtCalo state is not filled. Note that trackAtCalo is needed for p-flow as the impact position at the inner face at the ecal so this is what we really need here - I would not customise the algorithm further. And we prefer to read the positions automatically from the compact file rather than hardcoding them in the scripts to avoid mistakes when copying numbers from one place to another.

Hi @giovannimarchiori

That makes sense. Since the algorithm depends on the calorimeter, could you reference it in the name? The ARC group will need to develop a similar algorithm without this dependency, so a more explicit name would help avoid confusion. Thanks!

@atolosadelgado
Copy link
Collaborator

Currently, the data extension is only used to extract the size of the calorimeter. Would it be possible to add an option (as a non-default setting) to pass the calorimeter size as parameters of the Gaudi algorithm? This would make the algorithm more flexible and suitable for other systems that do not provide the data extension.

@giovannimarchiori
Copy link
Contributor Author

Hi @atolosadelgado I changed the algorithm name so that now it's clear it extrapolates to the ecal.

@atolosadelgado atolosadelgado enabled auto-merge (rebase) January 30, 2025 16:27
@BrieucF BrieucF disabled auto-merge January 31, 2025 08:48
@BrieucF BrieucF merged commit d75aeb0 into key4hep:master Jan 31, 2025
3 of 5 checks passed
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.

4 participants