sEPD Event Plane Calibration - Generation#4106
Merged
pinkenburg merged 16 commits intosPHENIX-Collaboration:masterfrom Feb 22, 2026
Merged
sEPD Event Plane Calibration - Generation#4106pinkenburg merged 16 commits intosPHENIX-Collaboration:masterfrom
pinkenburg merged 16 commits intosPHENIX-Collaboration:masterfrom
Conversation
This commit introduces the sEPD Q Vector calibration engine and CDB generation components for the sEPD Event Plane calibration pipeline. It establishes a centralized definition system and implements the physics logic for re-centering, flattening, and database payload generation. Summary: - sEPD_TreeGen.h/cc: Extract event-level and tower-level sEPD data into TTrees and QA histograms. - QVecDefs.h: Establishes a shared namespace for harmonics, centrality bins, and standardized naming conventions for histograms. - QVecCalib.h/cc: Orchestrates the three-pass calibration logic (Re-centering, Flattening, and Validation). - QVecCDB.h/cc: Manages the transformation of calibration moments and bad tower maps into standardized CDB payloads. - GenQVecCalib.cc & GenQVecCDB.cc: Executable drivers for the calibration processing and database commitment stages.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughA new sEPD event plane calibration module is introduced with build infrastructure, shared definitions, an event data container, and a three-pass Q-vector calibration pipeline for computing and applying recentering and flattening corrections. Changes
Sequence Diagram(s)sequenceDiagram
participant EVT as Event
participant TREE as sEPD_TreeGen
participant EPDATA as EventPlaneData
participant PASS as QVecCalib Pass
participant QA as QA Histograms
participant CORR as Correction Data
participant OUT as CDB/ROOT Output
EVT->>TREE: process_event()
TREE->>TREE: validate z-vertex & MB
TREE->>TREE: extract sEPD towers
TREE->>EPDATA: populate event data
EPDATA->>EPDATA: store per-channel charges
activate PASS
PASS->>QA: load QA histograms
PASS->>PASS: identify bad channels
alt ComputeRecentering Pass
PASS->>PASS: accumulate Q-vectors per event
PASS->>PASS: compute 1st-order averages
PASS->>OUT: write averages to CDB
else ApplyRecentering Pass
PASS->>CORR: load precomputed averages
PASS->>PASS: apply 1st-order corrections
PASS->>PASS: compute 2nd moments (Qxx, Qyy, Qxy)
PASS->>OUT: write centered Q-vectors
else ApplyFlattening Pass
PASS->>CORR: load centered averages & 2nd moments
PASS->>PASS: calculate flattening matrix per centrality/harmonic
PASS->>PASS: apply 2nd-order corrections
PASS->>OUT: write final corrected Q-vectors
end
deactivate PASS
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 |
Build & test reportReport for commit a284e7e6ed03611202bb5747d14bc9b68a4a6470:
Automatically generated by sPHENIX Jenkins continuous integration |
5 tasks
Refactor the QVecCalib and QVecCDB modules to support a third "Combined NS" calibration slot. This update ensures that the combined North-South event plane is derived from individually recentered sub-detectors and then flattened as a single entity, significantly improving event plane resolution and flatness. Changes to QVecCalib: - Calibration Logic: Implemented a "Best of Both Worlds" approach where North and South vectors are recentered individually to preserve natural multiplicity weighting before being summed to form the NS vector. - Flattening Procedure: Added logic to calculate a unique flattening matrix for the combined NS vector, ensuring its final distribution is circular (⟨Qx2⟩/⟨Qy2⟩=1 and ⟨QxQy⟩=0). - Memory Optimization: Simplified the CorrectionData struct by removing intermediate second-moment storage (avg_Q_xx, avg_Q_yy, avg_Q_xy), utilizing local variables during matrix computation instead. - Histogram Refactoring: Replaced high-memory TH3 histograms with a suite of TH2 histograms (Psi_S, Psi_N, Psi_NS) to track event plane angles against centrality. Changes to QVecCDB: - Database Schema: Expanded the m_correction_data array and the SEPD_EventPlaneCalib payload to include the 3rd calibration slot for the NS detector. - IO Updates: Updated load_correction_data and write_cdb_EventPlane to process and commit the NS-specific 2nd moments (xx, yy, xy) to the Calibration Database (CDB). Technical Notes: - Satisfies the requirement that the NS combination treats the sum of recentered sub-detectors as a distinct third detector for whitening. - Prevents the resolution degradation caused by equal-weighting individually flattened sub-detectors.
Build & test reportReport for commit 9ced97a03a5fb410bab67df8bc8f4a59009ca164:
Automatically generated by sPHENIX Jenkins continuous integration |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces the sEPD Event Plane calibration infrastructure for sPHENIX, implementing a three-pass calibration procedure to correct detector-induced biases in Q-vector reconstruction. The implementation includes data extraction, calibration computation, and CDB payload generation capabilities.
Changes:
- Implements a modular three-pass calibration framework (re-centering, flattening, validation) for sEPD Q-vectors
- Establishes centralized definitions and naming conventions for harmonics, centrality bins, and histogram management
- Provides executable drivers for calibration processing and database commitment with flexible command-line interfaces
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 27 comments.
Show a summary per file
| File | Description |
|---|---|
| configure.ac | Standard autoconf configuration for the calibration library |
| autogen.sh | Build bootstrap script with shell variable quoting issues |
| Makefile.am | Automake build configuration defining library and executables |
| QVecDefs.h | Centralized namespace defining constants, enums, and helper functions |
| sEPD_TreeGen.h | Header for SubsysReco module extracting event and tower data into TTrees |
| sEPD_TreeGen.cc | Implementation of tree generation with event selection and QA histograms |
| QVecCalib.h | Header defining multi-pass calibration orchestration class |
| QVecCalib.cc | Implementation of Q-vector calibration logic with three correction passes |
| QVecCDB.h | Header for CDB payload generation from calibration results |
| QVecCDB.cc | Implementation transforming calibration data into database format |
| GenQVecCalib.cc | Executable driver for running calibration passes |
| GenQVecCDB.cc | Executable driver for generating CDB payloads |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Addresses Major Issues: - Guard cd against failure to prevent silent misbehavior. - std::stoll can throw if events argument is non-numeric; exception is unhandled. - std::stoi can throw if runnumber argument is non-numeric; exception is unhandled. - Guard zero/NaN sigma before z-score calculations. - Validate pass before casting to Pass to avoid silent no-op runs.
Addresses Major Issues: - Type truncation: GetXmin()/GetXmax() return Double_t, stored as int. - Sigma guard placed after Fill — degenerate thresholds when sigma is zero. - Differentiate “already exists” from directory-creation failure. - Guard against null TowerInfo* to avoid crashes. The get_tower_at_channel() method returns.
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Addresses Major Issues: - Potential undefined behavior from unchecked cast; memory leak from ProfileY. - Memory leak from ProfileX calls. - Output file creation not validated. - Add guard to prevent histogram and tree filename collision. - Add error checking for TFile creation before writing.
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Addresses Major Issues: - Missing null check after dynamic_cast<TH1*>.
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Build & test reportReport for commit c09d98c9ad2981c0452b7e446276304f7e26d7cb:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 32892881d48e294245287b22dd09eb0befc05375:
Automatically generated by sPHENIX Jenkins continuous integration |
Comment on lines
+27
to
+31
| void set_sepd_charge(int channel, double chg) {sepd_charge[channel] = chg;} | ||
| double get_sepd_charge(int channel) const {return sepd_charge[channel];} | ||
|
|
||
| void set_sepd_phi(int channel, double phi) {sepd_phi[channel] = phi;} | ||
| double get_sepd_phi(int channel) const {return sepd_phi[channel];} |
Contributor
There was a problem hiding this comment.
Missing bounds validation on channel index—potential undefined behavior.
The per-channel accessors directly index into std::array without validating that channel is within [0, SEPD_CHANNELS). An out-of-bounds index causes undefined behavior (memory corruption on write, garbage/crash on read).
Consider adding bounds checks or at minimum using std::array::at() which throws std::out_of_range for invalid indices:
🛡️ Proposed fix using bounds-checked access
- void set_sepd_charge(int channel, double chg) {sepd_charge[channel] = chg;}
- double get_sepd_charge(int channel) const {return sepd_charge[channel];}
+ void set_sepd_charge(int channel, double chg) {sepd_charge.at(channel) = chg;}
+ double get_sepd_charge(int channel) const {return sepd_charge.at(channel);}
- void set_sepd_phi(int channel, double phi) {sepd_phi[channel] = phi;}
- double get_sepd_phi(int channel) const {return sepd_phi[channel];}
+ void set_sepd_phi(int channel, double phi) {sepd_phi.at(channel) = phi;}
+ double get_sepd_phi(int channel) const {return sepd_phi.at(channel);}Alternatively, if performance is critical and you trust all call sites, an assert(channel >= 0 && channel < SEPD_CHANNELS) gives debug-build protection without release overhead.
📝 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
| void set_sepd_charge(int channel, double chg) {sepd_charge[channel] = chg;} | |
| double get_sepd_charge(int channel) const {return sepd_charge[channel];} | |
| void set_sepd_phi(int channel, double phi) {sepd_phi[channel] = phi;} | |
| double get_sepd_phi(int channel) const {return sepd_phi[channel];} | |
| void set_sepd_charge(int channel, double chg) {sepd_charge.at(channel) = chg;} | |
| double get_sepd_charge(int channel) const {return sepd_charge.at(channel);} | |
| void set_sepd_phi(int channel, double phi) {sepd_phi.at(channel) = phi;} | |
| double get_sepd_phi(int channel) const {return sepd_phi.at(channel);} |
Comment on lines
+311
to
+317
| TFile output(m_outfile_name.c_str(), "recreate"); | ||
| output.cd(); | ||
|
|
||
| hSEPD_Charge->Write(); | ||
| h2SEPD_totalcharge_centrality->Write(); | ||
|
|
||
| output.Close(); |
Contributor
There was a problem hiding this comment.
Check histogram output file creation for failure
TFile in "recreate" can fail (permissions, disk full, bad path). Without an IsOpen()/IsZombie() check, histogram output is silently lost.
Proposed fix
TFile output(m_outfile_name.c_str(), "recreate");
+ if (output.IsZombie() || !output.IsOpen())
+ {
+ std::cout << PHWHERE << "Failed to open histogram output file: " << m_outfile_name << std::endl;
+ return Fun4AllReturnCodes::ABORTRUN;
+ }
output.cd();
hSEPD_Charge->Write();
h2SEPD_totalcharge_centrality->Write();
Build & test reportReport for commit f683f68bb17547a627e002e0a380707a17e82ac5:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 90f4f0103389d1b0b1931f3846bc61c98b593b8c:
Automatically generated by sPHENIX Jenkins continuous integration |
- Remove saving to regular TTrees, instead only save to DST - No need to save sepd channel phi as it can be reconstructed from the EpdGeom node that's saved to the DST by default - Remove usage of std::format - Remove usage of unique_ptr - Add Print Method for verbosity and debug - Ensure EventPlaneData has proper Reset Method to refresh buffer between events
- Transformed QVecCalib into a Fun4All module - Reads from slim DST instead of TTree - Merged Functionality of QVecCDB into QVecCalib - Remove QVecCDB and it's corresponding executable - Removed usage of unique_ptr - Using Fun4All server HistoManager to keep track of the histograms
- Prevent lots of output by increasing the progress report counter
Build & test reportReport for commit ddacfb7b8db24faa079fd7bd0f5051937a4588df:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 6e237318746a2dbfa20c83483c1184feaf705886:
Automatically generated by sPHENIX Jenkins continuous integration |
- removed GenQVecCalib.cc - Address readability-avoid-nested-conditional-operator
Build & test reportReport for commit 4aacac3f41e2980b17ff0b1385d8113d0beaed86:
Automatically generated by sPHENIX Jenkins continuous integration |
- Use 1% centrality bins instead of 10% - Allows to capture the finer variations of calibraitons that change on the order of 1% centrality
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Build & test reportReport for commit c700d69e3c66f9874bccded3051adbeb6ef1e162:
Automatically generated by sPHENIX Jenkins continuous integration |
Comment on lines
+828
to
+832
| void QVecCalib::process_recentering(double cent, size_t h_idx, const QVecShared::QVec& q_S, const QVecShared::QVec& q_N, const RecenterHists& h) | ||
| { | ||
| int cent_bin = hCentrality->FindBin(cent) - 1; | ||
|
|
||
| const auto& S = m_correction_data[cent_bin][h_idx][(size_t) QVecShared::Subdetector::S]; |
Contributor
There was a problem hiding this comment.
Out-of-bounds array access when cent falls outside histogram range.
hCentrality->FindBin(cent) returns 0 for underflow and nbins+1 for overflow. After subtracting 1, cent_bin becomes −1 or m_cent_bins — both out of bounds for m_correction_data[cent_bin][…], causing undefined behavior. The same pattern appears in process_flattening at line 874.
process_event_check() validates charge thresholds but does not enforce that cent maps to a valid bin index.
Suggested fix (apply in both methods)
int cent_bin = hCentrality->FindBin(cent) - 1;
+
+ if (cent_bin < 0 || cent_bin >= static_cast<int>(m_cent_bins))
+ {
+ return; // out of calibration range, skip
+ }Also applies to: 872-878
Build & test reportReport for commit 7dcddfcdc133399bbaf7f8a0ca7b849a8cf7b488:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 7d79c3af890e7d39e914b0dc79af8c81fc163450:
Automatically generated by sPHENIX Jenkins continuous integration |
Contributor
|
clang-tidy warnings not from this PR |
pinkenburg
merged commit 0fbb5c6
into
sPHENIX-Collaboration:master
21 of 22 checks passed
5 tasks
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
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.
Types of changes
What kind of change does this PR introduce? (Bug fix, feature, ...)
This PR introduces the sEPD Q Vector calibration engine and CDB generation components for the sEPD Event Plane calibration pipeline. It establishes a centralized definition system and implements the physics logic for re-centering, flattening, and database payload generation.
Summary:
TODOs (if applicable)
Links to other PRs in macros and calibration repositories (if applicable)
sPHENIX-Collaboration/macros#1270
Motivation
This PR establishes the sEPD Q-vector calibration engine and CDB generation components for the Event Plane calibration pipeline. The work implements a complete three-pass calibration workflow (Re-centering, Flattening, Validation) to correct for systematic effects in Q-vector measurements from the sEPD detector.
Key Changes
Core Calibration Infrastructure:
QVecCalib(module + implementation): Implements three-pass calibration logic as a Fun4All SubsysReco module, reads from slim DST, merges QVecCDB functionality, and writes calibration results to both ROOT and CDB formatssEPD_TreeGen(module + implementation): Extracts event-level and tower-level sEPD data into DST/TTrees with QA histogram generation, validates events against vertex/minimum-bias criteria, and produces per-channel charge recordingQVecDefs.h: Centralized definitions for calibration constants (HARMONICS = {2, 3, 4}, CENT_BINS = 80 at 1% granularity, SEPD_CHANNELS = 744) and utility naming functionsEventPlaneData: New PHObject-derived data structure (header + implementation + ROOT dictionary) for storing calibrated event-plane information with per-channel sEPD charge arraysBuild Configuration:
sepd_eventplanecalibsubsystem with dependencies on phool, SubsysReco, centrality_io, fun4all, ffamodules, globalvertex_io, calomodules, cdbobjects, and epd_ioCode Quality Improvements:
std::formatandunique_ptrusage for broader compiler compatibilityPotential Risk Areas
I/O Format Changes: Introduction of new CDB payload structures and EventPlaneData PHObject could impact downstream reconstruction if legacy code expects different object formats. Verify compatibility with existing reconstruction chains.
Centrality Binning: Transition from coarser to 1% bins increases memory/storage requirements for correction matrices and histograms. Monitor performance impact with high-occupancy datasets.
Three-Pass Workflow: The calibration pipeline has state dependencies between passes. Ensure correct pass sequencing (ComputeRecentering → ApplyRecentering → ApplyFlattening) to avoid incorrect calibration factors.
File I/O and DST Dependencies: Code reads from slim DST and QA histogram files; missing or malformed input files will fail gracefully but should be validated in large-scale production runs.
ROOT Dictionary Generation: Addition of EventPlaneDataLinkDef.h requires proper rootcint invocation during build. Check that the dictionary is correctly embedded in the shared library.
Possible Future Improvements
Note: This summary is AI-generated and may contain inaccuracies. Please use your best judgment when reviewing technical details, especially regarding the three-pass calibration interdependencies and data format compatibility with existing code paths.