feat: added new algorithm to clusterize cells in-place

[goetzgaycken]

The in-place clusterization could be used instead of the default clusterization but has a slightly different interface. It clusterizes by sorting the cell container in such a way that adjacent cells with the same label belong to the same cluster. This is advantageous since an additional cluster collection is avoid if the cluster collection resulting from the clusterization is only temporary.

[goetzgaycken]

• In synthetic benchmarks it seems to be 20+-4% faster than the current clusterization.
• When running a digitization only example job using a test sample (ttbar, PU200, Geant4, ODD), the improvements seem more substantial. A reason might be that the code path with the current clusterization is not fully optimized.

[github-actions]

📊: Physics performance monitoring for 8469555

Full contents

physmon summary

• ✅ Particles fatras
• ✅ Particles geant4
• ✅ Particles ttbar
• ✅ Vertices ttbar
• ✅ Truth tracking (KF)
• ✅ Truth tracking (GSF)
• ✅ Truth tracking (GX2F)
• ✅ Truth tracking (KF refit)
• ✅ Truth tracking (GSF refit)
• ✅ CKF finding performance | trackfinding | single muon | truth smeared seeding
• ✅ CKF fitting performance | trackfinding | single muon | truth smeared seeding
• ✅ CKF track summary | trackfinding | single muon | truth smeared seeding
• ✅ Seeding trackfinding | single muon | truth estimated seeding
• ✅ CKF finding performance | trackfinding | single muon | truth estimated seeding
• ✅ CKF fitting performance | trackfinding | single muon | truth estimated seeding
• ✅ CKF track summary | trackfinding | single muon | truth estimated seeding
• ✅ Seeding trackfinding | single muon | default seeding
• ✅ CKF finding performance | trackfinding | single muon | default seeding
• ✅ CKF fitting performance | trackfinding | single muon | default seeding
• ✅ CKF track summary | trackfinding | single muon | default seeding
• ✅ Seeding trackfinding | single muon | orthogonal seeding
• ✅ CKF finding performance | trackfinding | single muon | orthogonal seeding
• ✅ CKF fitting performance | trackfinding | single muon | orthogonal seeding
• ✅ CKF track summary | trackfinding | single muon | orthogonal seeding
• ✅ Seeding trackfinding | 4 muon x 50 vertices | default seeding
• ✅ CKF finding performance | trackfinding | 4 muon x 50 vertices | default seeding
• ✅ CKF fitting performance | trackfinding | 4 muon x 50 vertices | default seeding
• ✅ CKF track summary | trackfinding | 4 muon x 50 vertices | default seeding
• ✅ Ambisolver finding performance | trackfinding | 4 muon x 50 vertices | default seeding
• ✅ IVF notime | trackfinding | 4 muon x 50 vertices | default seeding
• ✅ AMVF gauss notime | trackfinding | 4 muon x 50 vertices | default seeding
• ✅ AMVF grid time | trackfinding | 4 muon x 50 vertices | default seeding
• ✅ Seeding trackfinding | ttbar with 200 pileup | default seeding
• ✅ CKF finding performance | trackfinding | ttbar with 200 pileup | default seeding
• ✅ CKF fitting performance | trackfinding | ttbar with 200 pileup | default seeding
• ✅ CKF track summary | trackfinding | ttbar with 200 pileup | default seeding
• ✅ Ambisolver finding performance | trackfinding | ttbar with 200 pileup | default seeding
• ✅ ML Ambisolver | trackfinding | ttbar with 200 pileup | default seeding
• ✅ AMVF gauss notime | trackfinding | ttbar with 200 pileup | default seeding
• ✅ AMVF grid time | trackfinding | ttbar with 200 pileup | default seeding
• 🔵️ Comparison - Truth tracking (GX2F vs KF)

[goetzgaycken]

I haven't heard any strong voices concerning the demonstration of this new clusterization algorithm in the examples framework. Therefore, the plan would be to leave for the time being the option in the DigitizationAlgorithm to toggle between the two clusterization algorithm which slightly increases its complexity. The longer term plan would be to only have one clusterization algorithm.

[CarloVarni]

This implementation does not support time-based clusterization as well. Can it be extended to cover that case?

[goetzgaycken]

I haven't tried it, and I haven't even tried to clusterize more than 2 dimensions. But in theory one could add time as an extra coordinate and transform the time such that a time difference <=1 means connected. The types of all coordinates have to be the same and the type must be an integral type (though the "integral" requirement is likely not really necessary, but the abort criterion would have to be relaxed to <=2, I suppose ) If the time cannot be transformed in such a way (I would not know why not) one would have to specialize isConnected or isConnected4 and isConnected8, time could not be used as a sorting direction since a difference of >1 in the sorting axis would abort the search for connections.

[CarloVarni]

right now we are doing it with way: https://github.com/acts-project/acts/blob/main/Core/include/Acts/Clusterization/TimedClusterization.hpp

do you think this is not adaptable to this implementation of yours?

[goetzgaycken]

to allow for different coordinate types (i.e. int for spacial coordinates and float for time) would make the code more complicated. Since in the cases I have seen a temporary cell collection is created anyways, then one can also transform the time into something more suitable for the clusterization, I would say.

[CarloVarni]

why is this a different implementation of the clustering routine instead of a replacement? If you can demonstrate this provides the same results and it is faster I'd suggest to simply replace the previous version and mark the PR as a breaking one.

[goetzgaycken]

If the current clusterization would produce the final clusters, than that might be advantageous. The final label sorting might add an unnecessary overhead in that case. So far I only encountered cases in which the cluster collection was temporary.

[goetzgaycken]

changes from 9aca2e0 to d1fd488:

• removed unnecessary inline keyword
• changed template parameter names from T_Name to name_t
• put clusterization into namespace Acts
• merged requirements i.e. requires(a){..} && requires(b) {...} -> requires(a,b) {...}

[goetzgaycken]

changes d1fd488 to b54ce39:

• specify concept for index_t
• removed unnecessary template parameter axis_t

[goetzgaycken]

btw. the names isConnected4 and isConnected8 only make sense in the 2D case. In the examples framework "commonCorrner" is used instead. To distinguish the two one could use commonEdge commonEdgeAndCorner. But that would get rather verbose.

[goetzgaycken]

At the moment the criterion for aborting the search for connections is diff[SORT_AXIS] > 1, which assumes that the coordinates are integral. In theory it might be possible to use floats, but then this criterion would be too restrictive. So one possibility would be to replace this by a function like isConnected, then alternative implementations could be provided.

It is not clear to me whether there would be a use case for that (or does a useful clusterization of location+time would really require none integral times? )

[goetzgaycken]

changes b54ce39 to 11f46ad:

• extended requirements for CellWithLabel:
  • coordinates can be compared,
  • label and dimension are of unsigned integral type.

[goetzgaycken]

changes 11f46ad to efc6765:

• fixed clang-tidy warnings, and silenced clang-tidy warnings about uninitialized values in 2 cases in which the variables are set just below.

[goetzgaycken]

changes 7b9aa72 to a2898e2:

• renamed connection types from fold -> ConnectionType, 8Fold -> CommonEdgeOrCorner, 4Fold -> ConnonEdge
• support unsigned coordinates i.e. when computing differences for unsigned types subtract smaller from larger value, the difference is now a template which can be specialized to support alternative coordinate types.
• moved abort criterion (diff[sort_axis]>1) to template which could be specialized.
• improved comment concerning types of coordinates.

[goetzgaycken]

changes a2898e2 to 94c8288:

• forgot a couple of places to rename fold -> connect type, common corner ...

[andiwand]

Suggested change

	unsigned int min_label;
	unsigned int max_label;
	unsigned int min_label{};
	unsigned int max_label{};

[goetzgaycken]

these are point-less initializations. The values are overwritten in the lines below. It is likely optimized away, but shouldn't the static code analyzer understand that there is no problem here rather than the variables to be set to some useless random values (i.e. the initialization with zero only satisfies clang-tidy but would not be a meaningful default value).

[andiwand]

To you it is pointless - ok, but to others, including me, it is not. A good optimizer will make this go away, a good static analyzer would understand that there is no problem. We only have one of both and we should work with what we have and don't turn off the linter for such unimportant cases. If you have better default values you can use those of course.

[goetzgaycken]

In the following example clang++ --analyze will complain

unsigned int absDiff(int a, int b) {
   int x0;
   int x1;
   // NOLINTBEGIN(cppcoreguidelines-init-variables)
  if ( a<b) {
     x0=a;
     x1=b;
  }
  else {
     x0=b;
  }
  // NOLINTEND(cppcoreguidelines-init-variables)
  return x1-x0;
}

If I change it as You say to satisfy clang-tidy:

unsigned int absDiff(int a, int b) {
   int x0{};
   int x1{};
  if ( a<b) {
     x0=a;
     x1=b;
  }
  else {
     x0=b;
  }
  return x1-x0;
}

Neither of the tools (clang-tidy, clang++ --analyze) will complain. So, is better to satisfy bad tools, or to not add pointless/incorrect initializations. For me it is clearly the latter because in the latter case, a good tool actually helps me to find the bug, where in the first case the bad tool just helps me to hide the mistake under the rug.

[paulgessinger]

I strongly think we should avoid toggling the linter even if we're convinced it's safe until we have reason to believe there is some performance to be gained.

I would ask you therefore to go with the (potentially unnecessary) initialization for now, and circle back later if we find this can improve things.

[goetzgaycken]

One possibility, I see, to write the code without bogus initialization and without triggering bogus clang-tidy warnings, would be to use a helper lambda e.g.

   auto [min_label,max_label] = [&]() {
      if (a < b) {
         ....
         return std::make_pair(a,b);
      } else {
        ....
         return std::make_pair(b,a);
      }
   }();

But that makes things much more complicated than necessary.

[goetzgaycken]

The point is not so much that it is unnecessary, but that the default value is not necessarily correct.

[goetzgaycken]

Another simple example. This is wrong

#include <array>
int main() {
   std::array<int,2> a;
   for (unsigned int i=0; i<1; ++i) {
      a[i]=i+1;
   }
   int sum=0;
   for (unsigned int i=0; i<a.size(); ++i) {
      sum += a[i];
   }
   return sum==3 ? 0 : 1;
}

but it is not more correct when changing std::array<int,2> a; to std::array<int,2> a{};.

I thought that I encountered a static analyzer which would detect the problem but the ones I tried won't. However valgrind and even clang++ with -fsanitize=memory will find the problem, but obviously not if a gets initialized. Anyway, initializing variables just for the sake of making clang-tidy happy, makes it harder for better tools to find the actual error. Therefore, I don't think it is an improvement, it just hides a potential problem.

[goetzgaycken]

changes d5bf037 to ea99ee1 and ea99ee1 to 72b2838:

• removed member prefix from m_ from public members of the reference Cell implementation.
• exit early and reduced indention in for_each_cluster
• improved comments for Cell,
• empty line after pragma once
• make use of CellCollecion::value_type which was required indirectly anyway, now more explicitly.

I did not:

• extend to CellCollection concept to require the container to provide the dimension of the grid,
• make a static_assert a requirement of the CellCollection since it is only relevant for the function and is more a sanity check.
• satisfy clang-tidy (i.e. I did not initialize variables to some random value just before they are actually set to a meaningful one just to avoid clang-tidy warnings)

[goetzgaycken]

change 72b2838 to 0c631b5:

• Like original clusterization there is now an object for the connection testing, which by default has no data members and only static methods, but could be replaced by something which can provide parameters to the connection test. In the clusterization function the object type replaces the connection type template parameter
• Extended the 2D clusterization unit test by an "example" which clusterizes cells with 2D spatial coordinates and time, where also time is given by a 16 bit integer.

I thought a little bit more about the customization of the connection testing. I suppose some customization will be necessary if the clusterization should take into account spatial and time cell components. I originally wanted to get rid of the connection object template parameter (current clusterization), but since I already introduced a template parameter to steer the connection test (enum), it could also just be turned into an arbitrary type which is more or less the current design. The overhead is minimal (or zero) if the "connection" object has no data members and only static methods which is the case for the default: "ConnectionHelper" (Maybe ConnectionTester would be a better name? ). This helper provides the original functions isConnected and canAbortSearch.

[goetzgaycken]

changes 0c631b5 to 8033eb6:

• obfuscated an aggressive constant.

[goetzgaycken]

changes 033eb6 to 077f30a:

• rebase to 2025-11-06_T1142
• solved merge conflicts in ClusterizationTests2D, and
• adjusted additions to ClusterizationTests2D for duplicate cell exception.

[goetzgaycken]

changes 077f30a to cbd9c04:

• clang-tidy and doc fixes

[goetzgaycken]

cbd9c04 to 095167c:

• more line and doc fixes.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
96.5% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[andiwand]

Sorry but I don't think this discussion is worth the time and effort given what this is about. I asked for very simple adjustments to the code to apply with out guidelines of readability and maintainability. I cannot approve this as. You can of course ask someone else for a review / approval.

[goetzgaycken]

changes ddb6bc2:

• as the comments says, works around the false clang-tidy positive without switching off the warning as requested. Not sure it is an improvement though.

[goetzgaycken]

changes 74723d2 to 8469555:

• bowing to the machine. Reverted the latest change because the solution which solved the clang-tidy complaint about uninitialized variables, by actually initializing the variables properly, added too much complexity. Moreover, instead of switching off the clang-tidy warning caused by its limited unstanding, clang-tidy is now satisfied by bogus initializations. Likely compilers are smarter than clang-tidy and understand that these intializations do not serve any purpose and optimize them away, but at the same time smarter tools are taken the possibility to actually find real initialization problems, since there are now the bogus initializations. Anyway clang-tidy is happy, and it seems to be the preferred solution even so the solution has no other purpose than satisfying clang-tidy and it makes harder to find problems with better tools.