Pawel plesniak/absorber shift - #1875
Conversation
|
Hi @PawelPlesniak,
which require these tests: build. @Mu2e/write, @Mu2e/fnalbuild-users have access to CI actions on main. ⌛ The following tests have been triggered for 3bb3d47: build (Build queue - API unavailable) |
|
…e really should not just ignore the pre-commit config
|
I leave this for review by @YongyiBWu and @AndrewEdmonds11 |
|
Note - I will review any failures that the CI workflow raises |
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for b19652b: build (Build queue - API unavailable) |
|
@YongyiBWu and @AndrewEdmonds11 The CI workflow issues have been addressed, this is now ready for review |
🔴 Bugs (likely functional problems)1. if (!stepHandle || stepHandle->empty()) { consecutiveEmptyFileCounter++; return; } // returns BEFORE threshold check
...
if (consecutiveEmptyFileCounter > consecutiveEmptyFileThreshold)
throw cet::exception("LogicError", "Too many consecutive empty files, stopping the job");Because the empty branches 2. ADC = _chargeDecayed[i] * chargeToADC;
_adcs[i] = (ADC > ADCMax) ? static_cast<int16_t>(std::round(ADC)) : ADCMax;
3. if (defaultMicrospillBufferLengthCount < 1)
throw cet::exception("RANGE", "defaultMicrospillBufferLengthCount has to be at least 1!\n");
4. if (E < std::numeric_limits<double>::epsilon())
throw cet::exception("RANGE") << "Energy must be greater than zero, exiting.";
5. With no 🟠 Correctness / robustness concerns6. Uninitialized counters read in summaries — In both 7. 8. Hard-coded 320 MHz coupling — 🟡 Minor / cleanup
|
|
📝 The HEAD of |
YongyiBWu
left a comment
YongyiBWu
left a comment
There was a problem hiding this comment.
I do not see any direct issue with the proposed changes
AndrewEdmonds11
left a comment
AndrewEdmonds11
left a comment
There was a problem hiding this comment.
This looks good to me. There are several files which just have whitespace changes, which would be good to clean up before merging. There are some very useful modules in here that would be good for generating STM mock datasets: PhotonGun, HPGeWaveformsFromStepPoints etc.
There was a problem hiding this comment.
This looks like it's only whitespace changes. I think this file also got renamed at some point so might cause a merge issue...
There was a problem hiding this comment.
I'll merge develop again before I merge, but I have a quick question - for the whitespace changes, what do you mean by "clean up before merging"? It took a while to be able to commit as the pre-commit action I had blocked the git commit before these were all changed, and I am unsure of how to disable this so I can re-commit the files with the whitespaces addressed
Responses to AI review🔴 Bugs (likely functional problems)1. SimParticleDump_module.cc — empty-file counter never resets, and threshold check is wrong
This has been extended to check that the threshold is exceeded before checking that the event data products are not empty.
This has been updated to unify the logic
Validation complete. HPGeWaveformsFromStepPointMCs_module.cc — digitize() clipping logic is invertedThis review is incomplete, and does not understand the physics of what is being simulated:
Positive overflow will not be encountered, unless the user sets the pedestal too high. This will be left for the user to address 3. HPGeWaveformsFromStepPointMCs_module.cc — validation constant defaultMicrospillBufferLengthCount < 1 is unreachableFair point, corrected. 4. PhotonGun_module.cc — new energy check rejects valid low-energy photonsCorrected. 5. HPGeWaveformGenerationAndAnalysis.fcl — dangling data-product tag after module renameGood catch! 🟠 Correctness / robustness concerns6. Uninitialized counters read in summariesCorrected. 7. SimParticleDump uses SimParticles.at(step.trackId())Corrected 8. Hard-coded 320 MHz couplingThe user is made aware of the difference if 🟡 Minor / cleanupTypo: unqiueEnabledVDsVec in CountVirtualDetectorHits_module.cc, and duplicate ;;Corrected Debug leftoversRemoved Copy-paste in .fclCorrected Author-acknowledged unfinished workThis is actually for the user to populate. Comment typo:Addressed where relevant |
|
I will wait for the build to be completed, review any issues should they arise, and await clarification on what the whitespace changes should be |
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for 265298f: build (Build queue - API unavailable) |
|
☔ The build is failing at 265298f.
N.B. These results were obtained from a build of this Pull Request at 265298f after being merged into the base branch at f408474. For more information, please check the job page here. |
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for 3cedaf7: build (Build queue - API unavailable) |
|
☀️ The build tests passed at 3cedaf7.
N.B. These results were obtained from a build of this Pull Request at 3cedaf7 after being merged into the base branch at f408474. For more information, please check the job page here. |
oksuzian
left a comment
oksuzian
left a comment
There was a problem hiding this comment.
PR Review Summary
Reviewed at head 3cedaf72
Decision
- 🔴 request changes
Scope understood
- STM simulation/analysis rework: HPGe waveform generation (preamplifier response, microspill carry-over, event-time accounting), MWD instrumentation, two new
SimParticleprovenance-dump analyzers, six new/rewritten STMMC driver fcls, and a full rewrite ofSTMMC/fcl/prolog.fcl. - A data-product instance rename in
STMResamplingProducer(""→"virtualdetector") paired with an open companion PR, Mu2e/Production#552. - Carried along: whitespace-only reformatting across
DAQ/,BTrkLegacy/,TrkReco/,RecoDataProducts/,STMReco/(pre-commit fallout, per the PR body).
Re-review accounting (prior review posted 2026-07-09 at head 74637245; delta 74637245..3cedaf72 = 8 files)
| Prior finding | Status |
|---|---|
1. SimParticleDump empty-file counter / threshold |
🟢 FIXED in e500ede0, verified |
2. digitize() one-sided clipping |
🟠 UNADDRESSED — carried forward (F4) |
3. defaultMicrospillBufferLengthCount < 1 unreachable |
🟢 FIXED (right variable now checked) / ⚪ message still wrong (F10) |
4. PhotonGun epsilon() energy guard |
🟢 FIXED, verified |
5. Dangling PHHPGe in keep |
🟢 FIXED, verified |
| 6. Uninitialized counters | 🟢 FIXED, verified |
7. SimParticles.at(...) |
🟢 FIXED in SimParticleDump; ⚪ still present in VirtualDetectorTree (pre-existing) |
| 8. Hard-coded 320 MHz coupling | 🟢 no longer silent — now throws (F14 notes the cadence) |
9. makeADCplot / waveform_time // TODO - REMOVE ME! |
🟢 FIXED in ddcc9c38/265298fc, verified absent at head |
10. MWD banner, Ou1tputs, LaBr process_name |
🟢 FIXED, verified |
11. unqiueEnabledVDsVec, [BRAKCETS], @nil "Populate me!" |
🟠 PARTIAL — carried forward (F9, F11) |
Other reviewers: @YongyiBWu approved (2026-07-19, no items). @AndrewEdmonds11 approved (2026-08-03) with one request — clean up the whitespace-only files before merging — still open (see PR hygiene). His PlotSTMWaveformDigis comment was addressed by ddcc9c38/265298fc.
Findings
-
🔴 [S0]
Offline/STMMC/fcl/prolog.fclincludes aProduction/file and cannot resolve against Productionmain- Evidence:
STMMC/fcl/prolog.fcl:6adds#include "Production/JobConfig/pileup/STM/prolog.fcl". The new tables then dereference@local::STMSimDataProducts.Stage1.CompressedOutput.StepPointMCs,...Stage2.CompressedOutput.StepPointMCs.Virtualdetector, and@local::STMPileup.ResamplingProducer.VirtualDetectorID.STMSimDataProductsdoes not exist in Productionmain— it is introduced only by Mu2e/Production#552 (still OPEN, head79419ad3, where it appears atJobConfig/pileup/STM/prolog.fcl:15andSTMPileup.ResamplingProducerat line 245). A code search overMu2e/OfflineforProduction/JobConfigin.fclreturnsSTMMC/fcl/prolog.fclplus only*/test/*.fcldriver files — no other Offline prolog depends on Production. - Impact: two problems. (a) Layering inversion: Offline is the base layer; making an Offline prolog unparseable without Production on
FHICL_FILE_PATHbreaks Offline-standalone use and inverts the dependency for every consumer ofOffline/STMMC/fcl/prolog.fcl. (b) Merge-order landmine: Productionmain'sJobConfig/pileup/STM/BeamToVD.fclstill#includesOffline/STMMC/fcl/prolog.fcl, so merging this PR before #552 makes the live STM pileup drivers fail to parse. Offline CI cannot catch this — no validation job in the suite (ceSimReco,ceDigi,ceMix,g4test_03MT, …) runs any STMMC fcl, which is whymu2e/buildtestis green at3cedaf72. - Suggested fix: move the new
DataProductstable into Production's STM prolog (that is whereSTMSimDataProductsalready lives) and delete the#include "Production/..."line from the Offline prolog. If any of it must stay in Offline, inline the literals. Either way, state the required merge order in both PR bodies and land Production#552 first.
- Evidence:
-
🔴 [S0]
STMResamplingProducerinstance rename breaks three Production drivers the companion PR does not touch- Evidence:
STMMC/src/STMResamplingProducer_module.cc:47,66changeproduces<StepPointMCCollection>()→produces<StepPointMCCollection>("virtualdetector")andevent.put(std::move(outputStepPointMCs), "virtualdetector"). Production#552 updatesBeamToVD.fclandBeamToVD1809.fclonly. Still on Productionmainand absent from #552's 16-file list:JobConfig/pileup/STM/BeamTo2VD.fcl:14,BeamTo2VD1809.fcl:14,NeutralsTo2VD.fcl:17, each of which hardcodesphysics.producers.compressDetStepMCsSTM116.stepPointMCTags : ["extractVirtualDetectorSteps116"]— a bare label, i.e. instance"".Compression/src/CompressDetStepMCs_module.cc:618resolves those withevent.getValidHandle<StepPointMCCollection>(i_tag), which throwsProductNotFoundon a miss. - Impact:
BeamTo2VD,BeamTo2VD1809andNeutralsTo2VDabort at the first event once this lands. These are the two-VD variants used for the VD116 studies. - Suggested fix: add
"extractVirtualDetectorSteps116:virtualdetector"to those three fcls in Production#552 (they are the only remaining bare-instance consumers).
- Evidence:
-
🟠 [S1] HPGe microspill charge carry-over is snapshotted before the deposits, so the buffer is permanently zero
- Evidence:
STMMC/src/HPGeWaveformsFromStepPointMCs_module.cc:288-292now runs at the top ofproduce(), ahead ofdepositCharge()at lines 308-319:The pre-PR code ran this block at the end of_chargeCollected.clear(); _chargeCollected.assign(_chargeCarryOver.begin(), _chargeCarryOver.end()); _chargeCollected.insert(_chargeCollected.end(), nADCs, 0); // tail := zeros _chargeCarryOver.clear(); _chargeCarryOver.assign(_chargeCollected.begin() + nADCs, _chargeCollected.end()); // reads the zeros
produce()and in the opposite order —_chargeCarryOverwas assigned from the tail first (after the deposits had landed there), and only then was_chargeCollectedrebuilt. Moving the block without swapping the two halves reversed the data dependency. Concretely, withnADCs_init = 543andmicrospillBufferLengthCount = 2:_chargeCollected= 543 old-carry-over values + 543 fresh zeros, and_chargeCarryOveris then assigned exactly that zero tail. It is all zeros for every event after the first. - Impact:
depositCharge()writes late deposits into_chargeCollected[i]fori >= nADCs(steps admitted by thestep.time() > microspillBufferLengthCount * micropulseTimecut at line 374), and those are wiped at the start of the next event. The delayed component thatmicrospillBufferLengthCountexists for — "charge deposits that are allocated to this event but happen after the microspill ends e.g. 844keV", per the member comment at line 150 — is silently discarded. This is a regression againstmain, and it is invisible: no exception, no counter, just missing charge. - Suggested fix: restore the original ordering — assign
_chargeCarryOverfrom the tail of_chargeCollectedafter the deposit loops, then rebuild_chargeCollectedfor the next event. If the block must stay at the top ofproduce()(e.g. for theresetEventNumberinteraction at 281-285), split it: rebuild_chargeCollectedat the top, snapshot_chargeCarryOverat the bottom.
- Evidence:
-
🟠 [S1]
digitize()clamps only the negative rail; positive overflow is undefined behaviour — carried over from the 2026-07-09 review, unaddressed- Evidence:
HPGeWaveformsFromStepPointMCs_module.cc:530with_adcs[i] = (ADC > ADCMax) ? static_cast<int16_t>(std::round(ADC)) : ADCMax;
ADCMax = static_cast<int16_t>((-1 * std::pow(2,15)) + 1)=-32767(line 111). Values below-32767are floored; nothing boundsADCabove+32767. Adoubleoutside theint16_trange converted bystatic_castis UB, so the result is whatever the target ISA's conversion does. The only downstream guard isif (j > 1000) throw cet::exception("LogicError", "ADC values too high!")at line 337 — it runs after the bad cast, and a hard-coded1000that aborts the job is itself a debug artefact rather than a saturation model. - Impact: with
NoiseSDnon-zero the baseline sits near 0 and positive excursions are reachable; the failure mode is either a wrapped ADC value written into the digi or a job abort with an unhelpful message. - Suggested fix:
_adcs[i] = static_cast<int16_t>(std::round(std::clamp(ADC, static_cast<double>(ADCMax), static_cast<double>(std::numeric_limits<int16_t>::max()))));and replace the> 1000guard with a saturation counter reported inendJob().
- Evidence:
-
🟠 [S1]
Efficiency.PhotonEnergy : @nilmakes all three Absorber drivers non-runnable, including one that worked before this PR- Evidence: the prolog rewrite changes
Efficiency.PhotonEnergyfrom1.809to@nil(STMMC/fcl/prolog.fcl,Efficiencytable). Three drivers consume it unconditionally:Absorber.fcl:31,AbsorberFromSTHPGe.fcl:34,AbsorberFromSTLaBr.fcl:34, allE : @local::Efficiency.PhotonEnergy.EventGenerator/src/PhotonGun_module.cc:47declaresfhicl::Atom<double> E— required, no default — and line 69 throws onE <= 0.0. - Impact:
@nilerases the key, so the reference does not deliver a value and the required atom fails validation at module construction.Absorber.fclran before this PR (PhotonEnergy : 1.809); after it, none of the three start. Note the same rewrite also (correctly) repairsAbsorber.fcl'smodule_type : MakeVirtualDetectorTree, a module that does not exist in the repo — so this file goes from broken-one-way to broken-another-way. - Suggested fix: keep
PhotonEnergy : 1.809in the prolog and let a driver override it. (TheLaBrDigitizationtable's@nilentries are fine — nothing references them.)
- Evidence: the prolog rewrite changes
-
🟡 [S2] Seven new members in
STMMovingWindowDeconvolutionare declared and never read- Evidence:
STMReco/src/STMMovingWindowDeconvolution_module.cc:118-125adds_isFirstWaveform,_last_ADC,_last_deconvolved,_deconvolved_history,_differentiated_history,_last_average, and (line 153)waveform_time_microspill_frame. Each string occurs exactly once in the file — the declaration. The twostd::deques carry the comment "Rolling history buffers to bridge the gap between chunks", i.e. they were added for a chunk-boundary continuity fix that is not in the diff.<deque>is also never included; it compiles only via a transitive boost/stdlib include. - Impact: dead state that reads as an implemented feature. A future reader will assume the chunk gap is handled.
- Suggested fix: delete them, or land the chunk-bridging logic they belong to. (Does not gate the decision.)
- Evidence:
-
🟡 [S2]
SimParticleAndVDBacktracecopies the wholeStepPointMCcollection per particle and per ancestor- Evidence:
STMMC/src/SimParticleAndVDBacktrace_module.cc:248const std::vector<StepPointMC> StepPointMCs = *stepHandle;copies the product; thenaddToTree,addParentToTreeandtraceStepsall takeconst std::vector<StepPointMC> stepsby value (declarations at lines 75-77, definitions at 158, 178, 213).addToTreerecurses up the parent chain (lines 204-210), copying the full collection at every hop, andtraceStepslinearly scans it each time (line 160). - Impact: O(N_particles x chain_depth) full-vector copies plus an O(N_steps) scan each, i.e. quadratic in a VD collection that can hold thousands of steps. This is a new module, so the cost is entirely introduced here.
- Suggested fix:
const std::vector<StepPointMC>&on all three signatures andauto const& StepPointMCs = *stepHandle;inanalyze.
- Evidence:
-
🟡 [S2] Per-microspill sample budget averages 543.6 ticks where the clock gives 542.4 — the concatenated timeline stretches
- Evidence:
HPGeWaveformsFromStepPointMCs_module.cc:273-278andnADCs = nADCs_init; // 543 at 320 MHz (line 197-199) eventTimeBuffer = eventId % 5; if (!(eventTimeBuffer == 0 || eventTimeBuffer == 3)) nADCs++;
eventTime += nADCsat line 361. Over any 5 consecutive microspills that emits543*5 + 3 = 2718ticks. The physical budget is5 * micropulseTime / tADC = 5 * 1695 ns / 3.125 ns = 2712ticks. The+1at line 197 ((micropulseTime/tADC) + 1, documented as "increase the number by one due to truncation") is a per-microspill ceiling, which is right for sizing one buffer but wrong as the base of a running total. - Impact: 6 extra ticks (18.75 ns) per 5 microspills, ~0.22 %. Over the
nMicrospillsPerSpill : 31858concatenation configured in the prolog that is roughly 24 µs of accumulated offset by the end of a macrospill, which lands directly on the MWD peak times. - Suggested fix: base
nADCs_initon the floor (542) with+1on 2 of every 5 microspills, or drive the pattern from an exact accumulator overmicropulseTime/tADC. Please confirm the intended tick budget against the DAQ spec — the old code (+= nADCs + 1wheneventId%5==0 || eventId%3==0) had a different, also non-exact, pattern, so this is a behaviour change either way.
- Evidence:
-
🟡 [S2] Committed drivers still carry
@nil"Populate me!" placeholders and one danglingmodule_type— carried over, partially addressed- Evidence:
HPGeWaveformGenerationAndAnalysis.fcl:90trigger_paths : @niland:99SelectEvents: @nil;ROOTAnalysisDump.fcl:103o1 : ["HPGeEDeps"] # Populate me!and:107;ROOTAnalysisDump.fcl:86setsmodule_type : MWDTreeand lines 93/96/99 setmodule_type : @nil. A code search overMu2e/OfflineforMWDTreereturns nothing — the module does not exist (STMMC/src/hasPHTree_module.ccandHPGeTree_module.cc). These are inert only because they sit off the active paths. - Impact:
HPGeWaveformGenerationAndAnalysis.fclcannot be run as shipped;ROOTAnalysisDump.fclships a reference to a non-existent module. The PR body states this file will not be brought up to the current analysis framework, so it will stay in this state. - Suggested fix: give both drivers a runnable default path (
trigger_paths : [digitization_path_ZS],SelectEvents : [digitization_path_ZS]), delete the@nil/MWDTreeanalyzer blocks, or relocate these toSTMMC/test/where non-runnable scaffolding is expected.
- Evidence:
-
⚪ [S3]
microspillBufferLengthCountguard message contradicts its own check — residual of prior finding 3HPGeWaveformsFromStepPointMCs_module.cc:192-193:if (microspillBufferLengthCount < 2) throw cet::exception("RANGE", "microspillBufferLengthCount has to be at least 1!\n");. The variable swap was the substance of the fix and is correct, but the bound is 2 (line 202 sizes_chargeCarryOverasnADCs * (count - 1)), so a user who sets1is told1is allowed. Change the message to "at least 2".
-
⚪ [S3] Carried-over typos still present
unqiueEnabledVDsVec—Analyses/src/CountVirtualDetectorHits_module.cc:55,60(the duplicate;;on that line was fixed).[BRAKCETS]—STMMC/fcl/HPGeWaveformGenerationAndAnalysis.fcl:10andSTMMC/fcl/ROOTAnalysisDump.fcl:10.- New:
mf::LogInfo log("HPGeWaevfrmsFromStepPointMCs")—HPGeWaveformsFromStepPointMCs_module.cc:536, misspelled message-facility category.
-
⚪ [S3] Dead and duplicated configuration state
STMMC/src/VirtualDetectorTree_module.cc:64addsuint simParticleIdKey = 0;— declared, never used (the tree fillssimParticleId, a different member).STMMC/fcl/prolog.fcldefinesComponentPositions.STandEfficiency.STwith identical values(-3904.0, 0.0, 627.0)in the same file — one should reference the other.DigitizationTester.HPGe.delta{x,y,z}are annotated# MeV/c, butPhotonGun::Config(lines 44-46) documentsdeltax/y/zas position differences used to build a direction;FirePhotonToDetector.fclnever reads them, so nothing catches the mismatch.- Please confirm
Efficiency.FromSTToDet.LaBr.z : 40264.5:ST.z + 40264.5 = 40891.5, whereasComponentPositions.LaBr.z(added in this PR) is40404. The HPGe pair is self-consistent (627 + 40072.1 = 40699.1=crystalCentreZin the module), so the LaBr offset looks like it targets a different reference point.
-
⚪ [S3] Includes and duplication in the new/modified modules
SimParticleAndVDBacktrace_module.ccusesstd::find(105, 163),std::fill(172, 174) andstd::distance(165) but does not#include <algorithm>; it compiles transitively.Analyses/src/CountVirtualDetectorHits_module.cc:6andHPGeWaveformsFromStepPointMCs_module.cc:12keep#include <bits/stdc++.h>— a GCC-internal header, non-portable and the opposite of the "include only headers actually needed" rule. Pre-existing in both, so not gating, butCountVirtualDetectorHitsis being edited here anyway and now only needs<algorithm>,<unordered_set>,<vector>.SimParticleAndVDBacktrace::addToTree(178-198) andaddParentToTree(213-235) duplicate ~15 lines verbatim. The comment at line 203 — "Require separate definitions of addParentToTree because of data accessors" — no longer applies now that both takeconst art::Ptr<SimParticle>&; they can be one function.
-
⚪ [S3]
overrideand check cadence inHPGeWaveformsFromStepPointMCsbeginJob()(line 78) and the newly addedendJob()(line 83) are declared withoutoverride; the base signatures match today, so they are called, but a base-class change would silently orphan them.- The
fADC == 320guard at lines 271-272 is a constructor-time invariant evaluated on every event. Move it next to thetADCcomputation at line 188.
-
⚪ [S3]
depositCharge()writes ~1000 dead values per StepPointMC- Lines 471-472 replaced
_charge[tIndex] = N_ehPairs;with a loop that fills the plateau across the entire remaining_chargebuffer. Only[tIndexStart, tIndex)is read by the_chargeCollectedupdate at 480-481, andstd::fill(_charge.begin(), _charge.end(), 0)at line 484 wipes the rest. Writing just_charge[tIndex] = _charge[tIndex - 1]preserves the (intended) continuity change at a thousandth of the cost.
- Lines 471-472 replaced
Observed, pre-existing, not gating
HPGeWaveformsFromStepPointMCs::addNoise()(lines 519-520) constructsstd::default_random_engine _randomGen;fresh inside the per-event call with the default seed, so every microspill receives the identical noise realization, and it bypassesSeedServiceentirely. Not introduced by this PR, but this PR reworks the charge path that feeds it (decayChargenow emitscurrentIntegrated), so the noise model is worth fixing in the same campaign: hoist to a member seeded viaart::ServiceHandle<art::RandomNumberGenerator>/SeedService.VirtualDetectorTree_module.cc:113still usesSimParticles.at(step.trackId()), which throwsstd::out_of_rangerather than acet::exceptionwith a useful category.SimParticleDumpwas fixed for exactly this ine500ede0; the sibling module was not.
Verified — no action needed
- 🟢 Build-system parity checked in both systems.
STMMC/CMakeLists.txtregisters both new plugins (SimParticleDumpat 60-66,SimParticleAndVDBacktraceat 68-74) withLIBRARIES REGmatching the siblingVirtualDetectorTreeentry (art_root_io::TFileService_service,Offline::GlobalConstantsService,Offline::MCDataProducts) — correct for their include sets;VirtualDetectorIdcomes in transitively viaMCDataProducts, same as the existingShiftVirtualDetectorStepPointMCsandVirtualDetectorTreeplugins.STMMC/src/SConscriptis a generichelper.make_plugins([...])glob over*_module.cc, so it needs no per-module edit. Noconfigure_fileinputs are involved;install_fhicl(SUBDIRS fcl SUBDIRNAME Offline/STMMC/fcl)(line 143) already covers the six new fcl files. No un-mirrored plugin and no missingLIBRARIES. - 🟢
Absorber.fclpreviously specifiedmodule_type : MakeVirtualDetectorTree— a module that exists nowhere in the repo — plus aVirtualDetectorId : 101key. This PR corrects both againstVirtualDetectorTree::Config(which declares onlyStepPointMCsTag,SimParticlemvTag,consecutiveEmptyFileThreshold). - 🟢
CountVirtualDetectorHitsorder change is intentional and safe: the oldstd::setsorted the IDs, the new path preserves config order, and Production#552 supplies an unsorted list ([10, 13, 15, 86, 115, 100, 101, 88, 89, 90]) whose report order now matches the config. - 🟢
HPGeWaveformGenerationAndAnalysis.fclmodule labels resolve:STMZeroSuppressionandConcatenateDigitizedWaveformsare both registered plugins;keep mu2e::STMPHDigis_MWDHPGe_*_HPGeReconow matches theMWDHPGeproducer label. - 🟢
mu2e/buildtestis green at3cedaf72(2026-08-04T06:53Z) along with all 14 validation jobs — note none of them exercise STMMC, which is why findings 1, 2, 5 and 9 are invisible to CI.
Validation check
- Build/tests run: yes, by Mu2e CI at the reviewed head —
mu2e/buildtestsuccess plusceSimReco,ceDigi,ceMix,ceSteps,cosmicSimReco,cosmicOffSpill,g4test_03MT,g4study,transportOnly,POT,muDauStepsall rc 0. No STM fcl is exercised by any of them, and the PR supplies no STM-specific validation evidence. - Config contract check: fail — findings 1 (unresolvable
@local::references against Productionmain), 5 (@nilfor a requiredAtom<double>), 9 (@nilpaths, non-existentMWDTree). - Cross-repo consistency: fail — finding 2 (three Production drivers not covered by Production#552) and finding 1 (hard, undeclared merge-order dependency on Production#552 in both directions).
Residual risk
- Findings 1 and 2 are release-blocking in the ordering sense: whichever of #1875 / Production#552 merges first leaves the STM pileup chain broken until the other lands, and #552 as written does not cover
BeamTo2VD/BeamTo2VD1809/NeutralsTo2VDat all. - Finding 3 is a silent physics regression. Any HPGe waveform sample already produced from this branch under-counts the delayed (844 keV-class) charge, and nothing in the job output signals it.
- Finding 8 changes the microspill time base relative to
main; MWD peak times from before and after this PR are not directly comparable. - The whitespace-only churn (
DAQ/src/STMBinaryDigisFromFragments_module.ccalone is +173/-173) inflates the diff and, per @AndrewEdmonds11's inline note, risks a merge conflict with a rename of that file. The physics changes are hard to isolate for review or forgit bisect.
Author follow-ups
- Decide the layering for
STMMC/fcl/prolog.fcl: move theDataProductstable into Production's STM prolog and drop#include "Production/JobConfig/pileup/STM/prolog.fcl"from Offline. (F1) - Add
:virtualdetectorto thestepPointMCTagsin ProductionBeamTo2VD.fcl,BeamTo2VD1809.fclandNeutralsTo2VD.fclin PR #552, and record the required Offline/Production merge order in both PR bodies. (F2) - Restore the carry-over ordering in
produce()so_chargeCarryOveris snapshotted afterdepositCharge(). Please add a short before/after check — e.g. a single microspill with a step att > 1695 ns— showing the charge appears in the next event's waveform. (F3) - Clamp
digitize()on both rails and replace the> 1000abort with a saturation counter. (F4) - Restore
Efficiency.PhotonEnergy : 1.809in the prolog so the three Absorber drivers run. (F5) - Either delete the seven unused MWD members or land the chunk-bridging logic they were added for. (F6)
- Pass the
StepPointMCcollection by const reference throughoutSimParticleAndVDBacktrace. (F7) - Confirm the intended ticks-per-microspill budget against the DAQ spec and reconcile the
543 + 3-in-5pattern with1695/3.125 = 542.4. (F8) - Give
HPGeWaveformGenerationAndAnalysis.fclandROOTAnalysisDump.fclrunnable defaults (or move them toSTMMC/test/), and drop theMWDTree/@nilanalyzer blocks. (F9) - Best practice reminder: please split the whitespace-only reformatting of
DAQ/,BTrkLegacy/,TrkReco/andRecoDataProducts/into a separate PR so the STM physics changes stand alone — this also addresses @AndrewEdmonds11's outstanding request. Answering your question about how: for files where only whitespace changed,git checkout main -- <path>restores them; to get past the hook for that commit usegit commit --no-verify(orSKIP=<hook-id> git commitfor a single hook). No need to disable the config permanently. - Your open question — "why was
STMMWDDigirenamed toSTMPHDigi?" — is still unanswered, and the PR leaves the naming inconsistent: the prolog now exposesSTMMWDDigiTag(STMMC/fcl/prolog.fcl,STMMCAnalysis.PH.{HPGe,LaBr}) while the data-product class is stillSTMPHDigi(keep mu2e::STMPHDigis_MWDHPGe_*_HPGeReco). Worth settling with the STMReco authors before merge so the config key and the product name agree.
5 participants
This PR addresses several issues found when completing the corrections to my normalization studies.
Changes are summarized in the bullet point list for each file affected by core code changes. Critically note - there are several other files that are also affected, but the
pre-commitconfig highlighted many changes that needed to happen before my changes could go in (after rebasing with branchmain). These changes have been addressed.CountVirtualDetectorHits_module.cc- this makes the vector of virtual detectors being summarized in the model unique, my mapping to and back from an unordered setPhotonGun_module.cc- this now allows for a set of photons to travel in a straight line from the origin to target position, defined by a direction vector asdelta_x,delta_y, anddelta_z. This vector is mapped to the unit vector, and the momentum is scaled accordingly.AbsorberFromSTHPGe.fcl- driver code that will allow future unfolding studies to be performed, especially important for the HPGe where additional absorber is present, and the results have not yet been fully quantified.AbsorberFromSTLaBr.fcl- as above but for LaBr.EventFilter.fcl- debugging tool to help with ongoing simulation studies. Allows for looking at individual events in a data file. For my studies, it was used to demonstrate the behaviour of the ZS and MWD algorithms on a macro-spill worth of beam-on time.FirePhotonToDetector.fcl- driver code that fires singular photons to a detector of choice, aimed for use with studying the STM detector waveform generators and analysis tools.STMMC/fcl/prolog.fcl- rewritten to define the simulation parameters more clearlyROOTAnalysisDump.fcl- driver code to convert the art files into ROOT format for easy plotting. I will not be rewriting this for the current analysis framework as I do not have the time for this.HPGeWaveformsFromStepPointMCs_module.cc- addresses issues with the waveform generator, namelySimParticleAndVDBacktrace_module.cc- new module generating ROOT files with data that backtraces of STM signal photons back through theSimParticleprovenance. The code to plot these will be merged in the STMAnalysis repositorySimParticleDump_module.cc- dumps the fullSimParticleprovenance to ROOT files for plotting, with a more compact format than the previous module.STMResamplingProducer_module.cc- Initial simulations had different data product names forSimParticles propagated with theBeamToVDandBeamToVD1809drivers, this change standardizes them.VirtualDetectorTree_module.cc- add more data to ROOT files generated for plotting.STMMovingWindowDeconvolution_module.cc- some more data for visualizing the behaviour of analyzed waveformsI would like to ask some questions
STMMWDDigirenamed toSTMPHDigi, and what is the difference in analysis chains?