SecondaryVertices factory based on Helix method #2144
Conversation
for more information, see https://pre-commit.ci
|
Hi Wouter and Dmitri, Based on the comments received this morning at the Reconstruction WG meeting, I updated the branch with the general calculation for any two-track combinations. I tested with the pi-pi selection in the down-stream analysis and it yields identical results compared to my early implementation. |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
This PR applies the include-what-you-use fixes as suggested by https://github.com/eic/EICrecon/actions/runs/18668462362. Please merge this PR into the branch `pr/secondaryvertex-helix` to resolve failures in PR #2144. Auto-generated by [create-pull-request][1] [1]: https://github.com/peter-evans/create-pull-request Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
starsdong
temporarily deployed
to
github-pages
GitHub Actions
Inactive
|
Hi Wouter and Dmitri, The two remaining failed checks seem to beyond my knowledge. Will you be able to help identify what went wrong there? Thanks and Regards /xin |
This looks like an irrelevant issue. Should resolve itself once latest geometry makes it into the container. I'm rerunning container build without cache now, so, hopefully, this will clear away. We can review the code meanwhile. |
Hi Dmitri and Wouter, Do you have any suggestions/comments to this PR? Any adjustment is needed before it gets merged? Thanks /xin |
|
This is on my radar, but I haven't had chance to look deeper yet. I can share a few early comments. We don't have a convention for utility libraries in EICrecon. For now, do you think we could merge Helix.cc and Helix.h into SecondaryVerticesHelix.cc? I also see you provide Config structure, but don't expose the parameters in the factory. And last is the hardcoded b_field, would it make sense to get the value from the geometry service instead (e.g. sample at the 0,0,0 coordinate)? |
There was a problem hiding this comment.
Is this copied from another framework?
There was a problem hiding this comment.
Yes, Wouter. The core part is copied from the code used in STAR. I made some adjustments to be compatible with edm4eic or edm4hep objects. And I added one constructor to create a Helix object from edm4eic::ReconstructedParticle.
There was a problem hiding this comment.
What license is the star code under? Who owns the copyright?
I believe it is in principle possible to get merged into SecondaryVerticesHelix.cc. But the Helix.cc is pretty sizable, and I believe many functions can be used in other down stream analysis. The code is largely copied from STAR with adjustments to be adaptable to edm4eic constainers. I think it is probably preferrable to keep them separated so it is easier to maintain. I wasn't sure that I understand completely about your comment "I also see you provide Config structure, but don't expose the parameters in the factory." Do you suggest to explicitly put them out in reco.cc when creating this factory? I see there, some factories do this way, some don't. I am fine with either requirement. Regarding the b_field, Yes, I was thinking about the way you suggested too. Let me try to update this part soon. Thanks |
| private: | ||
| SecondaryVerticesHelixConfig m_cfg; |
There was a problem hiding this comment.
Do not store this as an additional field. Inheriting from WithPodConfig already provides cfg instance for you.
|
|
||
| // Declare outputs | ||
| PodioOutput<edm4eic::Vertex> m_secondary_vertices_output{this}; | ||
|
|
There was a problem hiding this comment.
I meant there are no parameter definitions for Config fields. See any other factory for example
Maybe it's for the best to keep it separate, especially if we find that it has questions about authorship/license. But in principle, if left as is, we may have to relocate it at some point.
I did not mean this. See my inline comment above, I hope that will clarify. |
Briefly, what does this PR introduce?
What kind of change does this PR introduce?
Please check if this PR fulfills the following:
Does this PR introduce breaking changes? What changes might users need to make to their code?
None
Does this PR change default behavior?
New SecondaryVerticesHelix factory added into reco plugin, SecondaryVerticesHelix object (currently with edm4eic:Vertex structure) saved in PODIO