Calo cond remove

[bechenard]

Address issue #1053 to remove calorimeterConditions text

[FNALbuild]

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

• CaloReco
• Mu2eUtilities
• Analyses
• ConditionsService
• CaloMC

which require these tests: build.

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

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

About FNALbuild. Code review on Mu2e/Offline.

[FNALbuild]

☔ The build tests failed for 5ec3ee4.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged 5ec3ee4 at a500268
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. Return Code 9.
ceSteps	✅	Log file.
ceDigi	❌	Log file. Return Code 9.
muDauSteps	✅	Log file.
ceMix	❌	Log file. Return Code 9.
rootOverlaps	✅	Log file.
g4surfaceCheck	✅	Log file.
trigger	❌	Log file. Return Code 2.
check_cmake	❌	Log file. Return Code 1.
FIXME, TODO	➡️	TODO (0) FIXME (2) in 18 files
clang-tidy	➡️	20 errors 392 warnings
whitespace check	✅	no whitespace errors found

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

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]

@FNALbuild run build test with Mu2e/mu2e-trig-config#140

[FNALbuild]

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

[FNALbuild]

☀️ The build tests passed at 01e646a.

Test	Result	Details
test with	✅	Included Mu2e/mu2e-trig-config#140 @ 525178940726d872b0662f3f1fe8e288723493a5 by merge
merge	✅	Merged 01e646a at a500268
build (prof)	✅	Log file. Build time: 04 min 08 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 2.
check_cmake	✅	Log file.
FIXME, TODO	➡️	TODO (0) FIXME (2) in 18 files
clang-tidy	➡️	20 errors 392 warnings
whitespace check	✅	no whitespace errors found

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

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]

I'll review this pull request by first loading the PR details and its changes.Now let me look at a few more key files to check for remaining references and the CaloPulseShape constructor changes:Now let me check whether the CaloPulseShape constructor was updated to accept the new parameters, and look at CaloHitMakerFast fcl config to see if ADCToMeV is provided there:Now I have a thorough understanding of the PR. Let me check one more critical thing - whether CaloPulseShape.cc was also updated in this PR, and whether the conditions_01.txt data file is being deleted or if the CaloHitMakerFast fcl prolog was updated to include ADCToMeV:Now I have a comprehensive understanding of the PR. Let me compile my findings.

---

PR Review: Calo cond remove

Summary: This PR removes the CalorimeterCalibrations conditions entity from the ConditionsService, replacing the runtime conditions lookup with hardcoded FCL parameters passed directly to modules. It addresses issue #1053. The change spans 32 files across CaloMC, CaloReco, ConditionsService, CaloCluster, Analyses, and Mu2eUtilities.

Note: The API returned up to 30 files; there may be additional changed files. View all files on GitHub.

---

Potential Issues Found

1. 🔴 Copy-paste bug in CaloShowerROMaker — digitizationBuffer_ initialized from wrong config

            digitizationBuffer_  (config().digitizationEnd()),   // BUG: should be digitizationBuffer()

The member digitizationBuffer_ is initialized from config().digitizationEnd() instead of config().digitizationBuffer(). This bug is pre-existing (it exists on main too), but the PR reformatted this line and didn't fix it. Worth noting as a drive-by fix opportunity.

2. 🟡 Stale #include left in CaloPulseShape.cc

#include "Offline/ConditionsService/inc/ConditionsHandle.hh"

The CaloPulseShape.cc file no longer uses ConditionsHandle (the conditions lookup was replaced with ConfigFileLookupPolicy), but it still includes the ConditionsHandle.hh header. This is a dead include that should be removed for clean decoupling.

3. 🟡 BirkCorrHadron and ROnoise / Peak2MeV parameters orphaned

The deleted CalorimeterCalibrations class provided several parameters that appear to have no replacement in this PR:

Old Parameter	Old Config Key	Replacement in PR?
BirkCorrHadron()	"BirkCorrHadron"	❌ Not addressed
ROnoise()	"ReadOutElectronicNoise"	❌ Not addressed
Peak2MeV()	"Peak2MeVConversionFactor"	❌ Not addressed

If these were still in use elsewhere in the codebase (outside the 32 files changed), removing CalorimeterCalibrations would break those consumers. The search suggests these may not be actively used, but this warrants verification.

4. 🟡 conditions_01.txt data file not deleted but its configure_file removed

