[giro94]

This addresses #2028

[FNALbuild]

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

• CaloReco

which require these tests: build.

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

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

About FNALbuild. Code review on Mu2e/Offline.

[FNALbuild]

☔ The build is failing at 9d02be4.

scons: *** [build/al9-prof-e29-p108/Offline/tmp/CaloReco/src/CaloHitMakerFast_module.os] Error 1
scons: *** [build/al9-prof-e29-p108/Offline/tmp/CaloReco/src/CaloRecoDigiMaker_module.os] Error 1

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged 9d02be4 at 395c773
build (prof)	❌	Log file.
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 (1) in 2 files
clang-tidy	➡️	4 errors 36 warnings
whitespace check	✅	no whitespace errors found

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

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]

PR Review Summary — #2029

Reviewed at head 6797c6284c3a96621ef3994e9b58cce2c61f3e68. first pass

Decision

• 🔴 request changes: findings 1–3 break the build and the CaloReco prolog even once #2022 is in. That #2022 must merge first is a merge-order note, not a finding.

Merge order

• CaloConst::_digitizationPeriod is added by #2022, not by this PR, and is not on main (395c773e9). On main, all 10 changed .cc files fail with '_digitizationPeriod' is not a member of 'mu2e::CaloConst', which is why FNALbuild failed at 9d02be4ab. Please rebase onto #2022 (or merge after it) and say so in the description.
• Merge together with mu2e-trig-config#173, CaloCalibration#51 and Tutorial#40. This PR removes required digiSampling keys, so any config that still sets them fails validation.

Scope understood

• Implements #2028: every reader of the calo sampling period uses CaloConst::_digitizationPeriod instead of a fhicl parameter.
• 14 files, +37/−62: 10 modules and utilities, plus HitMakerDigiSampling and its keys removed from CaloReco/fcl.

Findings

1. 🔴 [S0] CaloDigiMaker still reads the removed config member

   • Evidence: CaloMC/src/CaloDigiMaker_module.cc:76 has startTimeBuffer_(config().pulseCache().digiSampling()*config().bufferDigi()), but this PR deletes digiSampling from CaloPulseUtil::Config (Mu2eUtilities/inc/CaloPulseUtil.hh). g++ -std=c++20 -fsyntax-only at the head, with #2022's CaloConst.hh supplied, fails with 'const struct mu2e::CaloPulseUtil::Config' has no member named 'digiSampling'.
   • Impact: the build fails, even after #2022.
   • Suggested fix: startTimeBuffer_(CaloConst::_digitizationPeriod*config().bufferDigi()).

2. 🔴 [S0] CaloConst is not in scope in two modules

   • Evidence:

     • Blinding/src/DisplaceDigiTimes_module.cc:202 uses CaloConst::_digitizationPeriod, but the file includes no CaloConst.hh. Error: 'CaloConst' has not been declared.
     • DAQ/src/CaloHitsFromDTCEvents_module.cc:332 and :407: the module is in namespace art, so the unqualified CaloConst does not resolve. Same error.

     Both errors remain with #2022's CaloConst.hh supplied.

   • Impact: the build fails, even after #2022.

   • Suggested fix: add #include "Offline/DataProducts/inc/CaloConst.hh" to DisplaceDigiTimes, and write mu2e::CaloConst::_digitizationPeriod in CaloHitsFromDTCEvents.

3. 🔴 [S0] CaloReco/fcl/prolog.fcl still references the deleted key

   • Evidence: CaloReco/fcl/prolog.fcl:45 has digiSampling : @local::HitMakerDigiSampling in CaloReco.CaloRecoDigiMaker, but common.fcl no longer defines HitMakerDigiSampling. fhicl-dump at the head fails with Can't find key HitMakerDigiSampling at prolog.fcl:48 (the prolog alone, and Production/Validation/ceSimReco.fcl).
   • Impact: every job that includes the CaloReco prolog fails to parse, which covers all of reco and the nightly validation. If the key resolved, validation would still reject it, since CaloRecoDigiMaker no longer declares digiSampling.
   • Suggested fix: delete line 45.

