Add option in GENIEGenerator to read simulations done with GENIE Geometry Driver#1136
Open
antonioiuliano2 wants to merge 13 commits into
Open
Add option in GENIEGenerator to read simulations done with GENIE Geometry Driver#1136antonioiuliano2 wants to merge 13 commits into
antonioiuliano2 wants to merge 13 commits into
Conversation
matclim
previously approved these changes
Apr 1, 2026
olantwin
reviewed
Apr 9, 2026
olantwin
left a comment
olantwin
left a comment
Contributor
There was a problem hiding this comment.
In the addition to the comments I sent you privately.
The legal issue is a blocker.
Contributor
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a Genie generation-mode selector and plumbing to read events either via the existing histogram/material-budget path or directly from a GENIE Geometry Driver TTree; the option is stored on GenieGenerator, validated in Init(), and selects the ReadEvent path. ChangesGenie generation mode + geometry-driver read path
Sequence Diagram(s)sequenceDiagram
participant CLI as run_simScript CLI
participant Generator as GenieGenerator
participant GENIE as GENIE TTree
participant Fair as FairPrimaryGenerator
CLI->>Generator: SetGenerationOption(option)
CLI->>Generator: (conditionally) SetPositions(...)
Generator->>Generator: Init() validates fGenOption
alt fGenOption == 3 (genie_geometry)
Generator->>GENIE: GetEntry(fn)
GENIE-->>Generator: entry fields (vtx*, pdgf, momentum, ...)
Generator->>Fair: Add neutrino/lepton/hadron tracks (scaled vtx)
else fGenOption == 0 (simple_gevgen)
Generator->>Generator: compute interaction pos via hist/material budget
Generator->>Fair: Add tracks using computed pos and generated momenta
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
shipgen/GenieGenerator.cxx (2)
288-288: Simplify boolean expression.The ternary
(cc || nuel) ? true : falseis redundant and can be simplified to justcc || nuel.♻️ Proposed fix
- bool track_outgoing_lepton = (cc || nuel) ? true : false; + bool track_outgoing_lepton = cc || nuel;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shipgen/GenieGenerator.cxx` at line 288, The assignment to track_outgoing_lepton uses a redundant ternary; replace "bool track_outgoing_lepton = (cc || nuel) ? true : false;" with a direct boolean expression using cc || nuel (i.e., "bool track_outgoing_lepton = cc || nuel;") so the variable is set without the unnecessary conditional. Ensure you update the initialization in GenieGenerator.cxx where track_outgoing_lepton is defined and leave cc and nuel usage unchanged.
71-75: Use lowercase FairLogger severity.
LOG(FATAL)uses deprecated uppercase severity. The codebase has been migrating to lowercase severities as noted in the changelog.♻️ Proposed fix
if (fGenOption != 0 && fGenOption != 3) { - LOG(FATAL) << "Invalid GenieGen Option: " << fGenOption + LOG(fatal) << "Invalid GenieGen Option: " << fGenOption << " Please check the option provided with --Genie " << endl; return kFALSE; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shipgen/GenieGenerator.cxx` around lines 71 - 75, Replace the deprecated uppercase FairLogger severity used in the GenieGenerator check: change the LOG(FATAL) invocation that references fGenOption to the lowercase form LOG(fatal) while keeping the same message and return behavior; also scan the same file for any other LOG(FATAL) occurrences and update them to LOG(fatal) to match the codebase convention.macro/run_simScript.py (1)
79-85: Restrict valid choices to prevent invalid options.The current implementation allows users to pass values 2 or 3, which would result in a fatal error in
GenieGenerator::Init()since onlyfGenOptionvalues of 0 and 3 are valid. This matches the past review comment about the-1offset.Consider adding
choices=[1, 4]to argparse to fail fast with a clear error message.♻️ Proposed fix
genie_parser.add_argument( "--GenieOption", dest="GenieOption", default=1, type=int, + choices=[1, 4], help="Genie generation option: (1 standard, 4 GENIE geometry driver)", )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@macro/run_simScript.py` around lines 79 - 85, The --GenieOption argument currently defined via genie_parser.add_argument (dest="GenieOption", default=1, type=int) allows invalid values (2 or 3) that later cause a fatal error in GenieGenerator::Init due to the expected fGenOption mappings; update the add_argument call for "--GenieOption" to enforce valid inputs by adding choices=[1,4] (and optionally update the help string) so argparse fails fast with a clear message when users provide unsupported options.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@macro/run_simScript.py`:
- Around line 79-85: The --GenieOption argument currently defined via
genie_parser.add_argument (dest="GenieOption", default=1, type=int) allows
invalid values (2 or 3) that later cause a fatal error in GenieGenerator::Init
due to the expected fGenOption mappings; update the add_argument call for
"--GenieOption" to enforce valid inputs by adding choices=[1,4] (and optionally
update the help string) so argparse fails fast with a clear message when users
provide unsupported options.
In `@shipgen/GenieGenerator.cxx`:
- Line 288: The assignment to track_outgoing_lepton uses a redundant ternary;
replace "bool track_outgoing_lepton = (cc || nuel) ? true : false;" with a
direct boolean expression using cc || nuel (i.e., "bool track_outgoing_lepton =
cc || nuel;") so the variable is set without the unnecessary conditional. Ensure
you update the initialization in GenieGenerator.cxx where track_outgoing_lepton
is defined and leave cc and nuel usage unchanged.
- Around line 71-75: Replace the deprecated uppercase FairLogger severity used
in the GenieGenerator check: change the LOG(FATAL) invocation that references
fGenOption to the lowercase form LOG(fatal) while keeping the same message and
return behavior; also scan the same file for any other LOG(FATAL) occurrences
and update them to LOG(fatal) to match the codebase convention.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ed5aee61-8f4f-4e91-a6a6-6ca558fc77fe
📒 Files selected for processing (4)
CHANGELOG.mdmacro/run_simScript.pyshipgen/GenieGenerator.cxxshipgen/GenieGenerator.h
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
shipgen/GenieGenerator.cxx (2)
500-502: 💤 Low valueUnreachable else, given
InitvalidatesfGenOption.If
Initalready rejects any value other than0or3(and the validation is moved to the top ofInitper the earlier comment), thiselse return kFALSE;becomes unreachable in practice. It is fine to keep as a defensive guard, but consider either an explicitLOG(fatal)/assert here or a comment noting thatInitis the single source of truth for valid options. Also, for consistency with the rest of the file, prefer braces around theelsebranch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shipgen/GenieGenerator.cxx` around lines 500 - 502, The trailing "else return kFALSE;" after the branch should be made explicit or documented: replace the unreachable lone-else with either a guarded fatal/assert (e.g., LOG(fatal) or assert) referencing fGenOption and Init as the single source of truth, or at minimum add braces and a clarifying comment stating that Init validates fGenOption (valid values 0 or 3) and this else is only a defensive guard; update the block containing "else return kFALSE;" to use braces and refer to Init/fGenOption to make intent clear.
260-299: 💤 Low valueMinor cleanups in
ReadEventGeometryDriver.A few small improvements:
- Duplicated
meterconstant. It is declared here (line 262) and again inReadEvent(line 303). Promote it to a file-levelconstexpr(or a class-level static member) to keep the conversion factor in one place.- Redundant ternary on line 288.
(cc || nuel) ? true : falseis equivalent tocc || nuel.fTree->GetEntry(fn) == 0only checks EOF.GetEntryreturns-1on I/O error, which would silently fall through here. Consider<= 0if you also want to bail on errors.- Behavior difference to flag.
OldReadEventrewinds at end-of-file (fn % fNevents); this method instead returnskFALSE. That looks intentional for the geometry driver path (each entry corresponds to a specific sampled vertex), but worth a one-line comment so future maintainers do not "fix" it.♻️ Proposed cleanups
-Bool_t GenieGenerator::ReadEventGeometryDriver(FairPrimaryGenerator* cpg) { - // Use GENIE geometry driver. - const Int_t meter = 100; // m to cm conversion factor, as in shipunit.py - // Get event from GENIE TTree. If we reach the end of the file, return - // false. - if (fTree->GetEntry(fn) == 0) return kFALSE; +Bool_t GenieGenerator::ReadEventGeometryDriver(FairPrimaryGenerator* cpg) { + // Use GENIE geometry driver. No wrap-around: each entry corresponds to a + // specific vertex sampled by GENIE's geometry driver. + if (fTree->GetEntry(fn) <= 0) return kFALSE; @@ - bool track_outgoing_lepton = (cc || nuel) ? true : false; + bool track_outgoing_lepton = cc || nuel;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shipgen/GenieGenerator.cxx` around lines 260 - 299, ReadEventGeometryDriver currently declares a local "meter" const (duplicate of ReadEvent), uses a redundant ternary "(cc || nuel) ? true : false", checks fTree->GetEntry(fn) with "== 0" (missing I/O error case), and lacks a comment about differing EOF behavior vs OldReadEvent; fix by promoting the meter constant to a shared file-level constexpr or a class static used by both ReadEventGeometryDriver and ReadEvent, change the GetEntry check to "<= 0" where fTree->GetEntry(fn) is evaluated in GenieGenerator::ReadEventGeometryDriver, replace the ternary expression with the direct boolean "cc || nuel" when setting track_outgoing_lepton, and add a one-line comment in ReadEventGeometryDriver explaining that returning kFALSE at EOF is intentional and differs from OldReadEvent's rewind behavior.macro/run_simScript.py (1)
572-573: 💤 Low valueMinor: prefer comparing on the symbolic option name.
The conditional re-looks-up the dict only to compare against
0. Comparing onoptions.GenieOptiondirectly is shorter and reads closer to intent.♻️ Suggested tweak
- if GenieOptions[options.GenieOption] == 0: + if options.GenieOption == "simple_gevgen": Geniegen.SetPositions(ship_geo.target.z0, options.z_start_nu, options.z_end_nu)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@macro/run_simScript.py` around lines 572 - 573, Replace the dict value lookup comparison with a direct comparison on the symbolic option name: instead of "if GenieOptions[options.GenieOption] == 0:" change to "if options.GenieOption == <SYMBOLIC_NAME_FOR_ZERO>:" (use the actual symbolic key/enum name that represents the zero/disabled value in GenieOptions). Keep the body calling Geniegen.SetPositions(ship_geo.target.z0, options.z_start_nu, options.z_end_nu) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@macro/run_simScript.py`:
- Around line 569-574: The Genie geometry-driver path never sets the NuOnly flag
so nuradiography runs can't use the geometry driver; update the Genie subcommand
wiring so that when the geometry-driver option is selected (GenieOptions +
Geniegen.SetGenerationOption(...)) and nuradiography is requested, you call
Geniegen.NuOnly(true) (or equivalent) on the same Geniegen instance before
Geniegen.Init/primGen.AddGenerator, and ensure the --NuRadio path does not
construct a separate GenieGenerator with fGenOption left at 0 (instead reuse the
Genie subcommand's Geniegen or set its generation option to the geometry driver
and call NuOnly as above); adjust option parsing to allow combining --NuRadio
with the "genie_geometry" choice or add an explicit --NuOnly flag and propagate
it to Geniegen.NuOnly().
In `@shipgen/GenieGenerator.cxx`:
- Line 303: The local constant meter declared in ReadEvent is unused and
triggers -Wunused-variable; remove the declaration from ReadEvent and, if a
shared conversion is desired, introduce a single shared constant (e.g., a static
constexpr double kMeter = 100) at file or class scope where both ReadEvent and
ReadEventGeometryDriver can reference it; update ReadEventGeometryDriver to use
that shared constant instead of its own local meter if you choose to hoist.
- Around line 68-75: Move the fGenOption validation to the very start of Init
(before TFile::Open and any SetBranchAddress calls) to avoid wasted setup and
leaks; replace LOG(FATAL) with LOG(fatal) to match FairLogger severity names,
remove the redundant << endl, and drop the unreachable return kFALSE (or convert
the logger call to non-fatal if you want to return instead); update the check at
the top that references fGenOption and ensure any early-exit path properly
closes or avoids opening files/branches.
---
Nitpick comments:
In `@macro/run_simScript.py`:
- Around line 572-573: Replace the dict value lookup comparison with a direct
comparison on the symbolic option name: instead of "if
GenieOptions[options.GenieOption] == 0:" change to "if options.GenieOption ==
<SYMBOLIC_NAME_FOR_ZERO>:" (use the actual symbolic key/enum name that
represents the zero/disabled value in GenieOptions). Keep the body calling
Geniegen.SetPositions(ship_geo.target.z0, options.z_start_nu, options.z_end_nu)
unchanged.
In `@shipgen/GenieGenerator.cxx`:
- Around line 500-502: The trailing "else return kFALSE;" after the branch
should be made explicit or documented: replace the unreachable lone-else with
either a guarded fatal/assert (e.g., LOG(fatal) or assert) referencing
fGenOption and Init as the single source of truth, or at minimum add braces and
a clarifying comment stating that Init validates fGenOption (valid values 0 or
3) and this else is only a defensive guard; update the block containing "else
return kFALSE;" to use braces and refer to Init/fGenOption to make intent clear.
- Around line 260-299: ReadEventGeometryDriver currently declares a local
"meter" const (duplicate of ReadEvent), uses a redundant ternary "(cc || nuel) ?
true : false", checks fTree->GetEntry(fn) with "== 0" (missing I/O error case),
and lacks a comment about differing EOF behavior vs OldReadEvent; fix by
promoting the meter constant to a shared file-level constexpr or a class static
used by both ReadEventGeometryDriver and ReadEvent, change the GetEntry check to
"<= 0" where fTree->GetEntry(fn) is evaluated in
GenieGenerator::ReadEventGeometryDriver, replace the ternary expression with the
direct boolean "cc || nuel" when setting track_outgoing_lepton, and add a
one-line comment in ReadEventGeometryDriver explaining that returning kFALSE at
EOF is intentional and differs from OldReadEvent's rewind behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 835fbeb5-a332-4e3c-93e0-5538bb28d0f6
📒 Files selected for processing (3)
macro/run_simScript.pyshipgen/GenieGenerator.cxxshipgen/GenieGenerator.h
Contributor
CI Plots
|
Contributor
Author
|
End of File Handling:
I am now moving in implementing the coderabbit review comments. |
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Contributor
Author
|
An addendum note: Before run_simScript.py, gevgen_fnal needs to be launched with an input flux ntuple (i.e. a gsimple root file). |
Contributor
|
@antonioiuliano2 Please rebase. |
…E Geometry Driver
…he nuradiography simulations
Contributor
Author
|
Done! Since rebase requires then a git push force, for safety I write here that |
antonioiuliano2
force-pushed
the
geniegenerator_3
branch
from
6277e0f to
fdf6d04
Compare
Contributor
|
@antonioiuliano2, could you fix the merge conflicts? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Dear all,
This pull request is part of the GENIE generation update discussed in #984 .
It adds to GenieGenerator the option to simply read the GST TTree with the simulation produced with the GENIE Geometry Driver, and I copied the relevant lines from the SNDSW version:
https://github.com/SND-LHC/sndsw/blob/master/shipgen/GenieGenerator.cxx
To avoid mistakes, I kept the same numbering option scheme, so this would be option 3 (to be called with Genie --GenieOption 4), even if we do not have options 1 (FLUKA input tree) and 2 (Genie input tree).
Other pull requests in the pipeline (they can be processed in parallel, since they affect indepedent files):
Please let me know if you have any advice or suggestion.
Antonio
Checklist
Summary by CodeRabbit