Updated for CRV reco and calibration

[ehrlich-uva]

• Bug fix for CRV reco (time offsets were not applied)
• Added CRV time offsets to EventDisplay
• Added new CRV calibration DB txt files (temporarily until we can use the real DB)
• Small adjustments for CRV calibration parameters
• Printing out volume index if volume name is not available for a SimParticle in the EventDisplay

[FNALbuild]

Hi @ehrlich-uva,
You have proposed changes to files in these packages:

• CRVResponse
• CRVConditions
• CRVReco

which require these tests: build.

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

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

About FNALbuild. Code review on Mu2e/Offline.

[FNALbuild]

☀️ The build tests passed at bb4f097.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged bb4f097 at 3e5e0e6
build (prof)	✅	Log file. Build time: 04 min 09 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 3 files
clang-tidy	➡️	6 errors 30 warnings
whitespace check	✅	no whitespace errors found

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

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]

@FNALbuild run build test

[FNALbuild]

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

[FNALbuild]

☀️ The build tests passed at cf93b85.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged cf93b85 at 3e5e0e6
build (prof)	✅	Log file. Build time: 08 min 58 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 (2) FIXME (0) in 4 files
clang-tidy	➡️	8 errors 202 warnings
whitespace check	✅	no whitespace errors found

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

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]

@FNALbuild run build test

[FNALbuild]

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

[FNALbuild]

☀️ The build tests passed at 574ea7d.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged 574ea7d at 3e5e0e6
build (prof)	✅	Log file. Build time: 04 min 09 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 (2) FIXME (0) in 4 files
clang-tidy	➡️	8 errors 202 warnings
whitespace check	✅	no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 574ea7d after being merged into the base branch at 3e5e0e6.

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 relevant ability and then examining the PR details and changes.Now let me look more closely at the key bug fix and the EventDisplay changes to understand the full context:Here's my review of PR #1743 — "Updated for CRV reco and calibration" by @ehrlich-uva.

Summary

This PR addresses several CRV (Cosmic Ray Veto) changes:

1. Bug fix: time offsets were not being applied in CRV reco due to a variable shadowing issue
2. Adds CRV time offsets to EventDisplay waveforms
3. Updates calibration DB text files (20260214 → 20260221)
4. Adjusts calibration parameters
5. Removes timeOffsetCutoffLow/timeOffsetCutoffHigh parameters
6. Prints volume index when volume name is unavailable for SimParticle in EventDisplay

---

Issues Found

🔴 Issue 1: Critical Bug Fix — Variable Shadowing (Good catch, but verify intent)

The most important change in this PR is the fix in CrvRecoPulsesFinder_module.cc. The old code had a variable shadowing bug:

// OLD (buggy):
double timeOffset = 0.0;
if(_useTimeOffsetDB)
{
  double timeOffset = calib.timeOffset(channel);  // ← SHADOWS outer variable!
  timeOffset*=_timeOffsetScale;
}
// timeOffset is always 0.0 here because inner `double timeOffset` went out of scope

// NEW (fixed):
double timeOffset = 0.0;
if(_useTimeOffsetDB)
{
  timeOffset = calib.timeOffset(channel);  // ← Correctly assigns to outer variable
  timeOffset*=_timeOffsetScale;
}

This is a legitimate bug fix — the old code re-declared timeOffset inside the if block, meaning the time offsets were never actually applied to the reconstructed pulse times. This is a significant data-quality fix. ✅

🟡 Issue 2: Potential integer overflow in DataInterface.cc channel calculation

size_t channel  = index*4 + sipm;

Here index is declared as int (line 1026) and sipm is also int (line 1027). The multiplication index*4 is done in int arithmetic before being assigned to size_t. If index is very large, this could theoretically overflow int before the widening conversion. In practice with CRV channel counts this is unlikely to be an issue, but it would be safer to cast:

size_t channel = static_cast<size_t>(index) * 4 + sipm;

Note: The same pattern barIndex.asUint()*CRVId::nChanPerBar + SiPM is used in CrvRecoPulsesFinder_module.cc (line 196), where asUint() already returns unsigned — that one is fine.

🟡 Issue 3: pedestalFromDB declared as double instead of bool

