speed up by x1000#4289
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesTruth/Reco Map Consolidation
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)simulation/g4simulation/g4eval/SvtxTruthRecoTableEval.ccIn file included from simulation/g4simulation/g4eval/SvtxTruthRecoTableEval.cc:2: ... [truncated 2200 characters] ... n file "src/clang/cTrans.ml", line 4782, characters 12-47 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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 73b0f85a-74e1-4e6f-902b-ed0758e5f21b
📒 Files selected for processing (2)
simulation/g4simulation/g4eval/SvtxTruthRecoTableEval.ccsimulation/g4simulation/g4eval/SvtxTruthRecoTableEval.h
| PHG4Particle *g4particle = truthinfo->GetParticle(fastsim_track->get_truth_track_id()); | ||
| const float clusCont = trackeval->get_nclusters_contribution(track, g4particle); | ||
| truthmap[clusCont].insert(g4particle->get_track_id()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check GetParticle return behavior and usage patterns
rg -nP --type=cpp -C3 'GetParticle\s*\(' -g '!*Test*' -g '!*test*' | head -100Repository: sPHENIX-Collaboration/coresoftware
Length of output: 8412
Critical: add null-check for g4particle in fast-sim truth handling.
PHG4TruthInfoContainer::GetParticle(...) can return nullptr, but SvtxTruthRecoTableEval.cc dereferences g4particle at g4particle->get_track_id() (and passes it to trackeval->get_nclusters_contribution) without a guard. Add an if (g4particle) before using it.
🛡️ Suggested null-check
if (fastsim_track)
{
// Preserve SvtxTrackEval::all_truth_particles fast-sim special case for reco->truth maps only.
PHG4Particle *g4particle = truthinfo->GetParticle(fastsim_track->get_truth_track_id());
+ if (g4particle)
+ {
const float clusCont = trackeval->get_nclusters_contribution(track, g4particle);
truthmap[clusCont].insert(g4particle->get_track_id());
+ }
}📝 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.
| PHG4Particle *g4particle = truthinfo->GetParticle(fastsim_track->get_truth_track_id()); | |
| const float clusCont = trackeval->get_nclusters_contribution(track, g4particle); | |
| truthmap[clusCont].insert(g4particle->get_track_id()); | |
| PHG4Particle *g4particle = truthinfo->GetParticle(fastsim_track->get_truth_track_id()); | |
| if (g4particle) | |
| { | |
| const float clusCont = trackeval->get_nclusters_contribution(track, g4particle); | |
| truthmap[clusCont].insert(g4particle->get_track_id()); | |
| } |
Build & test reportReport for commit 36cda703fe1169a1b1541855ab78177182b021ea:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 1f53daa5b4994223cec26fe113e569bcf9fc518a:
Automatically generated by sPHENIX Jenkins continuous integration |
osbornjd
left a comment
osbornjd
left a comment
There was a problem hiding this comment.
I tested this on the oo sims and with some default sims and inspected the output from the module with the current new build and with this PR, and the results look identical for a single event. Reading through the code it looks to do the same thing in just a much more clever way rather than going track by track and using the evaluation machinery. We may consider seeing what else codex can improve in the evaluation machinery because it all uses stl containers probably not in the most simplified way...
|
|
||
| // only record particle above minimal momentum requirement. | ||
| if (momentum < m_minMomentumTruthMap) | ||
| if (momentum2 < minMomentumTruthMap2) |
There was a problem hiding this comment.
Not sure why codex decided to change this from pT to pT^2 but in principle I don't think it matters, other than being potentially confusing when reading the code later
There was a problem hiding this comment.
I added a comment in the code - to point this out. It does save us a slow sqrt operation which is likely the reason why codex implemented the p cut this way
Build & test reportReport for commit 5d974c7f4af486ec02d84f2aecec3f13a0f188f6:
Automatically generated by sPHENIX Jenkins continuous integration |
2 participants
Types of changes
What kind of change does this PR introduce? (Bug fix, feature, ...)
codex managed to speed up SvtxTruthRecoTableEval by x1000. The resulting root files are binary identical with the previous version. This needs to be reviewed
TODOs (if applicable)
Links to other PRs in macros and calibration repositories (if applicable)
Motivation and Context
SvtxTruthRecoTableEval builds mapping tables between simulation truth particles and reconstructed SVTX tracks for downstream evaluation. This PR consolidates truth→reco and reco→truth mapping into a single pass to drastically reduce runtime (~1000× reported) while preserving the ROOT output format.
Key Changes
Potential Risk Areas
Possible Future Improvements
Note: AI summaries can miss subtle details—please review cluster-matching, weight accumulation, and the fast-sim path carefully during code review.