In ConditionsService/CMakeLists.txt, the configure_file command for conditions_01.txt is removed:

-configure_file(${CMAKE_CURRENT_SOURCE_DIR}/data/conditions_01.txt    ${CURRENT_BINARY_DIR} data/conditions_01.txt)

However, conditions_01.txt is still referenced in many other FCL files across the codebase (e.g., fcl/standardServices.fcl, Mu2eG4/fcl/rantest.fcl, ExtinctionMonitorFNAL/test/digiTuning.fcl, CalPatRec/test/trkPatRec_display_read.fcl, and many more). Those files still have:

ConditionsService : { conditionsfile : "Offline/ConditionsService/data/conditions_01.txt" }

The ConditionsService itself still exists (for non-calorimeter conditions), so this should still work. But the conditions_01.txt file itself is no longer being installed by CMake, which could cause a runtime error if the file can't be found. The actual data file (ConditionsService/data/conditions_01.txt) does not appear to be deleted in this PR, only the install rule is removed.

5. 🟡 Loss of per-SiPM calibration granularity

The old CalorimeterCalibrations accepted a SiPM ID for methods like peMeV(int roId), ADC2MeV(int roId), and MeV2ADC(int roId). While the old implementation returned the same constant regardless of roId, the interface allowed per-channel calibration in the future. The new approach uses a single global constant:

      float scaleFactor(MeVToADC_/pePerMeV_);  // same for all channels

This is an intentional simplification, but if per-channel calibration is needed later, the parameters would need to be changed to arrays/maps.

6. 🟡 Naming inconsistency: MeVToADC_ vs ADCToMeV

The member variable is named MeVToADC_ but the FCL parameter it reads from is named ADCToMeV, and it's computed as 1.0/config().ADCToMeV(). This inverse naming appears in three different modules (CaloDigiMaker, CaloHitTruthMatch, CaloNoiseSimGenerator), which is at least internally consistent, but could be confusing for future maintainers since the variable name suggests it stores MeV→ADC but the config name says the opposite direction.

7. 🟢 CaloHitMakerFast — Missing ADCToMeV in FCL prolog

The CaloHitMakerFast module now requires an ADCToMeV config parameter, but I don't see it being added to the CaloReco prolog for the CaloHitMakerFast configuration. The CaloReco/fcl/prolog.fcl only adds pulseFileName and pulseHistName to the TemplateProcessor. If the CaloHitMakerFast module is configured via a separate prolog (possibly in files beyond the 30 shown), this may already be handled. Otherwise, it would result in a missing parameter error at runtime.

8. 🟢 Pedestal type change: int → double

The pedestal() return type in CaloNoiseSimGenerator changed from int to double, and the computation changed from int(...) to std::floor(...). The buildOutputDigi method in CaloDigiMaker was also updated to accept double pedestal. This is a good improvement for precision, but note that the comparison if (waveform[i] > pedestal) now compares doubles, which is fine since std::floor returns a whole number as a double.

---

Summary of Risk Assessment

Severity	Issue	Action
🔴 High	digitizationBuffer_ copy-paste bug (pre-existing)	Should fix while touching this code
🟡 Medium	Stale ConditionsHandle.hh include in CaloPulseShape.cc	Clean up
🟡 Medium	conditions_01.txt CMake install rule removed but file still referenced	Verify runtime behavior
🟡 Medium	Orphaned parameters (BirkCorrHadron, ROnoise, Peak2MeV)	Verify no other consumers
🟡 Medium	CaloHitMakerFast may be missing ADCToMeV in FCL prolog	Verify complete FCL config
🟡 Low	Loss of per-channel calibration interface	Intentional design choice
🟡 Low	MeVToADC_ vs ADCToMeV naming confusion	Consider renaming for clarity

[FNALbuild]

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

[bechenard]

@FNALbuild run build test

[FNALbuild]

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

[FNALbuild]

☔ The build tests failed for 82d70f5.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged 82d70f5 at 9f2c9c8
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. Return Code 9.
ceSteps	✅	Log file.
ceDigi	❌	Log file. Return Code 9.
muDauSteps	✅	Log file.
ceMix	❌	Log file. Return Code 9.
rootOverlaps	✅	Log file.
g4surfaceCheck	✅	Log file.
trigger	❌	Log file. Return Code 2.
check_cmake	✅	Log file.
FIXME, TODO	➡️	TODO (0) FIXME (2) in 18 files
clang-tidy	➡️	18 errors 392 warnings
whitespace check	✅	no whitespace errors found

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

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.

