Repository navigation
Save passive-material xings in the Extracted and OffSpill reco configs - #582
Conversation
… and KKLine (extracted) so CRV strongback crossings are histogrammed
|
☀️ The build tests passed at a17e26d.
N.B. These results were obtained from a build of this Pull Request at a17e26d after being merged into the base branch at 0d1c370. For more information, please check the job page here. |
|
Did you ask (probably Dave or Yuri) if this can/should be put in Production/JobConfig/recoMC/Extracted.fcl? |
oksuzian
left a comment
There was a problem hiding this comment.
PR Review Summary — #582
Reviewed at head a17e26d474f8a4657b8ffd339751d3aad488bfcc. first pass
Decision
- 🟢 approve. Each job changes by exactly the one key described, and nothing else changes.
Scope understood
- Nightly validation only.
SaveMaterialXingsis turned on forKKCHmuinoffSpilland forKKLineinextracted. That fillsValKalSeed'sNCRVSBhistogram, which has read 0 in every nightly since #580 and Offline#2023. - The new
Validation/extracted.fclwrapsrecoMC/Extracted.fclanddatabase.fcl. The fivenightly/extracted_0*.fclinclude it instead of those two files.
Findings
- None.
Verified
-
🟢 One key per job, in the intended table. I ran
fhicl-dumpat this head against Offline mainf122c5b3, and all rc are 0. Compared with Production main:offSpilldiffers only inKKCHmu.KKFitSettings.SaveMaterialXings: true;nightly/extracted_00andextracted_03differ only inKKLine.KKFitSettings.SaveMaterialXings: true;- their input file lists and output names are unchanged.
The key is
Mu2eKinKal::KKFitConfig::saveMatXings(KKFitSettings.hh:105), so validated fhicl accepts it. -
🟢 The histogram this feeds exists.
ValKalSeed.cc:194-216countsSurfaceIdDetail::CRV_StrongBackintersections intoNCRVSB. -
🟢 The wrapper follows the existing pattern.
nightly/reco_0*andnightly/offSpill_0*already include aValidation/<job>.fclwrapper. Onlyextractedincluded theJobConfigfile directly. Nothing outside the five nightly files includesValidation/extracted.fcl. -
🟢 No conflict with #583. Both PRs edit
Validation/offSpill.fcl. The merged tree is clean, and its dump carries both changes and nothing else. -
On @rlcee's question about putting this in
JobConfig/recoMC/Extracted.fcl: that file is in production use. The prodtoolsmdc2025reco campaign names it at six entries, andmdc2030at one. Moving the setting there would change campaign output, about +0.8% onKalSeeds_KKLineby your numbers. Keeping it inValidation/leaves production untouched, so it fits a validation-only PR. Whether production wants it is for the reco conveners.
Validation check
- Build/tests run: FNALbuild passed at this head, but no buildtest job runs the
Validation/offSpillorextractedjobs. Locally I ranfhicl-dumpat main, at this head and merged with #583 (claude-scratch/pr583_582_review/fcldump.sh). I did not run an art job, because the inputs are on tape and I have no valid token. The author's 1000-event before/after comparison was not reproduced. - Config contract check: pass.
- Cross-repo consistency: pass. Offline#2023 and Production#580 are merged.
Residual risk
- Validation only. Expect one nightly FAIL on
NCRVSBandNInter, as the PR says.
Author follow-ups
- None.
brownd1978
left a comment
There was a problem hiding this comment.
As suggested by @rlcee this fix belongs in the reco config.
…KCHmu) reco configs instead of the nightly validation configs; drop the obsolete TCRV sample surface
|
I added the line to the production config, as requested. I tested the impact on CH OffSpill reco and found that it was negligible (no measurable change in CPU time, <1% increase in art output size). |
oksuzian
left a comment
There was a problem hiding this comment.
PR Review Summary — #582
Reviewed at head 2889c7cae30fd447e995b6cbc7dba3e71bcf0c8c. re-review of a17e26d4
Decision
- 🟡 comment for now; the code is approvable. Moving the setting into the reco configs is what @brownd1978 and @rlcee asked for. Each job changes only in
SaveMaterialXings, plus theTCRVremoval, which was already a no-op. The fit itself does not change. This now reaches production campaigns, so the scope is set out below for the record. There is no build at this head yet, so I am triggering one (with mu2e-trig-config#176, which Offlinemaincurrently needs) and will approve when it is green.
Scope understood
SaveMaterialXings : truemoves from the two validation wrappers intoJobConfig/reco{,MC}/Extracted.fcl(KKLine) andJobConfig/reco{,MC}/OffSpill.fcl(KKCHmu).Validation/extracted.fclis deleted, and the fiveextracted_0*nightlies includerecoMC/Extracted.fclplusdatabase.fcldirectly, as onmain."TCRV"is dropped fromKKLine.ModuleSettings.SampleSurfacesin bothExtracted.fcl.
Findings
- ⚪ [S3]
JobConfig/recoMC/NoFieldRun1B.fcl:11still appends"TCRV"toKKLine'sSampleSurfaces, with the same "temporary until extrapolation is working" comment. It is a no-op there too (see below), so dropping it in the same commit would finish the cleanup.
🟢 Verified at this head:
- Only the intended keys change. I ran
fhicl-dumpon all 12 affected files (the four job configs,Validation/offSpill.fcl, the fiveextracted_0*nightlies,offSpill_00and the deleted wrapper) at Productionmain(efebc0a2), ata17e26d4, at this head, and with this head merged ontomain. Offlinemainwasb8cfd1d7throughout. Every dump parses.- Merged onto
main, eachExtracted.fcland each nightly differs only inKKLineSaveMaterialXings: false → trueand the missing"TCRV". - Each
OffSpill.fcldiffers only inKKCHmuSaveMaterialXings. Validation/offSpill.fclandoffSpill_00dump the same as ata17e26d4; the setting now comes fromrecoMC/OffSpill.fcl.- The branch predates #583. The merge with
mainis clean and keeps #583's field-correction settings.
- Merged onto
- The flag changes storage, not the fit.
KKFit.hh:902is the only reader ofSaveMaterialXings. It copies the passive crossings that extrapolation already made (materialCylXings,materialPlaneXings) intoKalSeed::intersections(). No new product is made, andkeep mu2e::KalSeeds_K*_*_*already writesKKLineandKKCHmu. The size numbers in the description (+0.07% offSpill, +0.32% extracted) are yours; I did not rerun them. - Dropping
TCRVchanges nothing.TCRV(200) is marked "deprecated" inDataProducts/inc/SurfaceId.hh. No surface with that ID is ever added to the KinKalGeom map (GeometryService/src/KinKalGeomMaker.cc), sokkg.surfaces()returns nothing for it, and sampling atTCRVnever produced an intersection. A Mu2e org code search finds no reader of theTCRVsurface ID; the Stntuple hits areTCrv*class names. - Downstream readers.
ValKalSeedchanges onlyNInterandNCRVSB, as you expect for the nightly.- ArtAnalysis
CrvInferencepicksTT_Midby surface ID, so the extra entries do not affect it. - EventNtuple
fillTrkSegInfocopies every intersection intosegswith itssid. Ntuples from these jobs will therefore carry extrasegsentries:sid280 (CRV_StrongBack) for extracted, and DS shell, coil, absorber and hatch-concrete IDs for off-spill. That is the intent, but anyone who countssegswithout selecting onsidwill see a change.
Blast radius
recoMC/Extracted.fclis used bymdc2025CosmicCRYExtracted(Triggered)Reco(6 entries inreco.json) and bymdc2030(1 entry).recoMC/OffSpill.fclis used bymdc2025CosmicSignalOffSpillTriggered(2 entries).reco/Extracted.fcl(data) is included by PassNTracker/TrkRecoFromDigi.fcl, whose epilog does not overrideSampleSurfacesorSaveMaterialXings. So PassN tracker reco gets the same change.reco/OffSpill.fclhas no consumer in the org.- The campaigns run pinned SimJob musings, so none of this reaches them until the next SimJob tag.
Validation check
- Build/tests run: no FNALbuild run at this head yet (
mu2e/buildtest: "This test has not been triggered yet").a17e26d4passed. Locally:fhicl-dumpas above (claude-scratch/pr582_r2/fcldump.sh, dumps indumps/). No art job run; your 1000-event before/after on gpvm is the only execution evidence. - Config contract check: pass.
- Cross-repo consistency: pass. PassN inherits the change, which is consistent with what it does.
Author follow-ups
- Optional: drop the leftover
"TCRV"inrecoMC/NoFieldRun1B.fcl(finding 1).
|
@FNALbuild run build test with Mu2e/mu2e-trig-config#176 |
|
⌛ The following tests have been triggered for 2889c7c: build (Build queue - API unavailable) |
|
☀️ The build tests passed at 2889c7c.
N.B. These results were obtained from a build of this Pull Request at 2889c7c after being merged into the base branch at efebc0a. For more information, please check the job page here. |
oksuzian
left a comment
There was a problem hiding this comment.
PR Review Summary — #582
Reviewed at head 2889c7cae30fd447e995b6cbc7dba3e71bcf0c8c. re-review of a17e26d4
Decision
- 🟢 approve. Moving the setting into the reco configs is what @brownd1978 and @rlcee asked for. Each job changes only in
SaveMaterialXings, plus theTCRVremoval, which was already a no-op. The fit itself does not change. This now reaches production campaigns, so the scope is set out below for the record.
Scope understood
SaveMaterialXings : truemoves from the two validation wrappers intoJobConfig/reco{,MC}/Extracted.fcl(KKLine) andJobConfig/reco{,MC}/OffSpill.fcl(KKCHmu).Validation/extracted.fclis deleted, and the fiveextracted_0*nightlies includerecoMC/Extracted.fclplusdatabase.fcldirectly, as onmain."TCRV"is dropped fromKKLine.ModuleSettings.SampleSurfacesin bothExtracted.fcl.
Findings
- ⚪ [S3]
JobConfig/recoMC/NoFieldRun1B.fcl:11still appends"TCRV"toKKLine'sSampleSurfaces, with the same "temporary until extrapolation is working" comment. It is a no-op there too (see below), so dropping it in the same commit would finish the cleanup.
🟢 Verified at this head:
- Only the intended keys change. I ran
fhicl-dumpon all 12 affected files (the four job configs,Validation/offSpill.fcl, the fiveextracted_0*nightlies,offSpill_00and the deleted wrapper) at Productionmain(efebc0a2), ata17e26d4, at this head, and with this head merged ontomain. Offlinemainwasb8cfd1d7throughout. Every dump parses.- Merged onto
main, eachExtracted.fcland each nightly differs only inKKLineSaveMaterialXings: false → trueand the missing"TCRV". - Each
OffSpill.fcldiffers only inKKCHmuSaveMaterialXings. Validation/offSpill.fclandoffSpill_00dump the same as ata17e26d4; the setting now comes fromrecoMC/OffSpill.fcl.- The branch predates #583. The merge with
mainis clean and keeps #583's field-correction settings.
- Merged onto
- The flag changes storage, not the fit.
KKFit.hh:902is the only reader ofSaveMaterialXings. It copies the passive crossings that extrapolation already made (materialCylXings,materialPlaneXings) intoKalSeed::intersections(). No new product is made, andkeep mu2e::KalSeeds_K*_*_*already writesKKLineandKKCHmu. The size numbers in the description (+0.07% offSpill, +0.32% extracted) are yours; I did not rerun them. - Dropping
TCRVchanges nothing.TCRV(200) is marked "deprecated" inDataProducts/inc/SurfaceId.hh. No surface with that ID is ever added to the KinKalGeom map (GeometryService/src/KinKalGeomMaker.cc), sokkg.surfaces()returns nothing for it, and sampling atTCRVnever produced an intersection. A Mu2e org code search finds no reader of theTCRVsurface ID; the Stntuple hits areTCrv*class names. - Downstream readers.
ValKalSeedchanges onlyNInterandNCRVSB, as you expect for the nightly.- ArtAnalysis
CrvInferencepicksTT_Midby surface ID, so the extra entries do not affect it. - EventNtuple
fillTrkSegInfocopies every intersection intosegswith itssid. Ntuples from these jobs will therefore carry extrasegsentries:sid280 (CRV_StrongBack) for extracted, and DS shell, coil, absorber and hatch-concrete IDs for off-spill. That is the intent, but anyone who countssegswithout selecting onsidwill see a change.
Blast radius
recoMC/Extracted.fclis used bymdc2025CosmicCRYExtracted(Triggered)Reco(6 entries inreco.json) and bymdc2030(1 entry).recoMC/OffSpill.fclis used bymdc2025CosmicSignalOffSpillTriggered(2 entries).reco/Extracted.fcl(data) is included by PassNTracker/TrkRecoFromDigi.fcl, whose epilog does not overrideSampleSurfacesorSaveMaterialXings. So PassN tracker reco gets the same change.reco/OffSpill.fclhas no consumer in the org.- The campaigns run pinned SimJob musings, so none of this reaches them until the next SimJob tag.
Validation check
- Build/tests run: FNALbuild passed at this head, built with mu2e-trig-config#176 (which Offline
maincurrently needs) and merged onto Productionefebc0a2. Locally:fhicl-dumpas above (claude-scratch/pr582_r2/fcldump.sh, dumps indumps/). No art job run; your 1000-event before/after on gpvm is the only execution evidence. - Config contract check: pass.
- Cross-repo consistency: pass. PassN inherits the change, which is consistent with what it does.
Author follow-ups
- Optional: drop the leftover
"TCRV"inrecoMC/NoFieldRun1B.fcl(finding 1).
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for 0daa7b9: build (Build queue - API unavailable) |
|
☀️ The build tests passed at 0daa7b9.
N.B. These results were obtained from a build of this Pull Request at 0daa7b9 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 — #582
Reviewed at head 0daa7b9feaabbf1c5228a856c24045fbdbbe7218. re-review of 2889c7c
Decision
- 🟢 approve
Scope understood
- One new commit,
0daa7b9f, removes the same "temporary until extrapolation is working"TCRVsample-surface line fromrecoMC/NoFieldRun1B.fclthat this PR already removed fromreco/andrecoMC/Extracted.fcl.
Verified
- 🟢 I ran
fhicl-dumpofrecoMC/NoFieldRun1B.fcl,reco/Extracted.fclandrecoMC/Extracted.fclat2889c7caand at this head. The only difference anywhere is that"TCRV"leaves the threeKKFitSettings.ModuleSettings.SampleSurfaceslists inNoFieldRun1B, which now end atTT_Outer. BothExtractedconfigs are identical. - 🟢 The CRV is still reached in that job: the KKLine
ExtrapolationSettingsinNoFieldRun1BhasToCRV: true, unchanged by this PR. That is the extrapolation the removed line was standing in for, and it matches @brownd1978's point that the line is obsolete. - 🟢 FNALbuild passed at this head (merged onto
mainatac5d9c91).
Validation check
- Build/tests run: the
fhicl-dumpcomparisons above, against Offline from the CI buildbb141c56_ac5d9c91_32aec90c. No events were processed, so I have not compared the CRV intersections in the output with and without the sample surface. - Config contract check: pass.
- Cross-repo consistency: pass.
Residual risk
recoMC/NoFieldRun1B.fclis the reco config of the Run1B campaign, so this changes production output: Run1B KalSeeds will carry the CRV intersection from extrapolation only, no longer also from surface sampling. I expect that to be the intended result, but it was not checked on events.
Note on merge state
- @brownd1978's change request was made at
a17e26d4, before the line moved intoreco/Extracted.fcland was added to the OffSpill configs. Both of his requests look done, but his review state still blocks the merge until he re-reviews.
oksuzian
left a comment
There was a problem hiding this comment.
PR Review Summary — #582
Reviewed at head 0daa7b9feaabbf1c5228a856c24045fbdbbe7218. Correction to my re-review at the same head.
Decision
- 🟡 comment only. My approval stands; this corrects one statement in it.
I wrote under residual risk that Run1B KalSeeds would now carry the CRV intersection from extrapolation only, no longer also from surface sampling. That was wrong. On Offline main (78ea1fd5), TCRV is a deprecated SurfaceId (DataProducts/inc/SurfaceId.hh:25) that KinKalGeomMaker.cc never adds to the KinKal surface map, and KKFit::sampleFit only finds surfaces in that map (KinKalGeom::surfaces does a map_.equal_range). So sampling TCRV never produced an intersection, and dropping it from recoMC/NoFieldRun1B.fcl should not change the Run1B output. This is from reading the code; I did not run a job.
brownd1978
left a comment
There was a problem hiding this comment.
Thanks for making this change
Follow-up to #580 / Mu2e/Offline#2023, reworked after review (@rlcee, @brownd1978): the setting now lives in the reco
configs rather than in the nightly validation configs.
ValKalSeed's NCRVSB (CRV strongback intersections) has been identically 0 in every nightly: KKExtrap adds the
strongbacks (and, for CentralHelix, the DS shells and concrete) as passive material Xings, which KKFit::sampleFit only
copies into KalSeed::intersections() when KKFitSettings.SaveMaterialXings is true (Offline prolog default false).
Changes:
JobConfig/reco/Extracted.fcl,JobConfig/recoMC/Extracted.fcl:KKLine.KKFitSettings.SaveMaterialXings : true,replacing the obsolete
"TCRV"sample surface (a deprecated test plane that KinKalGeom no longer defines, so itsampled nothing).
JobConfig/reco/OffSpill.fcl,JobConfig/recoMC/OffSpill.fcl:KKCHmu.KKFitSettings.SaveMaterialXings : true.Validation/offSpill.fclandValidation/nightly/extracted_0*.fclpick this up through their includes;no Validation file changes.
Resolved configs (fhicl-dump on the 2026-10-19 nightly build, this branch vs main): each job differs only in that key
(and, for Extracted, the dropped
TCRVentry). 20-event art runs ofrecoMC/OffSpill.fclandrecoMC/Extracted.fclexit 0.
Cost:
extracted_00, 10-14): KKLine intersections per track 2.46 -> 2.92 (the strongbacks:NCRVSB = NCRV, 326/326);
KalSeeds_KKLine+0.8 %, art file +0.32 %; CPU unchanged.recoMC/OffSpill.fclwith CHFilter, 5000 events ofCosmicSignalOffSpill.MDC2025an_best_v1_1, same host, 10-19): KKCHmu intersections per track 2.30 -> 11.53 on theCentralHelix output (31 tracks) and 2.21 -> 6.56 on the LoopHelix output (313 KKCHmu tracks); NCRVSB = NCRV;
KalSeeds_KKCHmu+12.3 % / +7.3 %; art file +0.64 % (CentralHelix output, 9.8 MB) / +0.03 % (LoopHelix output,1.19 GB); CPU 790 -> 777 s (one run each); LoopHelix fits identical.
This changes the output of production campaigns that use these configs (mdc2025 / mdc2030 Extracted and OffSpill reco):
the KalSeeds gain the passive-material intersections; fits are unchanged.
Expect one nightly FAIL on NCRVSB/NInter for offSpill and extracted, then PERFECT.