add a list of panel to skip while filtering

[pavel1murat]

a few panels have eDep distributions significantly wider that the majority. Skip them in the simplest single hit eDep-based trigger

[FNALbuild]

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

• DAQ

which require these tests: build.

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

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

About FNALbuild. Code review on Mu2e/Offline.

[FNALbuild]

☀️ The build tests passed at 78c2f8e.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged 78c2f8e at e4c9d83
build (prof)	✅	Log file. Build time: 08 min 19 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. Return Code 1.
check_cmake	✅	Log file.
FIXME, TODO	✅	TODO (0) FIXME (0) in 1 files
clang-tidy	➡️	3 errors 49 warnings
whitespace check	➡️	found whitespace errors

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

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 by examining its details and changes.Here is my review of PR #1768 — "add a list of panel to skip while filtering".

Summary

This PR adds the ability to skip specific tracker panels (identified by their "Minnesota ID" / mnid) when counting good straw hits in StrawHitFilter_module.cc. It introduces a TrackerPanelMap conditions lookup to map each hit's (plane, panel) to its mnid, then checks if that mnid is in a configurable skip list.

Files changed: DAQ/src/StrawHitFilter_module.cc (+26/−1), DAQ/fcl/prolog.fcl (+1)

---

Issues Found

1. 🔴 No null-pointer check on panel_map_by_offline_ind return value

const TrkPanelMap::Row* tpm = _trkPanelMap->panel_map_by_offline_ind(pln,pnl);
int hit_mnid = tpm->mnid();

panel_map_by_offline_ind() can return nullptr if the plane or panel index is out of range (see TrackerPanelMap.hh L44-47). Dereferencing a null pointer would cause a segfault. A null check should be added:

const TrkPanelMap::Row* tpm = _trkPanelMap->panel_map_by_offline_ind(pln,pnl);
if (!tpm) continue;  // or handle the error appropriately
int hit_mnid = tpm->mnid();

2. 🔴 _trkPanelMap fetched even when _shc is null — and _nsht used uninitialized on that path

When getByLabel fails (line 213), _shc is set to nullptr but _nsht is not reset to 0. The code then proceeds into the loop at line 227 using whatever stale value _nsht holds, which would dereference the null _shc. Additionally, _trkPanelMap is fetched at line 205 before the straw-hit collection is even retrieved — this is not wrong per se, but the real danger is the _nsht value:

if (!ArtEvent.getByLabel(_shCollTag, shch)) {
  TLOG(TLVL_ERROR) << "No straw hit collection tag:" << _shCollTag.data();
  _shc = nullptr;
  // _nsht is NOT set to 0 here!
}

Pre-existing bug, but the new skip-list code now depends on _nsht and _shc being valid. Consider adding _nsht = 0; on the error path (or returning early).

3. 🟡 O(N×M) linear scan of _mnidToSkip per hit — consider using std::unordered_set

bool skip = false;
for (auto mnid: _mnidToSkip) {
  if (hit_mnid == mnid) {
    skip = true;
    break;
  }
}

For every straw hit, the code does a linear scan of _mnidToSkip. If the skip list is small (a few entries), this is fine. But this is in a hot loop over all hits in every event. Converting _mnidToSkip to a std::unordered_set<int> (populated once in the constructor) would make the lookup O(1) and be cleaner:

// In constructor:
_mnidToSkipSet.insert(_mnidToSkip.begin(), _mnidToSkip.end());
// In the loop:
if (_mnidToSkipSet.count(hit_mnid)) continue;

4. 🟡 _trkPanelMap is not initialized in the constructor

The member _trkPanelMap (a raw pointer) is not initialized to nullptr in the constructor's initializer list or member declaration. If filter() somehow accesses it before the first event (unlikely but possible in some art edge cases), it would be an uninitialized pointer. Consider initializing it:

const TrackerPanelMap* _trkPanelMap{nullptr};

5. 🟡 _nevt and _nevp are not initialized

The event counters _nevt and _nevp (used in endRun for the rate calculation and incremented in filter) are never initialized to 0. This is a pre-existing issue but worth noting since they affect correctness of the pass-rate log message. They should be initialized in the constructor.

6. 🟢 Minor: Typo in FHiCL comment

fhicl::Atom<bool> fillHistograms{Name("fillHistograms"), Comment("fill histogrms, default:false")};

"histogrms" → "histograms". Pre-existing typo, minor.

7. 🟢 _fillHistograms type mismatch

_fillHistograms is declared as int (line 83) but is initialized from a fhicl::Atom<bool>. This works via implicit conversion but is inconsistent. Pre-existing, minor.

---

Summary Table

#	Severity	Issue	New/Pre-existing
1	🔴 High	Null-pointer dereference from panel_map_by_offline_ind	New
2	🔴 High	_nsht not reset on getByLabel failure → null _shc dereference	Pre-existing (made worse)
3	🟡 Medium	O(N×M) linear scan in hot loop — use unordered_set	New
4	🟡 Medium	_trkPanelMap not initialized to nullptr	New
5	🟡 Medium	_nevt/_nevp not initialized	Pre-existing
6	🟢 Low	Typo "histogrms"	Pre-existing
7	🟢 Low	_fillHistograms declared int, initialized from bool	Pre-existing

Recommendation: Issues #1 and #2 should be addressed before merging, as they can lead to crashes. Issue #3 is a nice-to-have optimization. The rest are minor or pre-existing.

[bonventre]

Hi Pasha, could we not just mark these panels as noisy in the status tables for those runs? Is there any reason to not use strawid to refer to them as well?

[FNALbuild]

📝 The HEAD of main has changed to c570352. Tests are now out of date.