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

Retrieve B field and detector info in TracksFromGenParticlesWithECalExtrap functional #45

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

giovannimarchiori
Copy link
Contributor

add retrieval of B field and extrapolation to calorimeter to functional that produces tracks from gen particles.
Based on transformer code from TracksFromGenParticles.cpp and on detector retrieval and extrapolation code from [racksFromGenParticlesWithECalExtrapAlg.cpp.

BEGINRELEASENOTES

  • add retrieval of B field and extrapolation to calorimeter to functional that produces tracks from gen particles
    ENDRELEASENOTES

@giovannimarchiori
Copy link
Contributor Author

rebased

@atolosadelgado
Copy link
Collaborator

hi @giovannimarchiori

Thanks for this PR! I’m curious, am I correct in understanding that the main difference from the algorithm introduced three weeks ago is that the previous one was a Gaudi algorithm, while this PR introduces the same functionality as a Gaudi functional?

If that’s the case, do you think it would be beneficial to wrap the core logic of the Gaudi algorithm into a dedicated class? That way, we could avoid code duplication.

Thanks again for your work!

@giovannimarchiori
Copy link
Contributor Author

Hi @atolosadelgado,
before doing that - is there a reason on the long term to keep both versions? If at some point the Gaudi::Algorithm should just be phased out, then I am not sure it's worth spending time refactoring the code.
I don't know the answer myself, just let me know

@atolosadelgado
Copy link
Collaborator

Hi @atolosadelgado, before doing that - is there a reason on the long term to keep both versions? If at some point the Gaudi::Algorithm should just be phased out, then I am not sure it's worth spending time refactoring the code. I don't know the answer myself, just let me know

As in the TV show 'who wants to be a millionaire', I will play the joker card 'phone a friend': @BrieucF @kjvbrt @jmcarcell , do you think it is worth to refactor code, or it is ok to duplicate it as Gaudi algorithm is going to be deprecated in the near future?

@atolosadelgado
Copy link
Collaborator

So @BrieucF told me privately to merge, but please do not forget to remove the Gaudi::Algorithm once we are 'migrated' to functionals and Gaudi::Algorithm is phased out.

The CI test in ubuntu are failing [link]. It seems related to the EDM4hep data extension for the drift chamber. @jmcarcell what do you suggest to do?

@giovannimarchiori giovannimarchiori changed the title Retrieve B field and detector info in TracksFromGenParticles functional Retrieve B field and detector info in TracksFromGenParticlesWithECalExtrap functional Feb 22, 2025
@giovannimarchiori
Copy link
Contributor Author

Hi @atolosadelgado
I created an issue #46 to keep in mind "to remove the Gaudi::Algorithm once we are 'migrated' to functionals and Gaudi::Algorithm is phased out"
Thanks, Giovanni

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.

2 participants