Skip to content

Calo Surface Steps#1757

Open
sophiemiddleton wants to merge 30 commits intoMu2e:mainfrom
sophiemiddleton:interdet
Open

Calo Surface Steps#1757
sophiemiddleton wants to merge 30 commits intoMu2e:mainfrom
sophiemiddleton:interdet

Conversation

@sophiemiddleton
Copy link
Copy Markdown
Contributor

This pull request introduces support for calorimeter (Calo) surfaces in the geometry and extrapolation infrastructure, enabling track extrapolation to the calorimeter disks. The main changes include defining Calo surfaces, integrating them into surface mapping, and adding extrapolation logic to reach Calo disks. Additionally, configuration options are updated to allow extrapolation to Calo disks.

Calorimeter surface integration:

  • Added Calo class to define the geometry of calorimeter disks and edges, including inner/outer cylinders and front/back disks for both disk 0 and disk 1 (KinKalGeom/inc/Calo.hh, KinKalGeom/src/Calo.cc). [1] [2]
  • Integrated Calo surfaces into the SurfaceMap class and surface mapping logic, allowing retrieval and mapping of Calo surfaces (KinKalGeom/inc/SurfaceMap.hh, KinKalGeom/src/SurfaceMap.cc). [1] [2] [3]

Extrapolation enhancements:

  • Added ExtrapolateCalo class to handle extrapolation predicates and logic for reaching Calo disks, including methods for finding nearest disks and intersection checks (Mu2eKinKal/inc/ExtrapolateCalo.hh).
  • Updated KKExtrap class to include extrapolation methods for Calo disks and associated member variables for Calo disk surfaces and predicates (Mu2eKinKal/inc/KKExtrap.hh). [1] [2] [3]

Configuration updates:

  • Added new configuration flags (ToCaloD0, ToCaloD1) in the FCL files to control extrapolation to Calo disks (Mu2eKinKal/fcl/prolog.fcl). [1] [2] [3]

Surface ID and mapping updates:

  • Extended SurfaceId enumeration and mapping to include Calo disk surfaces, and updated string mappings for these IDs (DataProducts/inc/SurfaceId.hh, DataProducts/src/SurfaceId.cc). [1] [2]
  • Updated MakeSurfaceSteps_module.cc to map new Calo surface IDs and add debug output for Calo-related steps (CommonMC/src/MakeSurfaceSteps_module.cc). [1] [2]

Build system update:

  • Added Calo.cc to the build system for compilation (KinKalGeom/CMakeLists.txt).

@sophiemiddleton sophiemiddleton marked this pull request as draft March 12, 2026 18:13
@FNALbuild
Copy link
Copy Markdown
Collaborator

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

  • DataProducts
  • KinKalGeom
  • Mu2eKinKal
  • CommonMC
  • TrkReco

which require these tests: build.

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

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

About FNALbuild. Code review on Mu2e/Offline.

@sophiemiddleton
Copy link
Copy Markdown
Contributor Author

this is an initial draft with most of the work. I have a small issues to fix before it becomes a full PR

@FNALbuild
Copy link
Copy Markdown
Collaborator

☀️ The build tests passed at 2610df8.

Test Result Details
test with Command did not list any other PRs to include
merge Merged 2610df8 at 6a994e7
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. Return Code 1.
check_cmake Log file.
FIXME, TODO ➡️ TODO (4) FIXME (5) in 10 files
clang-tidy ➡️ 2 errors 48 warnings
whitespace check no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 2610df8 after being merged into the base branch at 6a994e7.

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.

Comment thread KinKalGeom/src/Calo.cc Outdated
@sophiemiddleton
Copy link
Copy Markdown
Contributor Author

I merged the HEAD and fixed a few conflicts

Copy link
Copy Markdown
Collaborator

@brownd1978 brownd1978 left a comment

Choose a reason for hiding this comment

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

Overall this looks really good. Thanks for adding extrapolation to KKLIne and CentralHelix fits. I just have a question about how multiple intersections are handled.

The current calo geom only describes VD surfaces, so extrapolation doesn't do any material corrections. In future it would be nice to add materials in front of the crystals like the CF and supports and the calibration system.

// update the cache
inter_ = newinter;
ann_ = disks_[idisk];
//sid_ = SurfaceId(SurfaceIdEnum::calo_Foils,idisk); //FIXME
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.

Is sid_ left out intentionally?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes, I agree, adding in the material corrections would be a good next step. I'll take a look and see what I can figure out as a future task here.

Comment thread TrkReco/fcl/prolog.fcl Outdated
#
# may use a more careful optimization, but this is it for now
#------------------------------------------------------------------------------
InterdetectorTimeBootstrap : {
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.

Does this correspond to code not in this PR?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah, this is old stuff. I had a module for the time calibration, it was removed, ill remove this comment too

@sophiemiddleton
Copy link
Copy Markdown
Contributor Author

Is there anything else needed for this PR?

Copy link
Copy Markdown
Collaborator

@brownd1978 brownd1978 left a comment

Choose a reason for hiding this comment

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

Could you please say a few words about this work at the next reco meeting? thanks.

@oksuzian
Copy link
Copy Markdown
Collaborator

oksuzian commented May 5, 2026

PR #1757 Review: "Calo Surface Steps"

Summary

Status: ✅ Approved by brownd1978 on 2026-05-04, mergeable but mergeable_state: unstable (build pending label). +558/−62 across 16 files. 29 commits.

Adds calorimeter (EMC) disk surfaces to the KinKalGeom infrastructure and enables track extrapolation to the two calo disks (D0 / D1), exposing front/back disks and inner/outer cylinders for surface mapping, plus FCL toggles ToCaloD0 / ToCaloD1. Also refactors KKExtrap and consolidates KKExtrapConfig into KKFitSettings.hh, and wires the extrapolator into CentralHelixFit_module and KinematicLineFit_module.


Issues Found

🔴 Critical / Likely Bugs

1. Duplicate ToTrackerEnds key in LINEEXTRAPOLATION FCL block — this is invalid FHiCL and will at best be ignored, at worst error.

LINEEXTRAPOLATION : {
  MaxDt : 200.0
  BCorrTolerance : 1e-5
  ToTrackerEnds : true        # <-- duplicate
  Upstream : true
  BackToTracker : false
  ToTrackerEnds : false       # <-- duplicate (overrides above)
  ToOPA : false
  ToCaloD0 : true
  ToCaloD1 : true
  IntersectionTolerance : 0.1
}

Also note: this block lost ToCRV : true and MinV that previously existed. If LINEEXTRAPOLATION is consumed via KKExtrapConfig (which has BCorrTolerance, IntersectionTolerance, etc. — verify the actual atom names — there's no BCorrTolerance atom shown in KKFitSettings.hh; only BCorrTol/InterTol/MaxDt). The keys here likely don't match the FHiCL atom names in KKExtrapConfig — please double-check BCorrTolerance vs BCorrTol and IntersectionTolerance vs InterTol against KKFitSettings.hh.

2. Massive use of const_cast for "lazy initialization" in KKExtrap::initGeometry — this is a const-correctness anti-pattern and is undefined behavior if any of the cast members were originally declared const. The clean fix is to declare the members mutable (the predicate members are already mutable; just mark the pointer members and geom_initialized_ mutable) and drop every const_cast.

const_cast<AnnPtr&>(tsdaptr_) = kkg.DS().upstreamAbsorberPtr();
const_cast<DiskPtr&>(trkfrontptr_) = kkg.tracker().frontPtr();
...
const_cast<bool&>(geom_initialized_) = true;

3. ExtrapolateCalo's disks_ vector is never populated — the constructor does disks_(2) (two default-constructed null AnnPtr) and nothing ever fills it. Any call to needsExtrapolation that reaches the disk loop dereferences null shared_ptrs (disks_[idisk]->center().Z()). The class also caches an inter_/ann_/sid_ but sid_ is left with the FIXME //sid_ = SurfaceId(SurfaceIdEnum::calo_Foils,idisk); //FIXMEbrownd1978 already flagged this. As-is the class appears non-functional; in practice it isn't actually used by KKExtrap::toCaloD0/D1 (which use ExtrapolateToZ instead), so consider removing ExtrapolateCalo.hh entirely from this PR or finishing it.

4. Whitespace / brace issues in MakeSurfaceSteps_module.cc — inconsistent indentation, the closing brace } is column-0 instead of inside the namespace indent, and there are stray leading spaces on the EdgeIn/EdgeOut lines:

      vdmap_[VirtualDetectorId(VirtualDetectorId::EMC_Disk_0_EdgeIn)] = SurfaceId("EMC_Disk_0_Inner");
       vdmap_[VirtualDetectorId(VirtualDetectorId::EMC_Disk_1_EdgeIn)] = SurfaceId("EMC_Disk_1_Inner");
       vdmap_[VirtualDetectorId(VirtualDetectorId::EMC_Disk_0_EdgeOut)] = SurfaceId("EMC_Disk_0_Outer");
       vdmap_[VirtualDetectorId(VirtualDetectorId::EMC_Disk_1_EdgeOut)] = SurfaceId("EMC_Disk_1_Outer");
}

🟡 Medium

5. Debug std::cout left in hot loopbrownd1978 asked it to be guarded; it now is (if(debug_>0)), but debug_ is hard-coded to 0 (the variable is never set from FHiCL in this module). Effectively dead code unless wired up; either expose Debug as an atom or drop it.

6. KinKalGeom::SurfacePairCollection surfaces_to_sample_ is filled in the constructor and again in beginRun in CentralHelixFit_module.cc. The constructor-time call uses a local KinKalGeom smap (no GeometryService) — it cannot resolve calo surfaces (the GeometryService isn't initialized), so its result is meaningless and is overwritten in beginRun. Drop the constructor-time smap.surfaces(...).

KinKalGeom smap;
smap.surfaces(ssids_,surfaces_to_sample_);  // <-- dead/incorrect; overwritten in beginRun

7. KKExtrap::extrapolate initializes only trackerFront locally but initGeometry already builds trackerFront_ member — duplicated logic. Same in extrapolateIPA/extrapolateST/extrapolateTracker/extrapolateTSDA/toOPA which all rebuild local ExtrapolateToZ/ExtrapolateIPA/ExtrapolateST objects, making the lazy-initialized members TSDA_, trackerBack_, extrapIPA_, extrapST_ unused. Either use the cached members or drop them.

8. KKExtrap header pulls GeomHandle<KinKalGeom> inside templated functions — every toCaloD0/D1, extrapolateIPA, etc. re-fetches GeomHandle even though initGeometry already cached pointers. Minor performance issue and clutter; prefer calling members.

9. nearestDisk returns size_t but is then compared as int idisk = nearestDisk(...) and against (int)disks_.size() — sign mismatch warnings + the function name zdir parameter is documented but the implementation calls it zvel. Cosmetic.

10. Calo.hh uses namespace KKGeom but Calo.cc original (per review comment) used KinKalGeom — verify all callers use one consistent namespace alias. The current Calo.cc does use KKGeom ✅, but ExtrapolateCalo.hh writes using KKGeom::Calo; — fine.

🟢 Minor / Style

  • SurfaceId.hh stylistic regression: removed a space (return sid_.name(); }return sid_.name();}) for no reason.
  • TrkReco/fcl/prolog.fcl — only change is an inserted blank line. The leftover dead code that prompted brownd1978's question was reportedly removed but the diff still shows a stray blank-line edit; confirm intentional.
  • Hard-coded disks_(2) assumes exactly 2 disks. Should be derived from Calo or calo_det.nDisk().
  • KinKalGeom.hh accessors changed from returning unique_ptr-by-ref to dereferenced object-by-ref — this is a breaking API change for any other caller; ensure no external code outside this PR still does kkg.tracker()->front(). (Diff already updated KKExtrap.hh; check the rest of the tree.)
  • KKFitSettings.hh comment says "Extrapolate tracks to the disk0 ends" — clearer wording: "Extrapolate to EMC disk 0 front/back surfaces".
  • The geom_initialized_ initializer is set both in the in-class default = false and in the ctor init list — redundant.
  • 4 TODOs and 5 FIXMEs reported by FNALbuild; clang-tidy reports 2 errors / 48 warnings. Worth a look before merge.
  • Approver requested a presentation at the next reco meeting — non-blocking.

Merge Readiness

Want me to

  1. Open follow-up review comments / suggested-change blocks on the most critical items (Update README.md #1merge with my branch trackerPatRec #4) directly on the PR diff.
  2. Verify whether BCorrTolerance/IntersectionTolerance keys match the FHiCL atom names in KKExtrapConfig by reading KKFitSettings.hh head version.
  3. Search for any remaining callers of kkg.tracker()->front() style (old unique_ptr accessor) elsewhere in the repo to confirm the API change is fully propagated.
  4. Draft a PR with the cleanup (remove const_casts, fix FCL duplicate, drop dead ExtrapolateCalo or finish it).

@oksuzian
Copy link
Copy Markdown
Collaborator

oksuzian commented May 5, 2026

@FNALbuild run build test

@FNALbuild
Copy link
Copy Markdown
Collaborator

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

@FNALbuild
Copy link
Copy Markdown
Collaborator

☀️ The build tests passed at 9b47730.

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

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

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

oksuzian commented May 6, 2026

@FNALbuild run build test

@FNALbuild
Copy link
Copy Markdown
Collaborator

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

@FNALbuild
Copy link
Copy Markdown
Collaborator

☀️ The build tests passed at f7baeaf.

Test Result Details
test with Command did not list any other PRs to include
merge Merged f7baeaf at 1d79377
build (prof) Log file. Build time: 04 min 10 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 (11) FIXME (6) in 13 files
clang-tidy ➡️ 8 errors 154 warnings
whitespace check no whitespace errors found

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

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

oksuzian commented May 6, 2026

PR #1757 Review — "Calo Surface Steps"

Author: sophiemiddleton · State: open, mergeable (clean) · Approved by: brownd1978 (2026‑05‑04)
Scope: 16 files, +557 / −62, 30 commits · Labels: build finished

Summary

Adds calorimeter (EMC) disk surfaces to the KinKal geometry/surface infrastructure and extends KKExtrap to extrapolate fits to D0 / D1 front, back, inner and outer surfaces. Wires new ToCaloD0 / ToCaloD1 flags into KKFitSettings, prolog.fcl, and the line/central‑helix fit modules. Surface IDs (EMC_Disk_*) are added to DataProducts/SurfaceId and mapped from VDs in MakeSurfaceSteps_module.

The PR has been approved, but it still contains several substantive issues worth addressing before merge — listed below in priority order.


🔴 Issues found

1. ExtrapolateCalo is dead / broken code

Mu2eKinKal/inc/ExtrapolateCalo.hh is added (160 lines) but never instantiated anywhere in the PR — KKExtrap uses ExtrapolateToZ for the calo predicates instead. Inside the class:

  • disks_ is constructed as disks_(2) — i.e. a vector of two null shared_ptr<Annulus>. Nothing ever fills it. Both needsExtrapolation() and nearestDisk() dereference disks_[idisk]->center()null deref / UB the moment this class is actually used.
  • sid_ is declared and reset()s it, but it is never set to a meaningful value because the only assignment is commented out: //sid_ = SurfaceId(SurfaceIdEnum::calo_Foils,idisk); //FIXME — Brian explicitly asked about this (discussion r3156375344) and the response was "future task" — but the unresolved FIXME remains in merged code.
  • The header pollutes namespace mu2e with global using aliases (AnnPtr, CaloDisk, CylPtr).

Recommendation: either remove ExtrapolateCalo.hh from this PR (and add it later when wired up), or finish wiring it (populate disks_ from the Calo object, set sid_, use it in KKExtrap).

2. Dead MinV / ToCRV fields in KinematicLineFit_module.cc

prolog.fcl removed MinV and ToCRV from LINEEXTRAPOLATION, and the local KKExtrapConfig struct that previously declared them was deleted in favor of the shared Mu2eKinKal::KKExtrapConfig (which has neither). However the module still has:

bool extrapolate_, toCRV_;
double maxdt_ = 0.0, btol_ = 0.0, minv_ = 0.0;
...
auto TCRV = ExtrapolateTCRV(maxdt_,btol_,intertol_,minv_,kkg.TCRV(),extrapdebug_);

minv_ is never assigned anywhere now, so CRV extrapolation always runs with MinV = 0.0 — this silently changes behavior compared to the previous default. Likewise toCRV_ is never set, so the field is effectively whatever its (uninitialized) default is. Either restore these as proper config atoms in KKExtrapConfig or hard‑code the intended value.

3. Heavy const_cast abuse in KKExtrap::initGeometry()

const_cast<AnnPtr&>(tsdaptr_)        = kkg.DS().upstreamAbsorberPtr();
const_cast<DiskPtr&>(trkfrontptr_)   = kkg.tracker().frontPtr();
...
const_cast<ExtrapolateToZ&>(TSDA_)   = ExtrapolateToZ(...);
...
const_cast<bool&>(geom_initialized_) = true;

Several problems:

  • geom_initialized_, TSDA_, trackerFront_, trackerBack_, extrapIPA_, extrapST_, calod0Front_, … are all already declared mutable, so const_cast on them is gratuitous — just assign directly.
  • The pointer members (tsdaptr_, trkfrontptr_, opaptr_, calod0frontptr_, …) are not mutable — using const_cast to write to them in a const method is undefined behavior if the object was originally constructed const. Make them mutable, or do the initialization in the constructor (the geometry handle is available there too if beginRun has run; if not, do it in beginRun and drop the lazy init entirely).
  • Lazy init through const/mutable is a thread‑safety hazard if extrapolate() is ever called concurrently on the same instance.

4. Member predicates initialized but then shadowed by locals

After initGeometry() populates trackerFront_, trackerBack_, TSDA_, extrapIPA_, extrapST_, every method that uses them re‑creates a local copy and uses that instead:

ExtrapolateToZ trackerFront(maxdt_,btol_,kkg.tracker().front().center().Z(),debug_);
...
ExtrapolateIPA extrapIPA(maxdt_,btol_,intertol_,kkg.DS().innerProtonAbsorberPtr(),debug_);
...
ExtrapolateST extrapST(maxdt_,btol_,intertol_,kkg.ST(),debug_);

So the member-variable initialization in initGeometry() is dead code for those predicates. Pick one approach: either use the cached members (preferred, cheaper) or drop them. Currently both exist and obscure the intent.

5. Possible logic bug in toCaloD0 / toCaloD1 time direction

The code copies the time‑direction logic from toTrackerEnds:

TimeDir fronttdir = (dir0.Z() > 0) ? TimeDir::backwards : TimeDir::forwards;
TimeDir backtdir  = (dir0.Z() > 0) ? TimeDir::forwards  : TimeDir::backwards;

That logic is correct for the tracker because the fit brackets the tracker (front upstream of fit start, back downstream of fit end). For the calorimeter, both disks are downstream of the tracker, so for a downstream electron (dir0.Z() > 0) reaching D0_Front and D0_Back both require forward time extrapolation, not one of each. As written, fronttdir = backwards will extrapolate the fit upstream (away from the calo) before searching for the front intersection — at best wasted work, at worst a missed intersection. Please re‑examine; you likely want both fronttdir and backtdir to be forwards (downstream tracks) or both backwards (upstream tracks).

6. Calo Z values: predicate vs surface center mismatch

The PR includes debug output that is itself a flag:

std::cout << "  Disk face Z values (from surface centers): front=" << kkg.calo().EMC_Disk_0_Front().center().Z()
          << " back=" << kkg.calo().EMC_Disk_0_Back().center().Z() << std::endl;
std::cout << "  Predicate Z values: front=" << kkg.calo().EMC_Disk_0_Front_Z()
          << " back=" << kkg.calo().EMC_Disk_0_Back_Z() << std::endl;

The EMC_Disk_0_Front_Z() accessor and the front Disk surface are both built from z0_front, so by construction these should agree — keeping a separate z0_front_ cache is duplicate state that will silently desync if Calo.cc is ever changed. Drop the z*_front_/z*_back_ doubles and read them from the surface (e.g. EMC_Disk_0_Front_->center().Z()).

7. prolog.fcl LINEEXTRAPOLATION block has IntersectionTolerance which doesn't exist in the schema

LINEEXTRAPOLATION : {
  MaxDt : 200.0
  BCorrTolerance : 1e-5
  ...
  IntersectionTolerance : 0.1 #  tolerance for intersections (mm)
}

The Mu2eKinKal::KKExtrapConfig struct in KKFitSettings.hh has neither BCorrTolerance nor IntersectionTolerance — those names don't match any fhicl::Atom declared there. This block will fail FHiCL validation as soon as anything tries to use it. (Compare with the working XTRAP/LHSEEDXTRAP blocks which use BTol/InterTol.)

8. Stray debug, indentation, and minor cleanups

  • MakeSurfaceSteps_module.cc: closing } introduced at column 0 (was indented). Cosmetic but inconsistent with the rest of the function.
  • DataProducts/inc/SurfaceId.hh line 41 changed name(); }name();} — drop the no‑op whitespace change.
  • TrkReco/fcl/prolog.fcl adds a blank line that is unrelated to this PR (Brian flagged the surrounding hunk in r3171314553; leftover from the deleted InterdetectorTimeBootstrap block).
  • The previous std::cout << " VID ..." debug was correctly gated behind debug_ > 0 per Brian's request — good. But it's still bare std::cout; consider mf::LogInfo/LogDebug for consistency with the rest of the codebase.

