feat: trkrclusterv6 implementation#4319
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)
📝 WalkthroughWalkthroughThe PR adds ChangesCluster diagnostics and TPC clustering
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}
There was a problem hiding this comment.
Actionable comments posted: 7
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8f88b0d4-799f-4cda-817b-c936f1f593d1
📒 Files selected for processing (11)
offline/packages/TrackingDiagnostics/TrackResiduals.ccoffline/packages/TrackingDiagnostics/TrackResiduals.hoffline/packages/TrackingDiagnostics/TrkrNtuplizer.ccoffline/packages/tpc/TpcClusterizer.ccoffline/packages/tpc/TpcClusterizer.hoffline/packages/trackbase/TrkrCluster.hoffline/packages/trackbase/TrkrClusterv4.ccoffline/packages/trackbase/TrkrClusterv4.hoffline/packages/trackbase/TrkrClusterv5.ccoffline/packages/trackbase/TrkrClusterv5.hoffline/packages/trackbase/TrkrClusterv6.h
| if (FixedWindow != 0) | ||
| { | ||
| tup = FixedWindow; | ||
| tdown = FixedWindow; | ||
| if (tbin + tup >= NTBinsMax) | ||
| { | ||
| tup = NTBinsMax - tbin - 1; | ||
| edge++; | ||
| counts.nedge++; | ||
| } | ||
| if ((tbin - tdown) <= 0) | ||
| { | ||
| tdown = tbin; | ||
| edge++; | ||
| counts.nedge++; | ||
| } | ||
| return; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve detailed edge flags in fixed-window mode.
The fixed-window early returns increment counts.nedge but never set tledge/tredge/sledge/sredge; find_phi_range also repeats the generic edge increment once per scanned time bin. Downstream diagnostics read the detailed flags, so fixed-window edge clusters report incomplete or inflated metadata.
Suggested direction
if (tbin + tup >= NTBinsMax)
{
tup = NTBinsMax - tbin - 1;
- counts.nedge++;
+ if (!ttop_edge)
+ {
+ counts.nedge++;
+ counts.tredge = 1;
+ ttop_edge = true;
+ }
}
if ((tbin - tdown) <= 0)
{
tdown = tbin;
- counts.nedge++;
+ if (!tbottom_edge)
+ {
+ counts.nedge++;
+ counts.tledge = 1;
+ tbottom_edge = true;
+ }
}Apply the same guarded/category update for phitop_edge and phibottom_edge in find_phi_range.
Also applies to: 327-341
There was a problem hiding this comment.
This will be fixed in a future PR
There was a problem hiding this comment.
@osbornjd, understood — thanks for the clarification.
This still looks worth tracking since the detailed edge metadata can be incomplete/inflated in fixed-window mode. If you'd like, I can open a follow-up GitHub issue for this and include links back to PR #4319 and the review thread so it doesn’t get lost.
🧠 Learnings used
Learnt from: pinkenburg
Repo: sPHENIX-Collaboration/coresoftware PR: 4268
File: offline/packages/CaloReco/ClusterCDFCalculator.cc:9-9
Timestamp: 2026-05-18T16:21:14.018Z
Learning: In the sPHENIX core software repository, C++20 std::format is supported (via the <format> header). During code review, do not flag uses of std::format / <format> as a potential C++-version or compiler-compatibility concern anywhere in the C++ codebase. (Other issues may still be flagged as appropriate.)
|
|
||
| char tsize = tbinhi - tbinlo + 1; | ||
| char phisize = phibinhi - phibinlo + 1; | ||
| char rsize = size; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not narrow cluster hit count to char.
size is an int count of accepted hits, but char rsize = size can wrap or become negative before setRSize, which corrupts the new getRSize() diagnostics/ntuple field for larger clusters. Keep this value in a wider integral type through the cluster API/storage.
| float getRSize() const override { return (float) m_rsize; } | ||
| void setRSize(unsigned char rsize) { m_rsize = rsize; } | ||
| void setRSize(const char rsize) override { m_rsize = rsize; } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Avoid storing the new cluster size metric in char.
setRSize(const char) stores the hit-count-derived size in m_rsize; the producer narrows an int size into this field. Larger TPC clusters can wrap or become negative on signed-char platforms, corrupting the new ntuple/diagnostic size value. Prefer at least unsigned short/int across the base API, v6 storage, and producer assignment.
Also applies to: 196-196
| auto *rcs = recoConsts::instance(); | ||
| m_runnumber = rcs->get_IntFlag("RUNNUMBER"); | ||
| m_segment = rcs->get_IntFlag("RUNSEGMENT"); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect PHFlag missing-key behavior and RUNNUMBER/RUNSEGMENT setup sites.
fd -i '^PHFlag\.(cc|cxx|cpp)$' --exec sed -n '/get_IntFlag/,/^}/p' {}
rg -n -C2 '"RUNNUMBER"|"RUNSEGMENT"|get_IntFlag\("RUNNUMBER"|get_IntFlag\("RUNSEGMENT"' .Repository: sPHENIX-Collaboration/coresoftware
Length of output: 14811
Preserve configured run/segment fallbacks when recoConsts flags are absent
The current code unconditionally reads RUNNUMBER and RUNSEGMENT using the single-argument overload. If these global flags are not set, the API returns 0 and logs an error, overriding any values explicitly set via the runnumber(int) or setSegment(int) member accessors.
Use the two-argument overload to default to the member values, maintaining correct configuration precedence.
Proposed fix
auto *rcs = recoConsts::instance();
- m_runnumber = rcs->get_IntFlag("RUNNUMBER");
- m_segment = rcs->get_IntFlag("RUNSEGMENT");
+ m_runnumber = rcs->get_IntFlag("RUNNUMBER", m_runnumber);
+ m_segment = rcs->get_IntFlag("RUNSEGMENT", m_segment);📝 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.
| auto *rcs = recoConsts::instance(); | |
| m_runnumber = rcs->get_IntFlag("RUNNUMBER"); | |
| m_segment = rcs->get_IntFlag("RUNSEGMENT"); | |
| auto *rcs = recoConsts::instance(); | |
| m_runnumber = rcs->get_IntFlag("RUNNUMBER", m_runnumber); | |
| m_segment = rcs->get_IntFlag("RUNSEGMENT", m_segment); |
Source: Path instructions
| int m_size = std::numeric_limits<int>::quiet_NaN(); | ||
| int m_phisize = std::numeric_limits<int>::quiet_NaN(); | ||
| int m_zsize = std::numeric_limits<int>::quiet_NaN(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep cluster size diagnostics as floats end-to-end.
TrkrCluster exposes getRSize(), getPhiSize(), and getZSize() as float; storing them in int scalars/vectors truncates residual and cluster-tree diagnostics. Please make these members float/std::vector<float> and update the ROOT leaflists from /I to /F where these scalars are branched. As per path instructions, header review focuses on API clarity/stability.
Proposed type fix
- int m_size = std::numeric_limits<int>::quiet_NaN();
- int m_phisize = std::numeric_limits<int>::quiet_NaN();
- int m_zsize = std::numeric_limits<int>::quiet_NaN();
+ float m_size = std::numeric_limits<float>::quiet_NaN();
+ float m_phisize = std::numeric_limits<float>::quiet_NaN();
+ float m_zsize = std::numeric_limits<float>::quiet_NaN();
...
- std::vector<int> m_clussize;
- std::vector<int> m_clusphisize;
- std::vector<int> m_cluszsize;
+ std::vector<float> m_clussize;
+ std::vector<float> m_clusphisize;
+ std::vector<float> m_cluszsize;Also applies to: 323-325
Source: Path instructions
| } | ||
| */ | ||
| fXcluster[n_cluster::nclusize] = cluster->getSize(); | ||
| fXcluster[n_cluster::nclusize] = cluster->getRSize(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve size for cluster versions without getRSize().
TrkrClusterv4 only overrides getSize(), so this change makes existing v4 clusters write the base getRSize() NaN into the size column. Either implement getRSize() for all cluster versions that reach this ntuple, or keep a fallback to the legacy size value.
Proposed fallback
- fXcluster[n_cluster::nclusize] = cluster->getRSize();
+ const float rsize = cluster->getRSize();
+ fXcluster[n_cluster::nclusize] = std::isnan(rsize) ? cluster->getSize() : rsize;📝 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.
| fXcluster[n_cluster::nclusize] = cluster->getRSize(); | |
| const float rsize = cluster->getRSize(); | |
| fXcluster[n_cluster::nclusize] = std::isnan(rsize) ? cluster->getSize() : rsize; |
Build & test reportReport for commit c0388d0084e12259c59356d9fd6b216b316e6014:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit a4e04a8c60d442de1c80a25b0e7b545afd835f73:
Automatically generated by sPHENIX Jenkins continuous integration |
1 participant
This implements the v6 option now without any performance changes
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)
AI-assisted summary; AI can make mistakes, so please use best judgment when reviewing.
Motivation / context
trkrclusterv6onmasterwhile keeping the existing v5 clustering path intact.Key changes
TpcClusterizer):ClusterCounters-based flow to classify overlap plus detailed edge/mixing categories (phi/time edges, dead/hot edges, mix flags) and drive cluster parameter computation.TrkrClusterv5andTrkrClusterv6when constructing cluster objects.m_debuginto threaded clustering context.DetailedClusterAnalysis()to enable extended debug behavior.TrkrClusterand cluster implementations):TrkrClustervirtual setter/getter signatures to tighten const-correctness and align return types (e.g.,getPadMax/getTBinMaxand several mix/bin/phase getters) with the v6/diagnostic needs.TrkrClusterv5/TrkrClusterv6method signatures to match the tightened interface expectations.TrackResiduals: captureevt_idfrom the event header and write it into multiple ROOT trees/branches; expand cluster-tree/residual-tree branch schema with additional cluster metadata (ADC center/max, geometry-derived size/bin/phase fields, detailed edge/mix/overlap counts and bounds)."/B"leaf specifiers (e.g.,m_overlap/B,m_slmix/B, etc.), reflecting the intended small-integer/char storage.TrkrNtuplizer: extend NTuple cluster column list and enum indices; fill new cluster fields from additionalTrkrClustergetters; adjust which cluster “size” getter is used.Potential risk areas
TrkrCluster/TrkrClusterv5/TrkrClusterv6signature and return-type changes may affect any external code building against these classes.Possible future improvements