Calo step selection in calo dts filtering

[michaelmackenzie]

This adds separate min/max particle momentum thresholds for calo steps, a total calo energy deposit selection, and some additional diagnostic printouts. In Run 1B work, we found the current filtering can bias the output by filtering out events with high deposits but they shower and separate into different sim particles. This helps resolve this issue, especially when using the new total calo energy deposit selection that will add up this total shower.

[FNALbuild]

Hi @michaelmackenzie,
You have proposed changes to files in these packages:

• Filters

which require these tests: build.

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

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

About FNALbuild. Code review on Mu2e/Offline.

[FNALbuild]

☀️ The build tests passed at a9839ee.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged a9839ee at 77250d9
build (prof)	✅	Log file. Build time: 04 min 14 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 1 files
clang-tidy	➡️	2 errors 11 warnings
whitespace check	✅	no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at a9839ee after being merged into the base branch at 77250d9.

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 #1819 Review — Calo step selection in calo dts filtering

PR Summary

Author	@michaelmackenzie
Branch	michaelmackenzie:CaloStepFiltering → Mu2e:main
State	Open, not draft, mergeable (unstable due to clang-tidy informational)
Files changed	1 (Filters/src/DetectorStepFilter_module.cc), +51/−6
Commits	1
Reviews	0 (no human reviews yet)
Comments	2 (FNALbuild bot only)
CI	✅ build passed at a9839ee, all integration tests green; ⚠️ clang-tidy reports 2 errors / 11 warnings, ✅ whitespace clean, ✅ no FIXMEs/TODOs

Intent: Adds three things to the DetectorStepFilter module:

1. Separate min/max particle-momentum cuts that apply only to calorimeter steps (MinimumCaloPartMom / MaximumCaloPartMom), independent of the global MinimumPartMom/MaximumPartMom.
2. A new optional total calo energy-deposit cut (MinimumSumCaloE) summed across all good particles, in addition to the existing per-particle sum (MinimumSumCaloStepE). This is motivated by Run 1B observations where high-energy showers got filtered out because their energy was split across multiple SimParticles.
3. A new DiagLevel knob with verbose printouts at four severity tiers.

No linked issue; no acceptance criteria to track.

---

Core Changes

1. Calo-specific particle momentum cut

fhicl::Atom<double> minCaloPartMom{ Name("MinimumCaloPartMom"), Comment("Minimum particle momentum for calo steps"), 0.0 };
fhicl::Atom<double> maxCaloPartMom{ Name("MaximumCaloPartMom"), Comment("Maximum particle momentum for calo steps"), maxE_ };

In the calo loop, minPartM_/maxPartM_ are replaced by minCaloPartM_/maxCaloPartM_. Defaults preserve effectively the prior behavior (0 / maxE_), so this is backward compatible only if users were relying on MinimumPartMom > 0 to filter calo steps — see Issue #1 below.

2. Total calo energy selection

fhicl::OptionalAtom<double> minSumCaloTotalE { Name("MinimumSumCaloE"), Comment("Minimum E sum of good calorimeter steps by all good particles (MeV) ")};

Track total_edep over all steps that pass the per-step minCaloE_, calo momentum cuts, goodParticle, and time cuts. If useMinSumCaloTotalE_ is true and total_edep > minSumCaloTotalE_, set selectcalo = true.

3. Diagnostic printouts gated by DiagLevel

• >0: print event id + selecttrk/calo/crv when event passes; print when total-edep cut fires.
• >1: print per-particle calo edep sum.
• >2: print per-step calo info.
• >3: always print event-level decision.

---

Issues Found

🔴 Issue 1 — Behavior change: minPartM_ / maxPartM_ no longer apply to calo steps

Previously the calo loop used minPartM_/maxPartM_. After this PR it uses only the new minCaloPartM_/maxCaloPartM_, whose defaults are 0 / maxE_. Any existing fhicl configuration that set MinimumPartMom expecting it to filter calo SimParticles will silently change behavior. Two options:

• Recommended: keep applying minPartM_/maxPartM_ AND the new calo-specific cuts (intersection). That way the new params are strictly tighter when set, never looser.
• Or: explicitly document the breaking change in the comment of MinimumCaloPartMom and grep for downstream fcl files using MinimumPartMom.

