Upstream muon collider calo digis. by aloeliger · Pull Request #253 · HEP-FCC/k4RecCalorimeter

aloeliger · 2026-04-24T23:58:45Z

This is the second of some amount smaller PRs, designed to split up PR key4hep/k4Reco#50 into more manageable pieces. This PR includes changes to calorimeter digis used by the muon collider project in k4Reco, but being redirected here (see: key4hep/k4Reco#51 (comment)). The original work was done by @samf25. I have renamed CaloDigi to RecCaloDigi to try and fit what I guess is the naming theme here, but I am entirely flexible on this point. The CMake file was added by me (although written by Claude, my CMake is still rudimentary at best) based on what seems to be the CMake architecture of this repository, with each distinct directory having it's own file. This CMake file works for me and compiles completely on lxplus using the latest nightly release.

As before, this is largely being opened for discussion to start the process of muon collider forks, if the ultimate PR requires a selected subset of these changes, that is something that can be discussed. This PR is being opened in draft because this is not strictly a key4hep repository, and I am not sure that this is a dependency the muon collider would like to add.

@tmadlener @madbaron @samf25, FYI

BEGINRELEASENOTES

Add muon collider calo digi work originally from k4Reco
ENDRELEASENOTES

tmadlener

I don't have enough permissions here to actually launch workflows and I also didn't yet have time to actually look into the code in a lot of detail. I am very happy that this pulls over the RealisticCalo{Digi,Reco} family of processors though.

Having said that, we had quite some discussion in the Key4hep meeting on March 31 and one of the key outcomes was that we should try to avoid algorithm names like realistic or detailed but rather go for more descriptive algorithm names. I don't have an immediate idea for this family of algorithms, unfortunately.

tmadlener · 2026-04-28T19:51:34Z

Question (most likely to @samf25): Are these direct translations of the one from https://github.com/iLCSoft/MarlinReco/tree/master/CaloDigi/Realistic, or have there been MuonCollider specific changes applied to these?

madbaron · 2026-04-28T20:47:42Z

I'm pretty sure these are 1:1 translations

tmadlener

Thanks a lot for starting this (and apologies up-front for once again being a bit pedantic and requesting quite a few changes).

CI is failing due to a sign comparison warning at the moment. Additionally, after #237 this will likely need some clang-format formatting to comply again.

There are a few general themes in the comments, but in general this looks pretty good to me already. The one thing that we will need to fix is the naming. Still no really good idea, as a basis for discussion:

RealisticCaloDigi (base class) -> ElectronicsSignalDigi (maybe include a Base suffix as well?)
RealisticCaloReco (base class) -> ElectronicsSignalReco

(and then similar for the others). This is all up for discussion, these are just some initial proposals, maybe others have better ideas.

Finally, there is quite some documentation about this here: https://github.com/iLCSoft/MarlinReco/tree/master/CaloDigi/Realistic/doc After all my proposed changes that is probably no longer accurate (or even less accurate than it is now), so my suggestion would be to bring that over in a separate PR, because it's also not entirely clear to me where that should go.

tmadlener · 2026-04-29T08:44:24Z

+
+#include <iostream>
+
+/** Helper class for decoding/encoding lcio::CalorimeterHit types for the ILD


@jmcarcell we already have this in k4GaudiPandora, right? Can we somehow make this available to both without having to copy things?

On a bigger picture, this looks like something that we should potentially revisit (also @BrieucF) as it is very much geared towards CALICE like detectors, resp. ILC / CLIC conventions and might fall apart with other CellID encoding strings.

Hence, also connected to key4hep/k4geo#568

tmadlener · 2026-04-29T08:45:45Z

+struct FilterDoubleLayerHits : public k4FWCore::Transformer<edm4hep::TrackerHitPlaneCollection(
+  const edm4hep::TrackerHitPlaneCollection &)> {


This looks like it is more tracking related (and then should probably go to https://github.com/key4hep/k4RecTracker).

Without having looked into any details about its usage yet, why is it in this PR?

tmadlener · 2026-04-29T08:52:48Z

+    /** Called after data processing for clean up.
+     */
+    StatusCode finalize();


Suggested change

/** Called after data processing for clean up.

*/

StatusCode finalize();

This doesn't seem to do anything (in the implementation file).

tmadlener · 2026-04-29T08:53:51Z

+    using integr_function = std::function<integr_res_opt(const edm4hep::SimCalorimeterHit*)>;
+
+   virtual float EnergyDigi(float energy, float event_correl_miscalib) const;
+   virtual integr_res_opt Integrate( const edm4hep::SimCalorimeterHit * hit ) const;
+
+   integr_res_opt StandardIntegration( const edm4hep::SimCalorimeterHit * hit ) const ;
+   integr_res_opt ROCIntegration( const edm4hep::SimCalorimeterHit * hit ) const ;


