CM Distortion dPhi vs R*dPhi#4184
CM Distortion dPhi vs R*dPhi#4184bkimelman wants to merge 6 commits intosPHENIX-Collaboration:masterfrom
Conversation
|
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:
📝 WalkthroughWalkthroughAdds configurable flags to treat phi histograms as radians and to control lamination-histogram output; interpolation, DP aggregation, and distortion-container initialization now branch on the phi-units flag to decide whether phi distortions are multiplied by radius. Changes
Possibly related PRs
✨ Finishing Touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
offline/packages/tpccalib/TpcCentralMembraneMatching.cc (1)
2879-2889:⚠️ Potential issue | 🟠 Major
m_dcc_out_aggregatednever inheritsm_phi_hist_in_radiansorm_interpolate_z— this is the container persisted to file.
m_dcc_out(lines 2879–2880) correctly receives both flags when initially created. However,m_dcc_out_aggregatedis unconditionally re-created at line 2889 withreset(new TpcDistortionCorrectionContainer)and never has these flags assigned. The shared initialization loop at lines 2932–2954 only setsm_dimensions = 2.The problem manifests at lines 2664–2665, where histogram fill logic checks
m_phiHist_in_rad(a member variable) to decide scaling:
- If
m_phiHist_in_rad = false: histograms store raw dPhi, code multiplies by R before storage- If
m_phiHist_in_rad = true: histograms store dPhi/RWhen
m_dcc_out_aggregatedis persisted and later loaded fromCMDistortionCorrections.root, downstream consumers readm_dcc_out_aggregated->m_phi_hist_in_radiansto decide correction application. Since this field defaults totrueand is never overwritten, downstream code will misinterpret histograms filled with raw dPhi (whenm_phiHist_in_rad = false), applying corrections incorrectly — a silent physics error.Additionally,
m_interpolate_zdefaults totruebut should matchm_dcc_outand be set tofalse.🐛 Proposed fix
m_dcc_out_aggregated.reset(new TpcDistortionCorrectionContainer); + m_dcc_out_aggregated->m_phi_hist_in_radians = m_phiHist_in_rad; + m_dcc_out_aggregated->m_interpolate_z = false;Alternatively, consolidate both assignments into the shared initialization loop to prevent future omissions:
for (const auto& dcc : {m_dcc_out, m_dcc_out_aggregated.get()}) { dcc->m_dimensions = 2; + dcc->m_phi_hist_in_radians = m_phiHist_in_rad; + dcc->m_interpolate_z = false; // create all histograms … }(Remove the individual assignments at lines 2879–2880.)
| m_dcc_out->m_phi_hist_in_radians = false; | ||
| m_dcc_out->m_interpolate_z = true; | ||
| m_dcc_out->m_phi_hist_in_radians = m_phiHist_in_rad; | ||
| m_dcc_out->m_interpolate_z = false; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "m_interpolate_z" --type cpp -C 4Repository: sPHENIX-Collaboration/coresoftware
Length of output: 6813
🏁 Script executed:
# Look at the context in TpcCentralMembraneMatching.cc around line 2880
sed -n '2860,2890p' offline/packages/tpccalib/TpcCentralMembraneMatching.cc
# Check recent git history for context
git log --oneline -20 offline/packages/tpccalib/TpcCentralMembraneMatching.cc 2>/dev/null || echo "Git history unavailable"
# Search for any comments or documentation about CM and z-interpolation
rg -n "central.{0,30}membran|CM.*distortion" --type cpp -i -A 2 -B 2 | head -50Repository: sPHENIX-Collaboration/coresoftware
Length of output: 5887
🏁 Script executed:
# Check if there are any comments in the code explaining the CM distortion assumption
sed -n '2850,2885p' offline/packages/tpccalib/TpcCentralMembraneMatching.cc | cat -n
# Look for any documentation about why CM produces 2D corrections
rg -n "fluctuation.*2d|2d.*fluct|dimensions.*2" --type cpp offline/packages/tpccalib/TpcCentralMembraneMatching.cc -B 2 -A 2 | head -40Repository: sPHENIX-Collaboration/coresoftware
Length of output: 2458
Confirm the PR description documents this z-interpolation change and its analysis impact.
The m_interpolate_z = false assignment is intentional and physically justified. The code comment at line 2934 explicitly states that CM-derived corrections are 2D because "central membrane flashes only provide distortions at z = 0". Setting m_interpolate_z = false logically follows: since the correction has no z-dependence information, applying z-interpolation to zero at readout would be meaningless.
However, this is still a reconstruction behavior change affecting downstream analyses. Per standard practices, the PR description should document the expected impact on analysis outputs and whether reprocessing of existing data is required.
|
|
||
| void set_ppMode(bool mode){ ppMode = mode; } | ||
|
|
||
| void set_phiHistInRad(bool rad){ m_phiHist_in_rad = rad; } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "m_phiHist_in_rad" --type cpp -C 3Repository: sPHENIX-Collaboration/coresoftware
Length of output: 5757
🏁 Script executed:
fd "TpcLaminationFitting\.cc" --type fRepository: sPHENIX-Collaboration/coresoftware
Length of output: 127
Remove set_phiHistInRad() and the unused member m_phiHist_in_rad from TpcLaminationFitting.
The member declared at line 126 and setter at line 53 are never read in TpcLaminationFitting.cc, making the setter a silent no-op. Users calling set_phiHistInRad() will believe they are configuring the module's phi unit handling, but the flag has no effect. Similar members in TpcCentralMembraneMatching are actively used (e.g., conditional logic at lines 2354, 2664, 2671), so this appears to be dead code that should be removed to prevent user misconfiguration.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
offline/packages/tpccalib/TpcLaminationFitting.cc (1)
756-768:⚠️ Potential issue | 🟡 MinorVerify that PR description documents the change in calibration output format and reprocessing implications.
The code change itself is technically sound: line 757 conditionally scales
phiDistortionby R based on them_phiHist_in_radflag, and bothTpcLaminationFittingandTpcCentralMembraneMatchingconsistently:
- Use the same flag with the same default (
true)- Store histogram values matching the flag setting (dPhi if
true; R·dPhi iffalse)- Set the container metadata
m_phi_hist_in_radiansaccordingly- Properly document the format choice for downstream consumers
The downstream code (
TpcDistortionCorrection.cc) correctly adapts based on this container flag, usingdivisor = 1when the flag istrue(radians) anddivisor = rotherwise (cm), so there is no silent format inconsistency.However, per the learnings context: this PR changes the default calibration output format (from
m_phiHist_in_rad = falsewould imply prior R·dPhi storage, to the new defaulttruestoring bare dPhi). The PR description should explicitly state:
- The new default behavior and what it means for calibration constants
- Whether previously processed data must be reprocessed
- Any impact on analysis workflows expecting the old format
Build & test reportReport for commit e6f71b22a33bb163edd8326a3f78f45691d11e3d:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 5951a0eb5ca60474018159618df5b8ec84041721:
Automatically generated by sPHENIX Jenkins continuous integration |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
offline/packages/tpccalib/TpcCentralMembraneMatching.cc (1)
2897-2907:⚠️ Potential issue | 🔴 Critical
m_dcc_out_aggregatednever receivesm_phi_hist_in_radiansorm_interpolate_zinitialization.
m_dcc_outis correctly configured at lines 2897–2898. Butm_dcc_out_aggregatedis unconditionally re-created at line 2907 and the shared loop at lines 2950–2953 only setsm_dimensions = 2. The missing flags will default tom_phi_hist_in_radians = trueandm_interpolate_z = true.The problem manifests at lines 2670–2690, where histograms are populated into
m_dcc_out_aggregatedusing the localm_phiHist_in_radflag to decide whether to apply R-scaling (multiply byRValif false, skip if true). However, downstream consumers that read this container object will query itsm_phi_hist_in_radiansflag—which will have the wrong default value—and apply opposite scaling, corrupting the corrections.Move both flags into the shared loop to ensure consistency:
🔧 Proposed fix
for (const auto& dcc : {m_dcc_out, m_dcc_out_aggregated.get()}) { // set dimensions to 2, since central membrane flashes only provide distortions at z = 0 dcc->m_dimensions = 2; + dcc->m_phi_hist_in_radians = m_phiHist_in_rad; + dcc->m_interpolate_z = false; // create all histograms for (int i = 0; i < 2; ++i) {
…ired. Will be off by default, but allows for more in-depth QA
| if(!m_phiHist_in_rad) | ||
| { | ||
| phiDistortion *= R; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd "TpcDistortionCorrection" --type fRepository: sPHENIX-Collaboration/coresoftware
Length of output: 228
🏁 Script executed:
rg -n "m_hDPint|hDPint|DPint" --type cpp -C 3 | head -100Repository: sPHENIX-Collaboration/coresoftware
Length of output: 9481
🏁 Script executed:
rg -n "get_corrected_position" --type cpp -A 20 -B 2 | head -150Repository: sPHENIX-Collaboration/coresoftware
Length of output: 13348
🏁 Script executed:
rg -n "phiHist_in_rad|phiHistInRad|set_phiHistInRad" --type cppRepository: sPHENIX-Collaboration/coresoftware
Length of output: 969
🏁 Script executed:
sed -n '70,120p' offline/packages/tpc/TpcDistortionCorrection.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 1642
🏁 Script executed:
rg -n "m_phi_hist_in_radians|divisor" offline/packages/tpc/TpcDistortionCorrection.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 305
🏁 Script executed:
cat -n offline/packages/tpc/TpcDistortionCorrectionContainer.h | head -60Repository: sPHENIX-Collaboration/coresoftware
Length of output: 1738
🏁 Script executed:
rg -n "m_dcc_out\|m_dcc_in" offline/packages/tpccalib/TpcLaminationFitting.cc | grep -E "(set_phiHistInRad|m_phi_hist_in_radians)" | head -5Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
git log --oneline -1 --format="%B" 2>/dev/null | head -20 || echo "PR description not directly accessible in repo context"Repository: sPHENIX-Collaboration/coresoftware
Length of output: 209
PR description must document the default calibration output unit change and reprocessing implications.
The code change itself is internally consistent—the container flag m_phi_hist_in_radians is explicitly set by TpcLaminationFitting and TpcCentralMembraneMatching, and TpcDistortionCorrection::get_corrected_position() reads this flag to apply the correct divisor (1.0 for dPhi [rad], or r for R·dPhi [cm·rad]). However, the PR description states only "Added toggle to save all lamination fitting histograms to file if desired. Will be off by default, but allows for more in-depth QA"—it omits the critical default behavior change.
With m_phiHist_in_rad{true} as the new default, existing calibration files containing R·dPhi (generated with the old default) will be misinterpreted by new code if the container flag is not preserved in saved files. Conversely, new calibrations with dPhi will be wrong if used with legacy analysis expecting R·dPhi.
Before merging, the PR description must explicitly state:
- The default output unit change (R·dPhi → dPhi)
- Whether existing calibration files must be regenerated
- Impact on downstream reconstruction and analysis
| else | ||
| { | ||
| m_hLamination[13][0]->Write(); | ||
| m_hLamination[13][1]->Write(); | ||
| m_hLamination[14][1]->Write(); | ||
| } |
There was a problem hiding this comment.
m_hLamination[14][0] (South side, lamination index 14) is missing from the default-save else branch.
The else path writes both sides of index 13 and only the North side ([14][1]) of index 14. m_hLamination[14][0] (South) is silently dropped. If the intent—consistent with the AI summary—is to save both indices 13 and 14 for both sides, this is an omission.
🐛 Proposed fix
m_hLamination[13][0]->Write();
m_hLamination[13][1]->Write();
+ m_hLamination[14][0]->Write();
m_hLamination[14][1]->Write();
Build & test reportReport for commit 008a0128e539b7b359faee88b9910ccc12d875c0:
Automatically generated by sPHENIX Jenkins continuous integration |
1 participant
Types of changes
What kind of change does this PR introduce? (Bug fix, feature, ...)
Added toggles to swap phi distortion generated by lamination fitting and stripe matching between dPhi and RdPhi. Originally was just RdPhi, but it has been agreed to use dPhi instead. Toggle defaults to storing dPhi, but can be switched using setter function.
TODOs (if applicable)
Links to other PRs in macros and calibration repositories (if applicable)
This PR adds runtime toggles to control whether central membrane (CM) phi distortions are represented/stored as dPhi (angle in radians) or as R*dPhi (radial-scaled angle), and adds an option to save all lamination-fitting histograms. The default is to treat phi histograms as radians (store dPhi).
Motivation / context
Key changes
Potential risk areas
Possible future improvements
Important note