Pawel plesniak/absorber shift

[PawelPlesniak]

Clean up of the STM simulation production code, including generation of data products with a standardized naming format

[FNALbuild]

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

• Scripts
• JobConfig

which require these tests: build.

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

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

About FNALbuild. Code review on Mu2e/Offline.

[FNALbuild]

‼️ It was not possible to prepare the workspace for this test. This is often caused by merge conflicts - please check and try again.

> git diff --check | grep -i conflict
JobConfig/pileup/STM/BeamToVD.fcl:122: leftover conflict marker
JobConfig/pileup/STM/BeamToVD.fcl:127: leftover conflict marker
JobConfig/pileup/STM/BeamToVD.fcl:132: leftover conflict marker
JobConfig/pileup/STM/BeamToVD1809.fcl:138: leftover conflict marker
JobConfig/pileup/STM/BeamToVD1809.fcl:143: leftover conflict marker
JobConfig/pileup/STM/BeamToVD1809.fcl:148: leftover conflict marker
JobConfig/pileup/STM/STMResampler.fcl:112: leftover conflict marker
JobConfig/pileup/STM/STMResampler.fcl:118: leftover conflict marker
JobConfig/pileup/STM/STMResampler.fcl:142: leftover conflict marker
JobConfig/pileup/STM/STMResampler.fcl:186: leftover conflict marker
JobConfig/pileup/STM/STMResampler.fcl:189: leftover conflict marker
JobConfig/pileup/STM/STMResampler.fcl:193: leftover conflict marker
JobConfig/pileup/STM/prolog.fcl:13: leftover conflict marker
JobConfig/pileup/STM/prolog.fcl:15: leftover conflict marker
JobConfig/pileup/STM/prolog.fcl:192: leftover conflict marker
JobConfig/pileup/STM/prolog.fcl:246: leftover conflict marker
JobConfig/pileup/STM/prolog.fcl:264: leftover conflict marker
JobConfig/pileup/STM/prolog.fcl:322: leftover conflict marker

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	❌	a2e2f31 into 173faea merge failed

[PawelPlesniak]

I leave this for review by @YongyiBWu and @AndrewEdmonds11

[PawelPlesniak]

Note - I will review any failures that the CI workflow raises

[PawelPlesniak]

@FNALbuild run build test

[FNALbuild]

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

[FNALbuild]

☀️ The build tests passed at 79419ad.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged 79419ad at 173faea
build (prof)	✅	Log file. Build time: 04 min 16 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.
FIXME, TODO	✅	TODO (0) FIXME (0) in 0 files
clang-tidy	✅	0 errors 0 warnings

N.B. These results were obtained from a build of this Pull Request at 79419ad after being merged into the base branch at 173faea.

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.

[PawelPlesniak]

@YongyiBWu and @AndrewEdmonds11 The CI workflow issues have been addressed, this is now ready for review

[AndrewEdmonds11]

These look good to me

[oksuzian]

PR Review Summary — #552 (STM simulation clean-up / standardized data-product naming)

Reviewed at head 79419ad3

Decision

• 🔴 request changes

Scope understood

• Restructures JobConfig/pileup/STM/ so that every module label, product instance and file name comes from two new prolog tables (STMSimDataProducts, STMPileup) instead of literals; re-homes ResamplingProducer / ResamplingFilter / VirtualDetectorCounter / ShiftVD101Steps from Offline/STMMC/fcl/prolog.fcl into Production; adds STMResampler1809.fcl; reorders the stage-1 path so the VD filter runs before compression.
• Reorganises Scripts/STM/ into Analysis/ and G4/, adds gen_STMResampler.sh, gen_STMResampler1809.sh, gen_ROOTAnalysisDump.sh.
• Paired with Offline#1875, which deletes the re-homed tables from STMMC/fcl/prolog.fcl, adds #include "Production/JobConfig/pileup/STM/prolog.fcl" there, and changes STMResamplingProducer to emit a named product instance.

Findings

