Fix TAGH/TAGM tagger-overlap efficiency double-counting (closes #850) - #1153
Merged
Conversation
|
Build status for this pull request: FAILURE Build log: make_Hao_Dev_TaggerOverlapEfficiencyFix.log |
DBeamPhoton_factory_TAGGEDMCGEN previously derived its single output photon from MCGEN's already-collapsed system choice (TAGM-preferring), so when a thrown photon genuinely registered truth hits in both TAGH and TAGM (as real coincident detector hits do in their overlap regions, e.g. TAGH#127/TAGM#1), the reconstruction-efficiency denominator stayed at 1 while the (correctly un-merged) reconstructed DBeamPhoton numerator was 2, producing the ~2x efficiency spike in issue #850. Iterate over all primary (bg==0) TRUTH-tagged systems instead, and match each independently against the reconstructed list, so the denominator tracks the numerator symmetrically -- mirroring how DBeamPhoton_factory_TRUTH and the official flux tool (psflux) already treat TAGH/TAGM overlap hits as two independent, additive counts rather than collapsing them. Verified against MC test data spanning both known overlap windows (TAGH#127/TAGM#1 near 8.9 GeV, TAGH#179/TAGM#102 near 7.86-7.87 GeV): TAGGEDMCGEN counts now match reconstructed DBeamPhoton counts event-by-event in every observed case, including partial-overlap bins where only one of the two truth-tagged systems is actually reconstructed. Non-overlap bins are byte-for-byte unchanged. Fixes #850
DEventWriterROOT::Fill_ThrownTree collapsed to DBeamPhoton:TAGGEDMCGEN[0], writing exactly one row per event regardless of how many truth-tagged systems TAGGEDMCGEN actually holds. In TAGH/TAGM overlap regions, TAGGEDMCGEN correctly holds one entry per truth-tagged system (mirroring DBeamPhoton_factory_TRUTH, per the #850 fix in ef49cb4), and the reconstructed side is likewise left un-merged -- so collapsing the thrown side to a single row undercounts the "thrown" denominator relative to the reconstructed numerator whenever an analysis divides by this tree's row count (e.g. via the mcthrown_tree plugin) instead of querying DBeamPhoton:TAGGEDMCGEN object counts directly via hd_dump. Loop over all TAGGEDMCGEN entries and fill one tree row per entry, so overlap events produce two rows (matching TAGGEDMCGEN/DBeamPhoton/ TRUTH object counts) instead of one. Verified on 500 MC events at 8.880-8.890 GeV (full TAGH#127/TAGM#1 overlap): Thrown_Tree row count goes from 500 (1/event, always) to 905, exactly matching the DBeamPhoton and DBeamPhoton:TAGGEDMCGEN object totals for the same sample. Fill_DataTree (used by all physics reaction trees) has the same [0]-only pattern but is intentionally left untouched here -- out of scope for this change, much larger blast radius. Related to #850
Adds a ThrownBeam__IsTAGH boolean branch to Thrown_Tree (true = TAGH, false = TAGM/SYS_NULL, always written), so downstream analyses can tell apart the two rows an overlap event now produces after c3deea4. Also adds ANALYSIS:STORE_TAGGEDMCGEN_PER_SYSTEM_ROWS (default true), a JANA parameter toggling Fill_ThrownTree between the current one-row-per-TAGGEDMCGEN-entry behavior and the pre-c3deea4b6 single-row-per-event collapse, for consumers that need the old row count preserved. Verified via native build against both raw hd_root/mcthrown_tree output and the DSelector_Thrown_Tree/DSelector_antip__B4_Tree pipeline, at both single-bin and wider-range (8.85-8.92 GeV) statistics -- see work/halld_recon_issue_850/FINDINGS.md.
lihaoahil
force-pushed
the
Hao_Dev_TaggerOverlapEfficiencyFix
branch
from
July 29, 2026 02:58
e7d960f to
57136b3
Compare
|
Test status for this pull request: SUCCESS Summary: summary.txt Build log: make_Hao_Dev_TaggerOverlapEfficiencyFix.log |
sdobbs
approved these changes
Jul 29, 2026
sdobbs
left a comment
sdobbs
left a comment
Contributor
There was a problem hiding this comment.
looks good, thanks for the thorough work!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Base:
masterHead:Hao_Dev_TaggerOverlapEfficiencyFixSummary
DBeamPhoton_factory_TAGGEDMCGEN(the reconstruction-efficiency denominator source) derived its single output photon fromDBeamPhoton_factory_MCGEN's already-collapsed, TAGM-preferring system choice. In the small TAGH/TAGM overlap windows (TAGH#127/TAGM#1 near 8.9 GeV, TAGH#179/TAGM#102 near 7.86-7.87 GeV) a thrown photon legitimately registers truth hits in both systems — real detector behavior, correctly left un-merged by the reconstructedDBeamPhoton_factoryand correctly treated as two independent counts byDBeamPhoton_factory_TRUTHand the official flux tool (psflux). BecauseTAGGEDMCGENcollapsed to one entry regardless, the denominator stayed at 1 while the numerator was 2, producing a spurious ~2x efficiency spike inside the overlap windows.Full root-cause discussion, the fix rationale, and testing are posted on #850 — this PR implements what's described there.
Changes
a296bbcff—DBeamPhoton_factory_TAGGEDMCGEN::Process()now iterates over all primary (bg==0)"TRUTH"-tagged systems and matches each independently against the reconstructed list, instead of inheriting MCGEN's single collapsed choice. Non-overlap events unaffected; overlap events emit up to two entries, but only for truth systems that actually find a reconstructed match (partial-reconstruction still correctly reports 1).MCGENitself is untouched.c3deea4b6—DEventWriterROOT::Fill_ThrownTreehad the same[0]-collapsing bug on the thrown side; now writes oneThrown_Treerow perTAGGEDMCGENentry so the thrown-side denominator symmetrically tracks the fixed reconstructed-side numerator.Fill_DataTreehas the same pattern but is intentionally left untouched (used by all physics reaction trees — much larger blast radius, out of scope here).e7d960f25— Two additions for downstream consumers:ThrownBeam__IsTAGHboolean branch onThrown_Tree(true=TAGH, false=TAGM/SYS_NULL) so analyses can tell the two rows of an overlap event apart.ANALYSIS:STORE_TAGGEDMCGEN_PER_SYSTEM_ROWSJANA parameter (defaulttrue) to fall back to the exact pre-fix single-row-per-eventFill_ThrownTreebehavior, for anyone who needs the old row count preserved.Testing
Tested natively against the GlueX stack pinned by
version_7.5.0.xml(halld_recon baseline 5.11.0 + this branch, halld_sim 5.7.0, ROOT 6.32.20, geant4 10.07.p04, hdgeant4 3.3.1, gluex_MCwrapper v2.13.0). Full details and figures on #850.Notes for reviewers
STORE_TAGGEDMCGEN_PER_SYSTEM_ROWSdefaults totrue(the fixed behavior) — this changesThrown_Tree's row count (not its per-row content) inside overlap windows for every consumer, not just this analysis. The toggle exists specifically so anyone who needs the exact old row count can opt back in without a rebuild.resolves #850