Repository navigation
Align names with Offline. - #29
brownd1978 wants to merge 4 commits into
Conversation
michaelmackenzie
left a comment
There was a problem hiding this comment.
I think it's better to fix the trigger sequences rather than override them if possible. If not, we need to understand why not, as the fragment collections the trigger runs from should be available to use as in the event builders
There was a problem hiding this comment.
Why is this deleted?
| # | ||
| # turn off MC version of digi crddeation (from digitize/prolog above) | ||
| BEGIN_PROLOG | ||
| TrigRecoSequences.cfoDataGen : [EWMProducer, CaloDigiMaker, makeSD, CrvDigi ] |
There was a problem hiding this comment.
Should we instead use the real trigger sequences, instead of modifying them? Is it necessary to add the Offline digi-making to the trigger sequences?
| producers : { | ||
| @table::TrigProducers.producers | ||
| @table::Digitize.producers | ||
| CaloDigiMaker : @local::DAQ.producers.CaloDigis |
There was a problem hiding this comment.
Can we not use the producers already in the trigger tables? If those need updates, we should update them there rather than replace them in PassN
| physics.trigger_paths : @local::Trig_extrPosMenu.trigger_paths | ||
| physics.end_paths : [ EndPath ] | ||
|
|
||
| #include "mu2e-trig-config/core/trigDigiInputsEpilog.fcl" |
There was a problem hiding this comment.
Why do we need this? Aren't the inputs fragments, not digis?
oksuzian
left a comment
There was a problem hiding this comment.
PR Review Summary — #29
Reviewed at head b68cecd124fea0bc54049c506ed164b85820266b. First pass.
Decision
- 🟡 comment. The CFO tag change is a real fix: on
main,Combined_Pass1finds no CFO data in any event, and at this head it does. But two configurations this PR touches no longer run on current Offline, the combined ntuple configuration is deleted without explanation, and the fixed event header does not reach the ntuple. No production campaign uses PassN, so each of these is S2. Still, they are breakages to fix before merging rather than suggestions, so I am commenting rather than approving.
Scope understood
Combined_Pass1.fcl:ProcessCFODatabecomesEWMProducer, readsdaq:CFOinstead offragmentAggregator:CFO, and now also makes theEventWindowMarkerandProtonBunchTime(createEWM : true). It replaces the MCEventWindowMarkerProducerthatTracker/TrkRecoFromFragments_epilog.fclused to define.Tracker/TrkDigi_epilog.fcl(new) takes the straw-response and hit-selection settings out ofTracker/TrkReco_epilog.fcl, which now includes it.TrigDigi.fcl(new) makes digis from fragments and runs the extracted-position trigger menu on them, writing the digi-production output.from_mcs-combined.fclis deleted.
Findings
-
🟡 [S2] The tracker-only fragment configurations no longer start
- Evidence:
Tracker/TrkRecoFromFragments_epilog.fclno longer definesEWMProducer, but itsRecoPathstill lists it (:12-15).mu2e -c PassN/Tracker/TrkRecoFromFragments.fclon run 125472 exits 9 before the first event: "Entry with name EWMProducer in path RecoPath does not have a module configuration." Onmainthe same job runs 20 events with exit status 0.Tracker/TrkSHDFromFragments.fclincludes the same epilog, and itsfhicl-dumpshows the same missing module. - Suggested fix: define
EWMProducer(the CFO module, asCombined_Pass1.fclnow does) inTrkRecoFromFragments_epilog.fcl, and drop the copy inCombined_Pass1.fcl, which includes that epilog. Then it lives in one place.
- Evidence:
-
🟡 [S2]
TrigDigi.fclnames a geometry file that Offline no longer has- Evidence:
TrigDigi.fcl:99usesOffline/Mu2eG4/geom/geom_common_commissioning_v02.txt. Mu2e/Offline#2082 renamed it togeom_common_extracted_v06.txt, and #27 already movedCombined_Pass1.fclto the new name. On Offlinemain(807d948f) the job stops on the first event withCan't find file "Offline/Mu2eG4/geom/geom_common_commissioning_v02.txt". - With the file name corrected, it runs 300 events of run 125472 with exit status 0, and writes 160 of them. The trigger counts are the same with mu2e-trig-config
mainand with #186:cst_TimeCluster160,minBias_SDCount300,calo_cosmic1 andminBias_CDCount1. So this file needs neither #186 nor Mu2e/Offline#2087.
- Evidence:
-
🟡 [S2]
from_mcs-combined.fclis deleted, and the description does not say why- It is the only ntuple configuration for
Combined_Pass1, and #28 changes it today. The deletion is in commitdb62067("Cleanup"). It makes this PR and #28 conflict (modify/delete, in either merge order);Combined_Pass1.fclitself merges cleanly. This is the question michaelmackenzie asked, and it still needs an answer.
- It is the only ntuple configuration for
-
🟡 [S2] The corrected event header does not reach the ntuple
- Evidence: EventNtuple reads the header from
EventHeaderTag, which defaults toProcessCFOData(EventNtuplefcl/prolog.fcl:328). When it finds nothing, it leavesewtat 0 without a warning (EventNtupleMaker_module.cc:1723-1726,inc/EventInfo.hh:18). - I ran #28's
from_mcs-combined.fcl(EventNtuplemaind05f8a2) on this PR'sCombined_Pass1output for 300 events.evtinfo.ewtis 0 in all 300. WithEventHeaderTag : "EWMProducer"it is nonzero in all 300. - Suggested fix: set
physics.analyzers.EventNtuple.EventHeaderTag : "EWMProducer"in the combined ntuple configuration, whichever version survives.
- Evidence: EventNtuple reads the header from
-
⚪ [S3] Small items
- The comment above
EWMProducerinCombined_Pass1.fcl:21-23still says not to make a second (placeholder) copy, but the module now makes the only copy. ItsProtonBunchTimeis still the0placeholder (EventHeaderFromCFOFragment_module.cc:186, marked FIXME). That is the same value the MC producer gave off-spill, so the fits do not change (see below). TrigDigi.fcl:93-97are five commented-outTrigMakeSDlines, and:12has a typo, "crddeation".TrigDigi.fcl:87-91usesTRACKER_VSTv2_0 with text calibration files, whileCombined_Pass1.fclusesKPP_reco. Say whether the trigger digis should use different conditions from pass 1.
- The comment above
Checked, no action needed
- 🟢
daq:CFOfixes the event header. Onmain,Combined_Pass1on 300 events of run 125472 (subrun 77) reports "Default EventHeader produced without CFO data in 300 of 300 events (300 with no CFO fragment collection…)". At this head, the same events give no such message. The Mu2e/Offline#2087 review'sproduct_sizes_dumperof the raw files for runs 125472 and 125483 lists onlydaqfragment collections, with no aggregator label. So pass-1 output onmaincarries default headers (EWT 0, mode 0, not on-spill), and this change fixes that with or without Mu2e/Offline#2087. michaelmackenzie's points are aboutTrigDigi.fcland the deleted ntuple configuration. None of them affects this part. - 🟢 Removing the MC event-window producer changes no fits. The 81
KKLinefits in those 300 events are identical, line for line, onmainand at this head. The random-number services it needed are gone too. - 🟢 Moving the settings into
TrkDigi_epilog.fclchanges nothing.Tracker/TrkRecoFromDigi.fcl,TrkRecoFromDigiMC.fclandTrkSHDFromDigi.fclresolve identically tomain. - On michaelmackenzie's other two questions (whether to add the digi makers to the trigger sequences in
TrigDigi.fcl, and why it includestrigDigiInputsEpilog.fcl), only the author can say what is intended. As written, the job is self-consistent: it makes the digis from fragments, and the epilog points the trigger's hit makers at those digis.
Validation check
- Environment: the Offline CI build
807d948f_94a4d81f_2b90c646, with PassN atmainor this head on the fcl path.TrigDigi.fclwas run with mu2e-trig-configmainand with #186 plus Mu2e/Production#594. The input,raw.mu2e.cosmics.kpp.125472_000077-001.art, was read in place. fhicl-dumpof every PassN config this PR touches, atmainand at this head. A test merge of #28 and #29:Combined_Pass1.fclmerges and parses, andfrom_mcs-combined.fclconflicts.- Not run:
TrkSHDFromFragments.fcl(its dump shows the same missing module as finding 1), the VST runs, and any comparison of the newEventWindowMarkerspill type and length with the old ones.
This PR makes EWMarkers from the cfo fragment instead of MC generator, and removes all the residual MC dependence. It also introduces a script to rerun the trigger and make digis consistent with those made by simulation production.