1. 🔴 [S0] Three of the four STM job configs do not parse at this head

   • Evidence: with Production@79419ad3 + Offline@main on FHICL_FILE_PATH, fhicl-dump aborts on all three:
     • BeamToVD.fcl:128 and BeamToVD1809.fcl:144 — @sequence::STMPileup.BeamToVDKeptProducts → Local lookup error / Can't find key: STMPileup.BeamToVDKeptProducts (at part "BeamToVDKeptProducts"). prolog.fcl moved BeamToVDKeptProducts out of STMPileup and into STMSimDataProducts (prolog.fcl:160), but neither consumer was updated.
     • STMResampler.fcl:112,113 — @sequence::STMPileup.stmResamplerSequence → Can't find key: STMPileup.stmResamplerSequence. The sequence was renamed to STMResamplerSequenceBeamCatDatasets (prolog.fcl:289); STMResampler1809.fcl was updated to the new name, STMResampler.fcl was not.
   • Impact: generate_fcl/mu2e -c fail immediately for all of BeamToVD, BeamToVD1809 and STMResampler. Every fcl in this directory except STMResampler1809.fcl is unusable as committed.
   • Suggested fix: @sequence::STMSimDataProducts.BeamToVDKeptProducts in both BeamToVD files, and @sequence::STMPileup.STMResamplerSequenceBeamCatDatasets in STMResampler.fcl. (Note BeamToVDKeptProducts now begins with its own "drop *_*_*_*", so drop the duplicate literal "drop *_*_*_*" that precedes it in both output blocks.)

