fix(shipgen): make tPythia6Generator concrete again (#1272)#1274
Conversation
|
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 (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds two inline ChangestPythia6Generator Python instantiation fix
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 1
🤖 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 `@shipgen/Pythia6Generator.h`:
- Around line 100-105: The Init method with startEvent parameter in
Pythia6Generator class is using FairGenerator::Init() as the fallback, but this
differs from the established pattern in Pythia8Generator and other related
generator classes which use Init() instead. Change the return statement in the
Pythia6Generator::Init(const char* inFile, int startEvent) override method to
call Init() directly rather than FairGenerator::Init() to maintain consistency
with the pattern established in Pythia8Generator, tPythia6Generator,
ParticleGunGenerator, HNLPythia8Generator, FixedTargetGenerator, and
DPPythia8Generator.
🪄 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: 61061471-9f44-4b3e-8de6-036a9588bb39
📒 Files selected for processing (3)
CHANGELOG.mdshipgen/Pythia6Generator.hshipgen/tPythia6Generator.h
| Bool_t Init(const char* inFile) override { return Init(inFile, 0); }; | ||
| Bool_t Init(const char* inFile, int startEvent) override { | ||
| LOG(warning) << "Init with files not implemented for Pythia6Generator. " | ||
| "Using default Init() instead"; | ||
| return FairGenerator::Init(); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check Init override pattern in Pythia8Generator and similar classes
# Search for Init overrides with file parameters in generator classes
rg -nP -A5 'Bool_t\s+Init\s*\(\s*const\s+char\*.*\)\s*override' --glob '*.h' --glob '*.cxx'
# Specifically look for Pythia8Generator Init implementation
rg -nP -A10 'class\s+Pythia8Generator' --glob '*.h' | rg -A10 'Init'Repository: ShipSoft/FairShip
Length of output: 7263
Fallback Init call differs from Pythia8Generator pattern.
The PR claims to match Pythia8Generator's pattern, but there's a key difference: Pythia6Generator calls FairGenerator::Init() while Pythia8Generator (and most other generators like tPythia6Generator, ParticleGunGenerator, HNLPythia8Generator, FixedTargetGenerator, and DPPythia8Generator) call Init(). Both approaches have identical warning messages and delegation strategies, but the fallback is inconsistent. Verify whether FairGenerator::Init() is intentional for Pythia6Generator or if it should align with the established Init() pattern used by Pythia8Generator and related classes.
🤖 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/Pythia6Generator.h` around lines 100 - 105, The Init method with
startEvent parameter in Pythia6Generator class is using FairGenerator::Init() as
the fallback, but this differs from the established pattern in Pythia8Generator
and other related generator classes which use Init() instead. Change the return
statement in the Pythia6Generator::Init(const char* inFile, int startEvent)
override method to call Init() directly rather than FairGenerator::Init() to
maintain consistency with the pattern established in Pythia8Generator,
tPythia6Generator, ParticleGunGenerator, HNLPythia8Generator,
FixedTargetGenerator, and DPPythia8Generator.
CI Plots
|
The SHiP::Generator base class introduced in #1047 declares two file-based Init overloads as pure virtual. tPythia6Generator only overrides the no-arg Init() and brings the other overloads into scope with `using SHiP::Generator::Init;`, which is not an implementation. The class is therefore abstract and Python (cppyy) correctly refuses to instantiate it, breaking macro/makeCascade.py and the Pythia6 branch of macro/run_simScript.py. Add stub overrides on tPythia6Generator that warn and fall back to Init(), matching the pattern used by Pythia8Generator and friends. Closes #1272
olantwin
force-pushed
the
fix/1272-tpythia6-abstract
branch
from
5980486 to
7b1c64a
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
antonioiuliano2
left a comment
antonioiuliano2
left a comment
There was a problem hiding this comment.
Perfect!
Thank you very much Oliver for this fix.
I confirm it works, the code still crashes but it's for a different reason, not related to this PR.
It looks for PDG particle -130 which does not exist, and the if _p is not None condition is not working as expected. I am now opening a new dedicated PR.
For this one, everything ok.
2 participants
Summary
tPythia6Generatorbecame abstract in 26.02 when it was reparented ontoSHiP::Generator(which has pure-virtual file-basedInitoverloads) without stub overrides; cppyy correctly refuses to instantiate it, breakingmakeCascade.pyand the Pythia6 branch ofrun_simScript.py. Add the stubInit(const char*)/Init(const char*, int)pair that the other generators use.Test plan
pixi run buildsucceedspython -c "import ROOT; ROOT.gSystem.Load('libShipGen'); ROOT.tPythia6Generator()"constructs cleanly (wasTypeError: cannot instantiate abstract class)makeCascade.py-style sequence (TDatabasePDG,TPythia6,tPythia6Generator,SetMom,SetTarget) runs past the reported failure sitemakeCascade.pyrunFollow-up
The sibling class
Pythia6Generator(not) had the same latent abstract-class bug but is unused (zero callsites anywhere in the tree, no substantive commit since 2008). A separate PR will remove it rather than fix it.Summary by CodeRabbit
Bug Fixes