Implement Calo Remnant Combiner (PFA2) #2195

palsgit · 2025-11-19T22:16:59Z

Briefly, what does this PR introduce?

This PR implements the CaloRemnantCombiner algorithm. This algorithm ingests remnant ecal and hcal clusters after track-cluster matching and converts them to neutral particle candidates, i.e. edm4eic::ReconstructedParticles with only associated clusters filled.

What kind of change does this PR introduce?

Bug fix (issue #__)
New feature (issue Implement PFA2: Remnant Cluster Combiner #2129 )
Documentation update
Other: __

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?

No.

for more information, see https://pre-commit.ci

veprbl · 2025-11-20T15:58:32Z

src/algorithms/particle/CaloRemnantCombiner.cc

-  // Get the cluster indices for merging
-  std::set<std::size_t> ecalcluster_indices = getcluster_indices_for_merging(*calo_clusters[0], visits_ecal, seed_ecal_index, m_cfg.delta_r_add_em, *calo_clusters[0]);
+    if (seed_ecal_index == -1) {
+      info("No Seed Ecal cluster found for remnant combination.");


Suggested change

info("No Seed Ecal cluster found for remnant combination.");

debug("No Seed Ecal cluster found for remnant combination");

info messages are too "loud" for a common algorithm message

ruse-traveler

Thanks for the awesome work on this, @palsgit ! This is looking great so far! Most of my comments are really about documentation (e.g. docstrings) and being stylistically consistent. The biggest things I noted in this round are some missing declarations in the factory, and adding the factory generators for the other eta regions in the plugin.

I think there might a few ways to simplify the algorithm further using podio::ObjectIDs and operating on the clusters themselves, but I'll think on it and we can follow-up in a subsequent pass if need be!

ruse-traveler · 2025-11-21T17:18:05Z

src/algorithms/particle/CaloRemnantCombiner.cc

+
+    // Using simple Euclidean distance in 3D space
+    ///double distance = edm4hep::utils::magnitude(clusters[i].getPosition() - seed[seed_cluster_index].getPosition());
+
+    // Using delta R in the transverse plane (x-y plane)
+    /*double dx = clusters[i].getPosition().x - seed[seed_cluster_index].getPosition().x;
+    double dy = clusters[i].getPosition().y - seed[seed_cluster_index].getPosition().y;
+    double distance = std::sqrt(dx * dx + dy * dy);*/
+
+    // Using angular distance (delta R) in eta-phi space


I'd suggest removing the unused code and instead flag using different metrics as a FIXME for later

Suggested change

// Using simple Euclidean distance in 3D space

///double distance = edm4hep::utils::magnitude(clusters[i].getPosition() - seed[seed_cluster_index].getPosition());

// Using delta R in the transverse plane (x-y plane)

/*double dx = clusters[i].getPosition().x - seed[seed_cluster_index].getPosition().x;

double dy = clusters[i].getPosition().y - seed[seed_cluster_index].getPosition().y;

double distance = std::sqrt(dx * dx + dy * dy);*/

// Using angular distance (delta R) in eta-phi space

// using angular distance (delta R) in eta-phi space

// - FIXME expand to allow for other distance metrics

ruse-traveler · 2025-11-21T17:26:54Z

src/algorithms/particle/CaloRemnantCombiner.h

+    algorithms::Algorithm<algorithms::Input<std::vector<edm4eic::ClusterCollection>>,
+                          algorithms::Output<edm4eic::ReconstructedParticleCollection>>;
+
+class CaloRemnantCombiner : public CaloRemnantCombinerAlgorithm,


I'd suggest adding docstrings in several places. These should get propagated to our doxygen page, and help explain what's happening in the code for anyone who's looking at the source code!

Suggested change

class CaloRemnantCombiner : public CaloRemnantCombinerAlgorithm,

// ==========================================================================

//! Calorimeter Remnant Cluster Combiner

// ==========================================================================

/*! An algorithm which takes multiple calorimeter cluster collections and combines them into

* neutral-particle candidates based on distance matching.

*/

class CaloRemnantCombiner : public CaloRemnantCombinerAlgorithm,

ruse-traveler · 2025-11-21T17:32:03Z

src/algorithms/particle/CaloRemnantCombiner.h

+
+namespace eicrecon {
+
+using CaloRemnantCombinerAlgorithm =


This would be another good place for a docstring, especially since the ordering of collections in the input vector matters!

Suggested change

using CaloRemnantCombinerAlgorithm =

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

//! Algorithm input/output

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

/*! Input is a vector of calorimeter cluster collections. For now:

* - 1st entry in the vector should be the EMCal collection, and

* - 2nd entry in the vector should be the HCal collection.

* This can be generalized in the future.

*/

using CaloRemnantCombinerAlgorithm =

ruse-traveler · 2025-11-21T17:34:58Z

src/algorithms/particle/CaloRemnantCombinerConfig.h

+  double delta_r_add_em = 0.03;
+  double delta_r_add_h  = 0.15;


We don't really enforce camel-vs-snake case yet, but the majority of config files use camel case so I'd suggest using that for consistency!

Suggested change

double delta_r_add_em = 0.03;

double delta_r_add_h = 0.15;

double deltaRAddEM = 0.03;

double deltaRAddH = 0.15;

ruse-traveler · 2025-11-21T17:37:07Z

src/factories/particle/CaloRemnantCombiner_factory.h

+  /*ParameterRef<double> m_min_energy_over_momentum{this, "minEnergyOverMomentum",
+                                                  config().min_energy_over_momentum};
+  ParameterRef<double> m_max_energy_over_momentum{this, "maxEnergyOverMomentum",
+                                                  config().max_energy_over_momentum};*/


Typo?

Suggested change

/*ParameterRef<double> m_min_energy_over_momentum{this, "minEnergyOverMomentum",

config().min_energy_over_momentum};

ParameterRef<double> m_max_energy_over_momentum{this, "maxEnergyOverMomentum",

config().max_energy_over_momentum};*/

ParameterRef<double> m_deltaRAddEM{this, "deltaRAddEM", config().deltaRAddEM};

ParameterRef<double> m_deltaRAddH{this, "deltaRAddH", config().deltaRAddH};

ruse-traveler · 2025-11-21T18:15:20Z

src/algorithms/particle/CaloRemnantCombiner.cc

+    const edm4eic::ClusterCollection& clusters, std::vector<bool>& visits,
+    std::size_t seed_cluster_index, double delta_r, const edm4eic::ClusterCollection& seed) {


I'd suggest making the delta_r argument here parallel the config name (makes the code a little more self-describing).

Suggested change

const edm4eic::ClusterCollection& clusters, std::vector<bool>& visits,

std::size_t seed_cluster_index, double delta_r, const edm4eic::ClusterCollection& seed) {

const edm4eic::ClusterCollection& clusters, std::vector<bool>& visits,

std::size_t seed_cluster_index, double delta_r_add, const edm4eic::ClusterCollection& seed) {

ruse-traveler · 2025-11-21T18:16:39Z

src/algorithms/particle/CaloRemnantCombiner.cc

+    float deta     = eta_cluster - eta_seed;
+    float distance = std::sqrt(deta * deta + dphi * dphi);
+
+    if (distance < delta_r) { // distance threshold for merging


Following up on the above comment: this way I think you can drop the comment after the braces!

Suggested change

if (distance < delta_r) { // distance threshold for merging

if (distance < delta_r_add) {

ruse-traveler · 2025-11-21T18:25:43Z

src/algorithms/particle/CaloRemnantCombiner.cc

+  return seed_cluster_index;
+}
+
+std::set<std::size_t> CaloRemnantCombiner::getcluster_indices_for_merging(


This is another good place for a docstring: you'll want to briefly explain what the method is doing!

Suggested change

std::set<std::size_t> CaloRemnantCombiner::getcluster_indices_for_merging(

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

//! Find cluster indices for merging

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

/*! Creates a set of indices corresponding to clusters which lie within

* a radius of `delta_r_add` around the seed cluster with index

* `seed_cluster_index`.

*/

std::set<std::size_t> CaloRemnantCombiner::get_cluster_indices_for_merging(

ruse-traveler · 2025-11-21T18:29:16Z

src/algorithms/particle/CaloRemnantCombiner.cc

+
+} // end of process
+
+std::size_t CaloRemnantCombiner::findSeedCluster_index(const edm4eic::ClusterCollection& clusters,


Another good place for a docstring! Here you'll definitely want to note the criteria for a seed cluster and what it's for.

Suggested change

std::size_t CaloRemnantCombiner::findSeedCluster_index(const edm4eic::ClusterCollection& clusters,

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

//! Find seed cluster

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

/*! Identifies a seed (highest energy) cluster in a collection

* which sets the center of the cone in which clusters are

* combined.

*/

std::size_t CaloRemnantCombiner::find_seed_cluster_index(const edm4eic::ClusterCollection& clusters,

ruse-traveler · 2025-11-21T18:40:41Z

src/algorithms/particle/CaloRemnantCombiner.cc

+
+namespace eicrecon {
+
+void CaloRemnantCombiner::process(const CaloRemnantCombiner::Input& input,


Definitely want a docstring here! This is a good place for a high-level summary of what the algorithm is doing!

Suggested change

void CaloRemnantCombiner::process(const CaloRemnantCombiner::Input& input,

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

//! Process inputs

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

/*! Construct a candidate neutral particle via the

* following algorithm.

* 1. Repeat the following the steps until every EMCal

* cluster has been used:

* a. Identify seed EMCal cluster

* b. Identify all EMCal clusters and HCal clusters which

* lie within a radius of deltaRAddEM and deltaRAddH

* around seed EMCal cluster respectively

* c. Combine all identified clusters into a neutral particle

* candidate

* 2. Repeat the following steps until every HCal

* cluster has been used:

* a. Identify seed HCal cluster

* b. Identify all HCal clusters which lie within a

* radius of deltaRAddH around seed HCal

* cluster

* c. Combine all identified clusters into a neutral particle

* candidate

*/

void CaloRemnantCombiner::process(const CaloRemnantCombiner::Input& input,

PFA2 : first implementation

ad853fe

palsgit linked an issue Nov 19, 2025 that may be closed by this pull request

Implement PFA2: Remnant Cluster Combiner #2129

Open

[pre-commit.ci] auto fixes from pre-commit.com hooks

5f26b12

for more information, see https://pre-commit.ci

palsgit requested a review from ruse-traveler November 19, 2025 22:18

palsgit changed the title ~~PFA2 : first implementation~~ Implement Calo Remnant Combiner (PFA2) #2129 Nov 19, 2025

palsgit changed the title ~~Implement Calo Remnant Combiner (PFA2) #2129~~ Implement Calo Remnant Combiner (PFA2) Nov 19, 2025

github-actions bot added the topic: infrastructure label Nov 19, 2025

veprbl reviewed Nov 20, 2025

View reviewed changes

ruse-traveler requested changes Nov 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Implement Calo Remnant Combiner (PFA2) #2195

Implement Calo Remnant Combiner (PFA2) #2195

palsgit commented Nov 19, 2025

Uh oh!

veprbl Nov 20, 2025

Uh oh!

ruse-traveler left a comment

Uh oh!

ruse-traveler Nov 21, 2025

Uh oh!

ruse-traveler Nov 21, 2025

Uh oh!

ruse-traveler Nov 21, 2025

Uh oh!

ruse-traveler Nov 21, 2025

Uh oh!

ruse-traveler Nov 21, 2025

Uh oh!

ruse-traveler Nov 21, 2025

Uh oh!

ruse-traveler Nov 21, 2025

Uh oh!

ruse-traveler Nov 21, 2025

Uh oh!

ruse-traveler Nov 21, 2025

Uh oh!

ruse-traveler Nov 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	info("No Seed Ecal cluster found for remnant combination.");
	debug("No Seed Ecal cluster found for remnant combination");

-class CaloRemnantCombiner : public CaloRemnantCombinerAlgorithm,
+// ==========================================================================
+//! Calorimeter Remnant Cluster Combiner
+// ==========================================================================
+/*! An algorithm which takes multiple calorimeter cluster collections and combines them into
+ *  neutral-particle candidates based on distance matching.
+ */
+class CaloRemnantCombiner : public CaloRemnantCombinerAlgorithm,

-using CaloRemnantCombinerAlgorithm =
+// --------------------------------------------------------------------------
+//! Algorithm input/output
+// --------------------------------------------------------------------------
+/*! Input is a vector of calorimeter cluster collections. For now:
+  *    - 1st entry in the vector should be the EMCal collection, and
+  *    - 2nd entry in the vector should be the HCal collection.
+  *  This can be generalized in the future.
+  */
+using CaloRemnantCombinerAlgorithm =

		const edm4eic::ClusterCollection& clusters, std::vector<bool>& visits,
		std::size_t seed_cluster_index, double delta_r, const edm4eic::ClusterCollection& seed) {

	if (distance < delta_r) { // distance threshold for merging
	if (distance < delta_r_add) {

-std::set<std::size_t> CaloRemnantCombiner::getcluster_indices_for_merging(
+// ----------------------------------------------------------------------------
+//! Find cluster indices for merging
+// ----------------------------------------------------------------------------
+/*! Creates a set of indices corresponding to clusters which lie within
+ *  a radius of `delta_r_add` around the seed cluster with index
+ *  `seed_cluster_index`.
+ */
+std::set<std::size_t> CaloRemnantCombiner::get_cluster_indices_for_merging(


		} // end of process

		std::size_t CaloRemnantCombiner::findSeedCluster_index(const edm4eic::ClusterCollection& clusters,

-std::size_t CaloRemnantCombiner::findSeedCluster_index(const edm4eic::ClusterCollection& clusters,
+// ----------------------------------------------------------------------------
+//! Find seed cluster
+// ----------------------------------------------------------------------------
+/*! Identifies a seed (highest energy) cluster in a collection
+ *  which sets the center of the cone in which clusters are
+ *  combined.
+ */
+std::size_t CaloRemnantCombiner::find_seed_cluster_index(const edm4eic::ClusterCollection& clusters,


		namespace eicrecon {

		void CaloRemnantCombiner::process(const CaloRemnantCombiner::Input& input,

-void CaloRemnantCombiner::process(const CaloRemnantCombiner::Input& input,
+// ----------------------------------------------------------------------------
+//! Process inputs
+// ----------------------------------------------------------------------------
+/*! Construct a candidate neutral particle via the
+ *  following algorithm.
+ *    1. Repeat the following the steps until every EMCal
+ *       cluster has been used:
+ *       a. Identify seed EMCal cluster
+ *       b. Identify all EMCal clusters and HCal clusters which
+ *          lie within a radius of deltaRAddEM and deltaRAddH
+ *          around seed EMCal cluster respectively
+ *       c. Combine all identified clusters into a neutral particle
+ *          candidate
+ *    2. Repeat the following steps until every HCal
+ *       cluster has been used:
+ *       a. Identify seed HCal cluster
+ *       b. Identify all HCal clusters which lie within a
+ *          radius of deltaRAddH around seed HCal
+ *          cluster
+ *       c. Combine all identified clusters into a neutral particle
+ *          candidate
+ */
+void CaloRemnantCombiner::process(const CaloRemnantCombiner::Input& input,

Implement Calo Remnant Combiner (PFA2) #2195

Are you sure you want to change the base?

Implement Calo Remnant Combiner (PFA2) #2195

Conversation

palsgit commented Nov 19, 2025

Briefly, what does this PR introduce?

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?

Does this PR change default behavior?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ruse-traveler 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants