Repository navigation
Extracted reco: record the KKLine track decision as KLPath without filtering the output - #587
Conversation
|
☀️ The build tests passed at 2883075.
N.B. These results were obtained from a build of this Pull Request at 2883075 after being merged into the base branch at ac5d9c9. For more information, please check the job page here. |
oksuzian
left a comment
There was a problem hiding this comment.
PR Review Summary — #587
Reviewed at head 288307561d63e6c65afe030dc7c80fcbacaba44b. first pass
Decision
- 🔴 request changes. The change does what it says for both Production configs, and recording the track decision instead of filtering is a nice way to get both views from one file. But three PassN tracker jobs on PassN
mainincludereco/Extracted.fclorrecoMC/Extracted.fcland then redefineRecoPath, and with this PR art rejects their schedule at startup (finding 1). The fix is small, in either repository.
Scope understood
reco/Extracted.fclandrecoMC/Extracted.fcladdKLTrackFilter(the productionKLFiltercuts, readingKKLine) and a trigger pathKLPath : [ @sequence::physics.RecoPath, KLTrackFilter ]. The output andSelectRecostill select onRecoPath, so the saved events do not change; theKLPathdecision is stored inTriggerResults::Reconstruct.
Findings
- 🟠 [S1] PassN
Tracker/TrkRecoFromDigi.fcl,TrkRecoFromDigiMC.fclandTrkRecoFromFragments.fclfail at startup with exit 9.- Evidence: each includes an
Extracted.fcland then sets its own, tracker-onlyphysics.RecoPath(makeSH, SimpleTimeCluster, CosmicShowerFilter, LineFinder, KKLine, KLFilter, SelectReco, plusmakeSD/EWMProducerfor the fragments job).KLPathwas already expanded from the full ExtractedRecoPath, so it keepsCaloClusterMaker,CrvCoincidenceClusterFinderand the other calo/CRV modules, andtrigger_pathsbecomes[RecoPath, KLPath].KKLineandSelectRecothen sit on both paths with different upstream modules, and art refuses the schedule:With Production at this head, all three exit 9 with that error; with ProductionModule KKLine on paths KLPath, RecoPath depends on Module CaloClusterMaker on path KLPath Module SelectReco on paths KLPath, RecoPath depends on Module CaloClusterMaker on path KLPath Module CrvCoincidenceClusterFinder on path KLPath Art has completed and will exit with status 9.main(ac5d9c91), all three build their schedule (rc 0). PassN atmain037b923, Offline and trig-config from CI backingbb141c56_ac5d9c91_32aec90c. - This is the case the description warns about ("A job that redefines
RecoPathafter including Extracted.fcl must rebuildKLPath"), but it already exists on PassNmain, and there is no PassN companion. - Impact: the standalone PassN tracker reconstruction (
TrkRecoFromDigi*andTrkRecoFromFragments, andTrkSHDFrom*, which include them) stops starting once this merges. No prodtools campaign json runs a PassN fcl, so production campaigns are not affected.Combined_Pass1.fclis not affected either: it includesTrkRecoFromFragments.fclbut sets its owntrigger_pathswithoutKLPath, soKLPathis defined but never scheduled. - Suggested fix, either of:
- a PassN companion that rebuilds
physics.KLPath : [ @sequence::physics.RecoPath, KLTrackFilter ]after eachphysics.RecoPathredefinition in those three files, merged together with this PR; or - here, keep
KLPathout oftrigger_pathsinExtracted.fclitself and add it only in the job files that want it, so an includer that rebuildsRecoPathis not affected.
- a PassN companion that rebuilds
- Evidence: each includes an
🟢 Verified at this head:
- Production configs change only as intended.
fhicl-dumpofreco/Extracted.fclandrecoMC/Extracted.fclat this head vs Productionmain: the only differences areKLPath, theKLTrackFiltertable andtrigger_paths : [ "RecoPath", "KLPath" ]. The output modules andSelectRecostill haveSelectEvents : [ "RecoPath" ],end_pathsis unchanged (EndPath), andkeep art::TriggerResults_*_*_*withprocess_name : ReconstructmeansReconstruct:KLPathis written. - The filter is in the right place and reads the right collection.
KLTrackFilteris@local::Reconstruction.filters.KLFilter, aKalSeedFilteron[ "KKLine" ], and it comes afterKKLineonKLPath. The shared producers run once per event, so the only added work is the filter. - The schedule is accepted for every Production includer. Building the schedule (0 events) gives rc 0 at this head for
reco/Extracted.fcl,recoMC/Extracted.fclandValidation/nightly/extracted_00.fcl/_03.fcl, as onmain. - No conflict with Production#582. Both edit these two files, #582 at the top and this PR at the end. A trial merge of both onto
mainis clean and the combined file carries both changes.
Validation check
- Build/tests run: FNALbuild passed at this head (merged at
ac5d9c91); no buildtest job runsExtracted.fcl. Locally:fhicl-dumpof both files at head andmain, a schedule build of the four Production includers and five PassN fcls at head andmain, and a trial merge with #582 (scripts inclaude-scratch/pr587_review/:fcldump.sh,passn_dump.sh,validate_passn.sh,run_passn.sh). Two PassN fcls fail on both sides for unrelated reasons and were set aside:TrkSHDFromDigineedsStrawHitDiag, which the CI backing does not build, andCombined_Pass1reaches endJob and fails only on geometry with 0 events. Not run: an event loop on Extracted data, since the on-disk mcs was already processed asReconstructand the raw/digi inputs are on/pnfs, which is not mounted here. Your 5000-event KPP comparison is the only execution evidence for the 52-eventKLPathselection. - Config contract check: pass for Production; fail for the PassN tracker jobs (finding 1).
- Cross-repo consistency: needs follow-up (finding 1).
Residual risk
- Any other job outside this repository that includes
Extracted.fcland redefinesRecoPathwill fail the same way. I searched Production and PassN; user configs elsewhere I cannot see.
Author follow-ups
- Fix the PassN tracker jobs, by a companion PR or by not scheduling
KLPathfromExtracted.fclitself (finding 1).
|
Thanks @oksuzian, finding 1 is right, and I could reproduce it. With this PR, On the two fixes: I would rather not take So I propose a two-line PassN companion instead: the tracker-only jobs opt out of the extra path right after their in
The For the residual risk (includers outside these two repos), the failure is loud and immediate (exit 9 at startup, |
oksuzian
left a comment
There was a problem hiding this comment.
PR Review Summary — #587
Reviewed at head fe97418dcdbc0b6fc149baa466670ff675a79baf. re-review of 2883075
Decision
- 🟡 comment only. The code is approvable: the one blocking finding from my first pass is fixed by the companion Mu2e/PassN#24. There is no FNALbuild result at this head yet, so I am holding the approval until the build is green.
Scope understood
- The new commit only extends the comment on the
KLPathline inreco/Extracted.fclandrecoMC/Extracted.fcl, naming the opt-out for jobs that redefineRecoPath. The processed configuration is unchanged.
Prior findings
-
🟢 [was S1] PassN tracker jobs exit 9 at startup — FIXED by Mu2e/PassN#24 (head
1bef26d4), verified. That PR setsphysics.trigger_paths : [ RecoPath ]right after the tracker-onlyRecoPathinTrkRecoFromDigi.fclandTrkRecoFromDigiMC.fcl. Building the schedule with no events, with Production at this head:job PassN main(037b923)PassN#24 TrkRecoFromDigi9 0 TrkRecoFromDigiMC9 0 TrkRecoFromFragments9 0 With PassN
mainthe failure is the same cross-path error as before (Module KKLine on paths KLPath, RecoPath depends on Module CaloClusterMaker on path KLPath). With PassN#24 all three end withtrigger_paths: [ "RecoPath" ].Combined_Pass1keeps its own four paths and noKLPath. PassN#24 also builds its schedule (rc 0) against Productionmain, so it can merge first, as its description says. I agree with keepingKLPathinExtracted.fclitself:recoMC/Extracted.fclis run directly by the mdc2025 and mdc2030 reco campaign configs, so moving the path out would lose the decision from the production output.
🟢 Verified at this head:
fhicl-dumpof bothExtracted.fclfiles is identical to the first-pass head apart from comments, withtrigger_paths : [ "RecoPath", "KLPath" ].- The four Production includers (
reco/andrecoMC/Extracted.fcl,Validation/nightly/extracted_00.fcland_03.fcl) still build their schedule (rc 0).
Validation check
- Build/tests run: FNALbuild passed at the first-pass head
2883075; at this headmu2e/buildtestreports "This test has not been triggered yet". Locally, against Offline and trig-config from CI backingbb141c56_ac5d9c91_32aec90c:fhicl-dumpof both files and a 0-event schedule build of the Production includers and the three PassN tracker jobs with PassNmainand with PassN#24. Not run: an event loop, for the same input reasons as in the first pass. - Config contract check: pass.
- Cross-repo consistency: pass, provided PassN#24 merges before or together with this PR.
Residual risk
- Jobs outside Production and PassN that include
Extracted.fcland redefineRecoPathwill still exit 9 at startup. The failure is loud and the new comment names the one-line fix, so I accept that.
Author follow-ups
- Merge Mu2e/PassN#24 first or together with this PR.
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for fe97418: build (Build queue - API unavailable) |
|
☀️ The build tests passed at fe97418.
N.B. These results were obtained from a build of this Pull Request at fe97418 after being merged into the base branch at ac5d9c9. For more information, please check the job page here. |
oksuzian
left a comment
There was a problem hiding this comment.
PR Review Summary — #587
Reviewed at head fe97418dcdbc0b6fc149baa466670ff675a79baf. Approval following my re-review at the same head.
Decision
- 🟢 approve
FNALbuild passed at this head (build 693, merged onto main at ac5d9c91): build, ceSimReco, ceDigi, ceMix, cosmicSimReco, cosmicOffSpill, g4surfaceCheck and rootOverlaps are all green, and clang-tidy reports nothing. My earlier finding is fixed by Mu2e/PassN#24, which I have approved. Please merge PassN#24 before or together with this PR.
brownd1978
left a comment
There was a problem hiding this comment.
This PR improves the current extracted production by restoring a KL fit filtered path. It doesn't make that the default reco path.
Outside the scope of this PR, we need to refactor our reco scripts to better support all use cases, both simulation studies and efficient processing of cosmic test data. I suspect we will need multiple output streams to efficiently handle disparate cases.
Motivation
Since 95956ca the Extracted reco (
reco/andrecoMC/Extracted.fcl) replacesKLFilterwith an always-trueFixedFilter, so track-less events are kept for CRV efficiency and tag-and-probe studies. A side effect is that theoutput no longer says which events had a selected track: on KPP cosmic data most saved events have none, which makes
event-display scanning and track-based analyses slow.
Change
Both files gain a second trigger path,
KLPath=RecoPath+ the productionKLFilter(instantiated asKLTrackFilter). The output module still selects onRecoPath, so the saved events are unchanged; theKLPathdecision is stored in
TriggerResults::Reconstruct. Readers pick their view without a second stream or dataset:SelectEvents : [ "Reconstruct:KLPath" ]on the analyzer or outputFillTriggerInfo : true,TriggerProcessName : "Reconstruct"gives atrig_KLPathbranch@local::Reconstruction.filters.KLFilterin a path of the reading jobKLPathis built fromphysics.RecoPathbecause art requires the producers a filter depends on to be on thefilter's own path; shared modules run once per event, so the added cost is the filter itself. A job that redefines
RecoPathafter including Extracted.fcl must rebuildKLPaththe same way(
physics.KLPath : [ @sequence::physics.RecoPath, KLTrackFilter ]).Validation
KPP run 124155 (trk.vst stream), 5000 events of
raw.mu2e.trk.vst.124155_000001.art, uncalibrated KPP data reco(
reco_data_vst.fclon top of Extracted.fcl), Offline nightly813ec61e0, EventNtuple3f5abca5. The same digiswere reconstructed with this branch and with
KLPatherased (= current configuration):RecoPath5000/5000,KLPath52/5000;SelectEvents : [ "Reconstruct:KLPath" ],trig_KLPath, and a read-timeKLFilter(on both files, including theone without
KLPath) select the same 52 event IDs.recoMC/Extracted.fclwas checked at configuration level only: it parses, and itsKLPathis itsRecoPath(including the MC truth-matching modules) plus
KLTrackFilter.Note on the cut values
KLPathuses the productionKLFiltercuts unchanged. On this uncalibrated sample 16% of events have a KKLinetrack but 1% pass:
minNStrawHits : 15removes most tracks (median 10 active hits). A use case that wants alooser selection can add its own path in the same way.
🤖 Generated with Claude Code