[michaelmackenzie]

This will allow for selection or vetoing of protons at the helix level.

[FNALbuild]

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

• TrkFilters

which require these tests: build.

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

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

About FNALbuild. Code review on Mu2e/Offline.

[FNALbuild]

☀️ The build tests passed at ce1567d.

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

N.B. These results were obtained from a build of this Pull Request at ce1567d after being merged into the base branch at 78ea1fd.

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 Review Summary — #2055

Reviewed at head ce1567d16861a8325f29b09b2c45fb819e229c9e. First pass.

Decision

• 🟢 approve

Scope understood

• HelixFilter gains two optional cuts, minEDepAvg and maxEDepAvg, on the mean straw-hit energy deposit of the helix hits. The stated use is selecting or vetoing protons at the helix level.
• Both cuts are off unless a table sets them. No fcl in Offline or mu2e-trig-config sets them yet.

Findings

None.

Verified

• 🟢 The value read is the right one. Helix.hits().eDepAvg() (HelixFilter_module.cc:137) recomputes the mean from the helix hits. The stored HelixSeed::eDepAvg() would be wrong here: AgnosticHelixFinder never fills _eDepAvg, so it stays 0 for every APR helix. Only CalHelixFinder, RobustHelixFinder and RobustMultiHelixFinder set it.
• 🟢 The comment "Minimum mean straw hit energy deposit" is accurate. ComboHitCollection::eDepAvg() (RecoDataProducts/src/ComboHit.cc:266-275) weights each combo hit's energyDep() by its nStrawHits(). A combo hit's _edep is itself the average over its straw hits (CombineStrawHits_module.cc:303, MakeStereoHits_module.cc:298-319). So the result is the mean per straw hit, as the comment says.
• 🟢 It matches the finder's own cut. AgnosticHelixFinder computes the same hseed._hhits.eDepAvg() at CalPatRec/src/AgnosticHelixFinder_module.cc:1622 and rejects on eDepAvg > _maxEDepAvg at :1653. The filter keeps edep <= _maxedep, so the boundary is the same.
• 🟢 An empty hit list gives 0, not NaN, because the denominator is nStrawHits + 1e-10. With the cuts unset, the defaults lowest() and max() (:196-197) therefore pass every helix.
• 🟢 Both new members are initialized where they are declared. OptionalAtom::operator()(T&) leaves the member unchanged when the key is absent, so an unset cut is a real no-op.
• 🟢 FNALbuild at this head is green, including the trigger test with return code 0. That is consistent with no change when the keys are unset. The one clang-tidy error is in the external cetlib/LibraryManager.h, not in this PR.

Validation check

• Build/tests run: no local build or job run. FNALbuild build 3484 at this head passed every test.
• Config contract check: pass. Two new optional keys; existing tables validate unchanged.
• Cross-repo consistency: no companion change is needed to merge. No trig-config table uses the keys yet.

Residual risk

• Merge order with #2035: both PRs edit the same debug print in HelixFilter_module.cc (:143-144 here). git merge-tree of the two heads reports a content conflict on that one statement only. Whichever merges second needs a one-line rebase.
• The accessor HelixSeed::eDepAvg() has no caller in Offline or EventNtuple. Only the finders that set the field read it, and it is 0 for APR helices. A later edit that switches this filter to the stored field would silently cut every APR helix when minEDepAvg is set. This is pre-existing and not a reason to hold this PR.

Author follow-ups

1. Which trigger path will set these cuts? TrigAprProtonHelixFinder (trigAprProducers.fcl:22-30) runs with maxEDepAvg: 100., so its helices look like the natural input. A short note on the intended values would help whoever reviews the trig-config side.
2. MultiHelixFilter does not get the cut. It now has one consumer (trigAprFilters.fcl:646). Is that intended?