Update TrackClusterMergeSplitter to output track-cluster associations (PFA0) #1699
Conversation
github-actions
bot
added
topic: calorimetry
for more information, see https://pre-commit.ci
…/EICrecon into output-splitmerge-track-associations
for more information, see https://pre-commit.ci
…/EICrecon into output-splitmerge-track-associations
ruse-traveler
temporarily deployed
to
github-pages
GitHub Actions
Inactive
Capybara summary for PR 1699
|
for more information, see https://pre-commit.ci
| // -------------------------------------------------------------------------- | ||
| //! Calculate cluster shape parameters | ||
| // -------------------------------------------------------------------------- | ||
| /*! Calculation originally written by Chao Peng, Dhevan Gangadharan, | ||
| * and Sebouh Paul. Code is copied from the `CalorimeterClusterRecoCoG` | ||
| * algorithm. | ||
| */ | ||
| void TrackClusterMergeSplitter::calculate_shape_parameters(edm4eic::MutableCluster& clust) const { |
There was a problem hiding this comment.
If this factorizes so well, we might as well move it to a separate factory.
There was a problem hiding this comment.
(This is already a big PR. If you decide to go along with my suggestion, you could refactor existing code separately, and then rebase this to re-use that new facility)
There was a problem hiding this comment.
I'd be very much in favor of that! Keeps it nice and modular 😉
But how would this work in the data model? Would we have one collection without shape parameters filled in (likely not saved by default) and then one with them filled in (saved by default)?
There was a problem hiding this comment.
Started work on this in #1734 !
There was a problem hiding this comment.
Rebase done! One new wrinkle I can immediately spot is how to handle the track-cluster matches: those will be pointing back to the split/merged clusters without cluster shapes...
A proposal: what if we had a new meta algorithm for copying associations? This could be useful for other algorithms that are one-to-one transformations. I have some ideas about how this might be implemented...
There was a problem hiding this comment.
Regarding general studies using the matches, aren't these a bit special? In the sense that they were used to construct resulting clusters, but they might be not what a track-cluster matcher would find by looking at those clusters.
I think, we can be optimistic about our ability to follow up with a relation re-writer.
There was a problem hiding this comment.
Regarding general studies using the matches, aren't these a bit special? In the sense that they were used to construct resulting clusters, but they might be not what a track-cluster matcher would find by looking at those clusters.
True! The matching procedure here does differ in that regard! But it's not clear to me how we would propagate the track-merged cluster relation downstream without outputting updated track-cluster matches...
There was a problem hiding this comment.
I'm only talking about what is being written to the file. IIRC "dangling" relations are not allowed (there is a bug in PODIO which allows for OneToOne relations to not get checked, but that may be fixed at some point). To fix those, we would need to also save clusters without the shapes.
There was a problem hiding this comment.
Ahhhh! I understand now! 😅
I'm absolutely good with disabling them in the output! 👍
There was a problem hiding this comment.
Just pushed changes to disable the track-cluster associations!
…/EICrecon into output-splitmerge-track-associations
ruse-traveler
temporarily deployed
to
github-pages
GitHub Actions
Inactive
…/EICrecon into output-splitmerge-track-associations
ruse-traveler
temporarily deployed
to
github-pages
GitHub Actions
Inactive
…/EICrecon into output-splitmerge-track-associations
ruse-traveler
temporarily deployed
to
github-pages
GitHub Actions
Inactive
ruse-traveler
temporarily deployed
to
github-pages
GitHub Actions
Inactive
Briefly, what does this PR introduce?
This PR updates the
TrackClusterMergeSplitteralgorithm to output bothedm4eic::TrackClusterMatchand MC particle-cluster associations. In this process, it reaps what was sown by originally writing the algorithm to operate on protoclusters rather than clusters: the algorithm will now ingest fully formed cluster and update relevant quantities.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?
No.
Does this PR change default behavior?
Yes. Track-cluster and MC particle-cluster associations will now be produced by the algorithm.