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

Add Track-Based Cluster Merging Algorithm #1406

Merged
merged 79 commits into from
Sep 17, 2024

Conversation

ruse-traveler
Copy link
Contributor

@ruse-traveler ruse-traveler commented May 1, 2024

Briefly, what does this PR introduce?

This PR introduces the TrackClusterMergeSplitter algorithm, an algorithm for merging and splitting calorimeter clusters based on matched track projections. Inspired by the Particle Flow algorithm from ATLAS.

What kind of change does this PR introduce?

Please check if this PR fulfills the following:

  • Tests for the changes have been added
  • Documentation has been added / updated
  • Changes have been communicated to collaborators

Does this PR introduce breaking changes? What changes might users need to make to their code?

No.

Does this PR change default behavior?

Yes. Now "SplitMerge" edm4eic::Cluster collections and corresponding associations will be created for 6 of the central calorimeters (NHCal, EEEMCal, BHCal, BIC, LFHCal, and FEMC). Track projections will need to be extended to other calorimeters before the algorithm introduced here can be used on them.

To-Do

The split-merge parameters for each calorimeter need to be updated from the current dummy values used for testing. In the interest of getting the functionality of this algorithm merged, this could update could be done in a separate PR.

@github-actions github-actions bot added topic: calorimetry relates to calorimetry topic: barrel labels May 1, 2024
@ruse-traveler ruse-traveler requested review from steinber and veprbl May 14, 2024 17:44
@ruse-traveler
Copy link
Contributor Author

Hi all, just wanted to check in on this: do the recent round of commits resolve the open issues? Or is there more to discuss? Thanks!

@veprbl
Copy link
Member

veprbl commented Sep 13, 2024

@ruse-traveler I'd like to spend some more time checking the actual change. Have you done any benchmarking on your end?

@ruse-traveler
Copy link
Contributor Author

Mostly just spot-checks to make sure that it runs and isn't producing stuff that's completely out-to-lunch. I can run some more detailed checks like looking at the E_{clust}/E_{par} in some DIS events.

veprbl
veprbl previously approved these changes Sep 16, 2024
Copy link
Member

@veprbl veprbl left a comment

Choose a reason for hiding this comment

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

Ship it

@veprbl veprbl enabled auto-merge September 16, 2024 23:01
Copy link

@veprbl veprbl added this pull request to the merge queue Sep 17, 2024
Merged via the queue into main with commit 5d4124e Sep 17, 2024
86 of 87 checks passed
@veprbl veprbl deleted the add-track-cluster-merge-algorithm branch September 17, 2024 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants