Add time calibration to reco digis by giro94 · Pull Request #2030 · Mu2e/Offline

giro94 · 2026-10-08T16:40:06Z

No time calibration was applied to reco digis.
Also, added energy and time calibration via proditions to online/trigger modules

FNALbuild · 2026-10-08T16:40:40Z

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 bec92d2: build (Build queue - API unavailable)

About FNALbuild. Code review on Mu2e/Offline.

oksuzian

PR Review Summary — #2030

Reviewed at head bec92d2048e3178c59e599d1fdf92f43ed1f8c3c. first pass

Decision

🟡 comment only, for now. The code is ready to approve: the per-SiPM time offset from CalCalib is now applied to the reco-digi time, with the sign that the calibration producing the table assumes. No FNALbuild run exists at this head, so I'm triggering one and will approve once it is green.

Scope understood

CaloRecoDigiMaker reads calCalib.timeoffset(SiPMID) and subtracts it from each reco-digi time: time = waveformProcessor_->time(i) - pbtOffset - timeOffset. Until now CalCalib::timeoffset() had no caller in Offline, so the loaded CalTimeCalib values never reached any calo time.
One file, +2/−1, on main after #2029.

Findings

🟡 [S2] The trigger and online hit makers do not apply the offset, and the online one would add it
- Evidence:
  - CaloHitMakerFast (trigger) has no CalCalib at all.
  - DAQ/src/CaloHitsFromDTCEvents_module.cc:329 and :404 add timeCalib_[SiPMID] to the hit time. That array is set to 0 at :131 and never filled.
- Impact: none today, because every table in use is zero (see the 🟢 below). Once real offsets are loaded, offline reco times, trigger times and online times will differ per channel. If timeCalib_ were filled from the same table, it would also have the opposite sign.
- Suggested fix: a follow-up that gives CaloHitMakerFast the same subtraction and either fills timeCalib_ from CalCalib with a minus sign or removes it. Not needed for this PR.
⚪ [S3] Two small items:
- The PR description is empty. One line on intent, plus "offsets are all zero in current tables, so no output change today", would help the next reader.
- The edit drops the comment // correct to time since protons from the time line. Consider keeping it, and documenting the sign next to it, for example // minus per-SiPM offset (CalTimeCalib::tcorr).

🟢 Verified at this head:

Sign. CaloCalibration's cosmic T0 alignment (CosmicsCalib/src/caloT0alig_module.cc:367,404) forms corrected channel times as recoDigi->time() - Tcor[SiPMID], and its last iteration writes Tcor as the table to upload. Subtracting tcorr therefore matches how the constants are derived. The correction is indexed by SiPMID on both sides.
Applied per SiPM, before the two SiPMs are merged. CaloHitMaker merges reco digis by time() within time4Merge (6 ns). So a relative offset between the two SiPMs of a crystal is corrected before that comparison, not after.
No output change with the tables in use today.
- MC and default reco use CalCalib from fcl: useDb: false, timeoffset: 0.0 in CaloConditions/fcl/prolog.fcl.
- The only configurations that set calCalib.useDb : true are PassN Combined_Pass1.fcl and Calo/Calo_CRR_pass1.fcl. Both load CaloConditions/data/TimeCalib_example.txt, which has 2720 rows, all 0.
- So every current reco time is unchanged; the change takes effect when a measured table is loaded.
Compiles. g++ -std=c++20 -fsyntax-only of CaloRecoDigiMaker_module.cc at this head passes. Script: claude-scratch/pr2030_review/syntax.sh.

Validation check

Build/tests run:
- Locally: -fsyntax-only only.
- FNALbuild: no run at this head.
- No events run. With all-zero offsets, ceSimReco and PassN output should be identical to main.
Config contract check: pass. No fcl change; the accessor already existed.
Cross-repo consistency: pass. The table format is unchanged.

Residual risk

Not checked: whether any private or DB-hosted cal.timecalib set with non-zero values is already in use. If so, its reco times shift by tcorr per SiPM with this change, which is the intent.

Author follow-ups

Add a one-line description (finding 2).
Consider the follow-up for the trigger and online hit makers (finding 1).

oksuzian · 2026-10-08T16:46:53Z

@FNALbuild run build test

FNALbuild · 2026-10-08T16:47:20Z

❌ Those tests have already run or are running for bec92d2 (build)

FNALbuild · 2026-10-08T17:01:27Z

☀️ The build tests passed at bec92d2.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged `bec92d2` at `24a1aa5`
build (prof)	✅	Log file. Build time: 09 min 05 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 (0) in 1 files
clang-tidy	➡️	1 errors 12 warnings
whitespace check	✅	no whitespace errors found

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

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 — #2030

Reviewed at head bec92d2048e3178c59e599d1fdf92f43ed1f8c3c. first pass

Decision

