Phase III: decouple truth (TMS_TrueHit/TMS_TrueParticle) from TMS_Hit/TMS_Track - #301
Phase III: decouple truth (TMS_TrueHit/TMS_TrueParticle) from TMS_Hit/TMS_Track#301SFBayLaser wants to merge 7 commits into
Conversation
50b2542 to
0a262ce
Compare
e2e5041 to
871547d
Compare
0a262ce to
99234fe
Compare
871547d to
5537325
Compare
99234fe to
9dc3827
Compare
5537325 to
dfe98a1
Compare
First step of truth/reco decoupling (Phase III of the real-data restructuring proposal). Adds a stable per-hit HitId (TMS_Hit) and an event-level truth side table keyed by it (TMS_Event::TrueHitByHitId), following LArSoft's Hit/SimChannel association pattern rather than a positional-index array -- hits get sorted/merged/sliced throughout the pipeline, and a stable ID survives all of that for free while a positional index would need fragile manual reindexing at every mutation site. TMS_Hit's embedded TMS_TrueHit member is deliberately left in place this round (dual-write): TMS_Event::ProcessTG4Event() populates both the embedded member and the new side table from the same source data, and TMS_SignalProcessing::MergeCoincidentHits() runs a parallel by-ID merge alongside the existing embedded-value merge inside TMS_Hit::MergeWith(). This lets the two paths be cross-checked against each other before the embedded member is removed in a follow-up commit. Every consumer (TMS_TreeWriter, TMS_ReadoutTreeWriter, TMS_DetectorSimulation, TMS_Kalman, TMS_Utils::GetPrimaryIdsByEnergy, DrawEvents, CherryPickEvents) now reads truth via the new side table with null-checks where one didn't already exist, so a hit with no truth (e.g. eventually real data) degrades to skipped/sentinel output instead of undefined behavior. TMS_Kalman's constructor and TMS_Utils::GetPrimaryIdsByEnergy() needed a TMS_Event& threaded through from their one caller each. Also deletes TMS_Track::fTrueParticle and GetTrueParticle() -- confirmed completely dead (embedded by value, never set anywhere, never read anywhere) -- along with the ~80-line hand-rolled copy constructor/ assignment operator that existed only to shepherd it around. The real track-truth association already works close to the target pattern: computed lazily at tree-write time from a track's hits, ranked by energy, indexed into TMS_Event::TMS_TrueParticles. Verified bit-identical against the pre-Phase-III baseline on a real MC spill: identical slice/hit/track counts per spill, identical RecoHitBar/ TrueHitBar distributions, and a targeted field-by-field comparison of TrueHitPE/PEAfterFibers*/E/X/Y/Z/T (the trickiest piece -- the write-then- read round trip through the new side table inside TMS_DetectorSimulation) all confirmed exactly identical, not just statistically close. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
TMS_Hit no longer stores a TMS_TrueHit by value; truth now lives only in TMS_Event::TrueHitByHitId (Stage A's side table), looked up by HitId. This is what actually makes constructing a truthless TMS_Hit possible, since TMS_TrueHit's default constructor is deleted. TMS_Hit's constructor drops its edep-sim/vertex-id parameters (reco-level fields only); ProcessTG4Event() now builds the TMS_TrueHit separately and inserts it via SetTrueHit(). MergeWith() no longer touches truth at all -- TMS_SignalProcessing::MergeCoincidentHits() already merges the side-table entries by HitId (added in Stage A). The four GetTrueDistanceFrom*() helpers move to TMS_DetectorSimulation.cpp as free functions taking the true hit explicitly, since they no longer have an embedded TrueHit to call through. Verified bit-identical against the Stage A baseline: build produced no new warnings, and a full sand_drift run (3878 entries) matches field- by-field across Line_Candidates, Truth_Info, and the Readout tree (RecoHitBar/Pos/Energy/PE, TrueHitBar/X/Y/Z/T/E/PE/PEAfterFibers*, TrueLeptonic/HadronicEnergy, RecoTrackTrueVisibleEnergy, all 23 Readout fields). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…a smoke test Adds TMS_Hit(x, y, z, energy, time, pe) -- the real-data construction path Stages A/B made possible but didn't yet add. It builds the bar via TMS_Bar's existing-but-previously-uncalled (x, y, z) constructor (a pure geometry lookup, no TG4HitSegment/truth involved) and sets reco-level fields directly. Adds app/RealDataSmokeTest.cpp, which exercises the "reco path runs with the truth table entirely empty" claim end to end: it walks the geometry tree directly to find one real bar-leaf position per plane (coordinate guessing was tried first and reliably found nothing -- module daughters only span their own module's local slot, not the full detector width), builds truthless TMS_Hits at those positions, and runs them through TMS_SignalProcessing::MergeCoincidentHits() -> TMS_TimeSlicer::RunTimeSlicer() -> TMS_TrackFinder::FindTracks() -> TMS_TreeWriter::Fill(), confirming TrueHitByHitId has zero entries throughout. Verified via an actual run against a local edep-sim file (geometry source only, its TG4Event/truth branches are never read): completes with no crash, and the output ROOT file has zero NaN across every field in Line_Candidates/Reco_Tree/Truth_Info, with all truth-derived fields sitting at clean empty/zero sentinels rather than garbage. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
TMS_Event::AddEvent() (used by OverlayEvents() for pileup/spill overlay) moved incoming hits into TMS_Hits without touching their HitId or merging in the source event's TrueHitByHitId -- a gap in Phase III Stage A's original design, invisible before Phase III since truth was embedded directly in each TMS_Hit and moved with it automatically. Each incoming hit's HitId was assigned by its own event's independent, zero-based HitIdCounter, so after a naive move it either collides with an unrelated hit already in the host event's TrueHitByHitId (silently wrong truth) or exceeds anything the host has an entry for (a genuine gap). Confirmed via a real crash: with Deadtime/ZombieTime forced on, TMS_DetectorSimulation::SimulateOpticalModel() dereferenced a null TMS_TrueHit* on an overlaid event's hit. Fix: reassign a fresh HitId from the host event's own counter for each incoming hit, and carry its truth (if any) over under that new id. Verified: the deadtime-forced-positive run that previously crashed at event ~387 now completes cleanly (3878/3878 entries, both output files written). A default-config re-run is bit-identical, field for field, to both the pre-Phase-III baseline and the pre-fix Stage A/B/C output for the sand_drift file used throughout this validation -- this fix changes nothing already verified correct, it only closes a real gap that this specific dataset's default-config overlay pattern didn't happen to trigger. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
app/Makefile's target list is hand-maintained (no glob), so the new Phase III smoke-test app was never actually built by the plain `make` path (CI) -- only by CMake, which globs app/*.cpp into its own per-target list separately.
9dc3827 to
f0b5a58
Compare
dfe98a1 to
6d1ea6a
Compare
…geometry, not the detector-wide survey bbox TMS_Geom's XBarLength()/YBarLength()/XBarPos|NegReadoutLocation()/ YBarReadoutLocation() previously derived a "bar length" from GetXStartOfTMS()/GetXEndOfTMS()/GetYStartOfTMS()/GetYEndOfTMS() -- the detector-wide bar-region bbox from the geometry survey -- treating every bar as if it spanned the entire TMS. TMS_Bar already computes each bar's own true length from its GDML shape (GetBarLength()). This mattered because these functions feed TMS_DetectorSimulation's light-attenuation optical model (GetTrueDistanceFromReadout() and friends), so the survey bbox's precision was entangled with simulated PE yield and, downstream, pedestal-suppression hit survival -- confirmed via temporary instrumentation showing CleanHits()'s z-bounds rejection filter never fires on real data (zRejected=0), so the hit-count sensitivity to the survey bbox traced entirely through this path instead. Pass each hit's own bar length through explicitly; formula shapes (the pre-existing half-length-for-X vs full-length-for-Y asymmetry) are preserved unchanged, only the data source moves from the detector-wide survey to the specific bar.
|
I ran a mini-validation with all the PRs. PR 301 is not like the others. The numbers change a lot. I checked 301's commit before dune-tms open-PR mini-validationInput:
Targets
|
|
Interesting… |
|
I think the positions of the bars are incorrect. Here's ChatGPT's interpretation of the breakdown: The validation indicates that the large reconstruction change is introduced by the final per-bar optical-model commit. Before that commit ( Looking more closely, the issue appears to be the coordinate assumption used when calculating the new readout positions. XBarPosReadoutLocation = +0.5 * barLength;
XBarNegReadoutLocation = -0.5 * barLength;
YBarReadoutLocation = +0.5 * barLength;That does not match the detector layout. For X bars, The Y geometry is also not centred around global The existing detector-wide calculation is approximate, particularly for individual and tilted bars, but it uses approximately the correct global readout locations. A proper per-bar implementation needs each bar’s global centre and orientation in addition to its length, for example: I suggest removing the per-bar optical-model commit from this truth-decoupling PR so that the PR retains its intended reconstruction behavior. We can then make a separate PR for the per-bar correction using the bar’s full global geometry. That will also let us validate the optical-model change independently with PE, hit-survival, and reconstruction comparisons. |
…r, not origin-centering 36a69e5 correctly moved XBarLength()/YBarLength()/etc. from a detector-wide survey bbox to each hit's own TMS_Bar::GetBarLength(), but its readout-location formulas (XBarPosReadoutLocation() etc.) computed a bar's readout end as +-0.5*barLength alone, implicitly assuming every bar is centered at the detector's coordinate origin. Bars aren't: e.g. an X-bar runs from the detector edge in to x=0, so its readout end sits at roughly barLength (its own full extent from origin), not half of it. Reviewer jdkio caught this via a mini- validation across all four phase PRs: PR #301 alone tracked ~11-21% away from main while the others stayed within ~1-2%, and traced it to this commit. Fix: TMS_Bar now captures its own global-frame center along its readout axis (new AxisReadoutCenter member/GetAxisReadoutCenter(), set in FindModules() right before the existing x/y sentinel-overwrite that already discards that coordinate for other purposes) so the three ReadoutLocation functions can compute barCenter +- 0.5*barLength instead of +-0.5*barLength alone. XBarLength()/YBarLength() are left untouched -- they're pure relative-length quantities with no need for a global anchor. Verified against the sand_drift spill file: aggregate reconstruction metrics (event/track counts, multi-track fraction) now land within ~1-5% of main, matching the band the sibling phase PRs show, down from the previous ~11-21% swing. No crash across all 3878 entries. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Thanks for catching this, @jdkio — your diagnosis was exactly right. Fixed in Sanity-checked the fix with some temporary instrumentation before trusting it: readout locations now land near the actual detector edges (X ~±3500mm, Y-top ~+398mm) instead of ~1600mm/~1750mm (bar centers). Reran the same sand_drift file/mini-validation approach. Reproducing your table with a new row for the fixed head:
Back in the same ~1-5% band as the sibling PRs and your pre-bar-change reference, down from the ~11-21% swing. No crashes across all 3878 entries. I validated my own comparison script's metric definitions against your |
|
PS I am away until Monday and will, unfortunately, miss today's TMS Studies meeting. If still issues I can work on addressing them when I am back. |
2 participants
Fourth and final step of the real-data restructuring proposal (see #298/#299/#300 for Phases 1-2, which this builds on; also discussed in #nd_muon_spectrometer_code). Heads-up issue: #294.
Pulls truth (
TMS_TrueHit) out ofTMS_Hitand dead truth-particle scaffolding out ofTMS_Track, so reconstruction types can represent a hit/track with no truth at all (real data has none). Truth now lives in an event-level side table (TMS_Event::TrueHitByHitId), keyed by a stable per-hit id that survives sorting/merging/slicing -- mirrors LArSoft'sHit/SimChannelassociation pattern, since no real DAQ format has been specified for TMS yet.Done in three staged commits:
TMS_Hitkeeps its embeddedTrueHitand gains aHitId+ side-table entry, so both paths can be cross-checked.TMS_TrueHitmember fromTMS_Hitentirely, forcing every remaining call site through the side table.RealDataSmokeTest.cpp): builds hits directly from real detector geometry (no G4/truth involved at all), asserts they have no truth, and runs them through the full merge -> time-slice -> track-find -> tree-write pipeline end to end.Plus a follow-up fix for a real bug the smoke test's deadtime stress-test uncovered: event overlay (pileup,
AddEvent()) wasn't carrying per-hit truth across combined events -- each event'sHitIds were independently zero-based, so overlaying could either collide with an unrelated hit's truth or leave a dangling reference, causing a real segfault underDeadtime/ZombieTimesettings that exercise the affected code more heavily than the default config.Verification: build clean. Full-spill run bit-identical against the pre-refactor baseline at each stage (including a targeted array-level check of every truth/reco field, not just distribution stats), and again after the
AddEvent()fix -- both against the pre-refactor baseline and against the pre-fix buggy output. Deadtime forced on (Deadtime=500,ZombieTime=20) runs clean with no crash across all 9 overlaid spills.Closes #294.