feat: Add option to split pions and kaons in run_fixedTarget#1063
feat: Add option to split pions and kaons in run_fixedTarget#1063THanae wants to merge 13 commits into
Conversation
olantwin
left a comment
olantwin
left a comment
There was a problem hiding this comment.
Minor comments, looks good.
What does the stackPopper do?
THanae
force-pushed
the
pion_kaon_splitting
branch
from
7f73907 to
45d51dc
Compare
stackPopper allows to add the manually added particles to the tracking |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR implements kaon-pion track splitting in the hadron absorber to increase muon yield. Changes span data structures, core decay detection and cloning logic, weight propagation, and runtime configuration. A merge-conflict artifact remains in CHANGELOG.md. ChangesKaon-Pion Splitting
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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 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 |
|
How do we proceed with this PR? The intermediate production does not use this, correct? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@macro/run_fixedTarget.py`:
- Around line 271-272: The current code sets KAON_PION_SPLITS as a
process-global env var from args.kaon_pion_splits which affects every
exitHadronAbsorber and optional PlanePostT/PlaneT; instead pass the split count
explicitly to the intended detector (e.g., add a parameter or attribute on the
HA plane constructor or factory that takes args.kaon_pion_splits) and have
exitHadronAbsorber read that parameter rather than os.environ, and if you cannot
refactor immediately, at minimum add an else branch to clear
os.environ["KAON_PION_SPLITS"] when args.kaon_pion_splits == 0 so inherited
values are not kept.
In `@muonShieldOptimization/exitHadronAbsorber.cxx`:
- Around line 205-207: The issue is that fIsSplitting is an event-global flag
preventing multiple splits per event; change it to be per-track (or per-clone)
instead: replace the single bool fIsSplitting with a keyed container (e.g.,
std::unordered_map<int,bool> fIsSplittingByTrack keyed by track ID or clone ID)
inside class exitHadronAbsorber, update BeginEvent() to clear that map rather
than a single flag, modify PostTrack() to check/set
fIsSplittingByTrack[track->GetTrackID()] when deciding to split, and ensure you
erase/clear the entry when the track finishes; apply the same per-track guarding
logic for the code paths in the 211-258 region.
- Around line 240-253: The clones are being assigned the original particle's
mother (part->GetFirstMother()) which breaks ancestry; instead set
clone.parentID to the current track's ID. Replace clone.parentID =
part->GetFirstMother() with the current track ID (e.g. obtain it via
gMC->TrackId() or by assigning ntr = gMC->TrackId() earlier) so cloned
TrackBuffer entries have the current track as parent; remove/repurpose the
unused ntr if necessary and ensure ShipStack::PushTrack() sees the correct
parent-child relation.
🪄 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: a302b467-8376-433c-9648-4aad215bbecb
📒 Files selected for processing (7)
CHANGELOG.mdgconfig/g4Config.Cmacro/run_fixedTarget.pymuonShieldOptimization/exitHadronAbsorber.cxxmuonShieldOptimization/exitHadronAbsorber.hshipdata/ShipStack.cxxshipdata/ShipStack.h
|
One thing I notice: By adding this to the exitHadronAbsorber stepping methods, does this still work for tracks in other volumes? If I am not mistaken, those methods are only called when in the exitHadronAborber volume. |
The intermediate production indeed does not use it -- more validation is required before it can be integrated. |
THanae
force-pushed
the
pion_kaon_splitting
branch
from
0a8d99a to
6e47e99
Compare
I think |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
muonShieldOptimization/exitHadronAbsorber.cxx (1)
43-51:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInitialize
fUseCaveCoordinatesin this constructor too.The default constructor sets
fUseCaveCoordinates(kFALSE), but this overload still leaves it indeterminate even thoughConstructGeometry()branches on it later. That makes behavior depend on uninitialized state wheneverexitHadronAbsorber(const char*, Bool_t)is used.Proposed fix
exitHadronAbsorber::exitHadronAbsorber(const char* Name, Bool_t Active) : Detector(Name, Active, kVETO), fOnlyMuons(kFALSE), fSkipNeutrinos(kFALSE), fVetoName("veto"), fzPos(3E8), withNtuple(kFALSE), fCylindricalPlane(kFALSE), + fUseCaveCoordinates(kFALSE), fNsplits(0) {}🤖 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 `@muonShieldOptimization/exitHadronAbsorber.cxx` around lines 43 - 51, The constructor exitHadronAbsorber::exitHadronAbsorber(const char* Name, Bool_t Active) leaves fUseCaveCoordinates uninitialized causing ConstructGeometry() to branch on indeterminate state; update this ctor's initializer list to explicitly initialize fUseCaveCoordinates to kFALSE (same as the default ctor) so the member has a defined value when ConstructGeometry() runs.
🤖 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 `@CHANGELOG.md`:
- Line 38: Remove the Git merge conflict marker string ">>>>>>> 8e56f36c6" from
the CHANGELOG (it's an artifact left in the file); simply delete that line so
the changelog contains only valid content and no conflict markers, then save the
file and run a quick git diff to confirm no other conflict markers (e.g.,
"<<<<<<<" or "=======") remain.
In `@muonShieldOptimization/exitHadronAbsorber.cxx`:
- Around line 296-304: The code currently sets secPart->SetStatusCode(-1) for
natural-decay secondaries, but ShipStack::PopNextTrack() uses
TParticle::StatusCode as the track ID so this corrupts identities; instead
remove those particles from the stack itself (do not touch StatusCode). Locate
the isNaturalDecay handling that iterates from stackSizeBefore to
currentStackSize and call the ShipStack removal API (e.g., stack->RemoveTrack(i)
or stack->RemoveParticle(i) as appropriate) to delete the newly added
secondaries, iterating backwards when removing to avoid index shifts; leave
StatusCode untouched on TParticle.
- Around line 166-169: BeginEvent currently clears fCloneTracks and
fContinuationTracks but not fSecondaryBuffer, so buffered clones left by
PreTrack() can leak into the next event; modify exitHadronAbsorber::BeginEvent
to also clear fSecondaryBuffer (in the same place where fCloneTracks.clear() and
fContinuationTracks.clear() are called) to ensure all per-event buffers are
reset at the event boundary.
In `@shipdata/ShipStack.cxx`:
- Around line 89-97: The change in ShipStack.cxx multiplies the supplied weight
by ancestor weights (parentPart->GetWeight()), effectively making PushTrack()
expect relative branch probabilities and causing duplicated scaling when callers
still pass absolute gMC->TrackWeight(); to fix, make PushTrack/store logic
accept the absolute weight as supplied by callers: remove the multiplication by
parentWeight (the parentPart->GetWeight() step) so weight remains the value
passed in, or alternatively update every caller (e.g., the splitter and places
that pass gMC->TrackWeight()) to pass only the branch probability instead of an
absolute weight — prefer removing the parent-weight multiplication in the code
handling fParticles/parentPart/GetWeight so PushTrack continues to store
absolute weights.
---
Outside diff comments:
In `@muonShieldOptimization/exitHadronAbsorber.cxx`:
- Around line 43-51: The constructor
exitHadronAbsorber::exitHadronAbsorber(const char* Name, Bool_t Active) leaves
fUseCaveCoordinates uninitialized causing ConstructGeometry() to branch on
indeterminate state; update this ctor's initializer list to explicitly
initialize fUseCaveCoordinates to kFALSE (same as the default ctor) so the
member has a defined value when ConstructGeometry() runs.
🪄 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: 2d634d98-9da4-45e4-8d0d-513cd3a57cee
📒 Files selected for processing (6)
CHANGELOG.mdgconfig/g4Config.Cmacro/run_fixedTarget.pymuonShieldOptimization/exitHadronAbsorber.cxxmuonShieldOptimization/exitHadronAbsorber.hshipdata/ShipStack.cxx
🚧 Files skipped from review as they are similar to previous changes (1)
- gconfig/g4Config.C
| * Change disable magnetic field in the MS and remove field map upload in run_fixedTarget.py | ||
| * Change the ShipMuonShield script to accept new magnet configuration: no fixed number of magnets and variable z-gap between them. | ||
| * Change naming convention for simulation files to `{sim,geo,params}_{uuid4}.root`, with optional `--tag` parameter to specify custom identifier | ||
| >>>>>>> 8e56f36c6 (Pion-kaon splitting in exitHadronAbsorber) |
There was a problem hiding this comment.
Remove the merge conflict marker.
This line contains a Git merge conflict resolution artifact (>>>>>>> 8e56f36c6) that must be removed before merging.
🔧 Proposed fix
->>>>>>> 8e56f36c6 (Pion-kaon splitting in exitHadronAbsorber)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| >>>>>>> 8e56f36c6 (Pion-kaon splitting in exitHadronAbsorber) |
🤖 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 `@CHANGELOG.md` at line 38, Remove the Git merge conflict marker string
">>>>>>> 8e56f36c6" from the CHANGELOG (it's an artifact left in the file);
simply delete that line so the changelog contains only valid content and no
conflict markers, then save the file and run a quick git diff to confirm no
other conflict markers (e.g., "<<<<<<<" or "=======") remain.
| void exitHadronAbsorber::BeginEvent() { | ||
| fCloneTracks.clear(); | ||
| fContinuationTracks.clear(); | ||
| } |
There was a problem hiding this comment.
Clear fSecondaryBuffer at the event boundary.
fSecondaryBuffer is only drained from PreTrack(). If the last split in an event happens after the last PreTrack() call, those buffered clones survive into the next event because BeginEvent() only clears the ID sets. That mixes tracks across events.
Proposed fix
void exitHadronAbsorber::BeginEvent() {
+ fSecondaryBuffer.clear();
fCloneTracks.clear();
fContinuationTracks.clear();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void exitHadronAbsorber::BeginEvent() { | |
| fCloneTracks.clear(); | |
| fContinuationTracks.clear(); | |
| } | |
| void exitHadronAbsorber::BeginEvent() { | |
| fSecondaryBuffer.clear(); | |
| fCloneTracks.clear(); | |
| fContinuationTracks.clear(); | |
| } |
🤖 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 `@muonShieldOptimization/exitHadronAbsorber.cxx` around lines 166 - 169,
BeginEvent currently clears fCloneTracks and fContinuationTracks but not
fSecondaryBuffer, so buffered clones left by PreTrack() can leak into the next
event; modify exitHadronAbsorber::BeginEvent to also clear fSecondaryBuffer (in
the same place where fCloneTracks.clear() and fContinuationTracks.clear() are
called) to ensure all per-event buffers are reset at the event boundary.
| if (isNaturalDecay && stack) { | ||
| Int_t currentStackSize = stack->GetNtrack(); | ||
| if (currentStackSize > stackSizeBefore) { | ||
| // Loop through the newly added natural secondaries | ||
| for (Int_t i = stackSizeBefore; i < currentStackSize; ++i) { | ||
| TParticle* secPart = stack->GetParticle(i); | ||
| if (secPart) { | ||
| secPart->SetStatusCode(-1); // Convention flag indicating a dead/discarded track | ||
| } |
There was a problem hiding this comment.
Don't use StatusCode as a discard flag.
ShipStack::PopNextTrack() treats TParticle::StatusCode as the track ID, so writing -1 here corrupts the identity of these decay secondaries without actually removing them from fStack. If any of them are still popped later, downstream point bookkeeping and mother remapping will be wrong.
🤖 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 `@muonShieldOptimization/exitHadronAbsorber.cxx` around lines 296 - 304, The
code currently sets secPart->SetStatusCode(-1) for natural-decay secondaries,
but ShipStack::PopNextTrack() uses TParticle::StatusCode as the track ID so this
corrupts identities; instead remove those particles from the stack itself (do
not touch StatusCode). Locate the isNaturalDecay handling that iterates from
stackSizeBefore to currentStackSize and call the ShipStack removal API (e.g.,
stack->RemoveTrack(i) or stack->RemoveParticle(i) as appropriate) to delete the
newly added secondaries, iterating backwards when removing to avoid index
shifts; leave StatusCode untouched on TParticle.
| if (parentId >= 0) { | ||
| if (parentId < fParticles->GetEntriesFast()) { | ||
| TParticle* parentPart = (TParticle*)fParticles->At(parentId); | ||
| if (parentPart) { | ||
| Double_t parentWeight = parentPart->GetWeight(); | ||
| weight = weight * parentWeight; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This silently changes PushTrack() to expect relative weights.
The new splitter still passes gMC->TrackWeight()-scaled values, so every non-primary clone/continuation gets multiplied by an ancestor weight again here. Either keep PushTrack() storing the supplied absolute weight, or update the new caller(s) to pass only branch probabilities; as written, split-track weights are inflated.
🤖 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 `@shipdata/ShipStack.cxx` around lines 89 - 97, The change in ShipStack.cxx
multiplies the supplied weight by ancestor weights (parentPart->GetWeight()),
effectively making PushTrack() expect relative branch probabilities and causing
duplicated scaling when callers still pass absolute gMC->TrackWeight(); to fix,
make PushTrack/store logic accept the absolute weight as supplied by callers:
remove the multiplication by parentWeight (the parentPart->GetWeight() step) so
weight remains the value passed in, or alternatively update every caller (e.g.,
the splitter and places that pass gMC->TrackWeight()) to pass only the branch
probability instead of an absolute weight — prefer removing the parent-weight
multiplication in the code handling fParticles/parentPart/GetWeight so PushTrack
continues to store absolute weights.
2 participants
Add option to split kaons and pions right before they decay. This allows to increase the number of muons (many muons post-HA stem from kaons and pions) in a smaller amount of time than that taken by increasing the number of PoTs.
Checklist
Summary by CodeRabbit
New Features
--kaon-pion-splits/-kpicommand-line option to configure splitting behavior