This is a pre-existing issue (not introduced by this PR) but worth noting:

double pedestalFromDB = true;

pedestalFromDB is semantically a boolean but is declared as double. It gets passed downstream (line 255), so the type should match whatever the consumer expects. This should ideally be bool.

🟡 Issue 4: Orphaned comment in prolog_v12.fcl

In CRVReco/fcl/prolog_v12.fcl, after removing the cutoff parameters, a comment about cutoffs was left behind:

      timeOffsetScale           : 1.0    //scale factor for the time offsets from the database
                                         //...
                                         // then the scale factor should be set to 0.5 (i.e. scaled to a sigma of 50%)
                                         //note: if measured time offsets are used, the cutoffs should be set to the maximum values
      ignoreChannels            : true   //...

The comment //note: if measured time offsets are used, the cutoffs should be set to the maximum values is now orphaned since the cutoff parameters have been removed. This was cleaned up in prolog_v11.fcl but not in prolog_v12.fcl. The same issue exists in CRVResponse/fcl/prolog_v11.fcl which also retains //note: if measured time offsets are used... without the corresponding parameters.

🟡 Issue 5: EventDisplayExtracted.fcl — DB text file line is commented out

services.ProditionsService.crvCalib.useDb            : true
services.ProditionsService.crvStatus.useDb           : true
#comment out this line, if displaying simulated events
#services.DbService.textFile                          : [...]

With useDb set to true but the textFile line commented out, this will attempt to fetch from the real database. The comment says to "comment out this line, if displaying simulated events" — but it's already commented out. This means for extracted data (which is what this config is for), users will need to manually uncomment this line. This is arguably confusing. Consider making the text file line active by default for this extracted-data config, with a comment saying to comment it out for simulated events — or vice versa with clearer instructions.

🟢 Issue 6: No bounds checking on _calib->timeOffset(channel) in EventDisplay

double timeOffset = _calib->timeOffset(channel);

Unlike CrvRecoPulsesFinder_module.cc which guards with if(_useTimeOffsetDB), the EventDisplay code always applies the time offset unconditionally. If the calibration DB is not configured (e.g., for simulated data), this could throw or return incorrect values. A _useTimeOffsetDB-style guard or a try/catch might be warranted here, depending on the EventDisplay's usage context.

🟢 Issue 7: Hardcoded index*4 magic number

size_t channel  = index*4 + sipm;

In CrvRecoPulsesFinder_module.cc, the equivalent uses CRVId::nChanPerBar (a named constant). In the EventDisplay code, it's hardcoded as 4. Consider using the named constant for consistency and maintainability.

---

Summary Table

#	Severity	File	Issue
1	🔴 Critical (fixed)	CrvRecoPulsesFinder_module.cc	Variable shadowing bug — core fix of this PR ✅
2	🟡 Minor	DataInterface.cc	Potential int overflow in channel calc
3	🟡 Minor	CrvRecoPulsesFinder_module.cc	pedestalFromDB should be bool not double (pre-existing)
4	🟡 Minor	prolog_v12.fcl, CRVResponse prolog_v11.fcl	Orphaned cutoff comments
5	🟡 Minor	EventDisplayExtracted.fcl	DB text file commented out may confuse users
6	🟢 Nit	DataInterface.cc	No guard on timeOffset fetch; always applied
7	🟢 Nit	DataInterface.cc	Hardcoded 4 instead of CRVId::nChanPerBar

Overall, the core bug fix (Issue 1) is correct and important. The removal of the cutoff parameters is a clean simplification. The calibration parameter adjustments and DB file updates look reasonable. I'd suggest addressing the orphaned comment (Issue 4) and the hardcoded magic number (Issue 7) before merging, but none of the remaining issues are blockers.

[ehrlich-uva]

The last commit addressed the above comments/suggestions.

[oksuzian]

@FNALbuild run build test

[FNALbuild]

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

[FNALbuild]

☀️ The build tests passed at dfc8725.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged dfc8725 at 3e5e0e6
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.
check_cmake	✅	Log file.
FIXME, TODO	➡️	TODO (3) FIXME (0) in 6 files
clang-tidy	➡️	12 errors 276 warnings
whitespace check	✅	no whitespace errors found

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

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.