Geantino ionization and Acts fitter update - #4352
Merged
osbornjd merged 7 commits intoJul 23, 2026
Merged
Conversation
Contributor
|
Warning Review limit reached
Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds ChangesGeantino ionization processing
Track surface handling
Sequence Diagram(s)sequenceDiagram
participant PHG4GeantinoIonization
participant PHCompositeNode
participant PHG4TruthInfoContainer
participant PHG4HitContainer
PHG4GeantinoIonization->>PHCompositeNode: fetch G4TruthInfo and detector hit nodes
PHG4GeantinoIonization->>PHG4TruthInfoContainer: match configured geantino particle
PHG4GeantinoIonization->>PHG4HitContainer: inspect detector hits
PHG4GeantinoIonization->>PHG4HitContainer: set edep and eion from path length
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b568b9f9-1d0f-4ae5-aad3-fa6c31543f6d
📒 Files selected for processing (4)
offline/packages/trackreco/PHActsTrkFitter.ccsimulation/g4simulation/g4detectors/Makefile.amsimulation/g4simulation/g4detectors/PHG4GeantinoIonization.ccsimulation/g4simulation/g4detectors/PHG4GeantinoIonization.h
Build & test reportReport for commit b3bf24a11e6d158803ed267360c9067e1e937dd1:
Automatically generated by sPHENIX Jenkins continuous integration |
osbornjd
reviewed
Jul 21, 2026
|
|
||
| namespace | ||
| { | ||
| struct GasFractions |
Contributor
There was a problem hiding this comment.
Is there a reason to have a separate struct for gas fractions? We have these set at the macro level, which ultimately sets them in the actual geometry object
Contributor
Contributor
Author
There was a problem hiding this comment.
Thanks Joe. Now module reads gas fraction directly from parameter node tree
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: db0555a7-bfc4-478c-83b8-dfa58834279e
📒 Files selected for processing (2)
simulation/g4simulation/g4detectors/PHG4GeantinoIonization.ccsimulation/g4simulation/g4detectors/PHG4GeantinoIonization.h
Comment on lines
+57
to
+62
| int PHG4GeantinoIonization::InitRun(PHCompositeNode* topNode) | ||
| { | ||
| if (!m_detectorConfigs[2].enabled) | ||
| { | ||
| return Fun4AllReturnCodes::EVENT_OK; | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Magic array index couples InitRun to unenforced DetectorConfig ordering.
m_detectorConfigs[2] silently assumes TPC is always the third entry in m_detectorConfigs. If the initializer order in the constructor is ever changed/reordered, this check (and downstream m_tpcMipDedx computation) would silently apply to the wrong detector with no compiler diagnostic — a latent correctness risk with no test to catch it.
🛠️ Proposed fix using enum-based lookup
- if (!m_detectorConfigs[2].enabled)
- {
- return Fun4AllReturnCodes::EVENT_OK;
- }
+ const auto tpcConfigIter = std::find_if(
+ m_detectorConfigs.begin(), m_detectorConfigs.end(),
+ [](const DetectorConfig& config)
+ { return config.detector == DetectorId::tpc; });
+ if (tpcConfigIter == m_detectorConfigs.end() || !tpcConfigIter->enabled)
+ {
+ return Fun4AllReturnCodes::EVENT_OK;
+ }(requires #include <algorithm> if not already present)
Build & test reportReport for commit b84e8b04d4fef6dc329928fec873c0db57faa5d4:
Automatically generated by sPHENIX Jenkins continuous integration |
Contributor
|
This kills the tracking efficiency in the QA, we need to understand why |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
offline/packages/trackreco/PHActsTrkFitter.cc (1)
1042-1043: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse
nextSurfacewhen computing the next radius.This currently calls
center()onsurface, sonextRadiusalways equalsthisRadius. Same-volume surfaces with decreasing radii therefore bypass the ordering check and remain incorrectly ordered.Proposed fix
- const Acts::Vector3 next_center = surface->center(m_tGeometry->geometry().getGeoContext()); + const Acts::Vector3 next_center = nextSurface->center(m_tGeometry->geometry().getGeoContext());
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fdab9389-7276-4e69-9169-9ff76ba9e47a
📒 Files selected for processing (1)
offline/packages/trackreco/PHActsTrkFitter.cc
Build & test reportReport for commit f124eca313bb1f8cdcedac3263258ce5bfa2fce0:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 7aef5cdc9a9c3324259e1762de1fbe2716b3897f:
Automatically generated by sPHENIX Jenkins continuous integration |
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
2 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.
PHG4GeantinoIonization, which assigns detector-dependent mean MIP energy deposition and ionization to charged-geantino G4 hits in MVTX, INTT, TPC, and TPOT before digitization.PHActsTrkFitterto support directed fitting with an empty ACTS material map by falling back to measurement surfaces when no material surfaces are available.This allows charged-geantino tracks to be digitized and fitted with ACTS while disabling ACTS material corrections.
Types of changes
What kind of change does this PR introduce? (Bug fix, feature, ...)
TODOs (if applicable)
Links to other PRs in macros and calibration repositories (if applicable)
Motivation
Charged-geantino hits were missing detector-appropriate mean MIP ionization before digitization, and ACTS directed fitting could fail when the material surface map was empty. This PR adds a geantino ionization pre-step and hardens
PHActsTrkFitterso charged-geantino digitization/fitting works without relying on material surfaces.Key changes
Added
PHG4GeantinoIonization(simulation/g4simulation/g4detectors)SubsysRecothat targets charged-geantino hits (matching truth particle name) and sets detector-dependent meanedep/eionin MVTX, INTT, TPC, and Micromegas/TPOT.edep/eionare already finite and nonnegative, the hit is left unchanged (preserves existing downstream digitization fluctuations).edep/eionas (mean MIP dE/dx) × path length, using configurable silicon MIP values for MVTX/INTT and TPC gas-mixture-dependent MIP derived from TPC geometry parameters.G4GEO_TPCis missing, attempts to rebuildG4GEO_TPCfromG4GEOPARAM_TPC(underPAR/TPC); aborts the run if required inputs/nodes are missing.Build integration
simulation/g4simulation/g4detectors/Makefile.amto install the new header and compile/linkPHG4GeantinoIonization.Updated
PHActsTrkFitter(offline/packages/trackreco/PHActsTrkFitter.cc)surfaces_tmp).checkSurfaceVec: it now returns immediately when fewer than two surfaces are provided.Potential risk areas
edep/eionfrom sentinel behavior to mean MIP-based values can affect digitization and downstream tracking/QA performance.G4GEO_TPCfromG4GEOPARAM_TPC) can fail or produce unexpected values if geometry parameter names/structure differ from assumptions.Possible future improvements
edep/eionsynthesis,Note: AI can make mistakes—use best judgment and verify the summary against the actual diffs/implementation details during review.