EventPlaneReco: Update sEPD channel selection and charge handling#4244
Merged
pinkenburg merged 2 commits intosPHENIX-Collaboration:masterfrom Apr 12, 2026
Merged
EventPlaneReco: Update sEPD channel selection and charge handling#4244pinkenburg merged 2 commits intosPHENIX-Collaboration:masterfrom
pinkenburg merged 2 commits intosPHENIX-Collaboration:masterfrom
Conversation
Updated the sEPD event plane reconstruction strategy based on recent QA findings. Major changes include: - Removed isHot channel exclusion: QA determined no channels are consistently hot in sEPD; using all available channels instead. - Implemented charge clamping: Added a configurable threshold (default 50) to cap the maximum charge per channel, mitigating the impact of non-linearities or outliers. - Default Ring 0 exclusion: Added logic to skip the innermost ring (Ring 0) by default to avoid high-intensity beam background noise. - Updated noise floor: Increased default minimum channel charge from 0.2 to 0.5. - Added setters for the new charge threshold and ring-skipping toggle.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughEventPlaneReco updates sEPD processing: ring-indexed skipping of ring 0 (configurable), replaces hot-channel gating with noise suppression (charge <= threshold), and adds optional charge clamping when charge exceeds a configurable threshold; also bumps default per-channel noise threshold. Changes
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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the sEPD contribution to event plane reconstruction in EventPlaneReco based on recent QA, adjusting channel selection and charge preprocessing before building Q-vectors.
Changes:
- Add configurable sEPD charge clamping (default threshold 50) and a toggle to skip Ring 0 by default.
- Remove
isHotchannel exclusion and increase the default sEPD noise floor (min channel charge) to 0.5. - Extend the public interface with setters for the new threshold and Ring 0 skipping behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
offline/packages/eventplaneinfo/EventPlaneReco.h |
Adds new configuration knobs (skip Ring 0, charge clamp threshold) and updates default noise floor. |
offline/packages/eventplaneinfo/EventPlaneReco.cc |
Applies Ring 0 skipping, updated noise cut, and charge clamping in the sEPD per-channel loop before Q-vector accumulation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+68
to
+72
| void set_charge_threshold(double threshold) | ||
| { | ||
| m_sEPD_charge_threshold = threshold; | ||
| } | ||
|
|
There was a problem hiding this comment.
set_charge_threshold is ambiguous in this class (threshold for which detector/quantity?). Since this setter configures sEPD-specific charge clamping, consider renaming it (and the backing member) to something like set_sepd_charge_threshold/m_sepd_charge_threshold to match the existing set_sepd_min_channel_charge naming and avoid confusion if other detectors get thresholds later.
Suggested change
| void set_charge_threshold(double threshold) | |
| { | |
| m_sEPD_charge_threshold = threshold; | |
| } | |
| void set_sepd_charge_threshold(double threshold) | |
| { | |
| m_sEPD_charge_threshold = threshold; | |
| } | |
| void set_charge_threshold(double threshold) | |
| { | |
| set_sepd_charge_threshold(threshold); | |
| } |
![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: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 83b21111-a0b7-42f7-963a-2c0cfed4646b
📒 Files selected for processing (2)
offline/packages/eventplaneinfo/EventPlaneReco.ccoffline/packages/eventplaneinfo/EventPlaneReco.h
Comment on lines
+68
to
+76
| void set_charge_threshold(double threshold) | ||
| { | ||
| m_sEPD_charge_threshold = threshold; | ||
| } | ||
|
|
||
| void set_skipRing0(bool skip) | ||
| { | ||
| m_skipRing0 = skip; | ||
| } |
Contributor
There was a problem hiding this comment.
Couple these new reconstruction knobs to calibration validity.
set_charge_threshold() / set_skipRing0() and the new defaults all change the raw sEPD Q-vector definition, but calibration selection is still independent of those settings. That makes it easy to apply recentering/flattening constants from a different channel/charge model and silently bias the corrected event planes. Please require a configuration-matched calibration payload for non-default settings (or disable calibration when the payload does not match) and spell out the reprocessing scope for existing productions.
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: 104-109
- Ensure during the setting that the threshold is at minimum 0. - Thus, if user does not wish to use a channel threshold then it can be disabled by setting a threshold of zero.
Build & test reportReport for commit 2d515ad605fea4d21699db65b98af8a8f414d943:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit cd1e1bf9d35c73815938b8399bd691bc28a76008:
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
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, ...)
Updated the sEPD event plane reconstruction strategy based on recent QA findings. Major changes include:
TODOs (if applicable)
sEPD Q Vector Calibration Generation: #4245
Links to other PRs in macros and calibration repositories (if applicable)
EventPlaneReco: Update sEPD channel selection and charge handling
Motivation / Context
QA showed no consistently "hot" sEPD channels, so excluding channels by an isHot flag was unnecessary and could remove useful information. At the same time, high-intensity beam background from the innermost ring and occasional large per-channel signals/non-linearities can bias Q-vector sums. This PR adjusts selection and per-channel charge handling to reduce those effects while allowing configurable control.
Key Changes
Potential Risk Areas
Possible Future Improvements
AI-generated summaries can be mistaken—please verify the technical details against the code diffs and tests during review.