Repository navigation
Configs to exercise CentralHelix fit and extrap in nightly validation - #580
Conversation
|
☀️ The build tests passed at 94f1a7f.
N.B. These results were obtained from a build of this Pull Request at 94f1a7f after being merged into the base branch at 7d83b5f. For more information, please check the job page here. |
oksuzian
left a comment
There was a problem hiding this comment.
PR Review Summary — #580
Reviewed at head 94f1a7f946976fd9ba362446a2759fc09657a013. first pass
Decision
- 🟢 approve. Nightly validation should exercise the CentralHelix fit and its CRV extrapolation, and the config is a clean delta on
JobConfig/recoMC/OffSpill.fcl. The notes below are questions, not blockers.
Scope understood
- New
Validation/offSpill.fcl. It reconstructsCosmicSignalOffSpilldigis through the stock OffSpill reco and replacesCHFilterwith a pass-allFixedFilter. It writes only the CentralHelix products, sending the LoopHelix output to/dev/null, for 10000 events. - Five
Validation/nightly/offSpill_0N.fcldrivers, oneMDC2025an_best_v1_1input file each, following theextracted_0N.fclpattern.
Verified, no action needed
- 🟢
fhicl-dumpresolves bothoffSpill_00.fclandoffSpill_04.fclagainst this head. The dump showsCHFilterasFixedFilterwithReturnValue: true,CentralHelixOutputselecting onCHPath, the per-job output file names,maxEvents: 10000, andDbServiceset toSim_best v1_5fromdatabase.fcl. - 🟢 The
CHFilteroverride does what the comment says. InJobConfig/reco/prolog.fcl:367-372,CentralHelixPathisPreTrk, OffSpillTrk, CHFilter, SelectReco.KKCHmuis fitted before the filter, so passing every event changes only what reachesSelectRecoand the output, not what is fitted. - 🟢 The keep list mirrors
Validation/reco.fcl, including theStageNormalizationkeep thatreco.fcldocuments as necessary.KalSeeds_KKCHmu,CosmicTrackSeeds_LineFinder,SimpleTimeClusterandCrvCoincidenceClustersare swapped in for the LoopHelix products. - 🟢 CI: the FNALbuild build test passed at this head, and all
mu2e/*contexts are green.
Findings
- 🟡 [S2] DB version versus input dataset. The inputs are
MDC2025an_best_v1_1digis, butdatabase.fclreconstructs them withSim_best v1_5.- That's the same pattern the existing
reco_0Njobs use (MDC2025au_best_v1_3inputs onv1_5), so it may be intended: validate current reco conditions on older digis. - When I reconstructed this same dataset for the KinKal#226 A/B, I used
Sim_best v1_1. I haven't checked whetherv1_5changes the CRV or calo calibration seen by these files. - If the intent is a stable reference, say so in the comment. Otherwise, consider pinning
v1_1. - Not run: a short job at this head (see Validation check).
- That's the same pattern the existing
- ⚪ [S3]
LHPathstill runs on every event. Its output goes to/dev/null, but theLoopHelixPathfits (KKDeetc.) are still done. Settingphysics.trigger_paths : [ CHPath ]would drop that work.- It would also keep
LHFilterout ofTriggerResults. - Not measured. The comment's "~0.2 s/event" may already include it.
- It would also keep
- ⚪ [S3] Not checked: how the nightly harness picks up these jobs. The other families with numbered drivers have a
nightly/<Family>/build_jobs.sh;extracted_0N.fcland these new files don't. I'm assumingoffSpill_*is picked up the same wayextracted_*is.
Validation check
- Build/tests run:
fhicl-dumpofoffSpill_00.fclandoffSpill_04.fclat this head, both rc=0. I didn't run an art job; an attempt was blocked in my session. The FNALbuild build test passed. - Config contract: pass, from the
fhicl-dumpoutput above. - Cross-repo: none. CentralHelix efficiency is under discussion in KFTrack/KinKal#226, and this job is how the nightly will see it.
Residual risk
- Low. The change is additive and touches only the nightly validation. The main open point is the DB question in finding 1.
Author follow-ups
- Confirm
Sim_best v1_5onMDC2025an_best_v1_1inputs is intended (finding 1). - Optional: drop
LHPathfromtrigger_paths(finding 2).
|
How does this not overlap the existing tests or can the existing test be adapted to or replaced by this test? Usually there is some discussion. |
|
Currently none of the existing tests run off-spill reconstruction, so they do not exercise the CentralHelix (mostly vertical cosmic muon) tracking nor the LoopHelix upstream e/mu fits. None of the existing tests use a digi dataset with field-on and cosmic muons that intersect the CRV, so extrapolation outside to the CRV cannot be tested using the existing configurations. I plan to open a PR with additions to Offline/Validation to histogram/compare CRV intersections for this dataset. Regarding the DB version, dbTool print-run --run 1430 gives the same calibration entries for all 20 tables in v1_1 and v1_5 Regarding turning off LHPath, CentralHelixPath and LoopHelixPath run the same reconstruction modules: preprocessing plus OffSpillTrk, which includes all four LoopHelix fits as well as the CentralHelix reco. They differ only in their output filter. |
|
I thought cosmicSimReco was field-on, but I see it is not, so we have two field-off cosmics. Some more options:
|
oksuzian
left a comment
There was a problem hiding this comment.
PR Review Summary — #580
Reviewed at head 253dc6c9d3f59ddb16b34c4814c5c8c70e7e9005. re-review of 94f1a7f946976fd9ba362446a2759fc09657a013
Decision
- 🟢 approve. The new commit only widens the output keep list and updates comments. The new keep patterns match products the job makes, and the prior DB question is now answered and documented.
Scope understood
- The increment is one commit,
253dc6c9, touching onlyValidation/offSpill.fcl:KalSeeds_KKCHmubecomesKalSeeds_KK*, andHelixSeeds_MH*andTimeClusters_MH*are added, so the single nightly file also validates the four LoopHelix fits on cosmics.- The DB-version comment and the timing comment are updated.
Verified, no action needed
- 🟢 The new keep patterns match real products. I ran
fhicl-dumponoffSpill_00.fclandoffSpill_04.fclat this head, and both returned rc=0. The only differences from the dumps at94f1a7f9are the three keep lines, plus the input file that differs between the two jobs.CHPathcontainsMHDe/MHUe/MHDmu/MHUmu.MergeHelices(TrkReco/src/MergeHelices_module.cc:77-78) producesHelixSeedCollectionandTimeClusterCollection.KK*coversKKDe/KKUe/KKDmu/KKUmu/KKCHmu, which are all inCHPath.KKLineis configured but not on any path, so it adds nothing.
- 🟢 The comment that
CHPathselects every event holds.CHFilteris aFixedFilterwithReturnValue: true. My local 100-event run ofoffSpill_01at94f1a7f9passedCHPathon 100 of 100 events, andKKDe/KKUe/KKDmu/KKUmu/KKCHmu/MHDeall ran on 100 of 100. - 🟢 The timing comment ("40-90 minutes") is consistent. The same run took 24.4 s CPU and 45.7 s wall for 100 events, which is about 41 to 76 minutes for 10000 events.
- 🟢 [was S2] DB version against the inputs: FIXED in
253dc6c9, verified. I randbTool print-run --run 1430forSim_bestv1_1andv1_5. Both list the same 21 tables with the same CIDs. The only difference is the IoV end ofTrkPanelStatusCID 110 (1499 versus 1439), and both IoVs cover run 1430. So the new comment, that both versions resolve to identical conditions for run 1430, is accurate.
Findings
- ⚪ [was S3] "
LHPathstill runs every fit": WITHDRAWN. art runs a module label once per event however many paths it appears on, andLHPathandCHPathshare every reco module, as the author pointed out. DroppingLHPathwould save onlyLHFilterand the/dev/nulloutput, not any fits. My earlier finding was wrong. - ⚪ [S3, carried over] Not checked: how the nightly harness picks up
offSpill_0N.fcl. As withextracted_0N.fcl, there's nobuild_jobs.shfor this family. I'm assuming it is picked up the same wayextracted_*is.
Validation check
- Build/tests run:
fhicl-dumpat this head (above) anddbTool print-runfor both DB versions. I didn't rerun the art job at this head, because the increment changes only the output keep list and comments. - CI: there is no FNALbuild build at this head yet.
mu2e/buildtestreports "This test has not been triggered yet". The build at94f1a7f9passed. - Config contract: pass, from the dump above.
- Cross-repo: pass. Mu2e/Offline#2023 adds the CRV-intersection histograms for this job's
KKCHmuoutput, and the two can merge in either order.
Residual risk
- Low.
- The output now carries five KalSeed collections. I didn't measure the file size at 10000 events.
- @rlcee's question about overlap with existing nightly jobs is a coverage decision for the maintainers, not a defect: replacing
cosmicSimReco, turning the field on there, or folding off-spill intoreco.
Author follow-ups
- None required.
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for 253dc6c: build (Build queue - API unavailable) |
|
☀️ The build tests passed at 253dc6c.
N.B. These results were obtained from a build of this Pull Request at 253dc6c after being merged into the base branch at 7d83b5f. For more information, please check the job page here. |
oksuzian
left a comment
There was a problem hiding this comment.
PR Review Summary — #580
Reviewed at head 253dc6c9d3f59ddb16b34c4814c5c8c70e7e9005. Follow-up to my comment review at this same head (pullrequestreview-5434185118), which held only for a build.
Decision
- 🟢 approve. FNALbuild's build at this head has passed ("The build tests passed at 253dc6c…"), and so has every
mu2e/*validation context. My earlier review at this head stands as written: the prior DB-version finding is fixed, theLHPathfinding is withdrawn, and the harness-pickup note is unchecked and does not gate.
|
replace cosmicSimReco with offSpill
No problem for me.
turn field on cosmicSimReco
The current job is CRV reco only, no track reco. It won't cover Rob's use
case.
add offSpill files to reco
OffSpill validation executes a different script than OnSpill. We would need
to go to full-on Pass1 reco for that to work.
…On Tue, Oct 6, 2026 at 11:57 AM Ray Culbertson ***@***.***> wrote:
*rlcee* left a comment (Mu2e/Production#580)
<#580 (comment)>
I thought cosmicSimReco was field-on, but I see it is not, so we have two
field-off cosmics. Some more options:
- replace cosmicSimReco with offSpill
- turn field on cosmicSimReco
- add offSpill files to reco
A while ago, I started an analysis of algorithm coverage in the
nightly, to try to be more efficient in the set of tests, but it didn't go
anywhere
—
Reply to this email directly, view it on GitHub
<#580?email_source=notifications&email_token=ABAH577VKJK5JWHG4FVP37T5SU6DPA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTMMBSGMZTCNJZGE3KM4TFMFZW63VMORSWC3K7NVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-6023315916>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABAH575YHLUU3FSOGXSPXOL5SU6DPAVCNFSNUABFKJSXA33TNF2G64TZHMZTKOJZGMYDKNRQHNEXG43VMU5TKNZSG4ZTIMZWGUZKC5QC>
.
You are receiving this because you are on a team that was mentioned.Message
ID: ***@***.***>
--
David Brown ***@***.***
Office Phone (510) 486-7261
Lawrence Berkeley National Lab
M/S 50R5008 (50-6026C) Berkeley, CA 94720
|
Currently the nightly validation does not exercise CentralHelix track fit or extrapolation.