Patch KKLine for KinKalGeom migration

[brownd1978]

This addresses the exception in extracted validation following the migration of KinKalGeom to GeometryService.

[FNALbuild]

Hi @brownd1978,
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 300ac74: build (Build queue - API unavailable)

About FNALbuild. Code review on Mu2e/Offline.

[FNALbuild]

☀️ The build tests passed at 300ac74.

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

N.B. These results were obtained from a build of this Pull Request at 300ac74 after being merged into the base branch at 5912724.

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]

@FNALbuild run build test

[FNALbuild]

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

[FNALbuild]

☀️ The build tests passed at fead67b.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged fead67b at 5912724
build (prof)	✅	Log file. Build time: 04 min 12 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 (19) FIXME (6) in 3 files
clang-tidy	➡️	4 errors 71 warnings
whitespace check	✅	no whitespace errors found

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

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 #1811 Review — "Patch KKLine for KinKalGeom migration"

Author: @brownd1978 · Base: Mu2e:main · State: open, mergeable (clean), draft: false
Scope: 3 files, +40 / −32, 2 commits, 5 conversation comments, 0 review comments, 0 reviews.
Reviewer requested: @rlcee · CI: build ✅ passed at fead67b.

1. PR Summary

Following the migration of KinKalGeom into GeometryService, objects of type KinKalGeom cannot be safely cached as data members at module construction time (they have to come from a GeomHandle once the geometry is available per run/event). This PR fixes the runtime exception in extracted validation by:

• Mu2eKinKal/inc/KKFit.hh — removes the cached KinKalGeom smap_ member and looks up GeomHandle<KinKalGeom> inside sampleFit().
• Mu2eKinKal/src/CentralHelixFit_module.cc — drops the cached KinKalGeom::SurfacePairCollection sample_, stores only the SurfaceIdCollection ssids_, and resolves the surface pairs from the GeomHandle in sampleFit().
• Mu2eKinKal/src/KinematicLineFit_module.cc — same idea, plus the ExtrapolateTCRV TCRV_ member is no longer constructed at module construction; instead, the inputs (maxdt_, btol_, minv_, extrapdebug_) are saved and ExtrapolateTCRV is rebuilt on every call to extrapolate().

Risk: medium. The build CI passes (3070), but there are at least two substantive (non-refactor) changes hiding inside what reads as a mechanical patch — see issues #1 and #2 below.

2. Core Changes (issues found)

🚨 Issue 1 — tcrvthick_ default changed by ~1400×, with a different comment

In KinematicLineFit_module.cc:

-    ExtrapolateTCRV TCRV_; // extrapolation predicate based on Z values
-    double tcrvthick_ = 0.1056; // st foil thickness: should come from geometry service TODO
+    double maxdt_ = 0.0, btol_ = 0.0, minv_ = 0.0;
+    SurfaceIdCollection ssids_;
+    int extrapdebug_ = false;
+    double tcrvthick_ = 150.0; // CRV sector thickness: should come from geometry service TODO

tcrvthick_ is passed straight into the KKCRVXING constructor and drives the material Xing thickness for the CRV. Old default was 0.1056 mm with the comment "st foil thickness" (clearly the wrong thing — looks like a stopping‑target foil value left over from a copy‑paste). New default is 150.0 mm with comment "CRV sector thickness".

This is almost certainly the right fix, but it is a physics‑affecting change that is not mentioned in the PR description. Please:

• Call it out in the PR body so reviewers/validation know about it.
• Confirm 150.0 mm is the intended CRV sector thickness (and not, e.g., one bar — CRV sector total thickness depends on layer count).
• Ideally open / reference a TODO issue to pull this from GeometryService since the comment still says it should.

🚨 Issue 2 — int extrapdebug_ = false;

int extrapdebug_ = false;

Type/initializer mismatch. Compiles (implicit bool→int), but it’s clearly a typo. The previous local was int debug = settings().Extrapolation()->Debug();. Use int extrapdebug_ = 0; (or make it bool if it’s really binary — but Debug() returns int).

⚠️ Issue 3 — Per‑call construction of ExtrapolateTCRV is a perf regression

void KinematicLineFit::extrapolate(KKTRK& ktrk) const {
  GeomHandle<mu2e::KinKalGeom> kkg_h;
  auto const& kkg = *kkg_h;
  // extrapolate to the front of the tracker
  auto TCRV = ExtrapolateTCRV(maxdt_,btol_,intertol_,minv_,*kkg.TCRV(),extrapdebug_);
  ...
}