4. 🟡 [S2] Conflicts with #2022 in four files, and a conflict resolved the wrong way would undo #2022's fix

   • Evidence: a trial merge of this head into #2022 (9b561be1d) conflicts in CaloHitMakerFast, CaloHitsFromDTCEvents, CaloTrigger and CaloDigiMaker. In each, this PR keeps main's convention that CaloDigi::t0 is in ns, for example caloDigi.t0() + caloDigi.peakpos()*P and int(t0/P). #2022 changes t0 to ticks, for example (caloDigi.t0() + caloDigi.peakpos())*P and int(t0). CaloRecoDigiMaker auto-merges into a mixed state that is correct but hard to read: t0 * digiSampling_ comes from #2022 and (i+0.5)*CaloConst::... from this PR.
   • Impact: taking this PR's side of any of the four conflicts silently reintroduces the 5× data calo-time error that #2022 fixes.
   • Suggested fix: rebase onto #2022 and keep its ticks expressions, replacing only digiSampling_ with the constant.

5. 🟡 [S2] One out-of-repo config not covered by a companion

   • Evidence: otsdaq-mu2e-calorimeter test/generateCaloDigiFromFragment.fcl:65 sets digiSampling : 5 for CaloHitsFromDTCEvents. Once this PR merges, that key fails validation. There is no open PR for it. The other configs are covered: mu2e-trig-config#173, CaloCalibration#51 and Tutorial#40.
   • Suggested fix: a one-line companion there, or a note to the otsdaq maintainers.

🟢 Verified:

• On main, every arithmetic change replaces a value that was 5, so behaviour is unchanged.
• No other digiSampling, HitMakerDigiSampling or CaloADCSamplePeriod references remain in Offline at the head, except the two in findings 1 and 3.
• CaloPulseUtil(fileName, histName, digiSampling) has no caller, so dropping its argument is safe.

Validation check

• Build/tests run:

  • g++ -std=c++20 -fsyntax-only on all 10 changed .cc files, twice: at the head, and at the head with #2022's CaloConst.hh. Results: 10/10 fail, then 3/10 fail.
  • fhicl-dump of the CaloReco prolog and ceSimReco: both fail.
  • fhicl-dump of the trigger menu with trig-config#173: passes.

  Scripts: claude-scratch/pr2029_review/syntax.sh and fcldump.sh. No full build, and no events run.

• Config contract check: fail (finding 3).

• Cross-repo consistency: needs follow-up (merge order; finding 5).

Residual risk

• There is no transition: every config that sets digiSampling must change in the same merge. That fails loudly, which is acceptable.
• Not checked: whether any user or grid fcl outside the Mu2e organization sets these keys.

Author follow-ups

1. Fix findings 1–3, then rebase onto #2022 and keep its ticks conventions in the four conflicting files (finding 4).
2. In the description, state that this depends on #2022 and lists its companions: trig-config#173, CaloCalibration#51, Tutorial#40.
3. Once it builds, run @FNALbuild run build test. On this change, ceSimReco and the trigger menu should give identical calo output.

[giro94]

Sorry forgot to say this depends on #2022

[oksuzian]

PR Review Summary — #2029

Reviewed at head f629a7e92e97b1f0a9f499caaecb3b8ab0703e91. re-review of 6797c628

Decision

• 🔴 request changes: one build error from the first pass is still there (finding 1). The other two blockers are fixed.

Merge order

• This depends on #2022 for CaloConst::_digitizationPeriod; thanks for confirming in the comment. Please also put that in the description.
• Merge together with mu2e-trig-config#173, CaloCalibration#51 and Tutorial#40.

Findings

1. 🔴 [S0] CaloConst is not in scope in CaloHitsFromDTCEvents (carried over)

   • Evidence: DAQ/src/CaloHitsFromDTCEvents_module.cc:332 and :407 use the unqualified CaloConst::_digitizationPeriod inside namespace art. At this head, with #2022's CaloConst.hh supplied, g++ -std=c++20 -fsyntax-only still fails: 'CaloConst' has not been declared.
   • Impact: the build fails even once #2022 is in.
   • Suggested fix: write mu2e::CaloConst::_digitizationPeriod at both lines.