🟢 approve. The per-SiPM time offset from CalCalib is now applied to the reco-digi time, with the sign that the calibration producing the table assumes. FNALbuild passes at this head.

Scope understood

CaloRecoDigiMaker reads calCalib.timeoffset(SiPMID) and subtracts it from each reco-digi time: time = waveformProcessor_->time(i) - pbtOffset - timeOffset. Until now CalCalib::timeoffset() had no caller in Offline, so the loaded CalTimeCalib values never reached any calo time.
One file, +2/−1, on main after #2029.

Findings

🟡 [S2] The trigger and online hit makers do not apply the offset, and the online one would add it
- Evidence:
  - CaloHitMakerFast (trigger) has no CalCalib at all.
  - DAQ/src/CaloHitsFromDTCEvents_module.cc:329 and :404 add timeCalib_[SiPMID] to the hit time. That array is set to 0 at :131 and never filled.
- Impact: none today, because every table in use is zero (see the 🟢 below). Once real offsets are loaded, offline reco times, trigger times and online times will differ per channel. If timeCalib_ were filled from the same table, it would also have the opposite sign.
- Suggested fix: a follow-up that gives CaloHitMakerFast the same subtraction and either fills timeCalib_ from CalCalib with a minus sign or removes it. Not needed for this PR.
⚪ [S3] Two small items:
- The PR description is empty. One line on intent, plus "offsets are all zero in current tables, so no output change today", would help the next reader.
- The edit drops the comment // correct to time since protons from the time line. Consider keeping it, and documenting the sign next to it, for example // minus per-SiPM offset (CalTimeCalib::tcorr).

🟢 Verified at this head:

Sign. CaloCalibration's cosmic T0 alignment (CosmicsCalib/src/caloT0alig_module.cc:367,404) forms corrected channel times as recoDigi->time() - Tcor[SiPMID], and its last iteration writes Tcor as the table to upload. Subtracting tcorr therefore matches how the constants are derived. The correction is indexed by SiPMID on both sides.
Applied per SiPM, before the two SiPMs are merged. CaloHitMaker merges reco digis by time() within time4Merge (6 ns). So a relative offset between the two SiPMs of a crystal is corrected before that comparison, not after.
No output change with the tables in use today.
- MC and default reco use CalCalib from fcl: useDb: false, timeoffset: 0.0 in CaloConditions/fcl/prolog.fcl.
- The only configurations that set calCalib.useDb : true are PassN Combined_Pass1.fcl and Calo/Calo_CRR_pass1.fcl. Both load CaloConditions/data/TimeCalib_example.txt, which has 2720 rows, all 0.
- So every current reco time is unchanged; the change takes effect when a measured table is loaded.
Compiles. g++ -std=c++20 -fsyntax-only of CaloRecoDigiMaker_module.cc at this head passes. Script: claude-scratch/pr2030_review/syntax.sh.

Validation check

Build/tests run:
- Locally: -fsyntax-only only.
- FNALbuild at this head: passed. trigger fails with return code 2, as it has on #2021, #2026 and #2029 since before this change, so it does not come from this PR.
- No events run. With all-zero offsets, ceSimReco and PassN output should be identical to main.
Config contract check: pass. No fcl change; the accessor already existed.
Cross-repo consistency: pass. The table format is unchanged.

Residual risk

Not checked: whether any private or DB-hosted cal.timecalib set with non-zero values is already in use. If so, its reco times shift by tcorr per SiPM with this change, which is the intent.

Author follow-ups

Add a one-line description (finding 2).
Consider the follow-up for the trigger and online hit makers (finding 1).

…e calibs

… calibs

giro94 · 2026-10-13T16:44:16Z

@FNALbuild run build test

FNALbuild · 2026-10-13T16:44:46Z

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

giro94 · 2026-10-13T16:47:52Z

@FNALbuild run build test

FNALbuild · 2026-10-13T16:48:24Z

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

FNALbuild · 2026-10-13T16:57:24Z

☔ The build tests failed for 80044dc.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged `80044dc` at `24a1aa5`
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 (0) FIXME (1) in 3 files
clang-tidy	➡️	3 errors 81 warnings
whitespace check	✅	no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 80044dc after being merged into the base branch at 24a1aa5.

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.

FNALbuild · 2026-10-13T17:09:54Z

☀️ The build tests passed at 5c0bc8e.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged `5c0bc8e` at `24a1aa5`
build (prof)	✅	Log file. Build time: 11 min 18 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 (1) in 3 files
clang-tidy	➡️	3 errors 81 warnings
whitespace check	✅	no whitespace errors found

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

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 · 2026-10-13T17:28:09Z

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

FNALbuild · 2026-10-13T17:28:36Z

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

FNALbuild · 2026-10-13T17:41:39Z

☀️ The build tests passed at 80044dc.

