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 !
| using MatrixF = std::vector<std::vector<float>>; | ||
| using VecMatrix = std::vector<MatrixF>; |
There was a problem hiding this comment.
Looks like, in some places you try to use maps, and here you give up and just rely on array indices? Is it true that every hit has a weight related to every cluster and every projection?
There was a problem hiding this comment.
So I do think we need a weight that's related to every cluster and every projection pointing to the merged cluster:
- the former is because the merged cluster will (naturally) be composed of all of the hits of the clusters being merged,
- and the latter is because we'll need to know the sum of all projections' momenta for the weight, so that summing over the split clusters should give the merged cluster energy before splitting.
(Unless I'm misunderstanding how the splitting should work!)
All that being siad, I probably could rework it to use maps instead of indices. I was thinking of this map of weights (hit_weight = hit_weight(cluster, projection)) as a "matrix", and this was an easy way of implementing a dynamic 2D matrix of floats 🤷
There was a problem hiding this comment.
So I do think we need a weight that's related to every cluster and every projection pointing to the merged cluster:
I think you're right. Looks like this only involves hits from the cluster group.
All that being siad, I probably could rework it to use maps instead of indices. I was thinking of this map of weights (
hit_weight = hit_weight(cluster, projection)) as a "matrix", and this was an easy way of implementing a dynamic 2D matrix of floats 🤷
Ideally we write the code to maximize clarity. Having a consistent use of maps could help with that, if there is a neat way to use those. Using maps also provides basic sanity checks to the code.
If you have a preference to use tensors here, then just remember std::vector<std::vector<std::vector< has triple dereferences/allocations and spoils data locality, so it won't win that much in terms of performance. We already use eigen library, that could be better for your case, perhaps.
There was a problem hiding this comment.
I see! I'll give it some thought and let you know which route (map or eigen) I go with!
There was a problem hiding this comment.
After mulling it over, I think it a natural way might be to actually use a mix of maps and eigen: for each projection, we know how many clusters and hits we're going to need so we could easily define a map of projections onto MatrixXd! Let me give it a shot...
There was a problem hiding this comment.
I stand corrected: it was much easier to just use std::map<edm4eic::CalorimeterHit, double>. Just pushed some changes that rewrite the splitting calculation to use that (it's a little cleaner now).
…/EICrecon into output-splitmerge-track-associations
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 paritcle-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.