Add optional proton time cluster output, update printouts

[michaelmackenzie]

Optionally produce proton time clusters for proton triggering and proton tracking. Also update some printouts for debugging.

[FNALbuild]

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

• CalPatRec

which require these tests: build.

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

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

About FNALbuild. Code review on Mu2e/Offline.

[FNALbuild]

☀️ The build tests passed at 3e3e6c4.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged 3e3e6c4 at 2cc428c
build (prof)	✅	Log file. Build time: 04 min 11 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 2.
check_cmake	✅	Log file.
FIXME, TODO	➡️	TODO (0) FIXME (2) in 5 files
clang-tidy	➡️	9 errors 634 warnings
whitespace check	✅	no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 3e3e6c4 after being merged into the base branch at 2cc428c.

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.

[michaelmackenzie]

@FNALbuild run build test

[FNALbuild]

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

[FNALbuild]

☀️ The build tests passed at 3e3e6c4.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged 3e3e6c4 at 2cc428c
build (prof)	✅	Log file. Build time: 04 min 08 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 (2) in 5 files
clang-tidy	➡️	9 errors 634 warnings
whitespace check	✅	no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 3e3e6c4 after being merged into the base branch at 2cc428c.

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.Now let me look at the full context of the key files to better understand the changes:Here's my review of PR #1796. I identified several issues across the changed files:

---

Issues Found

1. 🔴 Potential null pointer dereference in countProtons (critical)

      if(_protonTCs) {
        auto tcs = _data._protonTCColl;
        TimeCluster tc;
        for (size_t j=0; j<_f.chunks[i].hIndices.size(); j++) {
          tc._strawHitIdxs.push_back(StrawHitIndex(_f.chunks[i].hIndices[j]));
        }
        tc._t0 = TrkT0(_f.chunks[i].fitter.y0(), 0.);
        const int caloIdx = _f.chunks[i].caloIndex;
        if (caloIdx != -1) tc._caloCluster = art::Ptr<mu2e::CaloCluster>(_ccHandle, caloIdx);
        tc._nsh = _f.chunks[i].nStrawHits;
        tcs->push_back(std::move(tc));
      }

The _ccHandle may not be valid when _useCaloClusters != 1. The findData method at line 259 only populates _ccHandle when _useCaloClusters == 1. If _protonTCs is true but _useCaloClusters is not enabled, and a chunk happens to have a caloIndex != -1 (which shouldn't normally happen, but is not defensively prevented), constructing art::Ptr<mu2e::CaloCluster>(_ccHandle, caloIdx) with an invalid handle would crash. While the current logic makes this unlikely since caloIndex defaults to -1 and is only set in checkCaloClusters (which is gated on _useCaloClusters == 1), a defensive guard would be safer.

2. 🟡 Unnecessary local alias auto tcs = _data._protonTCColl

        auto tcs = _data._protonTCColl;

The variable tcs is a raw pointer copy of _data._protonTCColl. This is declared inside the loop body at line 766 (for (size_t i=0; ...)), meaning a new copy of the pointer is created on every iteration. While functionally correct, it would be cleaner and more efficient to hoist this above the loop:

auto tcs = _data._protonTCColl;
for (size_t i=0; i<_f.chunks.size(); i++) {
  ...
}

3. 🟡 _data._protonTCColl is not initialized when _protonTCs is false

    _data._protonTCColl = (_protonTCs) ? protonTCColl.get() : nullptr;

This is fine in produce(), but _data._protonTCColl is never initialized in the constructor or Data_t struct definition. The Data_t struct (in TZClusterFinder_types.hh) has no default member initializers, so _protonTCColl starts as an indeterminate value. If any diagnostic plugin or tool accesses _data._protonTCColl before produce() runs, it would be undefined behavior. Consider adding a default initializer:

      TimeClusterCollection*          _protonTCColl = nullptr;

4. 🟡 Debug printouts use mixed printf and std::cout styles

The new debug output in AgnosticHelixFinder_module.cc and AgnosticHelixFinderDiagDisplay.cc uses printf:

        if(_diagLevel > 1)
          printf("  Time cluster %zu has %i helices\n", i, _diagInfo.nHelices - nHelicesInitial);

While AgnosticHelixFinderDiag_tool.cc uses std::cout:

      std::cout << "TimeCluster stage:"
                << std::endl;

Mixing I/O styles can cause interleaving issues with output buffering. The existing codebase uses both styles, but the new additions should ideally be consistent. The new std::cout messages in the diag tool also have no guard on _debugLevel — they print whenever _data->diagLevel > 1 and _display is true, which is a different condition than the _debugLevel > 1 used in the other files.

5. 🟢 Minor: _diagLevel vs _debugLevel guard inconsistency

In AgnosticHelixFinder_module.cc, the new printouts are guarded by _diagLevel > 1:

        if(_diagLevel > 1)

But in AgnosticHelixFinderDiagDisplay.cc, the new printouts are guarded by _debugLevel > 1:

    if(_debugLevel > 1) printf("  %12s: Plotting %zu helices, stage = %i\n",

These are different variables with potentially different values. Confirm this is intentional — using _debugLevel for display-related debug and _diagLevel for event-processing debug is a reasonable pattern, but it should be documented or noted for future maintainers.

---

Summary

#	Severity	File	Issue
1	🔴 Medium-High	TZClusterFinder_module.cc	_ccHandle may be invalid when building proton TC calo pointers
2	🟡 Low	TZClusterFinder_module.cc	auto tcs alias recreated inside loop on every iteration
3	🟡 Medium	TZClusterFinder_types.hh	_protonTCColl has no default initializer — UB risk
4	🟡 Low	Multiple files	Mixed printf/std::cout and inconsistent stage label guards
5	🟢 Info	Multiple files	_diagLevel vs _debugLevel guard inconsistency

The most actionable fix is issue #3 — adding = nullptr to the _protonTCColl declaration in Data_t to avoid any uninitialized pointer risk.

[FNALbuild]

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

[michaelmackenzie]

Responding to the AI comments:

1. I've added this guard, but it's not possible to have non-negative indices without enabling calo clusters and then finding one.
2. I've moved this definition out of the loop.
3. I've initialized this, but nothing tries to use these variables before the first produce call.
4. Why does it matter if std::cout and printf are both used?
5. These are different modules with different debug/diagnostic output variables and values.