Added diagnostic kinkal strawhitupdater

[bonventre]

Simple SHU that allows you to force all hits from a single panel or plane to be inactive in the kinkal fits. If done by panel status instead no hits would be created, this allows information from that panel to still make it into EventNtuple for alignment diagnostics etc.

[FNALbuild]

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

• Mu2eKinKal

which require these tests: build.

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

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

About FNALbuild. Code review on Mu2e/Offline.

[FNALbuild]

☔ The build tests failed for 6cf7f2d.

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

N.B. These results were obtained from a build of this Pull Request at 6cf7f2d after being merged into the base branch at 95f0a83.

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.

[brownd1978]

Looks good. you just need to add the new class to the CMakeList.

[brownd1978]

@FNALbuild run build test

[FNALbuild]

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

[FNALbuild]

☀️ The build tests passed at 0a543f6.

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

N.B. These results were obtained from a build of this Pull Request at 0a543f6 after being merged into the base branch at 23fc715.

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 #1825 Review: Diagnostic kinkal strawhitupdater

Summary

• Author: @bonventre · State: open · Mergeable: clean
• Scope: 8 files, +75/-3 · Reviews: 1 approval (@brownd1978)
• CI: ✅ Build passes (commit 0a543f6)
• Purpose: Adds PanelDiagSHU — a diagnostic StrawHitUpdater that forces all hits in a chosen panel/plane to be inactive in KinKal fits. Unlike disabling via panel status, hits are still produced so they appear in EventNtuple for alignment diagnostics.

Core Changes

1. New class PanelDiagSHU (header + impl) — uses (StrawIdMask, StrawId, diag_level) config. If the hit's StrawId matches the masked target id, sets state_=inactive, algo_=PanelDiag, frozen_=true.
2. Wired into the updater enum (PanelDiag=9) and names_ table.
3. Hooked into KKFitSettings.{hh,cc} and KKStrawHit.hh updater dispatch.
4. Added to Mu2eKinKal/CMakeLists.txt (in response to @brownd1978's review).

---

🔴 Critical / Notable Issues

1. Deactivation only fires when ca.usable() is true

In Mu2eKinKal/inc/KKStrawHit.hh:

CA ca = unbiasedClosestApproach();
if(ca.usable()){
  auto dinfo = fillDriftInfo(ca);
  if(diagshu)whstate_ = diagshu->wireHitState(whstate_,straw_.id());
  if(cashu) ...
}

PanelDiagSHU::wireHitState only depends on the StrawId — not on closest approach. Burying it inside if(ca.usable()) means panels won't be deactivated for hits where CA is unusable. For an alignment-diagnostic tool whose entire job is "force this panel's hits inactive regardless," this is a behavioral hole. Consider moving the diagshu invocation outside the ca.usable() block (or the entire branch) so deactivation is unconditional.

2. npdiag advance logic is inconsistent with sibling updaters

In Mu2eKinKal/src/KKFitSettings.cc:

} else if(alg == StrawHitUpdaters::PanelDiag) {
  miconfig.addUpdater(std::any(PanelDiagSHU(paneldiagshusettings.at(npdiag))));
  if (paneldiagshusettings.size()>npdiag+1)npdiag++;
}

All other updaters use at(n++) (one config per meta-iteration). Here, the index is "sticky" at the last config, so a single config is reused for every meta-iteration. This may be intentional (you usually want the same panel forced off across all iterations), but:

• It silently diverges from the established pattern.
• There's no comment explaining why.
• If the user does supply N configs for N meta-iterations, the last one will still be reused for any further iterations rather than throwing.

Either add a brief comment justifying the sticky behavior, or unify with the at(npdiag++) pattern.

3. Minor: diagnostic std::cout in production code

PanelDiagSHU.cc prints to std::cout when diag_>0. The codebase typically uses MF logging; raw cout from a fit-time updater could clutter logs during long jobs. Low priority since gated on diag_.

4. Minor: missing default in config description

configDescription() returns "StrawId mask, StrawId to set inactive, diag level" — no documentation of valid StrawIdMask level names (e.g. "plane", "panel", "station"). Worth a one-liner so users of the FHiCL knob aren't guessing.

---

Merge Readiness

• ✅ CI green, ✅ approved, ✅ CMake fix applied, ✅ no whitespace/check_cmake errors on latest commit.
• ⚠️ Issue Update README.md #1 (CA-gated deactivation) is the only one I'd consider a real correctness concern — worth confirming with the author whether it's intentional before merging.

[FNALbuild]

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

[oksuzian]

@FNALbuild run build test

[FNALbuild]

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

[bonventre]

Looking at the AI review, I'd say 1 and 2 are intended behavior. If ca is unusable the hit will correctly get marked as unusable instead of just inactive, and since it is a diagnostic, it is assumed that you will want the same panel disabled in every iteration.

[FNALbuild]

☀️ The build tests passed at 0a543f6.

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

N.B. These results were obtained from a build of this Pull Request at 0a543f6 after being merged into the base branch at 9c0a491.

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.