set default ZS CRV data

[ehrlich-uva]

Should only be merged with the corresponding PR in PassN (PR 8)

[FNALbuild]

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

• DAQ
• CRVReco

which require these tests: build.

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

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

About FNALbuild. Code review on Mu2e/Offline.

[FNALbuild]

☀️ The build tests passed at 5b404f1.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged 5b404f1 at 1edce62
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 (1) FIXME (0) in 4 files
clang-tidy	➡️	8 errors 180 warnings
whitespace check	✅	no whitespace errors found

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

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 examining its details and changes.Now let me also check if there are any other FCL files or references that might use the removed parameters but weren't updated:Now let me check if the FCL prolog files have any other references to the removed NZS parameters that might also need updating:Here is my review of PR #1761 — "set default ZS CRV data". The PR changes 6 files across CRVReco and DAQ to switch the CRV default data path from NZS (Non-Zero Suppressed) to ZS (Zero Suppressed), adding a configurable useNZS flag to calibration/pedestal modules, and removing NZS-specific tracking from the DQM collector.

---

Issues Found

1. 🔴 FCL prolog configs missing the new useNZS parameter (potential silent breakage)

In CRVReco/src/CrvCalibration_module.cc and CRVReco/src/CrvPedestalFinder_module.cc, a new useNZS fhicl parameter is added (defaulting to false). However, the FCL prolog configs in prolog_v11.fcl and prolog_v12.fcl for CrvPedestalFinder and CrvCalibration do not include the new useNZS parameter. While the C++ default of false will apply, it's best practice to explicitly set new parameters in the prolog FCL files so that:

• Users can discover the parameter.
• If anyone needs NZS data for calibration runs, the knob is visible and documented.

     CrvPedestalFinder:
     {
       module_type               : CrvPedestalFinder
       crvDigiModuleLabel        : "CrvDigi"
+      useNZS                    : false
       maxADCspread              : 5
       tmpDBfileName             : "pedestals.txt"
     }

     CrvCalibration:
     {
       module_type               : CrvCalibration
       crvRecoPulsesModuleLabel  : "CrvRecoPulses"
+      useNZS                    : false
       tmpDBfileName             : "calibration.txt"
     }

The same applies to prolog_v11.fcl.

---

2. 🔴 Comment typo fixed but produceNZS default flipped — verify downstream impact

// Before (on main):
fhicl::Atom<bool> produceZS{fhicl::Name("produceZS"), fhicl::Comment("produce NZS digi collection"), true};   // ← comment said NZS, was a bug
fhicl::Atom<bool> produceNZS{fhicl::Name("produceNZS"), fhicl::Comment("produce NZS digi collection"), true}; // default: true

// After (this PR):
fhicl::Atom<bool> produceZS{fhicl::Name("produceZS"), fhicl::Comment("produce ZS digi collection"), true};    // comment fixed ✅
fhicl::Atom<bool> produceNZS{fhicl::Name("produceNZS"), fhicl::Comment("produce NZS digi collection"), false}; // default flipped: true → false

The comment fix for produceZS is good. However, changing produceNZS default from true to false means the NZS digi collection will no longer be produced by default. Any existing FCL job configurations that rely on the NZS collection being present (without explicitly setting produceNZS : true) will silently stop producing NZS data, and any downstream modules consuming the "NZS" instance label will fail at runtime with a product-not-found error. This is the correct behavior per the PR's intent, but make sure the corresponding PassN PR (PR 8) accounts for this, as noted in the PR description.

---

3. 🟡 Loss of DQM monitoring capability for NZS data

The CrvDQMcollector_module.cc changes remove all NZS digi rate monitoring: histograms, counters, and the entire NZS digi processing loop. This is a significant functionality reduction. Specifically:

• Removed histograms: _histDigisPerChannelAndEventNZS, _histDigiRatesROCNZS, _hist2DDigiRatesROCNZS
• Removed counters: _nDigisNZS, _nDigisROCNZS
• Removed processing loop: The entire for(size_t i=0; i<crvDigiCollectionNZS->size(); ...) block

Unlike CrvCalibration and CrvPedestalFinder, where the NZS/ZS choice was parameterized via the new useNZS flag, the DQM collector has no option to re-enable NZS monitoring. If NZS monitoring is ever needed again (e.g., for debugging or special calibration runs), the code would have to be re-added. Consider whether a useNZS flag approach similar to the calibration modules would be more appropriate here, to keep the flexibility without the separate module label.

---

4. 🟡 CrvCalibration uses crvRecoPulsesModuleLabel while CrvPedestalFinder uses crvDigiModuleLabel — inconsistent NZS instance semantics

In CrvCalibration_module.cc, the useNZS flag controls the instance label of CrvRecoPulseCollection:

event.getByLabel(_crvRecoPulsesModuleLabel, (_useNZS ? "NZS" : ""), crvRecoPulseCollection)

In CrvPedestalFinder_module.cc, the useNZS flag controls the instance label of CrvDigiCollection:

event.getByLabel(_crvDigiModuleLabel, (_useNZS ? "NZS" : ""), crvDigiCollection)

This is not necessarily a bug (they consume different product types), but it's worth documenting that useNZS has subtly different meanings in each module context. A comment in the code or the fhicl Comment strings clarifying "use NZS instance label for RecoPulses" vs. "use NZS instance label for Digis" would improve clarity.

---

5. 🟢 Minor: firstSampleOnly is not in FCL prolog for CrvPedestalFinder

This is a pre-existing issue (not introduced by this PR), but the CrvPedestalFinder config block in both prolog files does not include firstSampleOnly, which is a required parameter (no default). This should already be set somewhere downstream, but it's worth noting.

---

6. 🟢 Ternary readability

(_useNZS ? "NZS" : "")

The parenthesized ternary inside the function call arguments is fine functionally but can be hard to read at a glance. This is a minor style observation — no change needed.

---

Summary

Severity	Issue	File(s)
🔴 High	FCL prolog files don't include the new useNZS parameter	prolog_v11.fcl, prolog_v12.fcl
🔴 High	produceNZS default flipped — downstream consumers may break if not coordinated with PassN PR	CrvDigisFromArtdaqFragmentsFEBII_module.cc
🟡 Medium	Complete removal of NZS DQM monitoring with no option to re-enable	CrvDQMcollector_module.cc
🟡 Medium	useNZS semantics differ between calibration and pedestal modules (RecoPulses vs Digis) — could use better documentation	CrvCalibration_module.cc, CrvPedestalFinder_module.cc
🟢 Low	Pre-existing: firstSampleOnly missing from prolog FCL	prolog_v11.fcl, prolog_v12.fcl

The overall approach of making ZS the default and parameterizing NZS as opt-in is sound. The main actionable concern is ensuring the FCL prologs are updated to include the new useNZS parameter, and confirming that the PassN PR 8 is merged in lockstep to prevent runtime failures from the produceNZS default change.

[ehrlich-uva]

Responses

1. I added the useNZS : false to the prologs.
2. This is the desired behavior. There is no NZS data at the moment, and won't be used downstream. The NZS that could be produced by the digi generator is only a copy of the ZS data with an "NZS" instance tag.
3. This is the desired behavior. Future NZS data will have different properties to be monitored by the DQM.
4. The implementation is correct.
5. I added the firstSampleOnly parameter to the prologs.
6. I will keep the code the way it is.