switched from CRV no-fit to fit reco pulses

[ehrlich-uva]

No description provided.

[oksuzian]

I'll review this pull request by first loading the PR details and its changes.Now let me look at the existing code in the base branch for more context:Here's my review of PR #340. This PR switches the CRV reco pulses from using the no-fit option by default to using the fit option by default, controlled by a new CrvNoFitReco fhicl parameter.

Summary of Changes

3 files changed (+29, -17) across:

• inc/CrvInfoHelper.hh — adds bool crvNoFitReco parameter to two function signatures
• src/CrvInfoHelper.cc — implements conditional logic for no-fit vs. fit reco pulses
• src/EventNtupleMaker_module.cc — adds new fhicl configuration parameter and plumbs it through

---

Issues Found

1. 🔴 Inconsistency: FillCrvPulseInfoCollections accepts crvNoFitReco but doesn't fully use it

In FillCrvPulseInfoCollections, the crvNoFitReco parameter is added to the function signature and used for the PE value selection in the emplace_back call (line ~244), but there is no corresponding duplicate-pulse filtering like there is in FillCrvHitInfoCollections.

In FillCrvHitInfoCollections, when crvNoFitReco is true, duplicate pulses are skipped:

if(crvNoFitReco)
{
  // Skip duplicate pulses (those with multiple peaks)
  if(coincRecoPulses_.at(j)->GetRecoPulseFlags().test(CrvRecoPulseFlagEnums::duplicateNoFitPulse)) continue;
  // Get no-fit PEs associated with this reco pulse
  PEs = coincRecoPulses_.at(j)->GetPEsNoFit();
}

But in FillCrvPulseInfoCollections, no such duplicate-pulse filtering is added — only the PE value is conditionally switched. If the no-fit option requires skipping duplicateNoFitPulse flagged pulses, this should be applied consistently in FillCrvPulseInfoCollections as well.

2. 🟡 Missing Comment() on new fhicl parameter

fhicl::Atom<bool> crvNoFitReco{Name("CrvNoFitReco"),false};  //use the CRV no-fit option for the reco pulses

Other fhicl parameters in this block use Comment(...) to provide a description string (e.g. Comment("Flag for turning on crvpulses(mc) branches")). The new crvNoFitReco parameter only has a C++ comment, not a Comment() argument. For consistency and for fhicl documentation generation, this should be:

fhicl::Atom<bool> crvNoFitReco{Name("CrvNoFitReco"), Comment("Use the CRV no-fit option for the reco pulses"), false};

3. 🟡 Ternary expression readability in emplace_back

recoInfo.emplace_back(HitPos, barIndex.asInt(), sectorNumber, SiPMId,
    (crvNoFitReco?crvRecoPulse->GetPEsNoFit():crvRecoPulse->GetPEs()), crvRecoPulse->GetPEsPulseHeight(), crvRecoPulse->GetPulseHeight(),
    crvRecoPulse->GetPulseBeta(), crvRecoPulse->GetPulseFitChi2(), crvRecoPulse->GetPulseTime());

This inline ternary is dense and hard to read within the long emplace_back argument list. For consistency with the pattern used in FillCrvHitInfoCollections (which assigns to a local variable first), consider extracting it:

float PEs = crvNoFitReco ? crvRecoPulse->GetPEsNoFit() : crvRecoPulse->GetPEs();
recoInfo.emplace_back(HitPos, barIndex.asInt(), sectorNumber, SiPMId,
    PEs, crvRecoPulse->GetPEsPulseHeight(), ...);

4. 🟡 recoSummary.totalPEs not updated for crvNoFitReco flag

In FillCrvHitInfoCollections, the reco summary calculation (lines ~82-91 on main) always uses GetPEs():

recoSummary.totalPEs+=crvRecoPulses->at(i).GetPEs();

This is not updated to conditionally use GetPEsNoFit() when crvNoFitReco is true. The per-cluster PE sums will use the no-fit values, but the summary total PEs will still use the fit values, which is an inconsistency. This may be intentional (the cluster's own GetPEs() at line 74 also isn't changed), but it's worth confirming.

5. 🟢 Cosmetic: trailing whitespace cleanup

Several lines that only had trailing whitespace removed are included in the diff. These are fine but inflate the diff slightly. Not an issue, just noise.

---

Verdict

The PR logic is mostly sound for its intended purpose — making the fit/no-fit behavior configurable. The most important issue is #1: the FillCrvPulseInfoCollections function should likely also skip duplicateNoFitPulse-flagged pulses when crvNoFitReco is true, matching the behavior in FillCrvHitInfoCollections. Issue #4 (summary PEs) should also be clarified with the author as to whether it's intentional.

[ehrlich-uva]

1. duplicateNoFitPulse is only used for no-fit pulses. The code is correct.
2. The parameter below also doesn't use a comment, but I can add one.
3. It's fine the way it is now, but I can change it.
4. This was introduced before this PR. I will change it.

[ehrlich-uva]

Removed the CRV no-fit option, because it doesn't fit into the current EventNtuple structure. It will probably be removed completely from the CRV reco.