Add Track-Based Cluster Merging Algorithm

ruse-traveler commented 4 months ago

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?

[ ] Bug fix (issue #__)
[X] New feature (issue #897 )
[ ] Documentation update
[ ] Other: __

Please check if this PR fulfills the following:

[ ] Tests for the changes have been added
[ ] Documentation has been added / updated
[X] 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[bot] commented 4 months ago

Capybara summary for PR 1406

rec_dis_10x100_minQ2=0_craterlake
rec_dis_10x100_minQ2=1000_craterlake_tracking_only
rec_dis_18x275_minQ2=0_craterlake
rec_dis_18x275_minQ2=1000_craterlake
rec_dis_5x41_minQ2=0_craterlake
rec_e_1GeV_20GeV_craterlake
rec_pi_1GeV_20GeV_craterlake ^{_{Last updated 2024-08-23T16:46-04:00 41665c9b95441609197c700c4f73743bfa9d5bfe}}

ruse-traveler commented 3 months ago

All of the logic is here now, and it's functioning as I would expect for the (very) arbitrary parameters I've plugged in so far...

One thing I realized in the course of coding this up is that this would be better served as a step between the initial protocluster formation and ClusterRecoCoG (I ended up unnecessarily duplicating a lot of code from the latter). This will be a relatively straightforward change since the logic is identical and a lot of code can just be simply deleted...

ruse-traveler commented 2 months ago

@veprbl I think the HCal parameters are now good enough for this initial implementation and to start really kicking the tires on this.

Due to some internet connectivity issues and some condor issues on SDCC, I have to rerun the jobs for the ECal parameters (which I'm tuning to single electron/positron simulations). It's a relatively quick turnaround and I could get things done by later tonight, but I'd also be okay dropping the ECal instantiations for this PR and getting them in as a bugfix... What are your thoughts?

veprbl commented 2 months ago

It's great to see that this is ready for review! However, I don't think we are going to be able to review this in time for 24.07.0. This is a big chunk of new code, would be nice to have a proper review.

ruse-traveler commented 2 months ago

That's fair! We can mark this for 24.08.0...

sonarcloud[bot] commented 2 months ago

Quality Gate failed

Failed conditions
7.6% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

ruse-traveler commented 4 days ago

Hi all, I think this is ready for a re-review. But I am seeing some odd failures on the checks, e.g. on eicrecon-dis (clang, 10x100, 0, craterlake):

ValueError: failed to validate Range1d(id='p166776', ...).bounds: expected either None or a value of type MinMaxBounds(Auto, Tuple(Float, Float), Tuple(Nullable(Float), Float), Tuple(Float, Nullable(Float)), Tuple(TimeDelta, TimeDelta), Tuple(Nullable(TimeDelta), TimeDelta), Tuple(TimeDelta, Nullable(TimeDelta)), Tuple(Datetime, Datetime), Tuple(Nullable(Datetime), Datetime), Tuple(Datetime, Nullable(Datetime))), got (1.8247462191085126e+19, 1.8247462191085126e+19)

Which I'm not quite sure what it's trying to tell me... 🤔

sonarcloud[bot] commented 1 day ago