extrapolate() is called per fit. Building a fresh ExtrapolateTCRV (and re‑resolving the surface pair collection in sampleFit) on every call is wasteful — KinKalGeom does not change within a run.

The conventional Mu2e pattern is to fill these in beginRun(art::Run&) (which already exists for CentralHelixFit and is the natural place for run‑scoped geometry caches). Suggested refactor: keep TCRV_ as a member, but assign it in beginRun from a GeomHandle<KinKalGeom>. Same for sample_ in all three modules.

The new comment in CentralHelixFit::sampleFit is even slightly misleading:

// This must be done every call as the KKGeom object now comes from GeometryService

It does not need to be done every call — it needs to be done after the geometry service is initialized, i.e. once per run.

⚠️ Issue 4 — Inconsistent treatment across the three files

The three callsites all have the same problem (cached objects that depend on geometry) but the patch solves it three different ways:

File	sample list	TCRV
KKFit.hh	rebuilt per call	n/a
CentralHelixFit_module.cc	rebuilt per call	n/a
KinematicLineFit_module.cc	rebuilt per call	rebuilt per call

Worth aligning on one pattern (preferably "cache in beginRun"), even if as a follow‑up.

ℹ️ Issue 5 — New include may be unused

#include "Offline/Mu2eKinKal/inc/KKShellXing.hh"

This include is added but I don’t see a new symbol from KKShellXing referenced in the diff (KKCRVXING/KKCRVXINGPTR were already in use before). Please confirm it isn’t already pulled in transitively — if so, drop it.

ℹ️ Issue 6 — Pre‑existing typo retained, plus stray blank line

• KKFit.hh keeps the typo // surfaces to sample; this interface is deprecatecd (was already there — easy to fix while editing the line).
• KinematicLineFit_module.cc leaves a stray double blank line where the ExtrapolateTCRV block was removed.

ℹ️ Issue 7 — clang‑tidy delta

The CI table on the latest build (fead67b) reports 4 errors / 71 warnings and TODO (19) FIXME (6) in 3 files, up from 2 errors / 20 warnings and TODO (2) FIXME (1) in 1 files on the first push. Most of this is likely because the second push touches more files, but worth a glance at the clang‑tidy log to confirm none of the new diagnostics are introduced by this PR.

3. Merge Readiness

Item	Status
Mergeable	✅ clean
Build CI	✅ passed (job 3070)
Reviews	⏳ none yet (review requested from @rlcee)
Validation	The PR description says it fixes an exception in extracted validation — please post the validation/extracted‑run output as evidence before merge
Blocking issues	None compile‑level; Issue 1 (physics constant change) and Issue 2 (typo) should be addressed first

4. Suggested Improvements

1. Update the PR description to explicitly call out the tcrvthick_ value/semantics change.
2. Fix int extrapdebug_ = false; → int extrapdebug_ = 0;.
3. Move the GeomHandle<KinKalGeom> lookup, sample_ resolution, and ExtrapolateTCRV construction into beginRun() to avoid per‑track work.
4. Drop the (apparently) unused KKShellXing.hh include.
5. Fix the pre‑existing deprecatecd typo in KKFit.hh while you’re there.
6. Align the three modules on one caching pattern in a follow‑up.

5. Want me to…

1. Post these review comments inline on the PR (you’d need to confirm the file/line targets).
2. Open a small follow‑up issue tracking "cache KinKalGeom‑derived objects in beginRun" across Mu2eKinKal?
3. Pull and read the clang‑tidy log to confirm whether any new diagnostics are introduced by this PR?
4. Look up the actual CRV sector thickness from GeometryService to verify 150.0 mm is correct?

[FNALbuild]

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

[brownd1978]

Addressed AI issues:
1- original is correct
2- fixed
3- fixed
4-fixed
5- original is correct
6-clang-tidy errors are inside art, warnings are incorrect/irrelevant

[oksuzian]

@FNALbuild run build test

[FNALbuild]

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

[FNALbuild]

☀️ The build tests passed at 3d9133d.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged 3d9133d at b38c2bf
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 (21) FIXME (5) in 3 files
clang-tidy	➡️	4 errors 72 warnings
whitespace check	✅	no whitespace errors found

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

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.