Skip to content

Remove fireAll flag, as it is now done automatically#537

Merged
oksuzian merged 1 commit intoMu2e:mainfrom
michaelmackenzie:FireAllRemoved
May 4, 2026
Merged

Remove fireAll flag, as it is now done automatically#537
oksuzian merged 1 commit intoMu2e:mainfrom
michaelmackenzie:FireAllRemoved

Conversation

@michaelmackenzie
Copy link
Copy Markdown
Contributor

This fcl parameter is no longer defined, as it's done by the SingleProcessGenerator module

@FNALbuild
Copy link
Copy Markdown
Collaborator

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

  • JobConfig

which require these tests: build.

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

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

About FNALbuild. Code review on Mu2e/Offline.

@FNALbuild
Copy link
Copy Markdown
Collaborator

☀️ The build tests passed at 4c5c286.

Test Result Details
test with Command did not list any other PRs to include
merge Merged 4c5c286 at ed044a2
build (prof) Log file. Build time: 04 min 09 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 4c5c286 after being merged into the base branch at ed044a2.

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
Copy Markdown
Collaborator

oksuzian commented May 4, 2026

PR #537 Review — Remove fireAll flag, as it is now done automatically

Summary

  • Author: @michaelmackenzie
  • Branch: michaelmackenzie:FireAllRemovedMu2e:main
  • Description: "This fcl parameter is no longer defined, as it's done by the SingleProcessGenerator module"
  • Scope: 1 file changed, +0 / -1
  • State: Open, not draft, mergeable=true (mergeable_state=blocked — likely awaiting required review approval)
  • CI: ✅ All FNALbuild tests passed at commit 4c5c286 (build, ceSimReco, g4test_03MT, transportOnly, POT, g4study, cosmicSimReco, cosmicOffSpill, ceSteps, ceDigi, muDauSteps, ceMix, rootOverlaps, g4surfaceCheck, FIXME/TODO, clang-tidy — all green)
  • Reviews: 0 reviews so far

Core Change

A single line removal in JobConfig/primary/MuCap1809keV.fcl:

 physics.producers.generate : {
   decayProducts :
   {
     tool_type : MuCap1809keVGammaGenerator
-    fireAll   : true # Insert a photon into every event
   }
   verbosity : 0
 }

The fireAll parameter has been removed from the upstream SingleProcessGenerator module (in Mu2e/Offline), where the behavior is now applied automatically. Leaving the parameter would cause the fhicl parser to error on an undefined parameter.

🔎 Issue Found

The PR description claims this parameter is "no longer defined" and the title says it is "now done automatically", but I want to flag two concerns the reviewer should verify:

  1. Incomplete cleanup search? A code search across the Mu2e org for fireAll only returns the one occurrence here in MuCap1809keV.fcl, so within Mu2e/Production this is the only place it appears — that part is fine. However, the change relies on a corresponding update in Mu2e/Offline (the SingleProcessGenerator / MuCap1809keVGammaGenerator tool). The PR does not link or reference that Offline PR. Without that link, a reviewer cannot confirm:

    • Whether the Offline change has actually been merged.
    • Whether the new automatic behavior is semantically equivalent to fireAll : true (i.e., it always inserts a photon into every event, not just when some other condition is met).

    ➡️ Recommendation: Add a reference in the PR description to the corresponding Mu2e/Offline PR/commit that removed/replaced the fireAll parameter, so reviewers can verify the contract.

  2. Comment intent lost. The deleted line carried a meaningful comment: # Insert a photon into every event. That semantic intent is now invisible in the fcl. If the SingleProcessGenerator's new automatic behavior is "always fire on every event", that's effectively documented in Offline; but if a future reader of MuCap1809keV.fcl wonders whether every event gets a photon, they will have no hint here.

    ➡️ Minor suggestion: Consider replacing the removed line with a short comment, e.g.:

    # SingleProcessGenerator now inserts a photon into every event automatically (fireAll deprecated)
    

Merge Readiness

  • ✅ CI green (all 17 tests pass)
  • ✅ Mergeable, no conflicts
  • ⚠️ mergeable_state: blocked — needs an approving review (no reviewers requested yet, no reviews submitted)
  • ⚠️ No linked Offline PR to verify the upstream contract

Risk: Low — single-line fhicl cleanup, all tests pass including ceSimReco/POT/muDauSteps etc. The build test passing strongly indicates the new Offline already accepts (or expects) the absence of fireAll.

Want me to

  1. Search the Mu2e/Offline repo for the matching SingleProcessGenerator / fireAll removal and link the corresponding PR.
  2. Post the suggested explanatory comment as a review comment on the PR.
  3. Check whether any other *.fcl files in Mu2e/Offline (test/example configs) still reference fireAll and would also break.
  4. Approve and request merge once review requirements are clarified.

@oksuzian oksuzian merged commit 8aa74fb into Mu2e:main May 4, 2026
14 checks passed
@michaelmackenzie michaelmackenzie deleted the FireAllRemoved branch May 4, 2026 16:02
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.

3 participants