P104 for root 6.38 (update)

[rlcee]

A new attempt at root 6.38. I believe this version has both libGeom and ColorPicker code.

[FNALbuild]

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

• /
• DAQ
• CosmicReco

which require these tests: build.

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

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

About FNALbuild. Code review on Mu2e/Offline.

[FNALbuild]

☀️ The build tests passed at 2b317c6.

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

N.B. These results were obtained from a build of this Pull Request at 2b317c6 after being merged into the base branch at 0294ed5.

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]

Thanks for setting this up Ray. I'll ask Namitha to test it on Monday.

[oksuzian]

PR Review Summary — #1901 "P104 for root 6.38 (update)"

Reviewed at head 2b317c67

Decision

• 🟡 comment only

Scope understood

• .muse: ENVSET p103 → p104, i.e. the recommended build platform moves from ROOT 6.32.06 to ROOT 6.38.00.
• CosmicReco/src/MinuitDriftFitter.cc: port off the MnApplication parameter facade that ROOT 6.38 removed — limits/fix moved onto MnUserParameters before the MnMigrad is built, and SetPrecision/Fix/Release re-routed through migrad.State().
• DAQ/src/EventHeaderFromCFOFragment_module.cc: one whitespace-only line (a literal tab replaced by spaces).

Findings

1. 🟠 [S1] The envset bump carries more than ROOT 6.38, and two readiness items raised on the earlier attempt are not evidenced as closed

   • Evidence: /cvmfs/mu2e.opensciencegrid.org/DataFiles/Muse/p103 vs p104 differ in three exports, not one — ROOT_VERSION v6_32_06 → v6_38_00, MUSE_PYTHON p3915 → p31213 with PYTHON_LIBDIR python3.9 → python3.12, and FHICLCPP_VERSION v4_19_00 → v4_19_02. On the earlier attempt (#1859, same two files, still open as a draft) the stated open items were "EventDisplay requires changes to be compatible with root 6.38" (PR body) and "The one action item you need to get done before merging is to make sure online is prepared. Also for python@3.12.13" (your own comment, 2026-06-16). I found no EventDisplay/REve commit or PR through 2026-08-04 that addresses ROOT 6.38, and this PR's description mentions only ROOT.
   • Impact: .muse ENVSET is the platform default for everything built on top of Offline, including a local satellite package with a backing Offline — I checked and no Mu2e satellite repo pins its own ENVSET (see green section), so Offline's value governs everywhere. If EventDisplay or online is not 6.38/py3.12-ready, the breakage arrives on merge rather than on this PR.
   • Suggested fix: state in the PR body which of the two items are closed (EventDisplay 6.38 compatibility; online + python 3.12.13), or link the PRs that close them.

2. 🟡 [S2] Moving SetLimits from the state to the parameters changes behavior for a seed that starts outside its limits

   • Evidence: the old migrad.SetLimits(...) forwarded to MnUserParameterState::SetLimits (6.32 math/minuit2/src/MnApplication.cxx), which clamps an out-of-range start to low + 0.1*Error() / up - 0.1*Error() before transforming (6.32 MnUserParameterState.cxx:351-366). The new params.SetLimits(...) only records the limits (MnUserParameters.cxx:98 → MnUserTransformation.cxx:377-383); the clamp is never applied, and the state constructor instead runs Ext2int(value) (6.38 MnUserParameterState.cxx:27-39), which saturates an out-of-range value at the limit boundary itself (6.38 MnParameterTransformation.cxx:26-52).
   • Impact: identical whenever the seed lies inside its window — both paths then reduce to Ext2int(value) — but a seed outside the window now starts at the limit instead of 0.1σ inside it, which can select a different local minimum. Reachable only in DoFit, i.e. CosmicTrackFinder/CosmicTrackFinderLoose (UseTime: false, CosmicReco/fcl/prolog.fcl:91,110); the UseTime: true configuration goes through DoDriftTimeFit, which is bit-identical (see green section).
   • Suggested fix: nothing required if the planned cosmic validation compares CosmicTrack::MinuitParams distributions rather than only exit status; otherwise note the edge case in a comment next to the moved block.

3. ⚪ [S3] Leftover blank line, and casts that are now vestigial

   • Evidence: MinuitDriftFitter.cc:167-168 is a double blank line left where the moved block used to sit. The (signed)/(unsigned) casts existed to disambiguate the old facade's SetLimits(unsigned int, ...) from SetLimits(const char*, ...); MnUserParameters overloads on const std::string& instead (MnUserParameters.h:78,91), so params.SetLimits(0, -10000, 10000) is unambiguous now.
   • Impact: cosmetic only.
   • Suggested fix: drop the extra blank line; dropping the casts is optional.

4. ⚪ [S3] p105 appeared after this PR was opened — confirm p104 is still the target

   • Evidence: /cvmfs/.../DataFiles/Muse/p104 is dated 2026-07-25, p105 2026-07-31. diff p104 p105 is a single line (smack setup muse-al9-prof-e29-p104 → -p105), and the two spack envs differ only by art-root-io being hash-pinned (^/6uwdfmr) in p105; ROOT (6.38.00) and python (3.12) are identical.
   • Impact: none today; the PR has been open two weeks and the newer envset is version-equivalent.
   • Suggested fix: confirm p104 is deliberate, or retarget.

Verified, no action needed

• 🟢 migrad.State().SetPrecision/Fix/Release is exactly equivalent to the removed facade: in 6.32 MnApplication::SetPrecision/Fix/Release are one-line forwards to fState (MnApplication.cxx), 6.38 exposes that same fState through a new non-const State() (MnApplication.h), and operator() still ends with fState = min.UserState() in both — so the Fix(4) → minimize → Release(4) sequence in DoDriftTimeFit behaves identically.
• 🟢 The port genuinely requires 6.38 and cannot compile against 6.32, where MnApplication::State() is const-only and MnUserParameterState::Fix is non-const. FNALbuild being green at 2b317c67 therefore implies the CI toolchain supplied Minuit2 ≥ 6.38 — so the compile half of this platform bump really is covered here, which is not usually true of an envset change.
• 🟢 Build files: nothing to change in either build system. CosmicReco/CMakeLists.txt already lists ROOT::Minuit2; the PR adds no header or library (Minuit2/MnUserParameters.h was already included at line 29); CosmicReco has no SConscript to mirror; check_cmake is green.
• 🟢 No other Offline source touches the removed facade — only CosmicReco/{MinuitDriftFitter,PDFFit}.{cc,hh} reference Minuit2, and PDFFit.cc merely includes MnMigrad.h without instantiating an MnApplication subclass.
• 🟢 p104 is installed and matches the PR description: the envset file is present in $MUSE_ENVSET_DIR, spack env muse-al9-prof-e29-p104 exists, and its root@6.38.00 require line carries +geom (libGeom) along with +opengl+webgui+x.
• 🟢 No ENVSET conflict introduced: none of Production, EventNtuple, mu2e-trig-config, Stntuple, EventDisplay/REve, TrkAna, Analysis, Pass1, DQM, Tutorial, PassN, RefAna, ArtAnalysis, KinKal or BTrk sets ENVSET in its .muse (Stntuple's is commented out), so Offline's recommendation governs and muse emits no "conflicting ENVSET recommendations" warning.
• 🟢 The DAQ hunk is whitespace-only in a TLOG continuation line; no behavior change.

Prior review accounting

• brownd1978 approved 2026-07-27 ("I'll ask Namitha to test it on Monday") with no findings; there are no line comments or other reviews on this PR, so nothing to carry forward from #1901 itself. The two items in finding 1 are carried in from the companion draft #1859, whose own approvals (oksuzian, NamithaChitrazee) recorded no findings.

Validation check

• Build/tests run: none by me. FNALbuild is green at 2b317c67 (build prof, ceSimReco, cosmicSimReco, cosmicOffSpill, ceDigi, ceMix, trigger, check_cmake, whitespace). Its artifacts have since expired — the scons.log link now returns 404 — so the exact toolchain could not be re-read; the ≥ 6.38 conclusion above is an inference from the code not compiling under 6.32.
• Config contract check: pass — no FHiCL keys change and CosmicReco/fcl/prolog.fcl is untouched.
• Cross-repo consistency: needs follow-up (finding 1).

Residual risk

• ROOT file interoperability across the transition: files written by a p104 build may be read by p103-built releases and musings still in service. Not exercised by anything on this PR.
• Physics equivalence of the cosmic drift fit rests on the planned validation; CI shows only that the jobs run.
• Python 3.9 → 3.12 is invisible to the Offline C++ build but not to downstream tooling that inherits the Offline environment's python.

Author follow-ups

1. Confirm the two items from #1859 are closed — EventDisplay compatibility with ROOT 6.38, and the online side plus python 3.12.13.
2. Confirm p104 rather than the newer p105 is the intended landing target.
3. Note in the PR body that p104 also moves python 3.9 → 3.12 and fhiclcpp v4_19_00 → v4_19_02, so it is not read as a ROOT-only change.
4. When the cosmic validation runs, compare CosmicTrack::MinuitParams distributions rather than just exit status (finding 2).
5. Drop the stray blank line at MinuitDriftFitter.cc:167-168.
6. #1859 is still open as a draft covering the same two files — close it when this merges.