Filter to select events with the primary + pileup are above a threshold

[michaelmackenzie]

This is motivated by Run 1B studies, to select DIO primary events that either are high energy calo deposits on their own or through the addition of pileup. This is run before digi-making, which takes a significant amount of time and we therefore want to cut down on this time.

[FNALbuild]

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

• Filters

which require these tests: build.

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

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

About FNALbuild. Code review on Mu2e/Offline.

[Copilot]

Pull request overview

Adds a new ART filter module to preselect simulation events based on calorimeter energy deposition from the primary SimParticle plus nearby pileup, motivated by Run 1B studies to reduce downstream digi-making time.

Changes:

• Introduces CaloPileupDtsFilter EDFilter that computes primary calorimeter Edep and adds nearby-in-time/space pileup Edep.
• Applies configurable primary/total energy cuts and time/space proximity windows.
• Adds basic diagnostics and end-of-job pass/fail summary.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

goodParticle() currently ignores its argument and always returns true, which will typically trigger an unused-parameter warning under common warning flags. If it's intentionally a placeholder, consider omitting the parameter name or marking it [[maybe_unused]] until real selection logic is added.

[michaelmackenzie]

@copilot open a new pull request to apply changes based on this feedback

[Copilot]

The diagnostic loop dereferences p without checking isNull(). Since the later logic explicitly handles sim.isNull(), this printout can segfault when a null art::Ptr is present. Guard the print with if(p.isNull()) continue; (or print a placeholder) before using p->....

[michaelmackenzie]

@copilot open a new pull request to apply changes based on this feedback

[Copilot]

As soon as primary_edep + pileup_edep exceeds maxTotalEnergy_, the final selection can no longer pass, but the code continues scanning remaining steps. Consider early-exiting (or breaking out of loops) once the running total is already above maxTotalEnergy_ to reduce CPU for high-occupancy events.

[michaelmackenzie]

@copilot open a new pull request to apply changes based on this feedback

[FNALbuild]

☔ The build tests failed for ab08083.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged ab08083 at 191ad5d
build (prof)	✅	Log file. Build time: 08 min 57 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. Return Code 1.
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 ab08083 after being merged into the base branch at 191ad5d.

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. Let me start by loading the relevant ability and fetching the PR details.Now let me also check the existing review comments on this PR:Here's my review of PR #1747 — a new CaloPileupDtsFilter module that filters events based on combined primary + pileup calorimeter energy deposits. The PR adds a single new file:

// 327 lines added, 0 deleted

---

Issues Found

🔴 Bug — Time wrap-around not handled (High Severity)

double time_diff = std::fabs(std::fmod(css.time(), mbtime) - primary_time);

The time_diff calculation doesn't handle wrap-around at the microbunch boundary. Steps near 0 ns and near mbtime ns will appear far apart even if they are actually close in cyclic time. This will miss valid pileup contributions near the boundary.

Fix: Compute the wrapped circular distance:

double t = std::fmod(css.time(), mbtime);
double time_diff = std::fabs(t - primary_time);
time_diff = std::min(time_diff, mbtime - time_diff);

The same issue applies to the primary time averaging (line ~172), where primary_time is computed via fmod on individual steps — if primary steps straddle the wrap boundary, the energy-weighted average time will be incorrect.

---

🔴 Bug — Potential null pointer dereference in diagnostic loop (High Severity)

      for(const auto& p : primary.primarySimParticles()) {
        std::cout << "  Primary particle: PDG = " << p->pdgId()
                  << " E = " << p->startMomentum().e()
                  << " t = " << p->startGlobalTime()
                  << std::endl;
      }

The diagnostic loop dereferences p without checking p.isNull(). The later logic in the main loop (line ~299) explicitly guards with if(sim.isNull()) continue;, proving null art::Ptrs are expected. This will segfault when diagLevel_ > 1 and a null Ptr is present.

Fix: Add if(p.isNull()) continue; before dereference.

---

🟡 Missing includes — <iostream> and <cmath> (Medium Severity)

#include <string>
#include <map>
#include <sstream>
#include <iostream>
#include <cmath>

The file uses std::cout, std::endl, std::fmod, and std::fabs but relies on transitive includes for <iostream> and <cmath>. I see these were already added in a later commit, so this appears to be resolved in the latest revision.

---

🟡 goodParticle() unused parameter warning (Low Severity)

  bool CaloPileupDtsFilter::goodParticle(SimParticle const& simp) const {
    // Check if the SimParticle is a valid particle to consider for pileup filtering
    return true; // For now, accept all particles
  }

The parameter simp is never used, which will trigger -Wunused-parameter warnings. If it's a placeholder, either omit the name or annotate with [[maybe_unused]]:

bool CaloPileupDtsFilter::goodParticle(SimParticle const& /*simp*/) const {

---

🟡 Performance — Expensive stepPosition() called unnecessarily (Medium Severity)

In the latest revision, the code already separates the time and space checks (time first, then position), which is good. However, the pileup loop still doesn't early-exit when primary_edep + pileup_edep > maxTotalEnergy_. Once the running total exceeds the max, no amount of additional pileup can bring it back in range, but the code keeps scanning all remaining steps and computing geometry transforms.

Fix: Add an early-break check inside the pileup accumulation loop:

pileup_edep += css.energyDepBirks();
if(primary_edep + pileup_edep > maxTotalEnergy_) break; // early exit

---

🟢 Observation — consumes<>() declarations

The latest revision correctly declares consumes<CaloShowerStepCollection>() and consumes<PrimaryParticle>() in the constructor, which is good practice for the art framework. ✅

---

🟢 Observation — Config validation

The constructor validation is thorough (empty tags, energy bounds, window bounds). One minor note: minPrimaryEnergy_ <= 0. is rejected, which means a value of exactly 0.0 is forbidden, yet the fhicl default is 0.0. This means a user who doesn't explicitly set MinimumPrimaryEnergy will get a runtime config error. This may be intentional (force user to set it), but is worth noting.

---

Summary

Severity	Issue	Status
🔴 High	Time wrap-around bug in pileup matching	Needs fix
🔴 High	Null ptr dereference in diagnostic print	Needs fix
🟡 Medium	Missing <iostream>/<cmath> includes	Fixed in later commits
🟡 Low	Unused parameter warning in goodParticle()	Needs fix
🟡 Medium	No early-exit when total energy exceeds max	Optimization opportunity

The time wrap-around bug is the most critical issue — it will silently produce incorrect filtering results for events with pileup near the microbunch boundary.