Test	Result	Details
test with	✅	Included Mu2e/mu2e-trig-config#174 @ 996a8890cbb328be8b9e3b07ae5941e06a8e51cc by merge
merge	✅	Merged `80044dc` at `24a1aa5`
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.
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 (1) in 3 files
clang-tidy	➡️	3 errors 81 warnings
whitespace check	✅	no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 80044dc after being merged into the base branch at 24a1aa5.

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 · 2026-10-13T17:52:25Z

@bechenard @rlcee just letting you know I have expanded the use of proditions

@oksuzian this is ready to be merged

bechenard · 2026-10-13T18:27:15Z

        const ProtonBunchTime& pbt(*pbtH);
        double pbtOffset = pbt.pbtime_;
+        CalCalib const& calCalib = calCalibHandle_.get(event.id());



AArgh, the better style is obviously const CalCalib&....

Totally agree, I copied the style somewhere...
Will fix once for all

grep "const&" */*/*.cc | wc -l 2825

No I will not

michaelmackenzie · 2026-10-13T20:55:10Z

I merged this to restore the trigger and Offline CI functionality

giro94 and others added 2 commits October 8, 2026 11:39

Add time calibration to reco digis

bc5f1cc

Merge branch 'Mu2e:main' into dev

bec92d2

giro94 requested a review from bechenard October 8, 2026 16:40

FNALbuild added the build running label Oct 8, 2026

giro94 requested a review from simonagiov October 8, 2026 16:41

oksuzian reviewed Oct 8, 2026

View reviewed changes

FNALbuild added build finished and removed build running labels Oct 8, 2026

oksuzian approved these changes Oct 8, 2026

View reviewed changes

simonagiov approved these changes Oct 8, 2026

View reviewed changes

bechenard approved these changes Oct 9, 2026

View reviewed changes

oksuzian mentioned this pull request Oct 10, 2026

Geomfixafterai #2031

Merged

HitsFromDTCEvents now uses prodition, and applies both energy and tim…

0728f73

…e calibs

FNALbuild added build pending and removed build finished labels Oct 13, 2026

giro94 added 5 commits October 13, 2026 11:30

CaloHitMakerFast now uses prodition, and applies both energy and time…

0f1d931

… calibs

remove obsolete variables

4a2217c

bugfix

fbf726a

Forgot to forward calCalib inside function

e4d763e

removed unused beginRun

5c0bc8e

FNALbuild added build running and removed build pending labels Oct 13, 2026

Removed adc2mev fcl parameter

80044dc

FNALbuild removed the build running label Oct 13, 2026

FNALbuild added the build pending label Oct 13, 2026

FNALbuild added build running and removed build pending labels Oct 13, 2026

FNALbuild added build failed and removed build running labels Oct 13, 2026

giro94 mentioned this pull request Oct 13, 2026

Removed now unused ADCToMeV Mu2e/mu2e-trig-config#174

Merged

FNALbuild added build pending and removed build failed labels Oct 13, 2026

FNALbuild added build running and removed build pending labels Oct 13, 2026

FNALbuild added build finished and removed build running labels Oct 13, 2026

bechenard approved these changes Oct 13, 2026

View reviewed changes

michaelmackenzie merged commit fd9bfbd into Mu2e:main Oct 13, 2026
14 checks passed

rlcee mentioned this pull request Oct 14, 2026

fix missing cmake link #2037

Merged

oksuzian mentioned this pull request Oct 19, 2026

add caltimecalib #2049

Merged

Conversation

giro94 commented Oct 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FNALbuild commented Oct 8, 2026

Uh oh!

oksuzian left a comment

Choose a reason for hiding this comment

PR Review Summary — #2030

Uh oh!

oksuzian commented Oct 8, 2026

Uh oh!

FNALbuild commented Oct 8, 2026

Uh oh!

FNALbuild commented Oct 8, 2026

Uh oh!

oksuzian left a comment

Choose a reason for hiding this comment

PR Review Summary — #2030

Uh oh!

giro94 commented Oct 13, 2026

Uh oh!

FNALbuild commented Oct 13, 2026

Uh oh!

giro94 commented Oct 13, 2026

Uh oh!

FNALbuild commented Oct 13, 2026

Uh oh!

FNALbuild commented Oct 13, 2026

Uh oh!

FNALbuild commented Oct 13, 2026

Uh oh!

giro94 commented Oct 13, 2026

Uh oh!

FNALbuild commented Oct 13, 2026

Uh oh!

FNALbuild commented Oct 13, 2026

Uh oh!

giro94 commented Oct 13, 2026

Uh oh!

bechenard Oct 13, 2026

Choose a reason for hiding this comment

Uh oh!

giro94 Oct 13, 2026

Choose a reason for hiding this comment

Uh oh!

giro94 Oct 13, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

michaelmackenzie commented Oct 13, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

giro94 commented Oct 8, 2026 •

edited

Loading