3D cluster association algorithm by Mandri97 · Pull Request #280 · PandoraPFAOrg/LArContent

Mandri97 · 2026-04-28T19:02:30Z

An algorithm to associate clusters in 3D. It is a simple approach inspired by the (2D) longitudinal association algorithm, but it removes the reliance and constraints on pseudo-layer to better support 3D reconstruction

…sters

…rather than average PCA direction

CrossR · 2026-04-29T08:53:16Z

Not checked over yet in depth...but this looks very nicely self contained. Could it be split into a LArContent ClusterHelper update and LArRecoND new algorithm?

lhwhitehead · 2026-04-29T12:32:10Z

+    };
+
+    typedef std::unordered_map<const pandora::Cluster*, ClusterAttr> ClusterAttrMap;
+    mutable ClusterAttrMap clusterAttrMap;


Add m_ prefix for class member variables

lhwhitehead · 2026-04-29T12:33:11Z

+    for (ClusterList::const_iterator iter = pClusterList->begin(), iterEnd = pClusterList->end(); iter != iterEnd; ++iter)
+    {
+        const Cluster *const pCluster = *iter;
+        float clusterLength = LArClusterHelper::GetLength(pCluster);


Can be const?

lhwhitehead · 2026-04-29T12:34:27Z

+    CartesianVector currentClusterInnerCoordinate{ clusterAttrMap.at(pCurrentCluster).innerCoordinate };
+    CartesianVector currentClusterOuterCoordinate{ clusterAttrMap.at(pCurrentCluster).outerCoordinate };
+
+    CartesianVector testClusterInnerCoordinate{ clusterAttrMap.at(pTestCluster).innerCoordinate };
+    CartesianVector testClusterOuterCoordinate{ clusterAttrMap.at(pTestCluster).outerCoordinate };


All const?

And make them reference variables

lhwhitehead

Only minor things from my side. Once you are done with everything, run clang format to polish up things into the style that we use.

AndyChappell

Hi Manoa, Thanks for the PR. I think this is generally looking pretty good. I have a few, mostly minor requests and a couple of queries.

On some more general points, as Leigh noted, we'll ultimately pass this through clang format to get the Pandora style. You can skip that step if you want, as for LArContent PRs, I will apply the clang format is a normal matter of constructing release branches. However if you do choose to apply it yourself, can you ensure that it's isolated to a single commit and make it the last one in response to reviews (this makes it easier to distinguish substantive changes from formatting changes when reviewing updates. Thanks!

A secondary comment - and not anything that I think needs addressing for this PR, but I wonder if some of the sliding fit algorithms might provide some more flexibility versus PCA fits for 3D association?

AndyChappell · 2026-05-01T08:18:05Z


 //------------------------------------------------------------------------------------------------------------------------------------------

+


Remove excess whitespace.

AndyChappell · 2026-05-01T08:20:29Z

+    CartesianVector lhsOuterCoordinate(0.f, 0.f, 0.f); 
+    CartesianVector rhsOuterCoordinate(0.f, 0.f, 0.f);


To avoid the extra instantiation, given you don't actually care about these values you could just define

CartesianVector unused(0.f, 0.f, 0.f);

and pass that as the second parameter to both of the extremal coordinate functions below.

Good idea. It's fixed

AndyChappell · 2026-05-01T08:22:37Z

+        float length;
+        pandora::CartesianVector innerCoordinate = pandora::CartesianVector(0.f, 0.f, 0.f);
+        pandora::CartesianVector outerCoordinate = pandora::CartesianVector(0.f, 0.f, 0.f);
+        pandora::CartesianVector centroid = pandora::CartesianVector(0.f, 0.f, 0.f);
+        LArPcaHelper::EigenValues eigenValues = LArPcaHelper::EigenValues(0.f, 0.f, 0.f);
+        LArPcaHelper::EigenVectors eigenVectors;


Coding style in Pandora is to add a prefix m_ to member variables.

AndyChappell · 2026-05-01T12:15:42Z

+        const Cluster *const pCluster = *iter;
+        float clusterLength = LArClusterHelper::GetLength(pCluster);


In general (here and elsewhere), when initialising variables, prefer

type varname{value}; over type varname = value;

unless there's a specific reason not to. This has some benefits in terms of avoiding unintended type narrowing, etc. The main exception here would be if you are explicitly calling an object constructor that might otherwise be confused for an initialisation list in the case of {}. The classic here would be a std::vector

std::vector<int> v{10, 20}; Gets you a vector containing the elements 10 and 20
std::vector<int> v(10, 20); Gets you a vector containing 10 elements, all of which are 20

AndyChappell · 2026-05-01T12:19:09Z

+        if( clusterLength < m_minClusterLength )
+            continue;
+
+        this->PCAFit(pCluster);


I wonder if this would be better named PopulatePCAAttributes, or PopulateFitAttributes?

Renamed PCAFit to PopulateFitAttributes

AndyChappell · 2026-05-01T12:52:59Z

+    constexpr float shortClusterLength = 2.50f;
+
+    // Bypass opening angle cut if a short cluster is the extension of a longer one
+    if( (closestDistance > 0.5f) ||


Should 0.5f be a configurable parameter?

In ND-LAr, 0.5f corresponds to a 4 mm pixel pitch with a 1 mm tolerance. Updated to

const double channelPitch{ 1.3 * LArGeometryHelper::GetWirePitch(this->GetPandora(), m_view) };

to make it detector agnostic.

AndyChappell · 2026-05-01T12:57:23Z

+
+    constexpr float shortClusterLength = 2.50f;
+
+    // Bypass opening angle cut if a short cluster is the extension of a longer one


Would this not also bypass if the long cluster is an extension of the short one? Is that what you want?

Fair. Updated the comment for clarity

// Bypass opening angle check if a short cluster extends a long one (or vice versa)

AndyChappell · 2026-05-01T13:01:49Z

+    const CartesianVector innerEndPrimaryAxis  { centroid + primaryAxis * primaryAxis.GetDotProduct(innerClusterOuterCoordinate - centroid) };
+    const CartesianVector outerStartPrimaryAxis{ centroid + primaryAxis * primaryAxis.GetDotProduct(outerClusterInnerCoordinate - centroid) };


I can't quite get my head around what this is trying to do

These lines determine the coordinates of the inner cluster's end and the outer cluster's start along the longest cluster's primary axis, relative to the longest cluster's centroid.

AndyChappell · 2026-05-01T13:05:14Z

+    bool isInnerLongest{ LArClusterHelper::SortByNHits(pInnerCluster, pOuterCluster) && 
+        (clusterAttrMap.at(pInnerCluster).length > clusterAttrMap.at(pOuterCluster).length) };
+
+    const CartesianVector centroid{ isInnerLongest ? innerCentroid : outerCentroid };


I think all of these CartesianVectors can be reference variables.

AndyChappell · 2026-05-01T13:10:06Z

    d("LArThreeDLongitudinalTracks",            ThreeViewLongitudinalTracksAlgorithm)                                           \
+    d("LArThreeDAssociation",                   ThreeDAssociationAlgorithm)                                                     \


The header files will be reordered by clang format, so could you swap these two here? (clang format is not allowed to alter these preprocessor blocks).

AndyChappell · 2026-05-01T13:18:32Z

Could it be split into a LArContent ClusterHelper update and LArRecoND new algorithm?

Just to comment on this latter point - a clusterhelper style refactoring may have some value here - I'll let @CrossR comment on that, but to the more general point of LArRecoND versus LArContent, modulo a couple of points where some hard-coded values could do with becoming configurable, I think this is a case where the algorithm is sufficiently generic that it belongs in LArContent. It seems to be a general purpose association algorithm based on 3D hits, which, those hard-coded values aside, doesn't appear to have anything that is specific to the ND, and could equally apply to the built 3D clusters in wire/strip-based TPCS. Happy to hear thoughts on this.

AndyChappell

A couple of minor additional requests, and then I think this should be good to go (though looks like there will be merge conflicts to resolve).

AndyChappell · 2026-07-03T08:42:10Z

+    if( m_clusterAttrMap.find(pCurrentCluster) == m_clusterAttrMap.end() )
+        LArClusterHelper::GetExtremalCoordinates(pCurrentCluster, currentClusterInnerCoordinate, currentClusterOuterCoordinate);


If any part of the conditional is multiline, enclose each branch in braces, even if that branch is only a single line

AndyChappell · 2026-07-03T08:43:12Z

+
+    if( m_clusterAttrMap.find(pCurrentCluster) == m_clusterAttrMap.end() )
+        LArClusterHelper::GetExtremalCoordinates(pCurrentCluster, currentClusterInnerCoordinate, currentClusterOuterCoordinate);
+    else{


No need to address this here, as the pre-release clang format run will resolve it, but for future reference, Pandora style is to have the opening brace on its own line.

AndyChappell · 2026-07-03T08:43:21Z

+    if ( m_clusterAttrMap.find(pTestCluster) == m_clusterAttrMap.end() )
+        LArClusterHelper::GetExtremalCoordinates(pTestCluster, testClusterInnerCoordinate, testClusterOuterCoordinate);


As above re brace enclosure

Fixed them all

AndyChappell · 2026-07-03T08:47:37Z

-    const CartesianVector outerCentroid{ clusterAttrMap.at(pOuterCluster).centroid };
-
    const float openingAngle{ innerPrimaryAxis.GetCosOpeningAngle(outerPrimaryAxis) };
+    const double channelPitch{ 1.3 * LArGeometryHelper::GetWirePitch(this->GetPandora(), m_view) };


Should 1.3 be a configurable parameter here?

Yes! Fixed.

CrossR · 2026-07-06T10:36:35Z

I've got two more general / high-level comments (which may be explained if I knew the XML or some of the more underlying code):

Does the m_clusterAttrMap need to be mutable / a class var? It seemed at least that it might be cleaner to just pass it around in the few places that need it, rather than having a mutable var in const methods.
Similarly, I'm not 100% sure what the mutable pandora::HitType m_view; is for? My naive understanding is that as a 3D focused algorithm it would only ever had the 3D views, but is that wrong and the view is going to change a bunch? Regardless it didn't look like it was used very much so could that also just be passed around?

Mandri97 · 2026-08-04T21:36:15Z

Hi @CrossR. I'm not really sure where/how to reply to your comment, so I'm writing here 😅.

Does the m_clusterAttrMap need to be mutable / a class var? It seemed at least that it might be cleaner to just pass it around in the few places that need it, rather than having a mutable var in const methods.

m_clusterAttrMap was introduced to cache cluster attributes since computing them repeatedly is quite expensive. This map is used in both AreClustersAssociated and IsExtremalCluster. The former is called within this class, but IsExtremalCluster is called from the parent class (ClusterAssociationAlgorithm) that this class inherits from. And because of that, there isn't a practical way to pass m_clusterAttrMap to IsExtremalCluster, so it needs to be a member variable. For the mutable thing, I moved all the code that fills m_clusterAttrMap into PopulateFitAttributes and made that method non-const, so that we can remove the mutable keyword

Similarly, I'm not 100% sure what the mutable pandora::HitType m_view; is for? My naive understanding is that as a 3D focused algorithm it would only ever had the 3D views, but is that wrong and the view is going to change a bunch? Regardless it didn't look like it was used very much so could that also just be passed around?

Good catch! It's only used in one place, so it can be a local variable instead. It's only used to retrieve the wire pitch not to do any reco logic.

… consolidate cluster attribute population into PopulateFitAttributes

Mandri97 · 2026-08-11T00:02:08Z

Edit to my previous comment.

Ugh, I forgot that PopulateFitAttributes (renamed to PopulateClusterAttributes) is called from GetListOfCleanClusters, which is const, so PopulateClusterAttributes had to become const too, which meant m_clusterAttrMap had to become mutable

Mandri97 and others added 18 commits August 21, 2025 15:14

Add ThreeD cluster association algorithm

4c7231a

Fit clusters with PCA and changed the default opening angle threshold

ef00e63

Fix bugs

5cd8bfd

Merge branch 'PandoraPFA:master' into feature/ThreeDAssociationAlgorithm

4454fa8

sync update

23f344e

Cleaning a bit and bypass opening angle cut for (short) adjancent clu…

9d8c39a

…sters

Update cluster extension check to use projection of start/end points …

0712d12

…rather than average PCA direction

Merge branch 'PandoraPFA:master' into feature/ThreeDAssociationAlgorithm

f5630e7

Remove cluster visualizer

860808f

Cache cluster attributes

5cb8b9a

Guard infinite loop and skip overlapping clusters

a632635

Fix overlapping clusters cuts

3f89234

Use the longest cluster directions

df194d9

Cleaning up a bit

966a6e8

Merge branch 'PandoraPFA:master' into feature/ThreeDAssociationAlgorithm

54201e0

Reorganize files

783cdf7

Cleaning

45f3a06

Sync LArContent.cc

783b706

lhwhitehead reviewed Apr 29, 2026

View reviewed changes

AndyChappell requested changes May 1, 2026

View reviewed changes

Mandri97 added 3 commits May 5, 2026 11:14

Fix preprocessor order

a945ed5

Reuse dummy variable for unused parameters

e05e074

Apply reviewers suggestions

d765be3

AndyChappell requested changes Jul 3, 2026

View reviewed changes

Mandri97 added 2 commits August 10, 2026 18:40

Make channel pitch tolerance configurable, view a local variable, and…

24bd0e1

… consolidate cluster attribute population into PopulateFitAttributes

Rename PopulateFitAttributes to PopulateCluterAttributes

3657bcb


		//------------------------------------------------------------------------------------------------------------------------------------------

		CartesianVector lhsOuterCoordinate(0.f, 0.f, 0.f);
		CartesianVector rhsOuterCoordinate(0.f, 0.f, 0.f);

		const Cluster const pCluster = iter;
		float clusterLength = LArClusterHelper::GetLength(pCluster);


		constexpr float shortClusterLength = 2.50f;

		// Bypass opening angle cut if a short cluster is the extension of a longer one

		const CartesianVector innerEndPrimaryAxis { centroid + primaryAxis * primaryAxis.GetDotProduct(innerClusterOuterCoordinate - centroid) };
		const CartesianVector outerStartPrimaryAxis{ centroid + primaryAxis * primaryAxis.GetDotProduct(outerClusterInnerCoordinate - centroid) };

		d("LArThreeDLongitudinalTracks", ThreeViewLongitudinalTracksAlgorithm) \
		d("LArThreeDAssociation", ThreeDAssociationAlgorithm) \

		if( m_clusterAttrMap.find(pCurrentCluster) == m_clusterAttrMap.end() )
		LArClusterHelper::GetExtremalCoordinates(pCurrentCluster, currentClusterInnerCoordinate, currentClusterOuterCoordinate);

		if ( m_clusterAttrMap.find(pTestCluster) == m_clusterAttrMap.end() )
		LArClusterHelper::GetExtremalCoordinates(pTestCluster, testClusterInnerCoordinate, testClusterOuterCoordinate);

Uh oh!

Conversation

Mandri97 commented Apr 28, 2026

Uh oh!

CrossR commented Apr 29, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhwhitehead left a comment

Choose a reason for hiding this comment

Uh oh!

AndyChappell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mandri97 May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndyChappell commented May 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AndyChappell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mandri97 May 5, 2026 •

edited

Loading

AndyChappell commented May 1, 2026 •

edited

Loading

Mandri97 commented Aug 4, 2026 •

edited

Loading