Updated CRV pulse reconstruction for secondary pulses

[ehrlich-uva]

No description provided.

[FNALbuild]

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

• CRVReco
• EventDisplay
• RecoDataProducts

which require these tests: build.

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

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

About FNALbuild. Code review on Mu2e/Offline.

[FNALbuild]

☀️ The build tests passed at 00dce12.

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

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

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]

Issues found

1. failedFit propagation logic looks wrong / suspicious — MakeCrvRecoPulses.cc lines 195–200

    if(_failedFits.size()>0)
    {
      if(_failedFits.back()) _failedFits.push_back(true);
      else _failedFits.push_back(failedFit);
    }
    else _failedFits.push_back(failedFit);

Once any fit fails, _failedFits.back() is true, so every subsequent pulse is force-flagged as failed regardless of its own fit quality. Since pulses are found in time order and later pulses are the newly-exposed secondaries, a single early failed fit poisons all later pulses. If the intent is "a bad fit contaminates the subtraction so downstream pulses are unreliable," that's defensible — but it deserves a comment, and it's a behavioral change worth confirming. If not intended, this is a bug.

2. Unbounded loop risk if FindNextPeak doesn't advance start — lines 143 & 203

  while(FindNextPeak(g, start, peakStart, peakEnd, fitStart, fitEnd))
  {
    ...
    start=fitEnd;
  }

The loop advances via start = fitEnd. fitEnd can be as small as peakEnd - ... after the if(fitEnd-peakEnd>1) fitEnd--; adjustment, and FindNextPeak scans from start+1. It's probably monotone because a real peak requires a falling edge after start, but there's no explicit guard that start strictly increases. After SubtractPulse flattens the region, re-scanning near the same index should not re-trigger — but worth a defensive check or assertion to guarantee termination.

3. Removed config-validation guard, silently — CrvRecoPulsesFinder_module.cc
The check pulseAreaThreshold > minADCdifference → throw was removed (correct, since those params are gone). ✅ No action — just confirming this removal is intentional and complete (both the module ctor and MakeCrvRecoPulses ctor guards are gone).

4. CrvRecoPulse data-model break is not backward compatible — RecoDataProducts/inc/CrvRecoPulse.hh
Dropping _PEsNoFit, _pulseTimeNoFit, _pulseStart, _pulseEnd changes the persisted data product. Any existing art/ROOT files written with the old class, and any downstream code / analysis / plugins calling GetPEsNoFit(), GetPulseStart(), etc., will fail to compile or read. Confirm a classes_def.xml / ClassVersion bump and that no other package references these getters.

5. int16_t truncation in SubtractPulse — lines 92–94

    int16_t diff=static_cast<int16_t>(std::round(_f1.Eval(t)));
    g.SetPointY(bin,g.GetPointY(bin)-diff);

_f1.Eval returns a double; casting the subtracted amount to int16_t mimics ADC quantization (reasonable), but if _f1.Eval ever exceeds ±32767 the cast overflows (UB/wraparound). A large pulse height * TMath::E() could plausibly approach that. Consider clamping or using float/int for diff.

6. clang-tidy: 2 errors, 32 warnings — clang-tidy log
CI is green overall, but 2 clang-tidy errors were reported. Worth checking whether they're in the files this PR touched.

[ehrlich-uva]

Responses:

1. No code change required, but I added a comment. The intended behavior ("a bad fit contaminates the subtraction so downstream pulses are unreliable") was correctly identified.
2. No code change required, because the start variable will always advance.
3. No action required.
4. The functions that were removed are not used anywhere else.
5. Switched to double to address the concern.