Add CaloEntrantTruthMaker: calo-entrant truth for calo MC hits - #1911
Add CaloEntrantTruthMaker: calo-entrant truth for calo MC hits#1911zwl0331 wants to merge 3 commits into
Conversation
For each CaloHitMC energy deposit, walk SimParticle::parent() and store the calo-entrant ancestor (highest ancestor that also deposited in the same disk) in a new transient CaloHitEntrantCollection, index-parallel to the input CaloHitMCCollection. Crystal/disk resolution prefers CaloHitMC::crystalID() when filled; files produced before that member existed (reads back -1) fall back to the CaloCluster<->CaloClusterMC pairing. Cardinality or null-Ptr violations of that pairing throw rather than degrade. Analysis choices (purity cut, ambiguity, cluster IDs) deliberately stay downstream; the product records ancestry facts only. Registered in both build systems (CMakeLists cet_build_plugin + SConscript; MF_MessageLogger added to the SCons plugin link list for the mf logging call). Validated byte-exact against the Python ancestry-walk reference on 200 MDC2025 events under envset p103: 6554/6554 contribution roots identical (fingerprint-matched across ntuple formats).
|
☀️ The build tests passed at 3c8f625.
N.B. These results were obtained from a build of this Pull Request at 3c8f625 after being merged into the base branch at f96d9fc. For more information, please check the job page here. |
|
1. [S2] The legacy fallback asserts a positional invariant The module's comment claims the cluster↔clusterMC hit lists are "positionally-matched … the invariant CaloClusterTruthMatch establishes." What that producer actually does (
The per-index pairing 2. [S2] The product has no dictionary — and the cited precedent (
3. [S2] The cluster collections are hard dependencies even when they are not needed.
4. [S3] Robustness / style
|
|
📝 The HEAD of |
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for 3c8f625: build (Build queue - API unavailable) |
|
☔ The build is failing at 3c8f625.
N.B. These results were obtained from a build of this Pull Request at 3c8f625 after being merged into the base branch at e9b2fbd. For more information, please check the job page here. |
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for 3c8f625: build (Build queue - API unavailable) |
|
☀️ The build tests passed at 3c8f625.
N.B. These results were obtained from a build of this Pull Request at 3c8f625 after being merged into the base branch at ed58b12. For more information, please check the job page here. |
Review feedback on Mu2e#1911: - Legacy fallback no longer pairs cluster hit lists positionally. CaloClusterTruthMatch filters unmatched hits and re-sorts the MC list by energy, so only the collection-level zip is contracted. Each CaloHitMC referenced by a CaloClusterMC now inherits CaloCluster::diskID() directly (the disk is all the pairing was used for), tolerating unmatched hits instead of throwing on them. A ProductID cross-check rejects a CaloClusterMC built against a different CaloHitMCCollection than caloHitMCTag. - CaloHitEntrant gets a ROOT dictionary (classes.h + classes_def.xml) and is persistable like every other Mu2e data product. Each entry carries an art::Ptr<CaloHitMC> back-reference, so the product is self-describing and consumers can verify source identity instead of trusting configuration. Members are private with accessors. - caloClusterTag/caloClusterMCTag are optional (mayConsume, empty default, must be set together): on files with CaloHitMC::crystalID() filled the module runs without any cluster products. A once-per-job LogWarning flags legacy files with no fallback configured (entrants null). - simDisks/entrantCache keyed by art::Ptr<SimParticle> (ProductID + key) instead of bare SimParticle id, removing the single-collection assumption; the parent walk throws a clear message on an unreadable parent Ptr (dropped ancestor collection); conventional Parameters alias added. Re-validated against the Python ancestry-walk reference on 200 MDC2025 events: 3903/3903 MC hits matched, 6554/6554 contribution roots identical. Smoke-tested: no-cluster-tags job runs with all-null entrants plus warning; a keep-everything RootOutput job writes the product branch (write-side smoke; full read-back done in independent review).
Independent review of the previous commit found three gaps: - The cluster-wide disk fallback assumed disk-local clusters, but association strategy 2 (ClusterAssociator) can merge proto-clusters across disks while CaloCluster keeps only the seed's disk; such a cluster would have received silently wrong truth. The fallback now verifies every reco hit of a cluster sits on the cluster's disk and throws on a mixed-disk cluster (the filtered, energy-sorted MC list cannot recover per-hit disks there). Configure the cluster tags only when the fallback is actually needed. - The HMC-key -> disk map was unchecked last-write-wins. Keys are now bounds-checked against the collection, and a conflicting second disk assignment throws with both cluster indices and both disks. - The parent-chain walk guarded ancestor Ptrs but dereferenced the depositor SimParticle Ptr unguarded; sim.isAvailable() is now checked before the first dereference, with the same diagnostic style. No behavior change on disk-local, uniquely-referenced input: the same 200 MDC2025 events revalidate identically (200/200 events, 3903/3903 MC hits, 6554/6554 contribution roots, PARITY OK; no-tags and RootOutput smokes re-pass).
|
Thanks for the careful review. All four points are addressed in the two commits now on the branch, and the description is updated to match.
Revalidated end to end after the changes: 200 MDC2025 events, 3,903 MC hits, 6,554 contributions, all entrant assignments identical to my Python reference; the no-tags job (5 events, 94 hits, 154 contributions, all -1, one warning) and the write/read-back job pass as above. |
|
@FNALbuild run build test |
oksuzian
left a comment
There was a problem hiding this comment.
PR Review Summary — re-review of "Add CaloEntrantTruthMaker" (#1911)
Reviewed at head af6d54c1 (2026-08-02). Delta since the previously reviewed 3c8f6257: two commits (d3ba04d8, af6d54c1), 4 files, +185/−78, responding to the four-finding review posted 2026-07-31. Every prior finding was re-verified in the code at the new head, not taken from the reply. EventNtuple#392 companion was updated in lockstep (abd1e279 verifies entrant source identity via the product's new CaloHitMC Ptr).
Decision
- 🟢 Approve. All four prior findings are fixed and verified; the two new observations are nits. The only gate left is procedural: FNALbuild has not been triggered at
af6d54c1(author reports a clean local build + byte-identical revalidation post-#1908 merge, but the repo's own CI must confirm before merge).
Carry-forward accounting (prior review of 3c8f6257, posted on the PR 2026-07-31)
1. 🟢 [was S2] Positional cluster↔clusterMC pairing — FIXED in d3ba04d8/af6d54c1, verified.
The fallback now zips the two collections only at collection level (one CaloClusterMC per CaloCluster in input order — the invariant CaloClusterTruthMatch actually provides) and maps each referenced CaloHitMC key to clusters[ic].diskID() directly. No per-hit positional pairing remains; truth-unmatched hits are tolerated (the per-cluster size throw is gone; the surviving clusters.size() != clusterMCs.size() throw checks a real invariant). Beyond the prescription, the fallback is hardened fail-closed: ProductID cross-check of every referenced Ptr against caloHitMCTag, key bounds check, conflicting-disk-claim throw naming both clusters, and — a genuinely new catch by the author — per-cluster disk-locality validation, since cluster-association strategy 2 can merge proto-clusters across disks while CaloCluster keeps only the seed's disk. A mixed-disk cluster now throws rather than receiving a guessed disk. Failure paths exercised with synthetic inputs per the reply.
2. 🟢 [was S2] No dictionary + phantom CaloHitGraph precedent — FIXED in d3ba04d8, verified.
classes.h include added; classes_def.xml gains CaloHitEntrant, the collection, and the art::Wrapper. Member-type dictionaries confirmed pre-existing (art::Ptr<mu2e::CaloHitMC> line 193, std::vector<art::Ptr<mu2e::SimParticle>> line 57). The product is now persistable, verified by the author with a write/read-back RootOutput job. The provenance question is settled the right way: each entry carries an art::Ptr<CaloHitMC> back-reference, making the product self-describing. The PR description no longer cites the nonexistent precedent and says so explicitly.
3. 🟢 [was S2] Hard cluster dependencies — FIXED in d3ba04d8, verified.
caloClusterTag/caloClusterMCTag default to empty with mayConsume; a ctor check requires both-or-neither; the module runs cluster-free wherever crystalID() is filled, warns once per job when unresolved entrants remain with no fallback, and needs no prolog entry (consumers wire it explicitly — this also answers the author's original open question). Documented consequence, stated in the PR body: when the tags are set, disk-locality validation runs for all clusters, so set them only when the fallback is needed.
4. 🟢 [was S3] Robustness/style batch — FIXED in d3ba04d8/af6d54c1, verified.
simDisks/entrantCache keyed by art::Ptr<SimParticle> (ProductID+key); ProductID cross-check implemented (see 1); isAvailable() guards on both the depositor and every ancestor, throwing with the module name and the dropped collection; using Parameters = art::EDProducer::Table<Config> alias in; CaloHitEntrant members private with accessors.
No other reviewers' change requests exist on the PR; nothing else to carry.
New findings (delta 3c8f6257..af6d54c1)
1. ⚪ [S3] The new disk-locality loop dereferences reco-hit Ptrs without an isAvailable() guard.
- Evidence:
CaloEntrantTruthMaker_module.ccfallback validation —hit.isNull()is checked buthit->crystalID()would throw a generic art exception on a file where theCaloHitCollectionwas dropped. - Impact: potential risk only — legacy mcs files that carry clusters carry their hits; this is the same fail-loud-but-anonymous pattern the SimParticle walk just got guards for.
- Suggested fix: mirror the
isAvailable()+ named-module throw used for the SimParticle Ptrs, for consistency. Optional.
2. ⚪ [S3] Redundant dictionary selection.
- Evidence:
classes_def.xmllists bothmu2e::CaloHitEntrantCollectionandstd::vector<mu2e::CaloHitEntrant>— the same type through the alias. - Impact: harmless duplicate selection; neighboring product blocks list only one form.
- Suggested fix: drop one line. Optional.
Validation check
- Build/tests: 🟡 FNALbuild has not been triggered at
af6d54c1(mu2e/buildtest: "This test has not been triggered yet"); the last green CI is at the old head. Author's local evidence is strong (full build vs current main incl. #1908, byte-identicalentrantSimIds, 200-event revalidation, no-tags job, write/read-back job, synthetic fail-path fixtures) but CI must confirm. - Config contract: 🟢 validated
Config, optional tags with both-or-neither enforcement, no prolog entry by design. - Cross-repo: 🟢 EventNtuple#392 updated in lockstep (
abd1e279); merge order Offline first, unchanged.
Residual risk
- None blocking. The fallback is now strictly fail-closed; the cost is that legacy strategy-2 (cross-disk-cluster) files abort rather than proceed — a deliberate, documented choice.
Author follow-ups
- Trigger
@FNALbuild run build testataf6d54c1(required for merge anyway). - Optional ⚪s:
isAvailable()guard on reco-hit Ptrs in the validation loop; dedupe the dictionary selection. - The synthetic fail-path fixtures would make good permanent fail-path tests — worth a follow-up PR if cheap to port.
|
⌛ The following tests have been triggered for af6d54c: build (Build queue - API unavailable) |
|
☀️ The build tests passed at af6d54c.
N.B. These results were obtained from a build of this Pull Request at af6d54c after being merged into the base branch at f408474. For more information, please check the job page here. |
This adds a producer that assigns each energy deposit in a CaloHitMC to its "calo-entrant" ancestor, i.e. the highest ancestor in the Geant4 parent chain that also deposited energy in the same disk. The point is to group secondary shower products (brem photons, conversion pairs, etc.) with the particle that started the shower, rather than treating every SimParticle as its own truth object. I need this for GNN clustering truth labels, but it is a generic truth definition, so it lives in CaloMC rather than in my analysis code.
Background: I originally tried to do this at the ntuple level by persisting the ancestor chains (EventNtuple#366). That was closed since truth matching like this belongs in an art module. This is that module. It walks SimParticle::parent() live, and only the resulting assignment is stored, no genealogy anywhere.
New/changed files:
MCDataProducts/inc/CaloHitEntrant.hh(+classes.h/classes_def.xml): a persistent data product with a ROOT dictionary, like every other Mu2e product. One entry per CaloHitMC, holding anart::Ptr<CaloHitMC>back-reference to the hit it was computed for plus a vector ofPtr<SimParticle>entrants aligned withenergyDeposits(). The collection is index-parallel to the input CaloHitMCCollection, but consumers do not have to trust that or the configuration: the back-reference identifies the exact source collection and entry, and the companion EventNtuple fill verifies it per entry. (The earlier revision of this PR described the product as transient with no dictionary and cited a nonexistent precedent; both corrected after review.)CaloMC/src/CaloEntrantTruthMaker_module.cc: the producer. Walks the parent chain for each (SimParticle, disk), stopping at the chain root, and caches results per (Ptr, disk). An unreadable depositor or ancestor Ptr (dropped collection) throws with a message naming the problem.Registered in both CMake and SConscript.
Disk resolution: the module uses CaloHitMC::crystalID() when it is filled. In older productions it is not (MDC2025 predates the member, and the CaloHit/CaloHitMC Assns does not survive compression), so there it falls back to the CaloCluster/CaloClusterMC pairing. The cluster tags are optional and default to empty, so consumers wire the module explicitly and no prolog entry is needed; configure the tags only when the fallback is actually needed. The fallback takes only the disk, a cluster-level property, from the pairing (CaloClusterTruthMatch filters and re-sorts the per-cluster MC hit list, so hits are never paired positionally), tolerates truth-unmatched hits, and cross-checks the ProductID of every referenced CaloHitMC against caloHitMCTag, with key bounds and conflicting-disk checks on top. Because a cluster-wide disk is only meaningful for a disk-local cluster, the fallback validates every reco hit of each cluster against the cluster's diskID and aborts on a mixed-disk cluster (possible under cluster association strategy 2) rather than guessing; note this validation runs for all clusters whenever the tags are configured, even where crystalID() would have sufficed. CaloHitMC entries not referenced by any cluster get null entrants in fallback mode, and the module warns once per job if that leaves entrants unresolved with no fallback configured.
Validated on 200 MDC2025 events (run 001430): the assignment agrees with my existing Python implementation (which walks the ancestor chains persisted in files made with the old EventNtuple#366 patch) for all 6,554 contributions. Flattened through EventNtuple, the GNN truth labels built downstream come out byte-identical to my published pipeline (341/341 disk-graphs; a second file, 6,951 contributions, gives the same). The branch also builds cleanly and revalidates with identical output when merged with current main including the #1908 CalorimeterGeom rewrite, the persisted product has been written and read back through RootOutput, and the fallback's failure paths (mixed-disk cluster, out-of-range key, conflicting disk claim) have each been exercised with synthetic inputs.
Companion EventNtuple PR that flattens this into calohitsmc: Mu2e/EventNtuple#392