if(edep_step > minCaloE_ &&
    css.momentumIn() > minCaloPartM_ && css.momentumIn() < maxCaloPartM_ &&
    goodParticle(*css.simParticle()) &&
    timeCut(fmod(css.time(),mbtime))) {

🟠 Issue 2 — fhicl name MinimumSumCaloE collides conceptually with MinimumSumCaloStepE

The new parameter is named MinimumSumCaloE but the C++ variable is minSumCaloTotalE_. Users will have to distinguish:

• MinimumSumCaloStepE — min per-particle sum
• MinimumSumCaloE — min total sum across all particles

These are easy to confuse. Suggest renaming the fhicl key to MinimumSumCaloTotalE so it matches the variable and is unambiguous. (Since this is a brand-new param, renaming is cheap.)

🟠 Issue 3 — > vs >= inconsistency on the new total cut

The new total cut uses strict >, while the per-particle sum cut uses >=:

if(icalo->second >= minSumCaloE_ && icalo->second <= maxSumCaloE_ ){   // existing: >=
    ...
}
...
if(total_edep > minSumCaloTotalE_) {                                    // new: >
    selectcalo = true;

For consistency (and so a configured threshold of e.g. 50.0 means "≥ 50 MeV passes" everywhere), use >=.

🟠 Issue 4 — No upper bound on the new total cut

Existing per-particle sum has both MinimumSumCaloStepE and MaximumSumCaloStepE. The new total selection has only a min — there is no MaximumSumCaloE. If symmetry is desired (and to allow rejecting pathological events), add a matching MaximumSumCaloE (also OptionalAtom). At minimum, document why the asymmetry is intentional.

🟡 Issue 5 — total_edep declared outside the calocoltag loop (likely correct, worth confirming)

float total_edep = 0.; // for total calorimeter deposits
for(const auto& calocoltag : caloStepCols_) {
    CES caloESum;
    ...
    for(const auto& css : *csscolH ) {
       ...
       total_edep += edep_step;
       ...
    }
    ...
    if(useMinSumCaloTotalE_) {
      if(total_edep > minSumCaloTotalE_) { selectcalo = true; ... }
    }
    if(selectcalo) break;
}

total_edep accumulates across calo step collections, which is probably what you want ("total shower"), but the per-particle caloESum is reset per collection. Worth a one-line comment to make the intent explicit, since a future reader may "fix" it the wrong way.

🟡 Issue 6 — "Total shower" label is slightly misleading

The PR description says the new selection sums the "total shower," but total_edep only includes steps that already passed minCaloE_, calo momentum cuts, and goodParticle(). If a shower spawns sub-threshold daughters or non-keep-PDG daughters, their energy is excluded. This is recoverable by configuring MinimumCaloPartMom = 0, MinimumCaloStepE = 0, and a permissive keepPDG, but consider whether the cuts should be intentionally relaxed for the total-edep accumulation, or document the caveat in the fhicl comment.

🟡 Issue 7 — total_edep not reported in event diagnostic when selection comes from per-particle path

diagLevel_ > 0 prints total_edep only when the total cut fires. When selectcalo is set via the per-particle path, the event-level printout (diagLevel_ > 0 && retval) does not include total_edep. Including it always would make Run 1B–style debugging easier.

🟡 Issue 8 — Member declaration order vs. init list (minor, currently safe)

Header order:

double minSumCaloE_, minSumCaloTotalE_;
bool useMinSumCaloTotalE_;

Init list initializes useMinSumCaloTotalE_ via conf().minSumCaloTotalE(minSumCaloTotalE_) which writes to minSumCaloTotalE_ by reference. It's safe today, but minSumCaloTotalE_ is left uninitialized when the optional is absent. Consider explicitly initializing it (e.g. , minSumCaloTotalE_(0.)) before the line that reads the optional, to avoid future surprises and silence any uninitialized-read static analyzers.

🟡 Issue 9 — clang-tidy reports 2 errors / 11 warnings

Bot output: clang-tidy.log. Worth checking whether any of the 2 errors / 11 warnings were introduced (or surfaced) by this PR before merging.

---

Merge Readiness

Criterion	State
Build & integration tests	✅ all green
Code review approvals	⚠️ none yet
Behavior preservation	⚠️ Issue 1 — silent change to existing fcls that set MinimumPartMom
Naming clarity	⚠️ Issue 2 — MinimumSumCaloE vs MinimumSumCaloStepE
Static analysis	⚠️ clang-tidy errors not yet triaged

Recommended before merge: address Issue 1 (decide preservation vs. breaking) and Issue 2 (rename fhicl key while it's still cheap). Issues 3–9 are nice-to-haves.

---

Possible Improvements

1. Add a unit/integration test (or a tiny test fcl) exercising the new total-energy path so future refactors don't silently regress it.
2. Consolidate the four diagnostic blocks into a small helper or mf::LogDebug to keep filter() readable.
3. Initialize total_edep at the top of filter() per event (it currently is, since it's a local — fine, but worth a comment explaining "intentionally summed across all calo collections").
4. Bump MaximumPartMom's comment in the header to mention it now applies only to tracker/CRV steps.

---

Want me to…

1. Pull and summarize the clang-tidy errors/warnings from the build artifact to see if any are introduced by this PR.
2. Search downstream fcl files in Mu2e/Offline that set MinimumPartMom on DetectorStepFilter to quantify the blast radius of Issue 1.
3. Draft a suggested patch implementing Issues 1–4 (preservation of minPartM_, rename, >=, optional MaximumSumCaloE).
4. Look up other recent calo-filtering PRs / Run 1B discussions for added context on the motivation.

[michaelmackenzie]

Responding to the AI comments:

1. We should update the fcl, this is solving the issue that the current params are too strict here.
2. I can change the existing fcl parameter name if preferred, but this will require fcl updates in time with this PR merging.
3. Using > is necessary to not accept events with 0 steps, which is already true for the calo step check due to the for loop.
4. I don't believe a maximum is needed for this selection, but we can add one if it's necessary later.
5. This seems fine to me, it's the total deposit independent of the mixing source.
6. I think it's already described that those cuts are used when evaluating the total deposits, as is done for the per-particle deposit checks.
7. I've added an additional printout for this if diagLevel_ > 1.
8. The total value should only ever be used if it's enabled, so not being set when not enabled doesn't seem like an issue?
9. I don't think we enforce these.