[rlcee]

The service is now "blank" i.e. no service provided, which is fine, since the there should be no requests for information from the service. But there are still many references to the Service out there, and even in private areas. So when these run, the service will attempt to start and then fail to find the conditions_01.txt file. I think if you restore that file, then it should work. The plan then would be to find and fix the outlying references, and give some warning to users. Once that step is done, we can delete the service itself.

[bechenard]

I can do that, and add a warning that this file will be deleted soon and people should update their scripts. I'll see if I find more services mentioned in the code or production scripts.

[bechenard]

@FNALbuild run build test with Mu2e/mu2e-trig-config#140

[FNALbuild]

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

[bechenard]

@rlcee I have added a dummy conditions_01.txt file, and done a bit more cleaning. I think this covers the removal of the calorimeter conditions, I'll let you deal with the rest unless there is specific for me to do.

[FNALbuild]

☀️ The build tests passed at 90a2cf6.

Test	Result	Details
test with	✅	Included Mu2e/mu2e-trig-config#140 @ fbce3d45da1c14704c7f068373253d25e75a0dc6 by merge
merge	✅	Merged 90a2cf6 at 7a15006
build (prof)	✅	Log file. Build time: 04 min 08 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 (2) in 18 files
clang-tidy	➡️	18 errors 392 warnings
whitespace check	✅	no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 90a2cf6 after being merged into the base branch at 7a15006.

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.

[rlcee]

I pulled, built, and ran it with a ConditionsService in the fcl and throws. Is that the behavior you expect?
---- SimpleConfig BEGIN
SimpleConfigRecord: Not terminated by semicolon: #THIS IS A DEPRECATED FILE, DO NOT EDIT AND REMOVE ITS USE FROM YOUR SCRIPTS
---- SimpleConfig END

It runs if you add "//" to the front of your comment

[rlcee]

I think we can go with this version. Thanks

[oksuzian]

I'll review this pull request by first loading its details and the diff.Let me get the remaining files to see the full scope of changes, especially the C++ source files which are more likely to have code-level issues.Now let me get the remaining files and look specifically at the C++ source code changes, which are the most likely to have issues:Now I have a thorough understanding of this PR. Let me provide a comprehensive review.