9. Calo geometry only describes VD surfaces (no material)

Already acknowledged by author and reviewer (Brian's r4191458736). Worth tracking as a follow‑up issue — extrapolation through the calorimeter currently performs no material corrections (CF, supports, calibration system).


✅ What's good

  • Clean separation: KinKalGeom/Calo.{hh,cc} mirrors the existing Tracker/DS/StoppingTarget pattern; SurfaceId enum follows existing numbering.
  • Good comment in KinKalGeomMaker.cc explaining why using KinKal::Disk is dropped (collision with mu2e::Disk from CalorimeterGeom).
  • The KinKalGeom accessors were correctly switched from unique_ptr& to dereferenced refs (return *ds_, *tracker_, …) — that's a nice clean‑up that cascades through KKExtrap.hh.
  • Consolidating the duplicated KKExtrapConfig definition (was local to KinematicLineFit_module) into the shared Mu2eKinKal::KKExtrapConfig is the right direction.

Merge readiness

Approved, build‑green, mergeable. I'd still recommend addressing (in roughly this order before merge):

  1. Add diagnostics #2minv_/toCRV_ regression in KinematicLineFit_module (correctness)
  2. Add support for validation #7 — invalid FHiCL keys in LINEEXTRAPOLATION block (will fail at runtime)
  3. Added --as-needed flag to the link. #5 — verify toCaloD0/D1 time‑direction logic
  4. Update README.md #1 — either delete ExtrapolateCalo.hh or finish wiring it (it's currently a foot‑gun)
  5. Notcurrent fix #3 / merge with my branch trackerPatRec #4 — clean up const_cast and dead member predicates in KKExtrap.hh

The rest (#6, #8, #9) can be follow‑ups.


Want me to

  1. Open a draft fix‑up PR addressing items Add diagnostics #2 and Add support for validation #7 (the runtime regressions).
  2. Trace where minv_/toCRV_ are used elsewhere in the codebase to confirm the regression scope.
  3. Produce a side‑by‑side diff focused only on KKExtrap.hh showing how the const_cast/lazy‑init could be simplified.
  4. Summarize the unresolved review threads (Brian's sid_ question, the //FIXME) into a follow‑up issue.

@FNALbuild
Copy link
Copy Markdown
Collaborator

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

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.

5 participants