sEPD Event Plane Calibration - Application (Cleanup)#4200
Draft
Steepspace wants to merge 10 commits intosPHENIX-Collaboration:masterfrom
Draft
sEPD Event Plane Calibration - Application (Cleanup)#4200Steepspace wants to merge 10 commits intosPHENIX-Collaboration:masterfrom
Steepspace wants to merge 10 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
|
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:
📝 WalkthroughWalkthroughRemoved legacy EventPlaneCalibration and EventPlaneRecov2 implementations and Eventplaneinfov2; introduced a consolidated EventPlaneReco with per-harmonic/per-centrality calibration loading (CDB or direct URL), sEPD Q‑vector processing, recentering/flattening, and modernized Eventplaneinfo APIs. Changes
Sequence Diagram(s)sequenceDiagram
participant Framework as Framework/<br/>Reconstruction
participant EPReco as EventPlaneReco
participant CDB as CDBTTree<br/>(Calibration DB)
participant Centrality as CentralityInfo<br/>Node
participant sEPD as sEPD<br/>TowerInfo
participant Node as PHCompositeNode<br/>(EventplaneinfoMap)
Framework->>EPReco: Init(topNode)
EPReco->>EPReco: configure parameters
Framework->>EPReco: InitRun(topNode)
EPReco->>CDB: LoadCalib(direct URL or CDB)
CDB-->>EPReco: per-harmonic per-centrality corrections
EPReco->>EPReco: build trig cache
Framework->>EPReco: process_event(topNode)
EPReco->>Centrality: request centrality
Centrality-->>EPReco: centrality value
EPReco->>sEPD: process_sEPD() (read channel charges)
sEPD-->>EPReco: raw Q-vectors
EPReco->>EPReco: recenter and flatten Q-vectors
EPReco->>Node: CreateNodes / FillNode (populate EventplaneinfoMap)
Node-->>Framework: Eventplaneinfo objects available
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 |
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 |
Comment on lines
+571
to
+593
| const auto& Q_S_raw = m_Q_raw[h_idx][0]; | ||
| const auto& Q_S_recentered = m_Q_recentered[h_idx][0]; | ||
|
|
||
| const auto& Q_N_raw = m_Q_raw[h_idx][1]; | ||
| const auto& Q_N_recentered = m_Q_recentered[h_idx][1]; | ||
|
|
||
| const auto& Q_NS_raw = m_Q_raw[h_idx][2]; | ||
| const auto& Q_NS_recentered = m_Q_recentered[h_idx][2]; | ||
|
|
||
| // South | ||
| south_Qvec_raw[idx] = {Q_S_raw.x, Q_S_raw.y}; | ||
| south_Qvec_recentered[idx] = {Q_S_recentered.x, Q_S_recentered.y}; | ||
| south_Qvec[idx] = {Q_S.x, Q_S.y}; | ||
|
|
||
| // North | ||
| north_Qvec_raw[idx] = {Q_N_raw.x, Q_N_raw.y}; | ||
| north_Qvec_recentered[idx] = {Q_N_recentered.x, Q_N_recentered.y}; | ||
| north_Qvec[idx] = {Q_N.x, Q_N.y}; | ||
|
|
||
| // Combined (North + South) | ||
| northsouth_Qvec_raw[idx] = {Q_NS_raw.x, Q_NS_raw.y}; | ||
| northsouth_Qvec_recentered[idx] = {Q_NS_recentered.x, Q_NS_recentered.y}; | ||
| northsouth_Qvec[idx] = {Q_NS.x, Q_NS.y}; |
Contributor
There was a problem hiding this comment.
Recentered Q-vectors incorrectly set to (0,0) when calibration is skipped.
When m_doNotCalibEvent is true (due to invalid centrality, zero charge, or out-of-range centrality), correct_QVecs() is not called, leaving m_Q_recentered zero-initialized from ResetEvent. Lines 572-578 then store these zeros into the output node.
This means:
qvector_raw: ✓ correct raw valuesqvector_recentered: ✗ (0, 0) instead of NaN or raw fallbackqvector(final): ✓ correctly falls back to raw
The (0, 0) values are indistinguishable from physically valid recentered vectors and could mislead downstream analysis.
Proposed fix: Apply fallback logic consistently
- 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];Alternatively, skip writing recentered values entirely when uncalibrated, preserving the NaN initialization.
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
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 / Context
Refactor and consolidate sEPD event-plane calibration and reconstruction to improve calibration granularity, simplify the module surface, and centralize calibration handling in a single modernized reconstruction module. The goals are finer physics corrections (1% centrality bins), clearer lifecycle/API, and safer per-event processing.
Key changes
Potential risk areas
Possible future improvements
Review note / AI caution
This summary was produced with automated assistance. AI-generated summaries can contain mistakes—please inspect EventPlaneReco.{h,cc}, Eventplaneinfov1 changes, and the linked macros PR to confirm I/O and behavioral impacts (notably calibration sourcing, peripheral-event handling, node naming, and initialization error propagation).