PR Review: Calo cond remove (#1722)

Author: @bechenard | Addresses: Issue #1053 | Changed Files: 113 | +181 / -460

Summary

This PR removes the CalorimeterCalibrations class from the ConditionsService and migrates all calorimeter calibration parameters (pulse file names, PE/MeV, ADC conversion factors, crystal non-uniformity, etc.) to be passed directly through FHiCL configuration. It deletes the CalorimeterCalibrations.hh header, CalorimeterCalibrations.cc source, and replaces all ConditionsHandle<CalorimeterCalibrations> usage with direct fhicl parameters. The bulk of the changes (~90 files) are removing ConditionsService references from .fcl test/config files.

---

Issues Found

1. 🔴 Commented-out #include instead of deletion (Code Smell / Incomplete Cleanup)

In Analyses/src/CaloHitFinderInspect_module.cc, the ConditionsHandle.hh include is commented out rather than removed:

-#include "Offline/ConditionsService/inc/ConditionsHandle.hh"
-#include "Offline/ConditionsService/inc/CalorimeterCalibrations.hh"
+//#include "Offline/ConditionsService/inc/ConditionsHandle.hh"

The CalorimeterCalibrations.hh include is properly removed, but ConditionsHandle.hh is only commented out. If this header is no longer needed, it should be deleted entirely rather than left as a comment.

2. 🔴 Bug: digitizationBuffer_ initialized from wrong config value (Pre-existing bug, now partially fixed)

In CaloMC/src/CaloShowerROMaker_module.cc, the original code had:

digitizationBuffer_ (config().digitizationEnd()),  // BUG: should be digitizationBuffer()

The PR fixes this to:

digitizationBuffer_  (config().digitizationBuffer()),

This is a fix embedded in the PR — good catch — but it should be called out in the PR description since this is a behavior change beyond just the refactor.

3. 🟡 MeVToADC default value is an integer, should be a float

In CaloReco/fcl/common.fcl:

MeVToADC                     : 16

While ADCToMeV : 0.0625 is correctly expressed as a float, MeVToADC is specified as an integer 16. This is used in float/double divisions (e.g., MeVToADC_/pePerMeV_). Although FHiCL should handle the conversion, for consistency and clarity it should be 16.0.

4. 🟡 Consistency issue: ADCToMeV is not the mathematical inverse of MeVToADC

• MeVToADC = 16 → means 1 MeV = 16 ADC counts
• ADCToMeV = 0.0625 → means 1 ADC = 0.0625 MeV = 1/16 MeV

These are consistent, but having two separate parameters that are exact inverses of each other creates a maintenance risk. If someone changes one without the other, physics results could silently change. Consider computing one from the other, or adding a comment documenting the relationship.

5. 🟡 Some calibration parameters dropped without replacement

The old CalorimeterCalibrations class provided these parameters that are not migrated to FHiCL:

• BirkCorrHadron (= 4.0) — Birks correction for hadrons
• ReadOutElectronicNoise (= 0.15 MeV) — electronic noise sigma
• Peak2MeVConversionFactor (= 0.022981700) — peak to MeV conversion

If these are used elsewhere in the codebase (e.g., via ConditionsHandle in other packages not modified by this PR), removing the CalorimeterCalibrations entity will cause runtime failures. If they are unused, this is fine, but it should be verified.

6. 🟡 pedestal() return type changed from int to double — potential downstream impact

In CaloNoiseSimGenerator.hh:

-        int                          pedestal() const {return pedestal_;}
+        double                       pedestal() const {return pedestal_;}

And in CaloNoiseSimGenerator.cc, pedestal_ is now computed with std::floor() instead of int(). The buildOutputDigi method signature also changed to accept double pedestal. This is a subtle physics change — std::floor(-0.7) = -1.0 while int(-0.7) = 0 (truncation toward zero). Verify that the pedestal is always non-negative, otherwise behavior differs.

7. 🟡 Removed CalorimeterCalibrations files but kept conditions_01.txt as deprecated stub

The file ConditionsService/data/conditions_01.txt is reduced to a single deprecation comment rather than deleted. Meanwhile, CMakeLists.txt removes the configure_file for it. This means:

• The file exists in source but won't be installed
• Any downstream scripts still referencing it will get confusing behavior (file exists locally but not in build)

Consider either fully deleting the file or keeping the configure_file entry so the deprecation message is discoverable at runtime.

8. 🟡 Erroneous removal in EventGenerator/test/FromStepPointMCs.fcl

-    GlobalConstantsService : { inputFile : "Offline/ConditionsService/data/conditions_01.txt" }

This removes a GlobalConstantsService line, not a ConditionsService line. This was likely a misconfiguration (pointing GlobalConstantsService to conditions_01.txt instead of globalConstants_01.txt), but removing it entirely means this test FCL no longer has GlobalConstantsService configured, which could break it.

9. 🟢 Extra blank lines left after removals

Several .fcl files have double blank lines where the ConditionsService block was removed (e.g., materailsPropStudy.fcl, simParticlesWithHitsExample.fcl, transportMuonStudy.fcl, transportMuonStudy_onlyTransport.fcl). Minor cosmetic issue.

10. 🟢 Typo in error message preserved

In CaloPhotonPropagation.cc, the error message still reads "CaloROStepMaker:: Hitsogram" — "Hitsogram" should be "Histogram". This is a pre-existing typo, not introduced by this PR, but worth noting.

---

Summary of Concerns by Severity

Severity	Count	Description
🔴 High	2	Commented-out include should be deleted; buried bug fix needs documentation
🟡 Medium	5	Integer vs float constant, inverse parameter duplication risk, dropped calibration params, pedestal type change semantics, incorrect GlobalConstantsService removal
🟢 Low	2	Extra blank lines, pre-existing typo

Note: This review covers only the first ~90 of the 113 changed files. You can view the complete file changes on GitHub for the remaining files.

[bechenard]

@FNALbuild run build test with Mu2e/mu2e-trig-config#140

[FNALbuild]

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

[FNALbuild]

☀️ The build tests passed at abf10fa.

Test	Result	Details
test with	✅	Included Mu2e/mu2e-trig-config#140 @ fbce3d45da1c14704c7f068373253d25e75a0dc6 by merge
merge	✅	Merged abf10fa at 2fed5e8
build (prof)	✅	Log file. Build time: 04 min 08 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 (2) in 18 files
clang-tidy	➡️	18 errors 393 warnings
whitespace check	✅	no whitespace errors found

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

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.