Added crystalID field to caloHitMC and changed LY for LYSO by bechenard · Pull Request #1756 · Mu2e/Offline

bechenard · 2026-03-06T21:23:22Z

No description provided.

FNALbuild · 2026-03-06T21:23:48Z

Hi @bechenard,
You have proposed changes to files in these packages:

MCDataProducts
CaloReco
CaloMC

which require these tests: build.

@Mu2e/fnalbuild-users, @Mu2e/write have access to CI actions on main.

⌛ The following tests have been triggered for fa4d74c: build (Build queue - API unavailable)

About FNALbuild. Code review on Mu2e/Offline.

FNALbuild · 2026-03-06T21:36:42Z

☀️ The build tests passed at fa4d74c.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged `fa4d74c` at `9ffc000`
build (prof)	✅	Log file. Build time: 04 min 12 sec
ceSimReco	✅	Log file.
g4test_03MT	✅	Log file.
transportOnly	✅	Log file.
POT	✅	Log file.
g4study	✅	Log file.
cosmicSimReco	✅	Log file.
cosmicOffSpill	✅	Log file.
ceSteps	✅	Log file.
ceDigi	✅	Log file.
muDauSteps	✅	Log file.
ceMix	✅	Log file.
rootOverlaps	✅	Log file.
g4surfaceCheck	✅	Log file.
trigger	✅	Log file.
check_cmake	✅	Log file.
FIXME, TODO	✅	TODO (0) FIXME (0) in 5 files
clang-tidy	➡️	6 errors 114 warnings
whitespace check	✅	no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at fa4d74c after being merged into the base branch at 9ffc000.

For more information, please check the job page here.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

michaelmackenzie

Looks good to me, thank you! I just made one small comment, but it's not necessary to resolve in order to merge

michaelmackenzie · 2026-03-06T21:57:58Z

CaloMC/src/CaloShowerROMaker_module.cc

+                  bool isCaphri = std::find(CaloConst::_caphriId.begin(),CaloConst::_caphriId.end(),
+                                            crystalID) != CaloConst::_caphriId.end();
+                  float pePerMeV = isCaphri ? pePerMeVLyso_ : pePerMeV_;


Should this be moved outside the for loop, instead of evaluated in each loop?

Also, same comment as above

Obviously, I was tired....

oksuzian · 2026-03-07T15:58:28Z

Let me review this pull request by examining its details and the code changes.Here's my review of PR #1756. This PR adds a crystalID field to CaloHitMC and introduces a separate light yield (LY) parameter for LYSO crystals (CaPHRI detectors), distinguishing them from CsI crystals. I found several issues:

🔴 Bugs / Correctness Issues

1. Breaking change to `CaloHitMC` constructor — other callers may be broken

// Old:
CaloHitMC(const std::vector<CaloEDepMC>& edeps) : edeps_(edeps) {};
// New:
CaloHitMC(const std::vector<CaloEDepMC>& edeps, int crystalId) :
  edeps_(edeps), crystalId_(crystalId) {};

The constructor signature changed from 1 parameter to 2. Any existing code in the codebase that constructs a CaloHitMC with only an edeps vector will now fail to compile. Only CaloHitTruthMatch_module.cc is updated in this PR — there may be other call sites that were missed.

2. `CaloHitMC` move constructor lost — potential unnecessary copies

caloHitMCs.emplace_back(CaloHitMC(std::move(edeps),hit.crystalID()));

The old constructor accepted std::move(edeps) into a const std::vector<CaloEDepMC>& parameter, which means the move is silently ignored — the vector is always copied. The new constructor has the same problem. The parameter should be std::vector<CaloEDepMC>&& edeps (or accept by value) to actually benefit from std::move.

3. Merge conflict — PR is not mergeable

The PR shows mergeable: false and mergeable_state: "dirty". This needs to be rebased or have conflicts resolved against main before it can be merged.

🟡 Performance Issues

4. `isCaphri` lookup inside a loop (already flagged by @michaelmackenzie)