2. 🟡 [S2] Four files conflict with #2022, and this PR's side of each would undo #2022's fix (carried over)

   • Evidence: a trial merge (git merge-tree) of this head into #2022 (9b561be1d) still conflicts in CaloHitMakerFast, CaloHitsFromDTCEvents, CaloTrigger and CaloDigiMaker. In each file this PR keeps main's ns convention for CaloDigi::t0, for example caloDigi.t0() + caloDigi.peakpos()*P and int(t0/P). #2022 has t0 in ticks: (caloDigi.t0() + caloDigi.peakpos())*P and int(t0).
   • Suggested fix: rebase onto #2022, keep its ticks expressions, and replace only digiSampling_ with the constant.

3. 🟡 [S2] otsdaq-mu2e-calorimeter test/generateCaloDigiFromFragment.fcl:65 still sets digiSampling (carried over). It has no companion PR, and it will fail validation once this merges. A one-line companion, or a note to that repository's maintainers, would cover it.

🟢 Fixed in f629a7e9, verified:

• [was S0] CaloDigiMaker_module.cc:76 now uses CaloConst::_digitizationPeriod*config().bufferDigi(). It compiles with #2022's CaloConst.hh.
• [was S0, part] DisplaceDigiTimes now includes CaloConst.hh and compiles. The CaloHitsFromDTCEvents half of that finding is still open (finding 1).
• [was S0] The dangling digiSampling : @local::HitMakerDigiSampling is gone from CaloReco/fcl/prolog.fcl. fhicl-dump of the CaloReco prolog, Production/Validation/ceSimReco.fcl and the trigger menu with trig-config#173 now all pass, with no digiSampling left in any of the three.
• No digiSampling, HitMakerDigiSampling or CaloADCSamplePeriod remains in Offline .cc, .hh or .fcl at this head.

Validation check

• Build/tests run: g++ -std=c++20 -fsyntax-only on the 10 changed .cc files, with #2022's CaloConst.hh supplied. 9 of 10 pass; CaloHitsFromDTCEvents fails. fhicl-dump as listed above. No full build, and no events run. FNALbuild has not run at this head; it cannot pass until the PR is on top of #2022.
• Config contract check: pass.
• Cross-repo consistency: needs follow-up (merge order; finding 3).

Author follow-ups

1. Qualify CaloConst in CaloHitsFromDTCEvents (finding 1).
2. Rebase onto #2022, keeping its ticks expressions in the four files (finding 2), and note the dependency in the description.
3. Then run @FNALbuild run build test. With the period equal to 5 everywhere, ceSimReco and the trigger menu should give identical calo output.

[giro94]

@FNALbuild run build test

[FNALbuild]

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

[giro94]

@oksuzian you're too fast :D

[FNALbuild]

☔ The build is failing at 2ff8c17.

scons: *** [build/al9-prof-e29-p108/Offline/tmp/DAQ/src/CaloHitsFromDTCEvents_module.os] Error 1

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged 2ff8c17 at 1108542
build (prof)	❌	Log file.
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 (1) FIXME (1) in 12 files
clang-tidy	➡️	11 errors 268 warnings
whitespace check	✅	no whitespace errors found

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

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.

[giro94]

@FNALbuild run build test

[FNALbuild]

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

[FNALbuild]

☔ The build tests failed for f3ad644.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged f3ad644 at e602d59
build (prof)	✅	Log file. Build time: 04 min 27 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 (1) FIXME (1) in 12 files
clang-tidy	➡️	9 errors 268 warnings
whitespace check	✅	no whitespace errors found

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

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.

[giro94]

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

[FNALbuild]

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

[giro94]

The build fails as Mu2e/mu2e-trig-config#173 is needed

[oksuzian]

@oksuzian you're too fast :D

