Skip to content

Add script to rerun the trigger#523

Open
brownd1978 wants to merge 2 commits intoMu2e:mainfrom
brownd1978:retrigger
Open

Add script to rerun the trigger#523
brownd1978 wants to merge 2 commits intoMu2e:mainfrom
brownd1978:retrigger

Conversation

@brownd1978
Copy link
Collaborator

This replaces reco/RecoTrig.fcl

@FNALbuild
Copy link
Collaborator

Hi @brownd1978,
You have proposed changes to files in these packages:

  • JobConfig

which require these tests: build.

@Mu2e/write, @Mu2e/fnalbuild-users have access to CI actions on main.

⌛ The following tests have been triggered for 406ac35: build (Build queue - API unavailable)

About FNALbuild. Code review on Mu2e/Offline.

@FNALbuild
Copy link
Collaborator

☀️ The build tests passed at 406ac35.

Test Result Details
test with Command did not list any other PRs to include
merge Merged 406ac35 at a49d054
build (prof) Log file. Build time: 04 min 06 sec
ceSimReco Log file.
g4test_03MT Log file.
transportOnly Log file.
POT Log file.
g4study Log file.
cosmicSimReco Log file.
cosmicOffSpill Log file.
ceSteps Log file.
ceDigi Log file.
muDauSteps Log file.
ceMix Log file.
rootOverlaps Log file.
g4surfaceCheck Log file.
FIXME, TODO TODO (0) FIXME (0) in 0 files
clang-tidy 0 errors 0 warnings

N.B. These results were obtained from a build of this Pull Request at 406ac35 after being merged into the base branch at a49d054.

For more information, please check the job page here.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

@oksuzian
Copy link
Collaborator

Here is a thorough review of PR #523 — "Add script to rerun the trigger" by @brownd1978.


Summary

This PR refactors the approach for re-running the trigger. It removes JobConfig/reco/RecoTrig.fcl (which ran the trigger alongside reco), adds a new dedicated JobConfig/digitize/Retrigger.fcl (which re-runs the trigger on existing digis — without reco), and makes a minor fix to JobConfig/reco/drop_trigger.fcl.


CI / Build Status ✅

All build and integration tests passed (via FNALbuild) on commit 406ac35. The PR has also received one approval from @michaelmackenzie.


Issues Found

1. 🔴 TriggerableCHPath is defined but never used in trigger_paths

  TriggerableLHPath : [ @sequence::Digitize.TriggerableLHSequence ]
  TriggerableCHPath : [ @sequence::Digitize.TriggerableCHSequence ]   # <-- defined
  TriggerableTwoTrackPath : [ @sequence::Digitize.TriggerableTwoTrackSequence ]
  TriggerableCaloPath : [ @sequence::Digitize.TriggerableCaloSequence ]
  trigger_paths : [ @sequence::Trig_physMenu.trigger_paths,
  "20:TriggerableLHPath", "21:TriggerableTwoTrackPath", "25:TriggerableCaloPath"]  # <-- TriggerableCHPath missing!

TriggerableCHPath is declared as a path (for high-momentum charged particles, > 500 MeV/c) but is not included in trigger_paths. This means it will never be executed. Either it should be added to trigger_paths (with an appropriate stream number), or the path definition should be removed if it is intentionally excluded. Compare this to the normal digitize workflow in prolog.fcl where all four Triggerable*Sequences are available.


2. 🔴 TriggerableCHPath also missing from outputs.Output.SelectEvents

outputs.Output.SelectEvents : [@sequence::Digitize.SignalTriggers,
"TriggerableLHPath",    "TriggerableTwoTrackPath",    "TriggerableCaloPath"]

TriggerableCHPath is again absent from SelectEvents. Even if the omission from trigger_paths is intentional (i.e., to skip CH events), this should be commented upon for clarity. If the omission is unintentional, events from that path would be silently dropped from output.


3. 🟡 Hardcoded stream numbers in trigger_paths without explanation

  trigger_paths : [ @sequence::Trig_physMenu.trigger_paths,
  "20:TriggerableLHPath", "21:TriggerableTwoTrackPath", "25:TriggerableCaloPath"]

The stream indices 20, 21, and 25 are hardcoded with no comment explaining their significance. These numbers should be documented (e.g., where this numbering scheme originates), especially since 22, 23, and 24 are skipped. This is a maintainability concern.


4. 🟡 Inconsistent PrescaleFilterFraction drop pattern vs. drop_trigger.fcl

In Retrigger.fcl:

"drop mu2e::PrescaleFilterFraction_*_*_Digitize"

In the updated drop_trigger.fcl:

"drop mu2e::PrescaleFilterFraction_*_*_*"

Retrigger.fcl drops only PrescaleFilterFraction from process Digitize, while drop_trigger.fcl (the other file in this PR) is updated to drop it from all processes. If the intent is to fully clean up prescale fractions before re-running the trigger, Retrigger.fcl may be too narrow in its drop pattern and could leave stale prescale data from other processes in the event.


5. 🟡 No end_paths declaration for analyzers / diagnostics

The Retrigger.fcl job does not include any analyzer/end-path structure (e.g., no equivalent of Digitize.EndSequence). The original RecoTrig.fcl also lacked this, but it's worth considering whether compression checks or other diagnostics should be run here.


6. 🟢 PR description is minimal

The PR body just says: "This replaces reco/RecoTrig.fcl". There's no explanation of why the file was moved from reco/ to digitize/, the conceptual difference between the old and new approach (reco+trigger vs. digis-only trigger), or how/when Retrigger.fcl should be invoked in a production campaign. A more descriptive PR body would help reviewers and future users.


7. 🟢 Trailing whitespace in closing bracket

"drop mu2e::PrescaleFilterFraction_*_*_Digitize"  ]

There is trailing whitespace before ] (same pattern repeated in drop_trigger.fcl). Minor style issue but worth cleaning up for consistency.


Summary Table

# Severity Issue
1 🔴 Bug TriggerableCHPath missing from trigger_paths — path is never run
2 🔴 Bug TriggerableCHPath missing from SelectEvents — events silently dropped
3 🟡 Warning Hardcoded stream numbers 20, 21, 25 with no documentation
4 🟡 Warning Inconsistent PrescaleFilterFraction drop pattern vs. drop_trigger.fcl
5 🟡 Warning No end-path diagnostics/analyzers in new job config
6 🟢 Minor Minimal PR description — no context on motivation or usage
7 🟢 Minor Trailing whitespace before ]

The most significant issues are #1 and #2: TriggerableCHPath is defined but silently never executed and its events would not appear in output. This is likely a bug unless the CH stream is intentionally excluded from the retrigger job (which should then be documented and the path definition removed).

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants