Skip to content

CM Distortion dPhi vs R*dPhi #4184

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
osbornjd merged 6 commits into from
Feb 24, 2026
Merged

CM Distortion dPhi vs R*dPhi #4184

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e13a266
Added toggles for storting CM phi distortions as r*dPhi vs dPhi (defa…
bkimelman Feb 18, 2026
e6f71b2
Merge remote-tracking branch 'upstream/master' into laserClusterTuning
bkimelman Feb 18, 2026
5951a0e
Added lamination fitting cc that was missed in previous commit
bkimelman Feb 18, 2026
008a012
clag-tidy fixes
bkimelman Feb 19, 2026
3ec981b
Added toggle to save all lamination fitting histograms to file if des…
bkimelman Feb 19, 2026
0eafbd4
Changed to range-based loop
bkimelman Feb 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 26 additions & 5 deletions offline/packages/tpccalib/TpcCentralMembraneMatching.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2351,7 +2351,14 @@ int TpcCentralMembraneMatching::process_event(PHCompositeNode* topNode)
}
*/
m_dcc_out->m_hDRint[s]->SetBinContent(i, j, gr_dR[s]->Interpolate(phiVal,RVal));
m_dcc_out->m_hDPint[s]->SetBinContent(i, j, RVal*gr_dPhi[s]->Interpolate(phiVal,RVal));
if(!m_phiHist_in_rad)
{
m_dcc_out->m_hDPint[s]->SetBinContent(i, j, RVal*gr_dPhi[s]->Interpolate(phiVal,RVal));
}
else
{
m_dcc_out->m_hDPint[s]->SetBinContent(i, j, gr_dPhi[s]->Interpolate(phiVal,RVal));
}
}
}
}
Expand Down Expand Up @@ -2660,13 +2667,27 @@ int TpcCentralMembraneMatching::End(PHCompositeNode* /*topNode*/)
if(den > 0.0)
{
m_dcc_out_aggregated->m_hDRint[s]->SetBinContent(i, j, num_dR / den);
m_dcc_out_aggregated->m_hDPint[s]->SetBinContent(i, j, RVal*(num_dPhi / den));
if(!m_phiHist_in_rad)
{
m_dcc_out_aggregated->m_hDPint[s]->SetBinContent(i, j, RVal*(num_dPhi / den));
}
else
{
m_dcc_out_aggregated->m_hDPint[s]->SetBinContent(i, j, num_dPhi / den);
}
}
}
else
{
m_dcc_out_aggregated->m_hDRint[s]->SetBinContent(i, j, gr_dR_toInterp[s]->Interpolate(phiVal,RVal));
m_dcc_out_aggregated->m_hDPint[s]->SetBinContent(i, j, RVal*gr_dPhi_toInterp[s]->Interpolate(phiVal,RVal));
if(!m_phiHist_in_rad)
{
m_dcc_out_aggregated->m_hDPint[s]->SetBinContent(i, j, RVal*gr_dPhi_toInterp[s]->Interpolate(phiVal,RVal));
}
else
{
m_dcc_out_aggregated->m_hDPint[s]->SetBinContent(i, j, gr_dPhi_toInterp[s]->Interpolate(phiVal,RVal));
}
}
}
}
Expand Down Expand Up @@ -2873,8 +2894,8 @@ int TpcCentralMembraneMatching::GetNodes(PHCompositeNode* topNode)
std::cout << "TpcCentralMembraneMatching::GetNodes - creating TpcDistortionCorrectionContainer in node " << dcc_out_node_name << std::endl;
m_dcc_out = new TpcDistortionCorrectionContainer;
m_dcc_out->m_dimensions = 2;
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;
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

rg -n "m_interpolate_z" --type cpp -C 4

Repository: 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 -50

Repository: 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 -40

Repository: 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.

auto* node = new PHDataNode<TpcDistortionCorrectionContainer>(m_dcc_out, dcc_out_node_name);
runNode->addNode(node);
}
Expand Down
5 changes: 5 additions & 0 deletions offline/packages/tpccalib/TpcCentralMembraneMatching.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,9 @@ class TpcCentralMembraneMatching : public SubsysReco
m_stripePatternFile = stripePatternFile;
}

void set_phiHistInRad(bool rad){ m_phiHist_in_rad = rad; }


// void set_laminationFile(const std::string& filename)
//{
// m_lamfilename = filename;
Expand Down Expand Up @@ -156,6 +159,8 @@ class TpcCentralMembraneMatching : public SubsysReco
//! tpc distortion correction utility class
TpcDistortionCorrection m_distortionCorrection;

bool m_phiHist_in_rad{true};

//! CMFlashClusterContainer *m_corrected_CMcluster_map{nullptr};
LaserClusterContainer *m_corrected_CMcluster_map{nullptr};
CMFlashDifferenceContainer *m_cm_flash_diffs{nullptr};
Expand Down
55 changes: 36 additions & 19 deletions offline/packages/tpccalib/TpcLaminationFitting.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,22 +412,22 @@ int TpcLaminationFitting::process_event(PHCompositeNode *topNode)
{
for (int l = 0; l < 18; l++)
{
double shift = m_laminationIdeal[l][side];
double shift = m_laminationIdeal[l][side];

double phi2pi = tmp_pos.Phi();
if (side && phi2pi < -0.2)
{
phi2pi += 2 * M_PI;
}
if (!side && phi2pi < M_PI / 18 - 0.2)
{
phi2pi += 2 * M_PI;
}
double phi2pi = tmp_pos.Phi();
if (side && phi2pi < -0.2)
{
phi2pi += 2 * M_PI;
}
if (!side && phi2pi < M_PI / 18 - 0.2)
{
phi2pi += 2 * M_PI;
}

if (phi2pi > shift - 0.2 && phi2pi < shift + 0.2)
{
m_hLamination[l][side]->Fill(tmp_pos.Perp(), phi2pi);
}
if (phi2pi > shift - 0.2 && phi2pi < shift + 0.2)
{
m_hLamination[l][side]->Fill(tmp_pos.Perp(), phi2pi);
}
}
}

