move TrkDiag

[brownd1978]

Remove TrkDiag from Offline; it is now in ArtAnalysis

[FNALbuild]

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

• Analyses
• CaloFilters
• TrkFilters
• CaloDiag
• TrkDiag
• Trigger
• /
• CommonMC
• CosmicReco
• TEveEventDisplay
• Filters

which require these tests: build.

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

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

About FNALbuild. Code review on Mu2e/Offline.

[brownd1978]

Note this sits on top of PR 1738

[FNALbuild]

☔ The build tests failed for bfd39c7.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged bfd39c7 at e73ac0c
build (prof)	✅	Log file. Build time: 08 min 43 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 (4) in 57 files
clang-tidy	➡️	6 errors 413 warnings
whitespace check	✅	no whitespace errors found

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

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 7add477: build (Build queue - API unavailable)

[FNALbuild]

☀️ The build tests passed at 7add477.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged 7add477 at e73ac0c
build (prof)	✅	Log file. Build time: 08 min 42 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 (4) in 57 files
clang-tidy	➡️	6 errors 413 warnings
whitespace check	✅	no whitespace errors found

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

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]

Let me fetch the full set of changed files in this PR to do a comprehensive review.Due to the large size of this PR (many files), let me get the remaining changed files and understand the full scope of changes.Based on my thorough review of all the changed files in this PR, here is my analysis:

PR Summary

This PR removes the TrkDiag and TEveEventDisplay directories from the Offline repository, as they have been moved to a separate ArtAnalysis repository. It also relocates TrkMCTools (header and source) from TrkDiag to CommonMC, since that utility is still needed by remaining code.

---

Issues Found

1. 🔴 Missing ComboHitInfo.hh Header — Potential Build Breakage

The header Offline/TrkDiag/inc/ComboHitInfo.hh is being removed (the entire TrkDiag directory is deleted), but the file CosmicReco/src/CosmicTrackDiag_module.cc had its #include "Offline/TrkDiag/inc/ComboHitInfo.hh" line removed without replacement.

// Mu2e diagnostics
-#include "Offline/TrkDiag/inc/ComboHitInfo.hh"
 #include "Offline/GeneralUtilities/inc/ParameterSetHelpers.hh"

Question: Does CosmicTrackDiag_module.cc still use the ComboHitInfo or ComboHitInfoMC types? If it does, this will fail to compile. If it doesn't (the types were unused), the removal is fine. This should be verified.

Similarly, in CosmicFitDisplay_module.cc, both TrkMCTools.hh and ComboHitInfo.hh were removed:

-#include "Offline/TrkDiag/inc/TrkMCTools.hh"
-#include "Offline/TrkDiag/inc/ComboHitInfo.hh"

If CosmicFitDisplay_module.cc uses TrkMCTools, the include should have been updated to Offline/CommonMC/inc/TrkMCTools.hh rather than removed. Verify whether the remaining code in this file still references TrkMCTools functions.

2. 🟡 CommonMC/CMakeLists.txt — Unnecessary/Missing Dependencies in the New Library

The new cet_make_library for TrkMCTools.cc lists these PUBLIC dependencies:

cet_make_library(
    SOURCE
      src/TrkMCTools.cc
    LIBRARIES PUBLIC
      Offline::BFieldGeom
      Offline::ConfigTools
      Offline::DataProducts
      Offline::GeometryService
      Offline::MCDataProducts
      Offline::Mu2eUtilities
      Offline::ParticleID
      Offline::RecoDataProducts
      Offline::SeedService
      Offline::TrackerGeom
)

Concern: Offline::ParticleID and Offline::SeedService seem unlikely to be needed by TrkMCTools.cc. These may have been copied from the old TrkDiag/CMakeLists.txt without pruning. Extra unnecessary dependencies increase build coupling. Consider verifying which of these are actually required by TrkMCTools.cc.