That's not me :)

[FNALbuild]

☀️ The build tests passed at f3ad644.

Test	Result	Details
test with	✅	Included Mu2e/mu2e-trig-config#173 @ 085bc67f39c04ec01cc33d4b2de46279e4252809 by merge
merge	✅	Merged f3ad644 at e602d59
build (prof)	✅	Log file. Build time: 09 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 (1) FIXME (1) in 12 files
clang-tidy	➡️	9 errors 268 warnings
whitespace check	✅	no whitespace errors found

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

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]

PR Review Summary — #2029

Reviewed at head f3ad644dda4f8d8ac033bdf7c0cd9149ff66fb8d. re-review of f629a7e9

Decision

• 🟢 approve. Every finding from the earlier passes is fixed, and the build with mu2e-trig-config#173 passes at this head.

Merge order

• mu2e-trig-config#173 has now merged (16:27Z). Merge this one soon after, because trig-config main no longer sets digiSampling. The remaining companions are CaloCalibration#51, Tutorial#40 and otsdaq-mu2e-calorimeter#88.

Findings

• None open.

🟢 Fixed since the previous passes, verified at this head:

• [was S0] CaloHitsFromDTCEvents scope. Fixed in f3ad644d: :329 and :404 now use mu2e::CaloConst::_digitizationPeriod. That commit otherwise only reorders includes and removes blank lines. The earlier FNALbuild failure at 2ff8c17d was exactly this error.
• [was S2] Conflict with #2022, and the ticks expressions survive. The branch is now on top of main (1108542a1). I read the three-dot diff: every change replaces the sampling period and nothing else, and each t0 expression keeps #2022's ticks form:
  • CaloHitMakerFast: (caloDigi.t0() + caloDigi.peakpos())*P;
  • CaloHitsFromDTCEvents: (thisHitPacket.Time + peakIndex)*P;
  • CaloRecoDigiMaker: caloDigi.t0() * P;
  • CaloDigiMaker: t0 = int(sampleStart) + std::lround(.../P);
  • CaloTrigger:166 keeps #2022's int(t0) untouched.
• [was S2] otsdaq-mu2e-calorimeter companion. Mu2e/otsdaq-mu2e-calorimeter#88 removes digiSampling from both of its test fcl files.
• [was S0, first pass] Already fixed at f629a7e9, still fixed at this head:
  • CaloDigiMaker no longer reads pulseCache().digiSampling();
  • DisplaceDigiTimes includes CaloConst.hh;
  • the dangling HitMakerDigiSampling reference is gone from CaloReco/fcl/prolog.fcl.
• Compiles and parses. All 10 changed .cc files pass -std=c++20 -fsyntax-only. fhicl-dump passes for the CaloReco prolog, Production/Validation/ceSimReco.fcl, and the trigger menu with mu2e-trig-config#173. None of the three contains digiSampling.
• Arithmetic is unchanged. In CaloDigiMaker, startTimeBuffer_ is still the float product 5 × bufferDigi (80). Replacing the float digiSampling_ member with the double constant leaves every quotient and product exact for the values involved (450, 200, 80, 5). Calo output should be bit-identical.

Validation check

• Build/tests run:
  • FNALbuild at this head, with mu2e-trig-config#173 (16:28Z): build, ceSimReco, ceDigi, ceMix, cosmicOffSpill, g4surfaceCheck, rootOverlaps and the rest pass.
  • trigger fails with return code 2. That job also fails on #2026 and #2021, both built on main before this change, so it does not come from this PR.
  • The run without #173 (15:58Z) failed ceDigi, ceMix, cosmicOffSpill and trigger. That is expected, since trig-config main still set digiSampling at the time.
  • Locally: -fsyntax-only and fhicl-dump, as above. No full build, and no events run.
• Config contract check: pass.
• Cross-repo consistency: pass, with the merge order above.

Residual risk

• Not checked: user or grid fcl outside the Mu2e organization that sets digiSampling. Any such config fails loudly at validation rather than silently.

Author follow-ups

• None.