Expand Down Expand Up @@ -745,15 +745,19 @@ int TpcLaminationFitting::InterpolatePhiDistortions()
int phiBin = phiDistortionLamination[s]->GetXaxis()->FindBin(phi);
if(m_fieldOff)
{
m_laminationOffset[l][s] = m_fLamination[l][s]->GetParameter(0);
m_laminationOffset[l][s] = m_fLamination[l][s]->GetParameter(0);
m_fLamination[l][s]->SetParameter(1, 0.0);
}
else
{
//m_fLamination[l][s]->SetParameter(3, -1.0*m_laminationOffset[l][s]);
m_fLamination[l][s]->SetParameter(3, 0.0);
}
double phiDistortion = R * m_fLamination[l][s]->Integral(phiDistortionLamination[s]->GetYaxis()->GetBinLowEdge(i), phiDistortionLamination[s]->GetYaxis()->GetBinLowEdge(i + 1)) / (phiDistortionLamination[s]->GetYaxis()->GetBinLowEdge(i + 1) - phiDistortionLamination[s]->GetYaxis()->GetBinLowEdge(i));
double phiDistortion = m_fLamination[l][s]->Integral(phiDistortionLamination[s]->GetYaxis()->GetBinLowEdge(i), phiDistortionLamination[s]->GetYaxis()->GetBinLowEdge(i + 1)) / (phiDistortionLamination[s]->GetYaxis()->GetBinLowEdge(i + 1) - phiDistortionLamination[s]->GetYaxis()->GetBinLowEdge(i));
if(!m_phiHist_in_rad)
{
phiDistortion *= R;
}
Comment on lines +757 to +760
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

fd "TpcDistortionCorrection" --type f

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 228

🏁 Script executed:

rg -n "m_hDPint|hDPint|DPint" --type cpp -C 3 | head -100

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 9481

🏁 Script executed:

rg -n "get_corrected_position" --type cpp -A 20 -B 2 | head -150

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 13348

🏁 Script executed:

rg -n "phiHist_in_rad|phiHistInRad|set_phiHistInRad" --type cpp

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 969

🏁 Script executed:

sed -n '70,120p' offline/packages/tpc/TpcDistortionCorrection.cc

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 1642

🏁 Script executed:

rg -n "m_phi_hist_in_radians|divisor" offline/packages/tpc/TpcDistortionCorrection.cc

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 305

🏁 Script executed:

cat -n offline/packages/tpc/TpcDistortionCorrectionContainer.h | head -60

Repository: 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 -5

Repository: 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:

  1. The default output unit change (R·dPhi → dPhi)
  2. Whether existing calibration files must be regenerated
  3. Impact on downstream reconstruction and analysis

if(m_fieldOff)
{
m_fLamination[l][s]->SetParameter(1, m_laminationIdeal[l][s]);
Expand Down Expand Up @@ -1207,9 +1211,22 @@ int TpcLaminationFitting::End(PHCompositeNode * /*topNode*/)
}
m_laminationTree->Write();

m_hLamination[13][0]->Write();
m_hLamination[13][1]->Write();
m_hLamination[14][1]->Write();
if(m_saveAllLaminationHistograms)
{
for(auto &i : m_hLamination)
{
for(auto &j : i)
{
j->Write();
}
}
}
else
{
m_hLamination[13][0]->Write();
m_hLamination[13][1]->Write();
m_hLamination[14][1]->Write();
}
Comment on lines +1224 to +1229
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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();



outputfile->Close();
Expand Down
7 changes: 7 additions & 0 deletions offline/packages/tpccalib/TpcLaminationFitting.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@ class TpcLaminationFitting : public SubsysReco

void set_ppMode(bool mode){ ppMode = mode; }

void set_phiHistInRad(bool rad){ m_phiHist_in_rad = rad; }
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

rg -n "m_phiHist_in_rad" --type cpp -C 3

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 5757

🏁 Script executed:

fd "TpcLaminationFitting\.cc" --type f

Repository: 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.


void set_saveAllLaminationHistograms(bool save){ m_saveAllLaminationHistograms = save; }

void set_fieldOff(bool fieldOff){ m_fieldOff = fieldOff; }

void set_grid_dimensions(int phibins, int rbins);
Expand Down Expand Up @@ -121,6 +125,9 @@ class TpcLaminationFitting : public SubsysReco
//std::map<int, float> m_run_ZDC_map_pp;
//std::map<int, float> m_run_ZDC_map_auau;

bool m_phiHist_in_rad{true};
bool m_saveAllLaminationHistograms{false};

std::string m_stripePatternFile = "/sphenix/u/bkimelman/CMStripePattern.root";

bool m_fieldOff{false};
Expand Down