sEPD Event Plane Calibration - Application (Cleanup)#4200
Merged
pinkenburg merged 11 commits intosPHENIX-Collaboration:masterfrom Mar 10, 2026
Merged
sEPD Event Plane Calibration - Application (Cleanup)#4200pinkenburg merged 11 commits intosPHENIX-Collaboration:masterfrom
pinkenburg merged 11 commits intosPHENIX-Collaboration:masterfrom
Conversation
- Ensure that calibrations are performed on 1% centrality binning (previously 10%) - Use isHot flag is used over the isGood for the sEPD Bad Channels - sEPD Channels don't use the isBadChi2 check that's part of the isGood check (only the isHot which covers the basic dead/hot/cold cases)
- Use consistent naming convention as that for QVecCalib module in the `sepd_eventplanecalib` package
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:
📝 WalkthroughWalkthroughThis PR removes two legacy event plane reconstruction subsystems (EventPlaneCalibration and EventPlaneRecov2), consolidates their functionality into a restructured EventPlaneReco implementation, and updates data container interfaces throughout the eventplaneinfo package to use const references for parameter passing and apply the Rule of Five for copy/move semantics. Changes
Possibly related PRs
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
- Remove EventPlaneCalibration (old approach)
Build & test reportReport for commit f00f62ae243e3e6557479222560893767caf5d9f:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit afb23ff00a611f9333353ddf0a0780a25fa994eb:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit c219232c526ca4d1fc38aaaa335d80d27dbfdbf7:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit adbdacfb4cf21c6c3ce81cc154bc827d68466380:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit f555f0a4d2fc8d2b93f73e4e8bccb513eb4491c2:
Automatically generated by sPHENIX Jenkins continuous integration |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR consolidates the sEPD event plane calibration application from two separate modules (EventPlaneCalibration + EventPlaneRecov2) into a single updated EventPlaneReco module. It adjusts the calibration from a 10% centrality basis to a 1% basis (80 bins covering 0–80%), and removes now-obsolete classes (EventPlaneCalibration, EventPlaneRecov2, Eventplaneinfov2).
Changes:
- Remove
EventPlaneCalibration,EventPlaneRecov2, andEventplaneinfov2classes; port their logic intoEventPlaneReco, which becomes the single calibration+reconstruction module - Upgrade
Eventplaneinfov1to absorb functionality fromEventplaneinfov2(addingmQvec_raw,mQvec_recenteredmembers, safer bounds-checked accessors, and full Rule-of-Five declarations); add a trig cache inEventPlaneRecofor performance - Modernize base class housekeeping (
Eventplaneinfo,EventplaneinfoMap,EventplaneinfoMapv1): use= defaultdestructors,usingaliases, and explicit Rule-of-Five
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
offline/packages/eventplaneinfo/Makefile.am |
Removes deleted files from build, keeps only EventPlaneReco |
offline/packages/eventplaneinfo/EventPlaneCalibration.h/.cc |
Deleted (old calibration class) |
offline/packages/eventplaneinfo/EventPlaneRecov2.h/.cc |
Deleted (old reco+calibration class) |
offline/packages/eventplaneinfo/Eventplaneinfov2.h/.cc/.LinkDef.h |
Deleted (old info class) |
offline/packages/eventplaneinfo/EventPlaneReco.h/.cc |
Major rewrite: now handles centrality-aware Q-vector calibration with 1% bins and trig cache |
offline/packages/eventplaneinfo/Eventplaneinfov1.h/.cc |
Extended with raw/recentered Q-vector storage, safe accessors, and Rule-of-Five |
offline/packages/eventplaneinfo/Eventplaneinfo.h |
Modernized: = default dtor, const& parameters, Rule-of-Five |
offline/packages/eventplaneinfo/EventplaneinfoMap.h |
Modernized: using aliases, = default dtor, Rule-of-Five |
offline/packages/eventplaneinfo/EventplaneinfoMapv1.h |
Added explicit deleted copy/move (Rule-of-Five) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| bool m_doNotCalibEvent{false}; | ||
|
|
||
| double m_cent{0.0}; | ||
| double m_globalEvent{0}; |
There was a problem hiding this comment.
The m_globalEvent member is declared as double but used to hold the event sequence number (an integer). The get_EvtSequence() return type is typically an integer type. Using double for this counter may lose precision for large event sequence numbers. It should be declared as an appropriate integer type (e.g., int64_t or uint64_t).
![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: 4
🧹 Nitpick comments (1)
offline/packages/eventplaneinfo/EventPlaneReco.cc (1)
179-214: Add explicit reprocessing/migration guidance for this output change.These lines materially change calibration application and persisted event-plane payloads. Please state whether existing productions must be reprocessed and what analysis-level shifts are expected to prevent mixing incompatible outputs.
Based on learnings: If the PR changes reconstruction outputs, calibration constants, or simulation behavior, ensure the description states expected analysis impact and whether reprocessing is required.
Also applies to: 527-603
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
offline/packages/eventplaneinfo/EventPlaneCalibration.ccoffline/packages/eventplaneinfo/EventPlaneCalibration.hoffline/packages/eventplaneinfo/EventPlaneReco.ccoffline/packages/eventplaneinfo/EventPlaneReco.hoffline/packages/eventplaneinfo/EventPlaneRecov2.ccoffline/packages/eventplaneinfo/EventPlaneRecov2.hoffline/packages/eventplaneinfo/Eventplaneinfo.hoffline/packages/eventplaneinfo/EventplaneinfoMap.hoffline/packages/eventplaneinfo/EventplaneinfoMapv1.hoffline/packages/eventplaneinfo/Eventplaneinfov1.ccoffline/packages/eventplaneinfo/Eventplaneinfov1.hoffline/packages/eventplaneinfo/Eventplaneinfov2.ccoffline/packages/eventplaneinfo/Eventplaneinfov2.hoffline/packages/eventplaneinfo/Eventplaneinfov2LinkDef.hoffline/packages/eventplaneinfo/Makefile.am
💤 Files with no reviewable changes (7)
- offline/packages/eventplaneinfo/Eventplaneinfov2.cc
- offline/packages/eventplaneinfo/EventPlaneRecov2.cc
- offline/packages/eventplaneinfo/Eventplaneinfov2LinkDef.h
- offline/packages/eventplaneinfo/EventPlaneCalibration.cc
- offline/packages/eventplaneinfo/Eventplaneinfov2.h
- offline/packages/eventplaneinfo/EventPlaneCalibration.h
- offline/packages/eventplaneinfo/EventPlaneRecov2.h
| std::string m_calibName{"SEPD_EventPlaneCalib"}; | ||
| std::string m_inputNode{"TOWERINFO_CALIB_SEPD"}; | ||
|
|
||
| CDBTTree *m_cdbttree {nullptr}; |
Contributor
There was a problem hiding this comment.
Use RAII for m_cdbttree ownership.
Line 97 introduces an owning raw pointer, and its lifetime is manually managed in the .cc. This is brittle for memory safety; please switch to std::unique_ptr<CDBTTree>.
Proposed fix
--- a/offline/packages/eventplaneinfo/EventPlaneReco.h
+++ b/offline/packages/eventplaneinfo/EventPlaneReco.h
@@
- CDBTTree *m_cdbttree {nullptr};
+ std::unique_ptr<CDBTTree> m_cdbttree;--- a/offline/packages/eventplaneinfo/EventPlaneReco.cc
+++ b/offline/packages/eventplaneinfo/EventPlaneReco.cc
@@
- m_cdbttree = new CDBTTree(m_directURL_EventPlaneCalib);
+ m_cdbttree = std::make_unique<CDBTTree>(m_directURL_EventPlaneCalib);
@@
- m_cdbttree = new CDBTTree(calibdir);
+ m_cdbttree = std::make_unique<CDBTTree>(calibdir);
@@
- delete m_cdbttree;
- m_cdbttree = nullptr;
+ m_cdbttree.reset();
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
offline/packages/eventplaneinfo/EventPlaneReco.ccoffline/packages/eventplaneinfo/EventPlaneReco.hoffline/packages/eventplaneinfo/EventplaneinfoMapv1.ccoffline/packages/eventplaneinfo/Eventplaneinfov1.h
💤 Files with no reviewable changes (1)
- offline/packages/eventplaneinfo/EventplaneinfoMapv1.cc
Comment on lines
75
to
79
| std::vector<std::pair<double, double>> mQvec_raw; | ||
| std::vector<std::pair<double, double>> mQvec_recentered; | ||
| std::vector<double> mPsi_Shifted; | ||
| std::vector<std::vector<std::pair<double, double>>> ring_Qvec; | ||
| ClassDefOverride(Eventplaneinfov1, 1); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the header file
hdr="$(fd Eventplaneinfov1.h | head -n1)"
if [[ -z "$hdr" ]]; then
echo "ERROR: Eventplaneinfov1.h not found"
exit 1
fi
echo "=== Current state of: $hdr ==="
cat -n "$hdr" | sed -n '70,85p'
echo
echo "=== Checking git history for these members ==="
git log --oneline "$hdr" | head -10
echo
echo "=== Checking if members existed in previous commit ==="
git show HEAD~1:"$hdr" 2>/dev/null | grep -n -E "mQvec_raw|mQvec_recentered|ClassDefOverride" || echo "(Members not found in previous version)"
echo
echo "=== Checking current commit for these members ==="
git show HEAD:"$hdr" 2>/dev/null | grep -n -E "mQvec_raw|mQvec_recentered|ClassDefOverride" || grep -n -E "mQvec_raw|mQvec_recentered|ClassDefOverride" "$hdr"Repository: sPHENIX-Collaboration/coresoftware
Length of output: 1448
🏁 Script executed:
#!/bin/bash
# Check if this file was modified in the current commit
echo "=== Checking file status in current change ==="
git diff --name-only HEAD | grep -i eventplaneinfo || echo "File not in current staged changes"
echo
echo "=== Looking for all versions of ClassDefOverride in history ==="
git log -p --all -- "*Eventplaneinfov1.h" 2>/dev/null | grep -B2 -A2 "ClassDefOverride" | head -40 || echo "No history found"Repository: sPHENIX-Collaboration/coresoftware
Length of output: 224
Bump ROOT class version after adding persistent members.
mQvec_raw and mQvec_recentered are new data members introduced in this commit, but the ROOT class version remains 1 at line 79. ROOT requires the version to be incremented whenever persistent members are added, otherwise existing serialized objects will fail to deserialize correctly with the new schema.
Proposed fix
- ClassDefOverride(Eventplaneinfov1, 1);
+ ClassDefOverride(Eventplaneinfov1, 2);📝 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
| std::vector<std::pair<double, double>> mQvec_raw; | |
| std::vector<std::pair<double, double>> mQvec_recentered; | |
| std::vector<double> mPsi_Shifted; | |
| std::vector<std::vector<std::pair<double, double>>> ring_Qvec; | |
| ClassDefOverride(Eventplaneinfov1, 1); | |
| std::vector<std::pair<double, double>> mQvec_raw; | |
| std::vector<std::pair<double, double>> mQvec_recentered; | |
| std::vector<double> mPsi_Shifted; | |
| std::vector<std::vector<std::pair<double, double>>> ring_Qvec; | |
| ClassDefOverride(Eventplaneinfov1, 2); |
Build & test reportReport for commit 7a8c072bc56fd4df97e174c0d7a37d7da75607cc:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit ba39831a8890c245034a68a92c14a964740a5bfe:
Automatically generated by sPHENIX Jenkins continuous integration |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
offline/packages/eventplaneinfo/EventPlaneReco.h (1)
102-102:⚠️ Potential issue | 🟠 MajorUse RAII for calibration-tree ownership.
This still uses an owning raw pointer, with manual lifetime management in the
.cc, which is brittle for memory safety and error paths.Proposed fix
--- a/offline/packages/eventplaneinfo/EventPlaneReco.h +++ b/offline/packages/eventplaneinfo/EventPlaneReco.h @@ `#include` <string> `#include` <array> `#include` <vector> +#include <memory> @@ - CDBTTree *m_cdbttree {nullptr}; + std::unique_ptr<CDBTTree> m_cdbttree;--- a/offline/packages/eventplaneinfo/EventPlaneReco.cc +++ b/offline/packages/eventplaneinfo/EventPlaneReco.cc @@ - m_cdbttree = new CDBTTree(m_directURL_EventPlaneCalib); + m_cdbttree = std::make_unique<CDBTTree>(m_directURL_EventPlaneCalib); @@ - m_cdbttree = new CDBTTree(calibdir); + m_cdbttree = std::make_unique<CDBTTree>(calibdir); @@ - delete m_cdbttree; - m_cdbttree = nullptr; + m_cdbttree.reset();As per coding guidelines
**/*.{h,hpp,hxx,hh}: “Focus on API clarity/stability, ownership semantics (RAII), and avoiding raw new/delete.”offline/packages/eventplaneinfo/EventPlaneReco.cc (2)
31-32:⚠️ Potential issue | 🟠 MajorEnsure eventplaneinfo is explicitly built with C++20 (or remove C++20-only APIs).
std::formatandstd::ranges::max_elementare C++20-only. If this package is compiled under an older default standard, this change will fail to build.#!/bin/bash set -euo pipefail echo "== Build-standard settings relevant to eventplaneinfo ==" fd -HI 'configure.ac|Makefile.am|CMakeLists.txt|*.cmake' offline/packages/eventplaneinfo \ | xargs -r rg -n 'CMAKE_CXX_STANDARD|cxx_std_[0-9]+|std=c\+\+' echo echo "== C++20 APIs used in EventPlaneReco ==" rg -n '\bstd::format\s*\(|\bstd::ranges::max_element\s*\(' \ offline/packages/eventplaneinfo/EventPlaneReco.cc offline/packages/eventplaneinfo/EventPlaneReco.hExpected: configuration explicitly enables C++20 for this package; otherwise replace these calls with C++17-compatible alternatives.
Also applies to: 547-547
571-593:⚠️ Potential issue | 🟠 MajorDo not write zeroed recentered Q-vectors when calibration is skipped.
When
m_doNotCaliborm_doNotCalibEventis true, recentering is not performed, butqvector_recenteredis still filled fromm_Q_recentered(reset to zeros). That makes missing recentering look like a physical(0,0)result.Proposed fix
- const auto& Q_S_recentered = m_Q_recentered[h_idx][0]; + const auto& Q_S_recentered = (m_doNotCalib || m_doNotCalibEvent) ? m_Q_raw[h_idx][0] : m_Q_recentered[h_idx][0]; - const auto& Q_N_recentered = m_Q_recentered[h_idx][1]; + const auto& Q_N_recentered = (m_doNotCalib || m_doNotCalibEvent) ? m_Q_raw[h_idx][1] : m_Q_recentered[h_idx][1]; - const auto& Q_NS_recentered = m_Q_recentered[h_idx][2]; + const auto& Q_NS_recentered = (m_doNotCalib || m_doNotCalibEvent) ? m_Q_raw[h_idx][2] : m_Q_recentered[h_idx][2];
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
offline/packages/eventplaneinfo/EventPlaneReco.ccoffline/packages/eventplaneinfo/EventPlaneReco.h
Build & test reportReport for commit 33c91f38936dc97e301a32ced63c06bf2cd9c5c8:
Automatically generated by sPHENIX Jenkins continuous integration |
* Pre-calculate cos(n*phi) and sin(n*phi) values during InitRun * Store cached values in a nested vector indexed by harmonic and channel * Replace per-tower std::cos and std::sin calls in process_sEPD with cache lookups * Significant reduction in CPU overhead per event
- cleanup unused includes and redundant forward declarations
- Peripheral events (≥80% centrality) silently use bin 79 calibration data. - Setting m_doNotCalibEvent = true for out-of-range centrality and storing raw Q-vectors
- Ensure channel count does not exceed trig-cache size - Prevent potential out-of-bounds read
- Keep the default output node name unchanged ("EventplaneinfoMap")
- Allow adjusting of the output node name if needed
- Address the following existing clang-tidy warnings: - [hicpp-special-member-functions] - [hicpp-use-equals-default] - [modernize-use-equals-default] - [performance-unnecessary-value-param] - [modernize-use-using] - [readability-avoid-const-params-in-decls]
Steepspace
force-pushed
the
sEPD-EventPlaneInfo
branch
from
33c91f3 to
8820137
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
offline/packages/eventplaneinfo/EventPlaneReco.h (1)
18-71:⚠️ Potential issue | 🟠 MajorDocument whether this output break requires full reprocessing.
This module now changes the reconstruction path and the event-plane output, but the PR text only calls out the downstream macro update. Please state explicitly whether old productions/DSTs can be mixed with the new 1% calibration output or whether they must be regenerated.
As per coding guidelines: "If interfaces change, ask for compatibility notes and any needed downstream updates"; based on learnings: "If the PR changes reconstruction outputs, calibration constants, or simulation behavior, ensure the description states expected analysis impact and whether reprocessing is required."
offline/packages/eventplaneinfo/EventPlaneReco.cc (1)
566-578:⚠️ Potential issue | 🟠 MajorApply the same fallback to
qvector_recenteredwhen calibration is skipped.
Q_S/Q_N/Q_NSalready fall back tom_Q_rawin the uncalibrated path, butQ_*_recenteredstill come fromm_Q_recentered, whichResetEvent()zero-initializes andcorrect_QVecs()never fills on skipped events. That writes fake recentered(0,0)vectors into DST.Proposed fix
- const auto& Q_S_recentered = m_Q_recentered[h_idx][0]; + const auto& Q_S_recentered = + (m_doNotCalib || m_doNotCalibEvent) ? m_Q_raw[h_idx][0] : m_Q_recentered[h_idx][0]; @@ - const auto& Q_N_recentered = m_Q_recentered[h_idx][1]; + const auto& Q_N_recentered = + (m_doNotCalib || m_doNotCalibEvent) ? m_Q_raw[h_idx][1] : m_Q_recentered[h_idx][1]; @@ - const auto& Q_NS_recentered = m_Q_recentered[h_idx][2]; + const auto& Q_NS_recentered = + (m_doNotCalib || m_doNotCalibEvent) ? m_Q_raw[h_idx][2] : m_Q_recentered[h_idx][2];
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d0a37a2f-b1ae-4b4a-98f9-85dfcdb57f2f
📒 Files selected for processing (8)
offline/packages/eventplaneinfo/EventPlaneReco.ccoffline/packages/eventplaneinfo/EventPlaneReco.hoffline/packages/eventplaneinfo/Eventplaneinfo.hoffline/packages/eventplaneinfo/EventplaneinfoMap.hoffline/packages/eventplaneinfo/EventplaneinfoMapv1.ccoffline/packages/eventplaneinfo/EventplaneinfoMapv1.hoffline/packages/eventplaneinfo/Eventplaneinfov1.hoffline/packages/eventplaneinfo/Eventplaneinfov2.h
💤 Files with no reviewable changes (1)
- offline/packages/eventplaneinfo/EventplaneinfoMapv1.cc
🚧 Files skipped from review as they are similar to previous changes (1)
- offline/packages/eventplaneinfo/Eventplaneinfov2.h
Comment on lines
+382
to
+395
| // ensure both total charges are nonzero | ||
| if (sepd_total_charge_south == 0 || sepd_total_charge_north == 0) | ||
| { | ||
| if (Verbosity() > 1) | ||
| { | ||
| std::cout << PHWHERE << " Error: Total sEPD Charge is Zero: " | ||
| << "South = " << sepd_total_charge_south | ||
| << ", North = " << sepd_total_charge_north << std::endl; | ||
| } | ||
|
|
||
| // ensure raw Q vec is reset | ||
| m_Q_raw = {}; | ||
| m_doNotCalibEvent = true; | ||
| return Fun4AllReturnCodes::EVENT_OK; |
Contributor
There was a problem hiding this comment.
Do not turn zero-charge events into valid-looking raw Q-vectors.
When either arm has zero total charge, this branch zeroes m_Q_raw and returns EVENT_OK. FillNode() later serializes those zeros and derives psi from them, so an undefined event becomes a valid-looking (0,0) measurement, and a still-valid arm is discarded if only one side is empty.
Proposed fix
if (sepd_total_charge_south == 0 || sepd_total_charge_north == 0)
{
@@
- // ensure raw Q vec is reset
- m_Q_raw = {};
+ const double nan = std::numeric_limits<double>::quiet_NaN();
+ for (size_t h_idx = 0; h_idx < m_harmonics.size(); ++h_idx)
+ {
+ if (sepd_total_charge_south > 0)
+ {
+ m_Q_raw[h_idx][0].x /= sepd_total_charge_south;
+ m_Q_raw[h_idx][0].y /= sepd_total_charge_south;
+ }
+ else
+ {
+ m_Q_raw[h_idx][0] = {nan, nan};
+ }
+
+ if (sepd_total_charge_north > 0)
+ {
+ m_Q_raw[h_idx][1].x /= sepd_total_charge_north;
+ m_Q_raw[h_idx][1].y /= sepd_total_charge_north;
+ }
+ else
+ {
+ m_Q_raw[h_idx][1] = {nan, nan};
+ }
+
+ m_Q_raw[h_idx][2] = {nan, nan};
+ }
m_doNotCalibEvent = true;
return Fun4AllReturnCodes::EVENT_OK;
}- Reduce the number of static_casts by changing the type of `order` from int to unsigned int - Improve readability
Build & test reportReport for commit add82a131d1c1ee8e459990658c6928a018cc877:
Automatically generated by sPHENIX Jenkins continuous integration |
pinkenburg
merged commit 523a1b0
into
sPHENIX-Collaboration:master
13 of 22 checks passed
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, ...)
EventPlaneRecomodule with the newEventPlaneRecov2moduleLinks to other PRs in macros and calibration repositories (if applicable)
sPHENIX-Collaboration/macros#1306
sEPD Event Plane Calibration - Application (Cleanup)
Motivation and Context
This PR refactors the event plane calibration infrastructure for the sPHENIX sEPD (small Electromagnetic Pad Detector) by consolidating and modernizing the calibration and reconstruction modules. The primary motivation is to apply event-plane calibration on significantly finer centrality granularity (80 bins, ~1% per bin, instead of 10% bins) and to replace legacy EventPlaneCalibration and EventPlaneRecov2 modules with a unified, production-ready EventPlaneReco module.
Key Changes
Module Consolidation:
EventPlaneCalibrationclass (840 lines) – legacy calibration wrapperEventPlaneRecov2class (637 implementation + 134 header lines) – intermediate development versionEventPlaneRecoas the unified production module with comprehensive per-harmonic, per-centrality calibration support (net +109 lines with substantial architectural enhancements)Calibration System Modernization:
CDBTTreeLoadCalib()with support for direct URL override or default calibration directory fallbackCentralityInfonode for runtime centrality determinationData Processing Pipeline:
TowerInfoContainerdata (744 channels)calculate_flattening_matrix()with per-harmonic/per-centrality correction dataset_sepd_min_channel_charge()) and hot-channel detectionData Structure Improvements:
Eventplaneinfobase class: parameter signatures changed from by-value to const-reference (avoid unnecessary copies); added protected Rule of Five for safe derived-class copy/move semanticsEventplaneinfov1andEventplaneinfov2: now support defaulted copy/move semanticsEventplaneinfoMapv1: explicitly prohibits copying/moving to prevent shallow-copy vulnerabilitiesEventplaneinfoMap: modernized with protected Rule of Five for derived-class safetyAPI and Lifecycle Changes:
Init()andInitRun()for cleaner initialization and geometry setupEnd()with per-eventResetEvent()for explicit state cleanupset_inputNode(),set_directURL_EventPlaneCalib(),set_doAbortNoEventPlaneCalib(),set_sepd_min_channel_charge(),set_EventPlaneInfoNodeName()Build System:
Makefile.amto exportEventPlaneReco.hinstead ofEventPlaneCalibration.handEventPlaneRecov2.hEventplaneinfov2from public exports (now internal only)<iterator>include fromEventplaneinfoMapv1.ccPotential Risk Areas
Reconstruction Behavior Changes:
psi_n) are now computed from 80-bin, per-harmonic calibrated Q-vectors; physics results will differ noticeably from previous versionsCentralityInfocause early returns or abort (controlled byset_doAbortNoEventPlaneCalib()); event-by-event calibration validity strictly enforcedTowerInfoContainernodes result in silent returns; error handling should be verifiedIO Format and Compatibility:
Eventplaneinfosetter signatures changed to const-reference; downstream code passing temporary objects may trigger compiler warnings or require adjustmentCDBTTree) naming convention changed; existing calibration files are incompatible with new code – new calibration files must be generated before production useCorrectionDataandQVecstructures maintain calibration state; output node schema may differDependencies and Integration:
CentralityInfonode availability; events without valid centrality determination are skipped/abortedTowerInfoContainerfor sEPD channel data; detector name must matchset_inputNode()configurationData Member Access:
m_cent,m_globalEvent, correction matrices, Q-vectors); external code relying on direct member access will breakThread Safety:
m_cdbttree,m_correction_data,m_trig_cache) shared across events; concurrent multi-threaded event processing is unsafe without external synchronizationPossible Future Improvements
TTree, histograms) for per-event Q-vector validation and calibration quality monitoringNote: This summary is generated with AI assistance. Human review is strongly recommended to verify all implications, especially regarding: (1) compatibility with existing calibration data and ROOT files, (2) validation of new 80-bin centrality mapping against physics requirements, and (3) integration with downstream analysis code that depends on event plane information.