for (int i=0; i<nROs; ++i)
{
    bool isCaphri = std::find(CaloConst::_caphriId.begin(),CaloConst::_caphriId.end(),
                              crystalID) != CaloConst::_caphriId.end();
    float pePerMeV = isCaphri ? pePerMeVLyso_ : pePerMeV_;

The isCaphri determination and pePerMeV selection depend only on crystalID, which does not change across loop iterations (it's per-crystal, not per-RO). These should be hoisted before the for loop. This is a linear search (std::find on a vector) repeated nROs times unnecessarily.

5. Same pattern in `CaloDigiMaker_module.cc` — linear search could use `std::set` or sorted + `binary_search`

bool isCaphri = std::find(CaloConst::_caphriId.begin(),CaloConst::_caphriId.end(),
                          crystalID) != CaloConst::_caphriId.end();

If CaloConst::_caphriId is not tiny, using std::find (O(n) linear scan) for every hit is suboptimal. Consider using a std::unordered_set or ensuring the array is sorted and using std::binary_search.

🟡 Design / Consistency Issues

6. Type inconsistency: `double` in `CaloDigiMaker` vs `float` in `CaloShowerROMaker`

fhicl::Atom<double> pePerMeVLyso  { Name("readoutPEPerMeVLyso"), ... };
double pePerMeVLyso_;

fhicl::Atom<float> pePerMeVLyso  { Name("readoutPEPerMeVLyso"), ... };
float pePerMeVLyso_;

CaloDigiMaker uses double while CaloShowerROMaker uses float for the same conceptual parameter. This is inconsistent and could lead to subtle precision differences. (This pre-existing inconsistency for pePerMeV_ is carried over to the new LYSO parameter.)

7. `auto scaleFactor(...)` uses "most vexing parse"-adjacent syntax

auto scaleFactor(MeVToADC_/pePerMeV);

While this compiles, auto scaleFactor = MeVToADC_ / pePerMeV; is clearer and more idiomatic modern C++. The parenthesized initialization with auto can confuse readers who might think it's a function declaration.

8. Naming inconsistency: `crystalId_` (member) vs `crystalID()` (accessor)

int crystalID() const { return crystalId_; }  // accessor: "ID" (uppercase)
int crystalId_;                                 // member: "Id" (mixed case)

The member uses crystalId_ but the public accessor is crystalID(). While minor, consistency (e.g., crystalID_ to match the accessor) would improve readability.

🟢 Bugfix Acknowledgment

9. Good fix in `CaloHitTruthMatch_module.cc`

// Old (buggy): put caloHitMCTruth twice, caloShowerMCTruth never
-  event.put(std::move(caloHitMCTruth));
   event.put(std::move(caloHitMCs));
-  if (fillDetailedMC_) event.put(std::move(caloHitMCTruth));
// New (fixed):
   event.put(std::move(caloHitMCs));
+  event.put(std::move(caloHitMCTruth));
+  if (fillDetailedMC_) event.put(std::move(caloShowerMCTruth));

This is a genuine bug fix — the old code called event.put(std::move(caloHitMCTruth)) twice (the second call would be on an already-moved-from object, causing undefined behavior) and never put caloShowerMCTruth. Good catch.

Summary

Severity	Issue	File
🔴 Bug	Breaking constructor change — check all `CaloHitMC` call sites	`CaloHitMC.hh`
🔴 Bug	`std::move` into `const&` param is a no-op (copy instead of move)	`CaloHitMC.hh`
🔴 Merge	PR has merge conflicts with `main`	—
🟡 Perf	`isCaphri` lookup should be hoisted out of loop	`CaloShowerROMaker_module.cc`
🟡 Perf	Linear search for CaPHRI check on every hit	`CaloDigiMaker_module.cc`
🟡 Design	`double` vs `float` type inconsistency for same parameter	Multiple
🟡 Style	`auto var(expr)` vs `auto var = expr`	`CaloDigiMaker_module.cc`
🟡 Style	`crystalId_` vs `crystalID()` naming mismatch	`CaloHitMC.hh`
🟢 Fix	Correctly puts `caloShowerMCTruth` instead of double-moving `caloHitMCTruth`	`CaloHitTruthMatch_module.cc`

FNALbuild · 2026-03-07T15:58:51Z

📝 The HEAD of main has changed to dc50b36. Tests are now out of date.

giro94 · 2026-03-07T19:18:31Z

CaloMC/src/CaloDigiMaker_module.cc

+      int crystalID = CaloSiPMId(iRO).crystal().id();
+      bool isCaphri = std::find(CaloConst::_caphriId.begin(),CaloConst::_caphriId.end(),
+                                crystalID) != CaloConst::_caphriId.end();


You can (should?) use CaloSiPMId(iR0).crystal().isCaphri()

Oh, didn't know about this one, will modify

giro94 · 2026-03-07T19:22:39Z

CaloMC/src/CaloShowerROMaker_module.cc

+                  bool isCaphri = std::find(CaloConst::_caphriId.begin(),CaloConst::_caphriId.end(),
+                                            crystalID) != CaloConst::_caphriId.end();
+                  float pePerMeV = isCaphri ? pePerMeVLyso_ : pePerMeV_;


Also, same comment as above

giro94 · 2026-03-07T19:24:38Z

CaloMC/src/CaloDigiMaker_module.cc

+      bool isCaphri = std::find(CaloConst::_caphriId.begin(),CaloConst::_caphriId.end(),
+                                crystalID) != CaloConst::_caphriId.end();
+      auto pePerMeV = isCaphri ? pePerMeVLyso_ : pePerMeV_;
+      auto scaleFactor(MeVToADC_/pePerMeV);


I think this could be prone to future mistakes by swapping pePerMeV with pePerMeV_.
Perhaps better to swap pePerMeV_ with pePerMeVCsI_?
No strong opinion however.

Changed, I really start to hate these Caphri crystals..... We will adopt a better solution when we port these values into the database too.

P.S: I think we use the same SiPMs for the LYSO crystals, so the noise should be identical to that of the CsI crystals and I have left it this way. IF we use different readouts, then I'll make modifications when I refactor the noise generation

bechenard · 2026-03-10T01:54:43Z

@FNALbuild run build test

FNALbuild · 2026-03-10T01:55:10Z

⌛ The following tests have been triggered for bbce729: build (Build queue - API unavailable)

FNALbuild · 2026-03-10T02:08:33Z

☀️ The build tests passed at bbce729.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged `bbce729` at `dc50b36`
build (prof)	✅	Log file. Build time: 04 min 10 sec
ceSimReco	✅	Log file.
g4test_03MT	✅	Log file.
transportOnly	✅	Log file.
POT	✅	Log file.
g4study	✅	Log file.
cosmicSimReco	✅	Log file.
cosmicOffSpill	✅	Log file.
ceSteps	✅	Log file.
ceDigi	✅	Log file.
muDauSteps	✅	Log file.
ceMix	✅	Log file.
rootOverlaps	✅	Log file.
g4surfaceCheck	✅	Log file.
trigger	❓	Log file. Return Code 1.
check_cmake	✅	Log file.
FIXME, TODO	✅	TODO (0) FIXME (0) in 5 files
clang-tidy	➡️	6 errors 115 warnings
whitespace check	✅	no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at bbce729 after being merged into the base branch at dc50b36.

For more information, please check the job page here.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

oksuzian · 2026-03-10T17:14:07Z

I'll review this pull request by examining its details and the code changes.Now let me look at the existing review comments and also examine the CaloSiPMId and CaloConst classes to understand the CAPHRI detection logic better.Here is my review of PR #1756 with the critical issues I've identified:

PR Summary

This PR adds a crystalID field to CaloHitMC, differentiates the light yield (PE/MeV) for CsI vs LYSO (CAPHRI) crystals, and changes several member variable types from double to float in CaloDigiMaker.

Critical Issues

1. 🔴 Missing `std::move` in rvalue-reference constructor (`CaloHitMC.hh`)

CaloHitMC(const std::vector<CaloEDepMC>&& edeps, int crystalId) :
 edeps_(edeps),       // ← BUG: copies instead of moving!
 crystalID_(crystalId)
{};

The rvalue-reference overload accepts const std::vector<CaloEDepMC>&& but then initializes edeps_ by copy because edeps is not std::move'd, and even if it were, const&& cannot be moved from. This constructor should be:

CaloHitMC(std::vector<CaloEDepMC>&& edeps, int crystalId) :
 edeps_(std::move(edeps)),
 crystalID_(crystalId)
{};

The const on the rvalue reference defeats the purpose of having a move constructor entirely—it silently falls back to copying. This is a performance bug and a semantic correctness issue.

2. 🔴 `CaloNoiseSimGenerator` still references the old `readoutPEPerMeV` parameter

fhicl::Atom<double>      pePerMeV       { Name("readoutPEPerMeV"),Comment("Number of pe / MeV for Readout") };

The noise generator config still declares a single readoutPEPerMeV parameter. In CaloMC/fcl/prolog.fcl, the noise generator block (NoiseGenerator) is configured with:

readoutPEPerMeV         : @local::readoutPEPerMeVCsI

While this avoids an immediate runtime crash (the fhicl key name matches), the noise simulation always uses the CsI value and never differentiates for LYSO crystals. The author acknowledges this in a comment, stating the SiPMs are currently assumed identical, but this is a latent correctness issue if that assumption changes.

3. 🟠 Inconsistent CAPHRI detection between the two modules

The two C++ modules use different methods to determine if a crystal is CAPHRI:

File	Method
`CaloDigiMaker_module.cc`	`CaloSiPMId(iRO).crystal().isCaphri()`
`CaloShowerROMaker_module.cc`	`std::find(CaloConst::_caphriId.begin(), ...)` (manual linear search)

While both should give the same result, using two different code paths for the same logic is fragile and error-prone. CaloShowerROMaker should also use the CrystalId::isCaphri() method for consistency. The author was told about isCaphri() in the review comments and updated CaloDigiMaker but forgot to update CaloShowerROMaker.

4. 🟠 `double` → `float` type narrowing is unnecessary and reduces precision

In CaloDigiMaker_module.cc, multiple member variables and fhicl parameters are changed from double to float:

float                   digitizationStart_;
float                   digitizationEnd_;
float                   timeFromProtonsToDRMarker_;
float                   digiSampling_;
float                   startTimeBuffer_;
float                   MeVToADC_;

This reduces precision from ~15 significant digits to ~7. For timing values (ns-scale with sub-ns precision requirements in HEP), and for MeVToADC which is used in division (MeVToADC_/pePerMeV), the loss of precision could introduce subtle simulation artifacts. The CaloShowerROMaker already used float for some of these, but CaloDigiMaker previously used double presumably for a reason. This change is bundled silently with the LYSO feature and may not receive the scrutiny it deserves.

5. 🟡 Order of `event.put()` changed in `CaloHitTruthMatch`

- event.put(std::move(caloHitMCTruth));
  event.put(std::move(caloHitMCs));
+ event.put(std::move(caloHitMCTruth));

The order of event.put() calls was swapped. In the art framework, the order of event.put() for different product types typically doesn't matter, but if any downstream consumer relies on product insertion order for same-type products, this could cause a subtle issue. This should at minimum have a comment explaining why the reorder was needed.

Summary

Severity	Issue	File
🔴 Critical	`const&&` prevents move; silent copy	`CaloHitMC.hh`
🔴 Critical	Noise generator not updated for dual PE/MeV	`CaloNoiseSimGenerator.hh`
🟠 Medium	Inconsistent CAPHRI detection methods	`CaloShowerROMaker_module.cc`
🟠 Medium	Unnecessary `double` → `float` narrowing	`CaloDigiMaker_module.cc`
🟡 Low	`event.put()` order changed without explanation	`CaloHitTruthMatch_module.cc`

bechenard · 2026-03-10T19:08:59Z

@FNALbuild run build test

FNALbuild · 2026-03-10T19:09:23Z

⌛ The following tests have been triggered for 64bab70: build (Build queue - API unavailable)

FNALbuild · 2026-03-10T19:21:48Z

☀️ The build tests passed at 64bab70.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged `64bab70` at `dc50b36`
build (prof)	✅	Log file. Build time: 04 min 06 sec
ceSimReco	✅	Log file.
g4test_03MT	✅	Log file.
transportOnly	✅	Log file.
POT	✅	Log file.
g4study	✅	Log file.
cosmicSimReco	✅	Log file.
cosmicOffSpill	✅	Log file.
ceSteps	✅	Log file.
ceDigi	✅	Log file.
muDauSteps	✅	Log file.
ceMix	✅	Log file.
rootOverlaps	✅	Log file.
g4surfaceCheck	✅	Log file.
trigger	❓	Log file. Return Code 1.
check_cmake	✅	Log file.
FIXME, TODO	✅	TODO (0) FIXME (0) in 5 files
clang-tidy	➡️	6 errors 115 warnings
whitespace check	✅	no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 64bab70 after being merged into the base branch at dc50b36.

For more information, please check the job page here.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

michaelmackenzie · 2026-03-10T20:01:25Z

@bechenard are changes in the trigger test expected here? It looks like there are changes in the calo paths that weren't seen in the initial CI test

bechenard · 2026-03-10T20:33:03Z

Nope, this is weird. Will have a look at it

bechenard · 2026-03-10T20:48:25Z

The trigger ran correctly, but there are small changes in the trigger rates since I changes the light yield of the LYSO crystals. Looks reasonable to me

michaelmackenzie · 2026-03-12T21:50:18Z

@bechenard why would the LYSO light yield change trigger reco? Wouldn't this only change the CAPHRI crystals which (I assume) aren't included in the clustering for the trigger paths that changed?

bechenard · 2026-03-12T21:54:17Z

Aren't the Caphri crystals treated like any other crystals in the clustering?

…

On 3/12/2026 2:50 PM, Michael MacKenzie wrote: *michaelmackenzie* left a comment (Mu2e/Offline#1756) <#1756 (comment)> @bechenard <https://github.com/bechenard> why would the LYSO light yield change trigger reco? Wouldn't this only change the CAPHRI crystals which (I assume) aren't included in the clustering for the trigger paths that changed? — Reply to this email directly, view it on GitHub <#1756 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABJ27JH4I2DFH6WRB7ML7S34QMWLBAVCNFSM6AAAAACWJ6K5W2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DANJQGQYTKOJYGY>. You are receiving this because you were mentioned.Message ID: ***@***.***>

michaelmackenzie · 2026-03-12T22:10:03Z

I assumed they weren't since the CaloHitMakerFast produces a separate hit collection for CAPHRI vs. non-CAPHRI hits:
https://github.com/Mu2e/Offline/blob/main/CaloReco/src/CaloHitMakerFast_module.cc#L163-#L171

giro94 · 2026-03-12T22:43:31Z

They are treated the same in offline, separate in online

michaelmackenzie · 2026-03-13T01:35:05Z

@bechenard could you please check running mu2e-trig-config/ci/check_trigger_results.sh with the original LYSO light yield you restore the trigger results? Then I can open an issue to understand why the LYSO light yield impacts Online reco results...

bechenard requested review from giro94 and michaelmackenzie March 6, 2026 21:23

FNALbuild added the build running label Mar 6, 2026

FNALbuild added build finished and removed build running labels Mar 6, 2026

michaelmackenzie approved these changes Mar 6, 2026

View reviewed changes

FNALbuild added build pending and removed build finished labels Mar 7, 2026

giro94 suggested changes Mar 7, 2026

View reviewed changes

bechenard added 2 commits March 9, 2026 20:19

Resolved conflict in CaloHitTruthMatch

faa7503

Fixed bug and perf improvements

e9b9527

bechenard force-pushed the CaloLysoID branch from fa4d74c to e9b9527 Compare March 10, 2026 01:27

Included Paolo wishes

bbce729

FNALbuild added build running and removed build pending labels Mar 10, 2026

FNALbuild added build finished and removed build running labels Mar 10, 2026

Fixed bug

64bab70

FNALbuild added build pending and removed build finished labels Mar 10, 2026

FNALbuild added build running and removed build pending labels Mar 10, 2026

FNALbuild added build finished and removed build running labels Mar 10, 2026

oksuzian merged commit eda787f into Mu2e:main Mar 12, 2026
14 checks passed

Conversation

bechenard commented Mar 6, 2026

Uh oh!

FNALbuild commented Mar 6, 2026

Uh oh!

FNALbuild commented Mar 6, 2026

Uh oh!

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

oksuzian commented Mar 7, 2026

🔴 Bugs / Correctness Issues

1. Breaking change to CaloHitMC constructor — other callers may be broken

2. CaloHitMC move constructor lost — potential unnecessary copies

3. Merge conflict — PR is not mergeable

🟡 Performance Issues

4. isCaphri lookup inside a loop (already flagged by @michaelmackenzie)

5. Same pattern in CaloDigiMaker_module.cc — linear search could use std::set or sorted + binary_search

🟡 Design / Consistency Issues

6. Type inconsistency: double in CaloDigiMaker vs float in CaloShowerROMaker

7. auto scaleFactor(...) uses "most vexing parse"-adjacent syntax

8. Naming inconsistency: crystalId_ (member) vs crystalID() (accessor)

🟢 Bugfix Acknowledgment

9. Good fix in CaloHitTruthMatch_module.cc

Summary

Uh oh!

FNALbuild commented Mar 7, 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!

bechenard commented Mar 10, 2026

Uh oh!

FNALbuild commented Mar 10, 2026

Uh oh!

FNALbuild commented Mar 10, 2026

Uh oh!

oksuzian commented Mar 10, 2026

PR Summary

Critical Issues

1. 🔴 Missing std::move in rvalue-reference constructor (CaloHitMC.hh)

2. 🔴 CaloNoiseSimGenerator still references the old readoutPEPerMeV parameter

3. 🟠 Inconsistent CAPHRI detection between the two modules

4. 🟠 double → float type narrowing is unnecessary and reduces precision

5. 🟡 Order of event.put() changed in CaloHitTruthMatch

Summary

Uh oh!

bechenard commented Mar 10, 2026

Uh oh!

FNALbuild commented Mar 10, 2026

Uh oh!

FNALbuild commented Mar 10, 2026

Uh oh!

michaelmackenzie commented Mar 10, 2026

Uh oh!

bechenard commented Mar 10, 2026

Uh oh!

bechenard commented Mar 10, 2026

Uh oh!

Uh oh!

michaelmackenzie commented Mar 12, 2026

Uh oh!

bechenard commented Mar 12, 2026 via email

Uh oh!

michaelmackenzie commented Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

1. Breaking change to `CaloHitMC` constructor — other callers may be broken

2. `CaloHitMC` move constructor lost — potential unnecessary copies

4. `isCaphri` lookup inside a loop (already flagged by @michaelmackenzie)

5. Same pattern in `CaloDigiMaker_module.cc` — linear search could use `std::set` or sorted + `binary_search`

6. Type inconsistency: `double` in `CaloDigiMaker` vs `float` in `CaloShowerROMaker`

7. `auto scaleFactor(...)` uses "most vexing parse"-adjacent syntax

8. Naming inconsistency: `crystalId_` (member) vs `crystalID()` (accessor)

9. Good fix in `CaloHitTruthMatch_module.cc`

1. 🔴 Missing `std::move` in rvalue-reference constructor (`CaloHitMC.hh`)

2. 🔴 `CaloNoiseSimGenerator` still references the old `readoutPEPerMeV` parameter

4. 🟠 `double` → `float` type narrowing is unnecessary and reduces precision

5. 🟡 Order of `event.put()` changed in `CaloHitTruthMatch`

michaelmackenzie commented Mar 12, 2026 •

edited

Loading