2. 🔴 [S0] STMResampler.fcl:134 is not valid FHiCL syntax

   • Evidence: primaryTag : @local::STMSimDataProducts.Stage2.InputTag.@nil # Populate me!. After locally patching finding 1, fhicl-dump still aborts: Parse error ... detected at or near line 152, character 59 pointing at the @nil in the @local:: path. @nil is a value, not a name component, so this cannot be left as a fill-in-the-blank marker.
   • Impact: the file cannot be parsed even by a user who follows the header instructions, because the breakage is syntactic, not semantic.
   • Suggested fix: commit a working default (@local::STMSimDataProducts.Stage2.InputTag.HPGe) with the alternative in a comment — exactly the pattern STMResampler1809.fcl:148 already uses (... .HPGe # @nil # Populate me!), which parses cleanly. Same for SelectEvents : [ @nil ] at STMResampler.fcl:123-125.

3. 🔴 [S0] Required cross-repo change (Offline#1875) is not declared and is mutually blocking

   • Evidence: prolog.fcl:68 sets Stage1.ExtractedStepPointMCs : "extractVirtualDetectorSteps:virtualdetector", consumed as compressDetStepMCsSTM.stepPointMCTags and as filterVirtualDetectorSteps.StepPointMCsTag. On Offline@main, STMMC/src/STMResamplingProducer_module.cc:47,66 does produces<StepPointMCCollection>() / event.put(std::move(outputStepPointMCs)) — unnamed instance. Offline#1875 (3cedaf72) is what changes those two lines to "virtualdetector". Conversely Offline#1875 adds #include "Production/JobConfig/pileup/STM/prolog.fcl" to STMMC/fcl/prolog.fcl and resolves DataProducts.* from @local::STMSimDataProducts.*, so #1875 cannot merge before this PR either.
   • Impact: merging #552 alone leaves stage 1 asking art for a product that is never produced. The PR body does not mention #1875 at all, and neither PR links the other.
   • Suggested fix: state the dependency in both PR bodies and have the two merged together (or gate #552 on #1875). Please also confirm with the software team that an Offline/ prolog including a Production/ prolog is an acceptable dependency direction — that is new.

4. 🔴 [S0] STMResampler1809.fcl points Mu2eG4 at a resampler that never runs in its paths

   • Evidence: STMResampler1809.fcl:107,113 run @sequence::STMPileup.STMResamplerSequenceTargetStopsCatDataset (i.e. the stmResamplerTargetStopsCatDataset module). But Stage2.InputPhysVolumeMultiInfo (prolog.fcl:113) and Stage2.UpdateEventLevelVolumeInfos.Input (prolog.fcl:117) are scalars hard-wired to "stmResamplerBeamCatDatasets" / "stmResamplerBeamCatDatasets:eventlevel", and lines 151/153 use them unchanged. Confirmed in the expanded config: fhicl-dump STMResampler1809.fcl emits inputPhysVolumeMultiInfo: "stmResamplerBeamCatDatasets".
   • Impact: Mu2eG4::beginSubRun → writePhysicalVolumes does getValidHandle<PhysicalVolumeInfoMultiCollection> on that tag; the module is configured (via @table::STMPileup.filters) but not scheduled, so the product does not exist and the job throws on the first subrun.
   • Suggested fix: split Stage2.InputPhysVolumeMultiInfo and Stage2.UpdateEventLevelVolumeInfos.Input by dataset type the same way Stage1 and Stage2.PreSimulatedData already are, and reference the TargetStopsCatDataset variant here.

5. 🟠 [S1] Stage2.CompressedOutput names a module label that does not exist, and Offline#1875 consumes it

   • Evidence: prolog.fcl:149-155 defines compressDetStepMCs:virtualdetector / compressDetStepMCs:STMDet / compressDetStepMCs:. The stage-2 compressor is labelled compressSTMDet in STMResampler.fcl:71 and STMResampler1809.fcl:65, and STMResamplerKeptProducts (prolog.fcl:178-179) correctly keeps *_compressSTMDet_*_*. In Offline#1875, STMMC/fcl/prolog.fcl replaces the previously-correct literals "compressSTMDet:STMDet" / "compressSTMDet:virtualdetector" / "compressSTMDet:" with @local::STMSimDataProducts.Stage2.CompressedOutput.*, which feeds ROOTAnalysisDump.Stage2.*.
   • Impact: dead config in Production today, but a silent regression in the paired Offline PR — the stage-2 ROOT dump would request products that no module produces.
   • Suggested fix: compressDetStepMCs → compressSTMDet in all three Stage2.CompressedOutput entries.

6. 🟠 [S1] gen_STMResampler1809.sh embeds the wrong template fcl

   • Evidence: Scripts/STM/G4/gen_STMResampler1809.sh writes #include "Production/JobConfig/pileup/STM/STMResampler.fcl" and then overrides physics.filters.stmResamplerTargetStopsCatDataset.mu2e.MaxEventsToSkip. STMResampler.fcl is the BeamCat template; its paths use stmResamplerBeamCatDatasets, so the override lands on a filter that is configured but never scheduled, and --auxinput=...stmResamplerTargetStopsCatDataset.fileNames likewise.
   • Impact: even after findings 1-2 are fixed, the 1809 stage-2 generator produces jobs that resample the wrong stream with fileNames: @nil left unresolved.
   • Suggested fix: include Production/JobConfig/pileup/STM/STMResampler1809.fcl.

7. 🟠 [S1] gen_ROOTAnalysisDump.sh includes a file that does not exist in this repo

   • Evidence: the script writes #include "Production/JobConfig/pileup/STM/SignalParticleTrace.fcl". A recursive tree listing of Production@79419ad3 contains no SignalParticleTrace.fcl anywhere. The script's own header says the driver "should be configured in Offline/STMMC/fcl/ROOTAnalysisDump.fcl", which is the file Offline#1875 adds.
   • Impact: every generate_fcl call in the script fails.
   • Suggested fix: include Offline/STMMC/fcl/ROOTAnalysisDump.fcl, matching the script's documentation.

8. 🟠 [S1] simStageOverride : 2 looks off by one in STMResampler1809.fcl

   • Evidence: Mu2eG4/src/writePhysicalVolumes.cc sets simStage = mvi->size() (count of upstream stages) and appends exactly one entry per Mu2eG4 stage; Mu2eProductMixer::endSubRun re-emits the collection with subrunVolumes_.size() preserved, so a resampler does not change the count; Mu2eG4::beginSubRun/endSubRun throw cet::exception("BADINPUT") ... inconsistent simStage when the override disagrees. Assuming the unchanged BeamToVD1809.fcl:156 value of 2 is correct, its output carries 3 entries, so STMResampler1809.fcl:150 should be 3. The same arithmetic says the BeamCat chain's 3 → 2 change in STMResampler.fcl:136 is a genuine fix (BeamToVD writes 2), so I read this as the 1809 file inheriting the wrong constant.
   • Impact: the job burns full G4 CPU and then throws at end-of-subrun.
   • Suggested fix: confirm against a real stage-1 1809 file and set the value accordingly; a per-chain Stage2.SimStageOverride.{BeamCatDatasets,TargetStopsCatDataset} entry would keep this from drifting again.

9. 🟠 [S1] Multi-threading is silently switched on for both stage-2 configs

   • Evidence: STMResampler.fcl:162 and STMResampler1809.fcl:176 are now an active #include "Production/JobConfig/common/MT.fcl"; the previous STMResampler.fcl line was # comment out of MT issue #include ..., and BeamToVD.fcl/BeamToVD1809.fcl still carry that comment. fhicl-dump STMResampler1809.fcl confirms module_type: "Mu2eG4MT", num_schedules: 2, num_threads: 2.
   • Impact: an undiscussed switch to Mu2eG4MT with 2 schedules in jobs that also run an art mixing filter (ResamplingMixer), in files whose siblings explicitly keep MT off for a known issue. Memory footprint and reproducibility both change.
   • Suggested fix: either restore the comment, or state in the PR body that the MT issue is resolved for these jobs and give the validation run that shows it.

10. 🟡 [S2] All three generators now use --run-number=1204

    • Evidence: gen_BeamToVD.sh muons 1205 → 1204; gen_BeamToVD1809.sh 1206 → 1204; the new gen_STMResampler*.sh also use 1204. Electrons were already 1204.
    • Impact: the Ele, Mu and 1809 streams are generated with overlapping art::EventIDs and are later mixed together (STMStepMixerEle/Mu/1809) with writeEventIDs : true. Distinct run numbers were what kept those IDs disjoint.
    • Suggested fix: keep one run number per stream, or say in the PR body why collisions are harmless for the mixing/back-tracing chain.

11. 🟡 [S2] Known-broken compression shipped with a "debugging is left to the user" note

    • Evidence: STMResampler.fcl:11 and STMResampler1809.fcl:8: "When finalizing this script, there were some errors with the compression. I couldn't debug it and did not make time to run a separate simulation to propagate these changes."
    • Impact: a production config carrying a self-declared unresolved defect, with no issue to track it.
    • Suggested fix: open a GitHub issue describing the compression errors and reference it from the comment, so this is tracked rather than buried in a header.

12. 🟡 [S2] return used for early exit in executable scripts

    • Evidence: gen_STMResampler.sh:18/gen_STMResampler1809.sh:18 (return -1) and the two return statements in the else branch; both files are mode 100755 with #!/usr/bin/bash. return outside a function in a non-sourced script is an error and execution continues.
    • Impact: the "Missing arguments!" and "No dataset list file" guards do not actually stop the script. The script this replaces used exit 1 in the equivalent place.
    • Suggested fix: use exit 1.

13. ⚪ [S3] Dead and drifted config, and doc/path drift

    • STMPileup.ShiftVD101Steps.StepPointMCsTag (prolog.fcl:263) and STMSimDataProducts.Stage2.ExtractedStepPointMCs (prolog.fcl:146) are never referenced — stage 2 has no extractVirtualDetectorSteps module.
    • Headers point at Production/Scripts/STM/gen_*.sh, but the scripts moved to Scripts/STM/G4/; STMResampler1809.fcl:6 names gen_STMResampler.sh rather than gen_STMResampler1809.sh.
    • gen_ROOTAnalysisDump.sh is mode 100644 with no shebang, but its usage says ./gen_ROOTAnalysisDump.sh.
    • gen_STMResampler*.sh: nFiles=$(wc -c < ...) counts bytes, not files, and the zero-check only warns; the commented "for large simulation studies" block in gen_BeamToVD1809.sh still uses nEleEvts/nEleFiles variable names.
    • Typos in new text: virtualdetecctor (several), geomety, EleBemaCat, randonNoReplace; prolog.fcl has no trailing newline.

Verified, no action needed

• 🟢 Stage1.PreSimulatedData.TargetStopsCatDataset : ["TargetStopResampler:virtualdetector"] is correct: JobConfig/primary/prolog.fcl gives TargetStopResampler a stepPointMCMixer of [["TargetStopFilter:virtualdetector", ":"]], and Mu2eProductMixer.cc:20 resolves ":" to the input instance name.
• 🟢 Stage2.ResampledStepPointMCs.* = "stmResampler*:virtualdetector" is likewise correct for the new [["compressDetStepMCsSTM:virtualdetector", ":"]] mixing map.
• 🟢 Stage1.CompressedOutput.StepPointMCs : "compressDetStepMCsSTM:virtualdetector" matches CompressDetStepMCs_module.cc:247,328, which produces/puts under i_tag.instance().
• 🟢 Moving filterVirtualDetectorSteps ahead of compressDetStepMCsSTM in the stage-1 path and repointing it at extractVirtualDetectorSteps:virtualdetector is internally consistent (given finding 3).
• 🟢 protonTimeOffset and genCounter, used by the renamed resampler sequences, are supplied by @table::Pileup.producers (JobConfig/pileup/prolog.fcl:133) and @table::Common.producers (JobConfig/common/prolog.fcl:10); dropping the Offline/CommonMC and Offline/STMMC includes from BeamToVD.fcl does not break anything it still references.
• 🟢 STMResampler1809.fcl parses cleanly (fhicl-dump exit 0).

Validation check

• Build/tests run: FNALbuild reports green at 79419ad3, but none of its stages parse these files — ceSimReco, ceMix, g4test_03MT etc. touch no STM config, and Production ships no build file covering JobConfig/pileup/STM/ or Scripts/STM/ (JobConfig/ensemble/SConscript is the repo's only SConscript, and there is no CMakeLists), so scons/CMake mirroring is not in scope here and the green check is not evidence for this PR. I ran fhicl-dump on all four configs with Production@79419ad3 + Offline@main: 3 abort, 1 passes.
• Config contract check: fail — findings 1, 2, 4, 5.
• Cross-repo consistency: needs follow-up — findings 3 and 5; Offline#1875 does register its two new modules in STMMC/CMakeLists.txt and STMMC/src/SConscript globs *_module.cc, so that PR's build side is mirrored.

Residual risk

• No end-to-end run of the new chain is reported in the PR; with the configs unparseable, none is possible at this head.
• Findings 4, 5 and 8 are runtime/downstream failures that a fix for findings 1-2 will not surface until someone submits real jobs.
• Finding 8 rests on the unchanged BeamToVD1809.fcl value of simStageOverride : 2 being correct; please confirm against a real stage-1 1809 file.

Author follow-ups

1. Fix the three parse errors (findings 1 and 2) and add fhicl-dump output for all four configs to the PR body as validation evidence.
2. Link Offline#1875 from this PR body (and vice versa), state that they must merge together, and confirm the new Offline → Production prolog include direction with the software team.
3. Fix Stage2.CompressedOutput (compressDetStepMCs → compressSTMDet) before #1875 picks the value up.
4. Split Stage2.InputPhysVolumeMultiInfo / UpdateEventLevelVolumeInfos.Input per dataset type so STMResampler1809.fcl stops pointing at stmResamplerBeamCatDatasets.
5. Correct the two script includes (gen_STMResampler1809.sh → STMResampler1809.fcl; gen_ROOTAnalysisDump.sh → Offline/STMMC/fcl/ROOTAnalysisDump.fcl) and change return → exit.
6. Confirm simStageOverride for the 1809 stage-2 job, justify the --run-number consolidation to 1204, and either justify or revert the MT.fcl activation.
7. Open a tracking issue for the compression errors called out in the STMResampler*.fcl headers.

Note on prior review: the existing approval of this PR by @AndrewEdmonds11 at this same head ("These look good to me") carried no findings, so nothing is carried forward. The parse failures above are reproducible at 79419ad3, so they are not regressions introduced since that read.