feat: implement v6 option in tpc clusterizer#4312
Merged
Merged
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughTrkrCluster/TrkrClusterv4/v5/v6 field types are narrowed (float → int/char/unsigned short) for pad/tbin max and bin indices, with const parameters and override specifiers standardized. TpcClusterizer gains a ClusterCounters struct, DetailedClusterAnalysis() debug mode, refactored range-finding functions, and check_cluster_touching helper; calc_cluster_parameter now emits TrkrClusterv6 with all new fields populated. TrackResiduals retrieves EventHeader for event ID, extends cluster vectors, fills them from updated TrkrCluster getters, and adds branches to all output trees. TrkrNtuplizer extends its NTuple schema and FillCluster to record all new observables. ChangesTPC Cluster Detail Pipeline
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: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 236e322b-ac8a-4011-b00a-cfc0a3e43a9c
📒 Files selected for processing (7)
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/TrkrClusterv6.h
|
|
||
| char tsize = tbinhi - tbinlo + 1; | ||
| char phisize = phibinhi - phibinlo + 1; | ||
| char rsize = size; |
Contributor
There was a problem hiding this comment.
Potential truncation: cluster size stored in char.
size is an int counting hits in the cluster (incremented at line 673), but rsize is char (line 886). If a cluster has more than 127 hits (or 255 for unsigned char), the value will wrap around silently.
Large clusters from overlapping tracks or noise could exceed this limit. Consider either clamping to CHAR_MAX or using a wider type.
Suggested fix
- char rsize = size;
+ char rsize = (size > std::numeric_limits<char>::max())
+ ? std::numeric_limits<char>::max()
+ : static_cast<char>(size);Or add a sanity check if this should never happen:
+ if (size > std::numeric_limits<char>::max()) {
+ std::cout << PHWHERE << "Warning: cluster size " << size << " exceeds char range" << std::endl;
+ }
char rsize = static_cast<char>(size);📝 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.
Suggested change
| char rsize = size; | |
| char rsize = (size > std::numeric_limits<char>::max()) | |
| ? std::numeric_limits<char>::max() | |
| : static_cast<char>(size); |
Build & test reportReport for commit 2f80ad06ca0c895de1b979afb7383509f0a45c86:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit dc21c7e51b1b7f0996ee8e7771e653fe6ebf18f5:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 3854e03585d80e1ead504ce61dab0f602ce0414d:
Automatically generated by sPHENIX Jenkins continuous integration |
Contributor
Author
|
@pinkenburg can you give an extra set of eyes on the IO objects and make sure I haven't inadvertently mangled something, I tried cleaning them up and making them consistent |
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: 4ce901d2-f83a-4f4b-a4b5-2295a91e6f51
📒 Files selected for processing (9)
offline/packages/TrackingDiagnostics/TrackResiduals.ccoffline/packages/TrackingDiagnostics/TrackResiduals.hoffline/packages/tpc/TpcClusterizer.ccoffline/packages/trackbase/TrkrCluster.hoffline/packages/trackbase/TrkrClusterv4.ccoffline/packages/trackbase/TrkrClusterv4.hoffline/packages/trackbase/TrkrClusterv5.ccoffline/packages/trackbase/TrkrClusterv5.hoffline/packages/trackbase/TrkrClusterv6.h
✅ Files skipped from review due to trivial changes (3)
- offline/packages/trackbase/TrkrClusterv4.cc
- offline/packages/trackbase/TrkrClusterv5.cc
- offline/packages/trackbase/TrkrClusterv4.h
🚧 Files skipped from review as they are similar to previous changes (3)
- offline/packages/TrackingDiagnostics/TrackResiduals.h
- offline/packages/TrackingDiagnostics/TrackResiduals.cc
- offline/packages/tpc/TpcClusterizer.cc
Build & test reportReport for commit edbd44ed868c831eab405b8b50eb035b12584262:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 497ed8d0f58826246b9e3dacc60fa7ad21685af9:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 5e357d834043708cef8cb208b237e2d563b49b0a:
Automatically generated by sPHENIX Jenkins continuous integration |
pinkenburg
approved these changes
Jun 24, 2026
|
|
||
| auto *rcs = recoConsts::instance(); | ||
| m_runnumber = rcs->get_IntFlag("RUNNUMBER"); | ||
| m_segment = rcs->get_IntFlag("RUNSEGMENT"); |
Contributor
There was a problem hiding this comment.
The segment is really meaningless. It doesn't go back to the rawdata (so it won't help locating the event) but is just a feature of the filename depending on how many events we store in a file which is subject to change.
This was referenced Jun 25, 2026
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.
This should implement @Ishangoel11 v6 cluster object and get all of the types consistent between the base class and derived class IO object.
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 and Context
This PR introduces an optional v6 clustering mode in the TPC clusterizer to provide richer per-cluster characterization (edge/mixing/overlap) and to tighten type consistency between the
TrkrClusterbase interface and the v5/v6 derived cluster implementations. It also updates tracking diagnostics and cluster NTuples to serialize the new observables. Note: AI can make mistakes—please use best judgment and verify key API/IO impacts locally, with extra attention to the IO object refactoring/standardization mentioned by the author (independent verification that IO changes are correct and not unintentionally corrupted).Key Changes
TPC clustering (v6 support + richer bookkeeping)
ClusterCounters-based workflow to track cluster overlap, overall edge counts, detailed per-side edge categories, and phi/time “mix” touching indicators.check_cluster_touching) across adjacent phi/time cells to populate mix indicators.get_cluster,calc_cluster_parameter) to propagateClusterCountersinto the produced cluster object.DetailedClusterAnalysis()/m_debugdebug path to selectTrkrClusterv6(otherwiseTrkrClusterv5).ProcessSectorDatato preserve original ADC data for neighbor checks and to reset/pipelineClusterCountersappropriately.Cluster object API/type contract alignment
TrkrCluster.h: const-correct setter parameters plus multiple public type contract changes (notablygetPadMax/getTBinMaxfloat→int, mix getters int→char, phi/tbin bin lo/hi float→unsigned short) and associated sentinel changes.TrkrClusterv6.h: standardized signatures withoverrideand aligned member types/defaults to the updated contracts.Tracking diagnostics + ROOT output extensions
TrackResiduals: readEventHeader, storem_evt_id, and write a newevt_idbranch into multiple ROOT trees.Cluster NTuple schema updates
TrkrNtuplizer: extended_ntp_cluster/n_clusterschema and filling to include new per-cluster ADC/bin/max/phase and edge/mixing-related fields.nclusizeto usecluster->getRSize()instead ofcluster->getSize().Potential Risk Areas
TrackResiduals) and TNtuple column/schema layout changes (TrkrNtuplizer) may require downstream analysis updates.TrkrCluster(and v6 alignment) can break downstream code or subtly change behavior if callers assumedfloat/NaN or other sentinel semantics.ClusterCountersresets and any new debug flag plumbing do not leak state across parallel execution contexts.Possible Future Improvements
TrkrClustercontracts and updated ROOT/TNtuple schemas.