Skip to content

Align names with Offline. - #29

Open
brownd1978 wants to merge 4 commits into
Mu2e:mainfrom
brownd1978:test
Open

brownd1978 wants to merge 4 commits into
Mu2e:mainfrom
brownd1978:test

Conversation

@brownd1978

Copy link
Copy Markdown
Contributor

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.

@michaelmackenzie michaelmackenzie left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread from_mcs-combined.fcl

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this deleted?

Comment thread TrigDigi.fcl
#
# turn off MC version of digi crddeation (from digitize/prolog above)
BEGIN_PROLOG
TrigRecoSequences.cfoDataGen : [EWMProducer, CaloDigiMaker, makeSD, CrvDigi ]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Comment thread TrigDigi.fcl
producers : {
@table::TrigProducers.producers
@table::Digitize.producers
CaloDigiMaker : @local::DAQ.producers.CaloDigis

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread TrigDigi.fcl
physics.trigger_paths : @local::Trig_extrPosMenu.trigger_paths
physics.end_paths : [ EndPath ]

#include "mu2e-trig-config/core/trigDigiInputsEpilog.fcl"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this? Aren't the inputs fragments, not digis?

@oksuzian oksuzian left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review Summary — #29

Reviewed at head b68cecd124fea0bc54049c506ed164b85820266b. First pass.

Decision

  • 🟡 comment. The CFO tag change is a real fix: on main, Combined_Pass1 finds 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: ProcessCFOData becomes EWMProducer, reads daq:CFO instead of fragmentAggregator:CFO, and now also makes the EventWindowMarker and ProtonBunchTime (createEWM : true). It replaces the MC EventWindowMarkerProducer that Tracker/TrkRecoFromFragments_epilog.fcl used to define.
  • Tracker/TrkDigi_epilog.fcl (new) takes the straw-response and hit-selection settings out of Tracker/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.fcl is deleted.

Findings

  1. 🟡 [S2] The tracker-only fragment configurations no longer start

    • Evidence: Tracker/TrkRecoFromFragments_epilog.fcl no longer defines EWMProducer, but its RecoPath still lists it (:12-15). mu2e -c PassN/Tracker/TrkRecoFromFragments.fcl on run 125472 exits 9 before the first event: "Entry with name EWMProducer in path RecoPath does not have a module configuration." On main the same job runs 20 events with exit status 0. Tracker/TrkSHDFromFragments.fcl includes the same epilog, and its fhicl-dump shows the same missing module.
    • Suggested fix: define EWMProducer (the CFO module, as Combined_Pass1.fcl now does) in TrkRecoFromFragments_epilog.fcl, and drop the copy in Combined_Pass1.fcl, which includes that epilog. Then it lives in one place.
  2. 🟡 [S2] TrigDigi.fcl names a geometry file that Offline no longer has

    • Evidence: TrigDigi.fcl:99 uses Offline/Mu2eG4/geom/geom_common_commissioning_v02.txt. Mu2e/Offline#2082 renamed it to geom_common_extracted_v06.txt, and #27 already moved Combined_Pass1.fcl to the new name. On Offline main (807d948f) the job stops on the first event with Can'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 main and with #186: cst_TimeCluster 160, minBias_SDCount 300, calo_cosmic 1 and minBias_CDCount 1. So this file needs neither #186 nor Mu2e/Offline#2087.
  3. 🟡 [S2] from_mcs-combined.fcl is 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 commit db62067 ("Cleanup"). It makes this PR and #28 conflict (modify/delete, in either merge order); Combined_Pass1.fcl itself merges cleanly. This is the question michaelmackenzie asked, and it still needs an answer.
  4. 🟡 [S2] The corrected event header does not reach the ntuple

    • Evidence: EventNtuple reads the header from EventHeaderTag, which defaults to ProcessCFOData (EventNtuple fcl/prolog.fcl:328). When it finds nothing, it leaves ewt at 0 without a warning (EventNtupleMaker_module.cc:1723-1726, inc/EventInfo.hh:18).
    • I ran #28's from_mcs-combined.fcl (EventNtuple main d05f8a2) on this PR's Combined_Pass1 output for 300 events. evtinfo.ewt is 0 in all 300. With EventHeaderTag : "EWMProducer" it is nonzero in all 300.
    • Suggested fix: set physics.analyzers.EventNtuple.EventHeaderTag : "EWMProducer" in the combined ntuple configuration, whichever version survives.
  5. ⚪ [S3] Small items

    • The comment above EWMProducer in Combined_Pass1.fcl:21-23 still says not to make a second (placeholder) copy, but the module now makes the only copy. Its ProtonBunchTime is still the 0 placeholder (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-97 are five commented-out TrigMakeSD lines, and :12 has a typo, "crddeation".
    • TrigDigi.fcl:87-91 uses TRACKER_VST v2_0 with text calibration files, while Combined_Pass1.fcl uses KPP_reco. Say whether the trigger digis should use different conditions from pass 1.

Checked, no action needed

  • 🟢 daq:CFO fixes the event header. On main, Combined_Pass1 on 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's product_sizes_dumper of the raw files for runs 125472 and 125483 lists only daq fragment collections, with no aggregator label. So pass-1 output on main carries default headers (EWT 0, mode 0, not on-spill), and this change fixes that with or without Mu2e/Offline#2087. michaelmackenzie's points are about TrigDigi.fcl and the deleted ntuple configuration. None of them affects this part.
  • 🟢 Removing the MC event-window producer changes no fits. The 81 KKLine fits in those 300 events are identical, line for line, on main and at this head. The random-number services it needed are gone too.
  • 🟢 Moving the settings into TrkDigi_epilog.fcl changes nothing. Tracker/TrkRecoFromDigi.fcl, TrkRecoFromDigiMC.fcl and TrkSHDFromDigi.fcl resolve identically to main.
  • On michaelmackenzie's other two questions (whether to add the digi makers to the trigger sequences in TrigDigi.fcl, and why it includes trigDigiInputsEpilog.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 at main or this head on the fcl path. TrigDigi.fcl was run with mu2e-trig-config main and with #186 plus Mu2e/Production#594. The input, raw.mu2e.cosmics.kpp.125472_000077-001.art, was read in place.
  • fhicl-dump of every PassN config this PR touches, at main and at this head. A test merge of #28 and #29: Combined_Pass1.fcl merges and parses, and from_mcs-combined.fcl conflicts.
  • Not run: TrkSHDFromFragments.fcl (its dump shows the same missing module as finding 1), the VST runs, and any comparison of the new EventWindowMarker spill type and length with the old ones.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants