Filling the newly added variables and saving them. As well as printing them.#4296
Closed
Ishangoel11 wants to merge 3 commits into
Closed
Filling the newly added variables and saving them. As well as printing them.#4296Ishangoel11 wants to merge 3 commits into
Ishangoel11 wants to merge 3 commits into
Conversation
Contributor
|
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)
📝 WalkthroughWalkthroughAdds evt_id sourcing and recoConsts-run/segment, expands cluster diagnostics across TrackResiduals and TrkrNtuplizer, and refactors TPC cluster finding to use ClusterCounters to record overlap/edge/mix and populate TrkrClusterv6 metadata. ChangesTPC Cluster Diagnostics Expansion
Possibly Related PRs
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
offline/packages/TrackingDiagnostics/TrkrNtuplizer.cc (1)
2339-2348:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInitialize
ncluredgebefore the boundary-layer check to avoid stale NTuple values.At Line 2343,
ncluredgeis only written for a subset of layers. Since callers pass non-zero-initialized stack buffers intoFillCluster, non-matching layers can carry stale values into theredgecolumn.Suggested fix
fXcluster[n_cluster::nclusize] = cluster->getRSize(); fXcluster[n_cluster::ncluphisize] = cluster->getPhiSize(); fXcluster[n_cluster::ncluzsize] = cluster->getZSize(); fXcluster[n_cluster::nclupedge] = cluster->getEdge(); + fXcluster[n_cluster::ncluredge] = 0; if (layer_local == 7 || layer_local == 22 || layer_local == 23 || layer_local == 28 || layer_local == 39 || layer_local == 54) { fXcluster[n_cluster::ncluredge] = 1; }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 78b00e0a-6b92-4663-aacf-5cff722a7333
📒 Files selected for processing (4)
offline/packages/TrackingDiagnostics/TrackResiduals.ccoffline/packages/TrackingDiagnostics/TrackResiduals.hoffline/packages/TrackingDiagnostics/TrkrNtuplizer.ccoffline/packages/tpc/TpcClusterizer.cc
Comment on lines
+342
to
+344
| auto *rcs = recoConsts::instance(); | ||
| m_runnumber = rcs->get_IntFlag("RUNNUMBER"); | ||
| m_segment = rcs->get_IntFlag("RUNSEGMENT"); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== recoConsts API =="
fd -i 'recoConsts*' . | xargs -r rg -n -C2 'class recoConsts|get_IntFlag|FlagExist'
echo
echo "== RUNNUMBER/RUNSEGMENT producers and consumers =="
rg -n -C2 'RUNNUMBER|RUNSEGMENT|get_IntFlag\("RUNNUMBER"|get_IntFlag\("RUNSEGMENT"|FlagExist\("RUNNUMBER"|FlagExist\("RUNSEGMENT"' .Repository: sPHENIX-Collaboration/coresoftware
Length of output: 15207
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search for RUNSEGMENT in codebase =="
rg -n "RUNSEGMENT" .
echo
echo "== Inspect TrackResiduals::process_event() region =="
sed -n '300,380p' offline/packages/TrackingDiagnostics/TrackResiduals.cc
echo
echo "== Inspect recoConsts / PHFlag get_IntFlag behavior =="
sed -n '1,200p' offline/framework/phool/recoConsts.h
# PHFlag.h might hold the actual flag-getter implementation/contract
sed -n '1,220p' offline/framework/phool/PHFlag.h 2>/dev/null || trueRepository: sPHENIX-Collaboration/coresoftware
Length of output: 5350
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search for RUNSEGMENT producers (set_IntFlag) =="
rg -n 'set_IntFlag\("RUNSEGMENT"|FlagExist\("RUNSEGMENT"|get_IntFlag\("RUNSEGMENT"|RUNSEGMENT' .
echo
echo "== Search for PHFlag::get_IntFlag implementation =="
fd -i '*PHFlag*' offline/framework/phool | head -n 50
rg -n "get_IntFlag\\(const std::string &name\\)" offline/framework/phool -S || true
rg -n "get_IntFlag\\(const std::string &name, const int defaultval\\)" offline/framework/phool -S || trueRepository: sPHENIX-Collaboration/coresoftware
Length of output: 596
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate PHFlag source files =="
fd -i "PHFlag" offline/framework/phool | head -n 50
echo
echo "== Find PHFlag::get_IntFlag implementations =="
rg -n "PHFlag::get_IntFlag" offline/framework/phool
echo
echo "== Search for RUNSEGMENT usage patterns (all forms) =="
rg -n 'set_IntFlag\("RUNSEGMENT"|FlagExist\("RUNSEGMENT"|get_IntFlag\("RUNSEGMENT"' .
rg -n 'get_IntFlag\("RUNSEGMENT"\s*,\s*' . || trueRepository: sPHENIX-Collaboration/coresoftware
Length of output: 585
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '70,160p' offline/framework/phool/PHFlag.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 2465
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== TrackResiduals.cc uses of m_segment =="
rg -n "m_segment" offline/packages/TrackingDiagnostics/TrackResiduals.cc
echo
echo "== TrackResiduals.cc uses of m_runnumber =="
rg -n "m_runnumber" offline/packages/TrackingDiagnostics/TrackResiduals.cc
echo
echo "== Inspect surrounding code near first m_segment usage (if any) =="
rg -n -C 2 "m_segment" offline/packages/TrackingDiagnostics/TrackResiduals.cc | head -n 80Repository: sPHENIX-Collaboration/coresoftware
Length of output: 3316
Ensure RUNSEGMENT is set before TrackResiduals::process_event() fills the TTree branches
auto *rcs = recoConsts::instance();
m_runnumber = rcs->get_IntFlag("RUNNUMBER");
m_segment = rcs->get_IntFlag("RUNSEGMENT");
TrackResiduals::process_event() calls PHFlag::get_IntFlag("RUNSEGMENT") unconditionally; when the flag is missing, PHFlag::get_IntFlag prints an error + stack trace and returns 0, and that 0 is then written into the "segment" branches for every event. Since "RUNSEGMENT" is not set/checked anywhere else in the codebase (only read here), add a FlagExist("RUNSEGMENT") guard (or use the default overload) or ensure the flag is produced upstream.
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 (2)
offline/packages/tpc/TpcClusterizer.cc (2)
214-234:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
FixedWindowmode is effectively disabled here.Lines 214-341 no longer apply
my_data.FixedWindowduring range growth, so the firstget_cluster(...)call at Line 1279 always runs the step-down algorithm. The fallback block at Lines 1281-1319 then just reruns step-down on the same seed, which changes clustering behavior for any job that enables fixed-window mode.As per coding guidelines,
**/*.{cc,cpp,cxx,c}reviews should prioritize correctness, memory safety, error handling, and thread-safety.Also applies to: 321-341, 1281-1319
Source: Coding guidelines
647-648:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
TrainingHits::nedgeis saved before the dead/hot edge accounting is finalized.
training_hits->nedgeis copied fromcounts.nedgeat Lines 647-648, but Lines 754-802 can still incrementcounts.nedgefor masked dead/hot neighbors before the finalTrkrClusterv6metadata is written. That makes the saved training record disagree with the persisted cluster metadata whenever these masks fire.Suggested fix
- training_hits->ntouch = counts.overlap; - training_hits->nedge = counts.nedge; training_hits->v_adc.fill(0); @@ if (my_data.maskHot) { auto it = my_data.hotMap->find(tpcHitSetKey); @@ } } + + if (gen_hits && training_hits) + { + training_hits->ntouch = counts.overlap; + training_hits->nedge = counts.nedge; + }As per coding guidelines,
**/*.{cc,cpp,cxx,c}reviews should prioritize correctness, memory safety, error handling, and thread-safety.Also applies to: 754-802
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9f5cfe53-3830-4d72-a3bf-ba0b7a02a59e
📒 Files selected for processing (2)
offline/packages/TrackingDiagnostics/TrackResiduals.ccoffline/packages/tpc/TpcClusterizer.cc
🚧 Files skipped from review as they are similar to previous changes (1)
- offline/packages/TrackingDiagnostics/TrackResiduals.cc
Build & test reportReport for commit 7fa7f9f1d73c83764129124ffce9a4f369ef7aae:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 28ad3fb72dea49e025a610db1ee28609745c3778:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 3e4473304b5b87d9493672577ff41fb3c8a31a25:
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
1 participant
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.
them.
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)
Summary
This pull request enhances tracking diagnostics and TPC cluster characterization by adding explicit event identification, expanding per-cluster diagnostic metrics, and introducing cluster mixing/overlap detection during TPC clustering.
Motivation / context
Better event correlation and richer cluster diagnostics are needed to analyze reconstruction behavior, identify clustering artifacts (overlap/mixing, edge effects, ADC/centroid behavior), and improve downstream validation and physics-quality studies.
Key changes
Event identification
Cluster-finding and counters (TPC)
Output schema and diagnostics
Potential risk areas
Possible future improvements
Note: This summary was AI-assisted. Please validate against the code changes directly—AI can make mistakes or omit subtle details.