All of these functions can just take the edm4hep::SimCalorimeterHit by value or const reference. There is no need to turn it into a pointer (which was probably done because in LCIO you only get pointers).

Additionally (also due to where this comes from likely), these functions should probably be camel cased to follow our usual conventions.

tmadlener · 2026-04-29T08:55:54Z

+    using integr_res_opt = std::optional<integr_res>;
+    using integr_function = std::function<integr_res_opt(const edm4hep::SimCalorimeterHit*)>;
+
+   virtual float EnergyDigi(float energy, float event_correl_miscalib) const;


I don't think this is used in a way that requires it to be virtual. All inheriting classes implement the virtual functions below.

Suggested change

virtual float EnergyDigi(float energy, float event_correl_miscalib) const;

float EnergyDigi(float energy, float event_correl_miscalib) const;

tmadlener · 2026-04-29T09:08:39Z

+  std::string initString;
+  initString = m_geoSvc->constantAsString(m_encodingStringVariable.value());
+  dd4hep::DDSegmentation::BitFieldCoder bitFieldCoder(initString);  // check!


Can also go to a member variable and then be populated once in initialize.

tmadlener · 2026-04-29T09:11:36Z

+    RecCaloDigi
+    Gaudi::GaudiKernel
+    EDM4HEP::edm4hep
+    k4FWCore::k4FWCore
+    k4FWCore::k4Interface
+    DD4hep::DDCore
+    DD4hep::DDRec
+    ROOT::Core
+    ROOT::Hist
+    CLHEP::Random


In principle many of these dependencies are transitively pulled in via RecCaloDigi, but I have nothing against being explicit here.

tmadlener · 2026-04-29T09:21:25Z

+  } else if (m_threshold_unit.value().compare("px") == 0){
+    m_threshold_iunit=NPE;
+  } else {
+    error() << "could not identify threshold unit. Please use \"GeV\", \"MIP\" or \"px\"! Aborting." << endmsg;


This does not abort yet. It would have to return a StatusCode::FAILURE for that.

tmadlener · 2026-04-29T09:21:35Z

+    error() << "Could not guess timing calculation method!" << endmsg;
+    error() << "Available are: Standard, ROC. Provided: " << m_integration_method << endmsg;
+    error() << "Aborting..." << endmsg;


also not aborting

tmadlener · 2026-04-29T09:21:43Z

+      error() << "Fast/slow shaper parameter(s) not set. Required for ROC integration!" << endmsg;
+      error() << "Aborting..." << endmsg;


and once more, not aborting.

madbaron · 2026-04-29T09:59:00Z

The one thing that we will need to fix is the naming. Still no really good idea, as a basis for discussion:

RealisticCaloDigi (base class) -> ElectronicsSignalDigi (maybe include a Base suffix as well?)

RealisticCaloReco (base class) -> ElectronicsSignalReco

(and then similar for the others).

Personally I'm not 100% clear on why the second step is referred to as reco. I overall find it a bit misleading, since all this algorithm does is converting an absolute MIP scale into a particle level energy calibration.

Perhaps one could refer these two as:

BaseDigitiser
BaseCellEnergyCalibrator
or something along these lines?

Add muon collider calo digis

e04a373

tmadlener reviewed Apr 27, 2026

View reviewed changes

tmadlener reviewed Apr 29, 2026

View reviewed changes


		#include <iostream>

		/** Helper class for decoding/encoding lcio::CalorimeterHit types for the ILD

		struct FilterDoubleLayerHits : public k4FWCore::Transformer<edm4hep::TrackerHitPlaneCollection(
		const edm4hep::TrackerHitPlaneCollection &)> {

	/** Called after data processing for clean up.
	*/
	StatusCode finalize();

	virtual float EnergyDigi(float energy, float event_correl_miscalib) const;
	float EnergyDigi(float energy, float event_correl_miscalib) const;

		error() << "Fast/slow shaper parameter(s) not set. Required for ROC integration!" << endmsg;
		error() << "Aborting..." << endmsg;

Conversation

aloeliger commented Apr 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tmadlener left a comment

Choose a reason for hiding this comment

Uh oh!

tmadlener commented Apr 28, 2026

Uh oh!

madbaron commented Apr 28, 2026

Uh oh!

tmadlener 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!

madbaron commented Apr 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aloeliger commented Apr 24, 2026 •

edited

Loading

madbaron commented Apr 29, 2026 •

edited

Loading