sEPD Event Plane Calibration - Generation#4106
Draft
Steepspace wants to merge 13 commits intosPHENIX-Collaboration:masterfrom
Draft
sEPD Event Plane Calibration - Generation#4106Steepspace wants to merge 13 commits intosPHENIX-Collaboration:masterfrom
Steepspace wants to merge 13 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
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a complete sEPD calibration toolset: tree producer, multi‑pass Q‑vector calibration, CDB payload generator, CLI programs, shared definitions, and Autotools build/bootstrap files. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
participant User as User (CLI)
participant Exec as Exec (GenQVecCalib / GenQVecCDB)
participant Calib as Calib (QVecCalib / QVecCDB)
participant ROOT as ROOT (TFile / TChain)
participant FS as Filesystem (output CDB)
end
User->>Exec: invoke with arguments
Exec->>Calib: construct(input paths, options) & run()
Calib->>ROOT: open TFile / TChain, load histograms/profiles
Calib->>Calib: process events / compute corrections / populate moments
Calib->>FS: write SEPD_EventPlaneCalib and HotMap CDB TTrees (output_dir, cdb_tag)
Exec->>User: exit status / summary
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 |
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.
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 |
Build & test reportReport for commit 89bb30bbe4ce00b5112f95e5534d047dda8cfc01:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 2d1a74a2ea14888af525b3dd549015ca127684e2:
Automatically generated by sPHENIX Jenkins continuous integration |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
90
to
99
| PHNodeIterator node_itr(topNode); | ||
| PHCompositeNode *dstNode = dynamic_cast<PHCompositeNode *>(node_itr.findFirst("PHCompositeNode", "DST")); | ||
|
|
||
| EventPlaneData *evtdata = findNode::getClass<EventPlaneData>(topNode, "EventPlaneData"); | ||
| if (!evtdata) | ||
| { | ||
| evtdata = new EventPlaneData(); | ||
| PHIODataNode<PHObject> *newNode = new PHIODataNode<PHObject>(evtdata, "EventPlaneData", "PHObject"); | ||
| dstNode->addNode(newNode); | ||
| } |
Contributor
There was a problem hiding this comment.
Guard against missing DST node before addNode
If dstNode is null, this dereference will crash the job. Add a null check and abort early with a clear message.
Proposed fix
PHNodeIterator node_itr(topNode);
PHCompositeNode *dstNode = dynamic_cast<PHCompositeNode *>(node_itr.findFirst("PHCompositeNode", "DST"));
+ if (!dstNode)
+ {
+ std::cout << PHWHERE << "DST node missing, cannot attach EventPlaneData." << std::endl;
+ return Fun4AllReturnCodes::ABORTRUN;
+ }
EventPlaneData *evtdata = findNode::getClass<EventPlaneData>(topNode, "EventPlaneData");
if (!evtdata)
{
evtdata = new EventPlaneData();
PHIODataNode<PHObject> *newNode = new PHIODataNode<PHObject>(evtdata, "EventPlaneData", "PHObject");
dstNode->addNode(newNode);
}📝 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
| PHNodeIterator node_itr(topNode); | |
| PHCompositeNode *dstNode = dynamic_cast<PHCompositeNode *>(node_itr.findFirst("PHCompositeNode", "DST")); | |
| EventPlaneData *evtdata = findNode::getClass<EventPlaneData>(topNode, "EventPlaneData"); | |
| if (!evtdata) | |
| { | |
| evtdata = new EventPlaneData(); | |
| PHIODataNode<PHObject> *newNode = new PHIODataNode<PHObject>(evtdata, "EventPlaneData", "PHObject"); | |
| dstNode->addNode(newNode); | |
| } | |
| PHNodeIterator node_itr(topNode); | |
| PHCompositeNode *dstNode = dynamic_cast<PHCompositeNode *>(node_itr.findFirst("PHCompositeNode", "DST")); | |
| if (!dstNode) | |
| { | |
| std::cout << PHWHERE << "DST node missing, cannot attach EventPlaneData." << std::endl; | |
| return Fun4AllReturnCodes::ABORTRUN; | |
| } | |
| EventPlaneData *evtdata = findNode::getClass<EventPlaneData>(topNode, "EventPlaneData"); | |
| if (!evtdata) | |
| { | |
| evtdata = new EventPlaneData(); | |
| PHIODataNode<PHObject> *newNode = new PHIODataNode<PHObject>(evtdata, "EventPlaneData", "PHObject"); | |
| dstNode->addNode(newNode); | |
| } |
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 |
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.
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
sEPD Event Plane Calibration - Generation
Motivation & context
Provide a reproducible pipeline to produce sEPD Q‑vector calibrations for event‑plane reconstruction: extract per‑event and per‑tower sEPD data, run a three‑pass calibration (re‑centering → flattening → validation), and emit standardized CDB payloads usable by reconstruction and downstream calibration tooling.
Key changes
Potential risk areas
Possible future improvements
Note: This summary was produced with AI assistance. AI can make mistakes or miss subtle implementation details — please verify critical numerical logic, I/O schemas, and assumptions against the code and tests.