Trying again!#4300
Conversation
📝 WalkthroughWalkthroughThis PR expands TPC cluster metadata and refactors edge-detection accounting. It captures per-event IDs from EventHeader, extends cluster schema with centroid ADC and pad/tbin center/max values alongside edge/mix/phase categorization, updates the NTuple output columns accordingly, and replaces scalar touch/edge tracking in clustering with a structured ClusterCounters approach that distinguishes overlap, sector/time edges, dead/hot edges, and mixing flags. ChangesEvent ID & Cluster Attribute Schema
NTuple Schema & Cluster Filling
TPC Cluster Edge/Touching Accounting Refactor
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: 5
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)
2343-2348:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
ncluredgeis left uninitialized for most layers
fx_clusteris stack-allocated in callers and not pre-zeroed per entry; inFillCluster,ncluredgeis only set inside the specific-layerif(Line 2343-2348). For other layers, that field can carry indeterminate data into the NTuple row.Initialize
ncluredgebefore the conditional, then override to1for the listed layers.Suggested fix
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; }Also applies to: 2349-2361
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f5cd96b7-4cc1-482b-a675-e64ce4d33174
📒 Files selected for processing (4)
offline/packages/TrackingDiagnostics/TrackResiduals.ccoffline/packages/TrackingDiagnostics/TrackResiduals.hoffline/packages/TrackingDiagnostics/TrkrNtuplizer.ccoffline/packages/tpc/TpcClusterizer.cc
| struct ClusterCounters | ||
| { | ||
| int overlap = 0; | ||
|
|
||
| int nedge = 0; // Total No. of Edges | ||
|
|
||
| int sledge = 0; // Touching Left Sector Edge | ||
| int sredge = 0; // Touching Right Sector Edge | ||
|
|
||
| int tledge = 0; // Touching Left Time Edge | ||
| int tredge = 0; // Touching Right Time Edge | ||
|
|
||
| int dledge = 0; // Touching Left Dead Edge | ||
| int dredge = 0; // Touching Right Dead Edge | ||
|
|
||
| int hledge = 0; // Touching Left Hot Edge | ||
| int hredge = 0; // Touching Right Hot Edge | ||
|
|
||
| int slmix = 0; // Touching Cluster at Left in Phibin | ||
| int srmix = 0; // Touching Cluster at Right in Phibin | ||
|
|
||
| int tlmix = 0; // Touching Cluster at Left in Timebin | ||
| int trmix = 0; // Touching Cluster at Right in Timebin | ||
|
|
||
| void clear() | ||
| { | ||
| *this = ClusterCounters{}; | ||
| } |
There was a problem hiding this comment.
Preserve overlap direction instead of collapsing it into counts.overlap.
This refactor records only an aggregate overlap count, but remove_hit() still needs boundary-specific information to decide which bins stay USHRT_MAX. In get_cluster(), any overlap now marks all phi-edge hits and never marks time-edge hits. That breaks the overlap handoff deterministically: a cluster that only overlaps in time gets fully zeroed before the next seed is grown, while a phi-only overlap leaves extra USHRT_MAX stripes on rows that never touched the neighbor.
Also applies to: 210-315, 317-425, 574-580
| char tsize = tbinhi - tbinlo + 1; | ||
| char phisize = phibinhi - phibinlo + 1; | ||
| char rsize = size; | ||
| // std::cout << "phisize: " << (int) phisize << " phibinhi " << phibinhi << " phibinlo " << phibinlo << std::endl; |
There was a problem hiding this comment.
RSize can wrap for large clusters.
size is the total number of contributing bins, but it is narrowed to char before being stored. Any cluster above 127 cells will write a wrapped value into the new RSize field, and the stack context shows downstream code now consumes getRSize() for output. Please widen this storage path, or at minimum clamp explicitly before persisting it.
Also applies to: 926-928
| auto *rcs = recoConsts::instance(); | ||
| m_runnumber = rcs->get_IntFlag("RUNNUMBER"); | ||
| m_segment = rcs->get_IntFlag("RUNSEGMENT"); |
There was a problem hiding this comment.
Do not overwrite configured run/segment metadata on every event.
These lines unconditionally replace m_runnumber and m_segment from recoConsts, which makes the existing configuration API (runnumber(), segment(), setSegment()) ineffective and can silently change the values written to every tree. Please only fall back to recoConsts when no explicit value was configured, or guard on flag existence before overwriting.
| m_cluscenadc = cluster->getCenAdc(); | ||
| m_padcen = cluster->getPadCen(); | ||
| m_tbincen = cluster->getTBinCen(); | ||
| m_padmax = cluster->getPadMax(); | ||
| m_tbinmax = cluster->getTBinMax(); | ||
| m_scluslx = cluster->getLocalX(); | ||
| m_scluslz = cluster->getLocalY(); | ||
| m_phibinlo = cluster->getPhiBinLo(); | ||
| m_phibinhi = cluster->getPhiBinHi(); | ||
| m_tbinlo = cluster->getTBinLo(); | ||
| m_tbinhi = cluster->getTBinHi(); | ||
| m_padphase = cluster->getPadPhase(); | ||
| m_tbinphase = cluster->getTBinPhase(); |
There was a problem hiding this comment.
Guard the new pad/tbin/edge/mix fields to TPC clusters.
fillClusterTree, fillClusterBranchesKF, and fillClusterBranchesSeeds all run over MVTX/INTT/Micromegas clusters as well, but the newly added pad/tbin, phase/bin, and edge/mix quantities are TPC-cluster semantics. Writing them unconditionally will serialize backend/default values for non-TPC clusters into clustertree and residualtree, which makes those branches look like real measurements for detectors that do not have pad/tbin or TPC edge categories. Please gate these writes on TrkrDefs::tpcId or assign explicit N/A sentinels for the other detector types.
Also applies to: 724-737, 1158-1170, 1243-1257, 1511-1522, 1545-1559
| std::string str_event = {"event:seed:run:seg:job"}; | ||
| std::string str_hit = {"hitID:e:adc:layer:phielem:zelem:cellID:ecell:phibin:zbin:tbin:phi:r:x:y:z"}; | ||
| std::string str_cluster = {"locx:locy:x:y:z:r:phi:eta:theta:phibin:tbin:fee:chan:sampa:ex:ey:ez:ephi:pez:pephi:e:adc:maxadc:thick:afac:bfac:dcal:layer:phielem:zelem:size:phisize:zsize:pedge:redge:ovlp:trackID:niter"}; | ||
| std::string str_cluster = {"locx:locy:x:y:z:r:phi:eta:theta:phibin:tbin:fee:chan:sampa:ex:ey:ez:ephi:pez:pephi:e:adc:maxadc:cenadc:padcen:tbincen:padmax:tbinmax:thick:afac:bfac:dcal:layer:phielem:zelem:size:phisize:zsize:pedge:redge:sledge:sredge:tledge:tredge:dledge:dredge:hledge:hredge:slmix:srmix:tlmix:trmix:ovlp:phibinlo:phibinhi:tbinlo:tbinhi:padphase:tbinphase:trackID:niter"}; |
There was a problem hiding this comment.
In-place TNtuple schema expansion can silently break downstream readers
Line 363 changes str_cluster column ordering for both ntp_cluster and ntp_clus_trk without a schema/version boundary. Any consumer decoding by position will silently misread legacy fields after maxadc.
Please add a compatibility plan in this PR (e.g., versioned tuple name/metadata, or explicit migration notes and coordinated downstream updates).
|
Hi, I need help with this. I don't know why it shows so many errors/warnings from other macros? |
Build & test reportReport for commit bece3f0bd1612bda0d655d3066bcd8b5ab53b537:
Automatically generated by sPHENIX Jenkins continuous integration |
1 participant
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
This PR extends the TPC Debugger diagnostic output to capture additional cluster-formation details and event-level metadata. The goal is to enable deeper investigation of cluster properties and track-residual correlations by persisting new cluster reconstruction parameters and event identifiers to ROOT output trees.
Key Changes
Event-Level Tracking:
evt_id(event identifier) fromEventHeadertoTrackResiduals, with fallback to-1when unavailableevt_idbranch added to multiple output TTrees:eventtree,failedfits,vertextree,hittree,clustertree, andresidualtreem_runnumberandm_segmentfromrecoConstsCluster Storage Expansion:
m_clusAdc/m_clusMaxAdcvectors with detailed centroid and boundary tracking:m_clusCenAdc,m_clusPadCen,m_clusTBinCen,m_clusPadMax,m_clusTBinMaxm_clussize) alongside existing phi/z size vectorsm_clussledge,m_clussredge), top/bottom time-bin edges (m_clustledge,m_clustredge), and dead/hot channel edges (dledge,dredge,hledge,hredge)m_clusslmix,m_clussrmix,m_clustlmix,m_clustrmixto identify clusters with signal in neighboring binsCluster Reconstruction Refactoring:
ClusterCountersstruct inTpcClusterizerto systematize per-cluster metric tracking (overlap, sector/time edges, dead/hot edges, mixing flags)check_cluster_touchinghelper to detect adjacent bin adjacencies and set mixing flags dynamicallycalc_cluster_parameterto populate centroid ADC (viaadc_map), size/bin/phase information, and edge-category flags during cluster constructionTrkrClusterv6(from v5), which provides new getter/setter methods for the expanded propertiesProcessSectorDatato carryClusterCountersthrough stepdown retry logic and preserve original ADC snapshots for touching checksNTuple Schema Updates:
TrkrNtuplizercluster schema with new fields: centroid ADC, pad/tbin cent/max, edge/mix metrics, overlap, bin ranges, phase informationgetSize()togetRSize()Potential Risk Areas
IO Format Changes:
evt_idbranch and significantly expanded cluster variable vectors; existing analysis code expecting older schema will require updatesTrkrClusterv5toTrkrClusterv6is a breaking change; older reconstruction code reading v5-format files may failReconstruction Behavior:
check_cluster_touchingand revisedcalc_cluster_parameterlogic may subtly alter which hits are included/excluded in final clusters or marked as boundary hitsPerformance:
Suggestions for Future Improvements
TpcClusterQAto cross-check new cluster parameters against expected distributionsNote: This summary is AI-generated and may contain inaccuracies. Please review the actual diff carefully to confirm all changes and their implications.