3. 🟡 CommonMC/src/SConscript — Overly Broad Library Dependencies for mainlib

The old CommonMC/src/SConscript had an empty mainlib:

mainlib = helper.make_mainlib ( [] )

The new version adds 20+ dependencies to the mainlib. This is necessary to support the moved TrkMCTools.cc, but some of these look excessive (e.g., art_root_io_TFileService, art_Framework_Services_Optional_RandomNumberGenerator, boost_filesystem). These should be audited to ensure only required libraries are linked.

4. 🟡 Analyses/CMakeLists.txt — Trailing Blank Line

After removing the TrackSummaryDataAnalyzer, TrackSummaryMCAnalyzer, and TrackSummaryTruthRFSelector build plugin blocks, there's a residual blank line before the TSTrackAna section:

)
 
 cet_build_plugin(TSTrackAna art::module

This is a minor cosmetic issue (extra blank line) but worth cleaning up.

5. 🟢 CommonMC/fcl/TestFindMCPrimary.fcl — Good Change

The removal of #include "Offline/TrkDiag/fcl/prolog.fcl" is correct since the TrkDiag directory is being removed. Verify the included prolog wasn't providing definitions needed by this fcl file.

6. 🟢 Consistent Include Path Updates

The SelectRecoMC_module.cc and TrkMCTools.cc include path updates from Offline/TrkDiag/inc/TrkMCTools.hh to Offline/CommonMC/inc/TrkMCTools.hh look correct and consistent.

7. 🟢 Build System — TEveEventDisplay Removal

The removal of add_subdirectory(TEveEventDisplay) from the top-level CMakeLists.txt is consistent with the deletion of the entire TEveEventDisplay directory. The PR description only mentions TrkDiag but the TEveEventDisplay is also being removed — this should be noted in the PR description for clarity.

---

Summary

Severity	Issue	File(s)
🔴 High	ComboHitInfo.hh removed without verifying usage in remaining files	CosmicTrackDiag_module.cc, CosmicFitDisplay_module.cc
🔴 High	TrkMCTools.hh include removed (not updated) in CosmicFitDisplay_module.cc — possible compile error if symbols are used	CosmicFitDisplay_module.cc
🟡 Medium	Potentially unnecessary library dependencies in new cet_make_library	CommonMC/CMakeLists.txt
🟡 Medium	Overly broad mainlib dependencies in SConscript	CommonMC/src/SConscript
🟡 Low	PR description doesn't mention TEveEventDisplay removal	PR body
🟢 Info	Minor trailing blank line	Analyses/CMakeLists.txt

The most critical items to verify are whether CosmicFitDisplay_module.cc and CosmicTrackDiag_module.cc still compile correctly after the removal of the TrkDiag includes without corresponding replacements.

[FNALbuild]

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

[oksuzian]

I'll review this pull request by first loading the PR details and its changes.Here is my review of PR #1742 — "move TrkDiag" by @brownd1978.

Note: This PR modifies 217 files. The file list retrieved is limited to 30 results. View all changed files on GitHub.

---

PR Summary

This PR removes the TrkDiag package (and TEveEventDisplay) from the Offline repository, as they have been moved to ArtAnalysis. The key actions are:

1. Removal of TrkDiag/ directory — entire directory deleted, including all source, headers, and FCL files.
2. Removal of TEveEventDisplay/ — also removed from the top-level CMakeLists.txt.
3. Relocation of TrkMCTools — TrkMCTools.hh/.cc are moved from TrkDiag/ to CommonMC/ and include paths are updated.
4. Removal of obsolete TrackSummary-related code — TrackSummaryTruthAssns, TrackSummaryDataAnalyzer, TrackSummaryMCAnalyzer, TrackSummaryTruthRFSelector, TrackSummaryTruthUpdater, HistTrackSum, TrackCuts, and related FCL prolog entries are all removed.
5. Removal of several cosmic analyzer modules — CosmicAnalyzer, CosmicMCRecoDiff, CosmicTrackDetails are deleted (they depended on TrkDiag).
6. Build system cleanup — All SConscript and CMakeLists.txt references to mu2e_TrkDiag / Offline::TrkDiag are removed across many packages.

---

Issues Found

1. 🔴 TEveEventDisplay removed without explanation in PR description

The PR description says "Remove TrkDiag from Offline; it is now in ArtAnalysis", but the CMakeLists.txt change also removes TEveEventDisplay:

-add_subdirectory(TEveEventDisplay)

This is an undocumented, potentially surprising change. If TEveEventDisplay was also moved to ArtAnalysis, the PR description should mention it. If it was removed by mistake, this is a bug.

2. 🟡 ComboHitInfo.hh include removed but header file removal not visible

Several files had #include "Offline/TrkDiag/inc/ComboHitInfo.hh" removed:

-#include "Offline/TrkDiag/inc/ComboHitInfo.hh"

-#include "Offline/TrkDiag/inc/ComboHitInfo.hh"

Since the entire TrkDiag directory is being removed, these removals are necessary. However, you should verify that CosmicFitDisplay_module.cc and CosmicTrackDiag_module.cc don't actually use any types/functions from ComboHitInfo.hh. If they do, those modules will fail to compile. The CosmicTrackDiag_module.cc previously had a vector<ComboHitInfoMC> member in the deleted analyzers — confirm the remaining code in CosmicTrackDiag doesn't reference ComboHitInfoMC.

3. 🟡 CosmicFitDisplay_module.cc — removed TrkMCTools.hh include but module is retained

-#include "Offline/TrkDiag/inc/TrkMCTools.hh"

The TrkMCTools.hh header was moved to CommonMC/inc/TrkMCTools.hh, but CosmicFitDisplay_module.cc has the include simply removed rather than updated. If this module actually uses functions from TrkMCTools, it will fail to compile. If it doesn't use them (was an unnecessary include), this is fine.

4. 🟡 Potential incomplete cleanup — remaining references to TrkDiag elsewhere

Because the file listing is truncated at 30 files out of 217, there may be additional files beyond what I can see that still reference TrkDiag. The build system changes visible so far look consistent, but given the scale of the change, a full grep -r TrkDiag on the resulting branch should be performed to confirm no dangling references remain.

5. 🟢 TrkMCTools relocation looks correct

The move from TrkDiag/ → CommonMC/ is properly done:

• Header renamed: TrkDiag/inc/TrkMCTools.hh → CommonMC/inc/TrkMCTools.hh
• Source renamed: TrkDiag/src/TrkMCTools.cc → CommonMC/src/TrkMCTools.cc
• Internal #include path updated in both .cc and consuming module (SelectRecoMC_module.cc)
• CommonMC/CMakeLists.txt correctly adds a new cet_make_library for TrkMCTools.cc with proper dependencies
• CommonMC/src/SConscript updated to include library dependencies for the main library

6. 🟡 TrackSummary and TrackCuts in RecoDataProducts / Mu2eUtilities — partial removal?

The PR removes TrackSummaryTruthAssns.hh/.cc from MCDataProducts, HistTrackSum.hh/.cc from Mu2eUtilities, the TrackSummaryDataAnalyzer, TrackSummaryMCAnalyzer, TrackSummaryTruthRFSelector modules, and FCL entries. However, from the visible files alone, I cannot confirm whether RecoDataProducts/inc/TrackSummary.hh and Mu2eUtilities/inc/TrackCuts.hh (which were included by the deleted modules) are also removed. If TrackSummary.hh is still present but nothing uses it, that's dead code. The later commits ("More TrackSummary removal", "TrackSummary fcl removal") suggest this was addressed iteratively, but it's worth verifying completeness.

7. 🟢 classes_def.xml cleanup looks correct

The removal of all TrackSummary-related dictionary entries from MCDataProducts/src/classes_def.xml is properly done — all Assns, Wrapper, and pair entries for TrackSummary/TrackSummaryMatchInfo are removed together.

8. 🟡 Build state is "unstable"

The PR's mergeable_state is "unstable", which typically indicates CI checks have not fully passed. This should be resolved before merge.

---

Summary

Severity	Issue	Location
🔴 High	TEveEventDisplay removal undocumented in PR description	CMakeLists.txt
🟡 Medium	Verify ComboHitInfo.hh removal doesn't break remaining modules	CosmicFitDisplay, CosmicTrackDiag
🟡 Medium	TrkMCTools.hh include removed (not updated) from CosmicFitDisplay	CosmicFitDisplay_module.cc
🟡 Medium	Verify no dangling TrkDiag references in the remaining 187 files	Full repo grep needed
🟡 Medium	Verify TrackSummary.hh and TrackCuts.hh removal is complete	RecoDataProducts, Mu2eUtilities
🟡 Medium	CI state is unstable	Build system

Overall, the PR is a large but straightforward cleanup operation. The main risks are around incomplete removal of includes/references in files that were modified (not deleted), particularly where headers were removed rather than relocated.

[brownd1978]

AI comment 1 is out-of-date, the dependencies it found were removed in a subsequent commit. comment 2 is addressed. Comment 3 is wrong, scons wants all the dependencies, separating them out is nearly impossible. Comment 4 is potentially relevant, but a search for TrkDiag in the repo finds nothing. Comment 6 is addressed.

[brownd1978]

@FNALbuild run build test

[FNALbuild]

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

[FNALbuild]

☔ The build tests failed for b61b9b4.

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

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

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 a6a14e4: build (Build queue - API unavailable)

[FNALbuild]

☀️ The build tests passed at a6a14e4.

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

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

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.

[bonventre]

We will have to consider any diag modules possibly used in pass 1 production or in DQM for example StrawHitDiag that may need similar functionality moved back to Offline

[rlcee]

Asking AI to expand all the fcl in Production and find any modules that come from Offline/Analyses. This seems low, but I'm not sure how to verify.

Files with matches (fcl -> module types):

• Production/JobConfig/pbar/vertex_s1.fcl: ReadAntiProtonSteps, ReadVirtualDetector
• Production/JobConfig/pileup/STM/BeamToVD1809.fcl: CountVirtualDetectorHits
• Production/JobConfig/pileup/STM/STMResampler.fcl: CountVirtualDetectorHits
• Production/JobConfig/test/MuonTransport/MuonTransportSingleStage.fcl: StepPointMCDumper
• Production/JobConfig/test/MuonTransport/MuonTransportStage1.fcl: StepPointMCDumper
• Production/JobConfig/test/MuonTransport/MuonTransportStage2.fcl: StepPointMCDumper, StoppedParticlesDumper, StoppedParticlesPrinter
• Production/JobConfig/validation/psEnclosureBeamWindow.fcl: StepPointMCDumper
• Production/JobConfig/validation/psEnclosureExtMonWindow.fcl: StepPointMCDumper
• Production/Tests/TGTstops.fcl: StoppedParticlesDumper

[rlcee]

After merging this PR, asking it to find modules referenced in Production fcl, but can't be found in Offline,
Remaining unique missing module types:

• CaloDigisFromDataDecoders
• CosmicAnalyzer
• CosmicDriftFit
• CosmicMCRecoDiff
• CosmicTrackDetails
• FromCorsikaBinary
• FromTrackerPrototypeData
• G4
• MergeHelixFinder
• PBISequence
• ProcessDTCAndCFOEvents

[bonventre]

CosmicAnalyzer, CosmicDriftFit, CosmicMCRecoDiff, CosmicTrackDetails, and FromTrackerPrototypeData are all deprecated or deleted already, so we can clean those up in Production. Not sure about the rest