Skip to content

Implement passive material crossings in KinKal track extrapolation#1883

Open
RobMina wants to merge 5 commits into
Mu2e:mainfrom
RobMina:crv-trk2crv-extrap
Open

Implement passive material crossings in KinKal track extrapolation#1883
RobMina wants to merge 5 commits into
Mu2e:mainfrom
RobMina:crv-trk2crv-extrap

Conversation

@RobMina

@RobMina RobMina commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Add DS and concrete crossings during KinKal track extrapolation to the CRV (material was defined in the earlier PR #1868). Passive x'ings can optionally be stored in the KalSeed (off by default).

NB: to run stably, this still requires setting bfcorr=false (the current default) for CentralHelix fit/extrapolation. Running bfcorr=true CentralHelix requires upstream stability improvements in KinKal.

@RobMina RobMina requested a review from brownd1978 July 11, 2026 22:11
@FNALbuild

Copy link
Copy Markdown
Collaborator

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

  • Mu2eKinKal

which require these tests: build.

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

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

About FNALbuild. Code review on Mu2e/Offline.

@FNALbuild

Copy link
Copy Markdown
Collaborator

☀️ The build tests passed at ca33b1a.

Test Result Details
test with Command did not list any other PRs to include
merge Merged ca33b1a at 1d2690a
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 (19) FIXME (8) in 11 files
clang-tidy ➡️ 0 errors 1 warnings
whitespace check no whitespace errors found

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

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

Copy link
Copy Markdown
Collaborator

Issues found

1. KKBField.cc — non-zero placeholder field is a silent physics hack (medium)

static const VEC3 nullfield(0.0,0.0,1.0e-3);

Returning a fabricated 1e-3 axial field outside all maps avoids the omega→NaN singularity, but every out-of-map query now silently gets a fake field. The removed fieldDeriv range-check (throw "Position out of range") means a genuinely broken position also passes silently. Consider gating this behind bfcorr or logging once, and confirm 1e-3 T is negligible for all fit types, not just CentralHelix.

2. extrapolatePassiveMaterialbreak after first add is O(N²) and order-fragile (low/medium)
The concrete-plane loop adds one plane then breaks and re-extrapolates, relying on extrapPlanes.intersections() being re-computed each pass. Combined with the processed-set + do/while this works, but is subtle. The parallel extrapolateDSMaterial uses a different pattern (bank-all-sorted, newbank flag). Two different idioms for the same problem invites future divergence — worth a comment cross-referencing why they differ, or unifying.

3. Magic numbers / TODOs left as literals (low)

  • MinVLong : 265.0 mm/ns hardcoded in two places in prolog.fcl and defaulted in KKFitSettings.hh — 265 ≈ 0.88c; the derivation should be documented, not just inline.
  • Pre-existing ipathick_ = 0.511, stthick_ = 0.1056 still carry "should come from geometry service TODO". Not new, but now surrounded by new material code that does pull thickness from geometry — inconsistency worth flagging.
  • minShellCosine_ = 0.1 in ExtrapolateCylinders is an unexplained tangency cutoff.

4. Stability caveat is a real blocker for enabling by default (medium)
Per the description, this requires bfcorr=false for CentralHelix; bfcorr=true NaNs without upstream KinKal fixes. The KKBField placeholder papers over the symptom rather than fixing root cause. This should be tracked so the workaround isn't forgotten once KinKal is fixed.

5. Member-init reordering in KKTrack.hh copy ctor (verify)
inters_/calohits_ were reordered in both the init list and declaration order. Looks intentional (fixing a -Wreorder warning) and consistent, but double-check no other ctor depends on the old order.

@FNALbuild

Copy link
Copy Markdown
Collaborator

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

@brownd1978 brownd1978 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Rob, this is great work, thanks. It will take me some time to go through the details. I have a few general comments/questions:

  • Did you try using 'git mv' to rename the CRV extraplation function file? That generally keeps the commit history and line-by-line provenance
  • The addition of ExtrapolateToR is logical. I wonder if that could be consolidated with ExtrapolateToZ by adding a closed cylinder surface with corresponding intersection function in KinKal.
  • The Tracker perimeter is now being defined in several ways. Those should be consolidated, with values coming from GeometryService, along with a generalized toTrackerPerimeter function.
  • Hard-coding a fixed field outside the range is fine for testing, but the underlying problem(s) needs to be addressed at its root.

@RobMina

RobMina commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Hi Dave,

Thanks for the review comments.

The ExtrapolateCRV file changed >50% of the lines, so git registered it as a remove+add rather than a rename.

I updated the tracker perimeter limits by reading the values from GeometryService and dropping the unnecessary fcl parameters. ExtrapolateToZ is still needed because it is used for the the tracker front plane by extrapolateTracker and for the TSDA by extrapolateTSDA (two different z planes that do not match the logic of the tracker perimeter).

Reverted KKBField.cc -- the edits there were not needed for this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants