Optimized running ped length, fixed all waveform contamination from prior crossings.#4178
Conversation
…on to waveform due to events from prior crossings better
📝 WalkthroughWalkthroughAdds parameterized pileup accessors to MbdCalib, introduces per-event pedestal tracking and output in MbdSig, significantly refactors pileup-removal logic to separate time/charge handling, adjusts pedestal-running stats ownership and window, and updates end-of-run outputs and some debug logging. Changes
Sequence Diagram(s)sequenceDiagram
participant EventLoop as EventLoop
participant MbdSig as MbdSig
participant MbdCalib as MbdCalib
participant TemplateFit as TemplateFitter
participant Writer as OutputWriter
EventLoop->>MbdSig: Process event samples
MbdSig->>MbdCalib: query pileup params (get_pileup)
alt time channel
MbdSig->>TemplateFit: perform template fit / compute correction
TemplateFit-->>MbdSig: template/background model
MbdSig->>MbdSig: subtract per-sample background
else charge channel
MbdSig->>TemplateFit: perform charge-specific fit/subtraction
TemplateFit-->>MbdSig: correction
MbdSig->>MbdSig: apply saturation checks & subtract
end
MbdSig->>MbdSig: update running pedestals (ped0stats)
alt pedestal study enabled
MbdSig->>MbdSig: append point to gPedvsEvent
end
EventLoop->>Writer: End of run
Writer->>MbdSig: WritePedvsEvent(), WriteChi2Hist()
Writer-->>EventLoop: files written
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/mbd/MbdSig.cc (1)
309-312:⚠️ Potential issue | 🟠 MajorMissing
_pileup_p0validity check inSetXYpileup path.
SetY()at line 235 guardsRemove_Pileup()withispileup==1 && !std::isnan(_pileup_p0), butSetXY()at line 309 only checksispileup==1. If pileup calibration parameters are not loaded (NaN),Remove_Pileup()will proceed with invalid coefficients, producing garbage corrections on the waveform.Align with SetY guard
- if ( ispileup==1 ) + if ( ispileup==1 && !std::isnan(_pileup_p0) ) { Remove_Pileup(); }
| if ( x_at_max != 0 ) | ||
| { | ||
| gSubPulse->Fit( fit_pileup, "R" ); | ||
| gSubPulse->Draw("ap"); | ||
| PadUpdate(); | ||
| // Fit a pulse in prev crossing | ||
| template_fcn->SetParameters(ymax, x_at_max); | ||
| template_fcn->SetRange(0, x_at_max+2.1); | ||
|
|
||
| if (_verbose == 0) | ||
| { | ||
| //std::cout << PHWHERE << std::endl; | ||
| gSubPulse->Fit(template_fcn, "RNQ"); | ||
| } | ||
| else | ||
| { | ||
| std::cout << "pre-pileup " << _ch << "\t" << x_at_max << "\t" << ymax << std::endl; | ||
| gSubPulse->Fit(template_fcn, "R"); | ||
| gSubPulse->Draw("ap"); | ||
| gSubPulse->GetHistogram()->SetTitle(gSubPulse->GetName()); | ||
| gPad->SetGridy(1); | ||
| PadUpdate(); | ||
| //gSubPulse->Print("ALL"); | ||
| } | ||
|
|
||
| } | ||
| else | ||
| { | ||
| gSubPulse->Fit( fit_pileup, "RNQ" ); | ||
| // Fit the tail | ||
| if ( fit_pileup == nullptr ) | ||
| { | ||
| TString name = "fit_pileup"; name += _ch; | ||
| fit_pileup = new TF1(name,"gaus",-0.1,4.1); | ||
| fit_pileup->SetLineColor(2); | ||
| } | ||
|
|
||
| fit_pileup->SetRange(-0.1,4.1); | ||
| fit_pileup->SetParameters( _pileup_p0*gSubPulse->GetPointY(0), _pileup_p1, _pileup_p2 ); | ||
|
|
||
| // fix par limits | ||
| double plow{0.}; | ||
| double phigh{0.}; | ||
| fit_pileup->GetParLimits(2,plow,phigh); | ||
| if ( phigh < _pileup_p2 ) | ||
| { | ||
| phigh = 2*_pileup_p2; | ||
| fit_pileup->SetParLimits(2,plow,phigh); | ||
| } | ||
|
|
||
| if ( _verbose ) | ||
| { | ||
| gSubPulse->Fit( fit_pileup, "R" ); | ||
| gSubPulse->Draw("ap"); | ||
| PadUpdate(); | ||
| } | ||
| else | ||
| { | ||
| gSubPulse->Fit( fit_pileup, "RNQ" ); | ||
| } | ||
| } | ||
|
|
||
| // subtract pre-pulse | ||
| for (int isamp = 0; isamp < _nsamples; isamp++) | ||
| { | ||
|
|
||
| double bkg = fit_pileup->Eval(isamp); | ||
| double bkg = 0.; | ||
| if ( x_at_max != 0 ) | ||
| { | ||
| bkg = template_fcn->Eval(isamp); | ||
| } | ||
| else | ||
| { | ||
| bkg = fit_pileup->Eval(isamp); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check template_fcn declaration and usage patterns in MbdSig
rg -n 'template_fcn' offline/packages/mbd/MbdSig.cc -B2 -A2Repository: sPHENIX-Collaboration/coresoftware
Length of output: 6869
🏁 Script executed:
# Check SetTemplate implementation to confirm it initializes template_fcn
rg -n 'SetTemplate|template_fcn.*new' offline/packages/mbd/MbdSig.cc -B2 -A5Repository: sPHENIX-Collaboration/coresoftware
Length of output: 1763
🏁 Script executed:
# Examine SetXY and SetY implementations for guards
rg -n 'void MbdSig::Set(XY|Y)\(' offline/packages/mbd/MbdSig.cc -A20 | head -80Repository: sPHENIX-Collaboration/coresoftware
Length of output: 1248
🏁 Script executed:
# Check MbdEvent.cc for SetTemplate() call patterns and guards
rg -n 'SetTemplate|do_templatefit' offline/packages/mbd/MbdEvent.cc -B2 -A2Repository: sPHENIX-Collaboration/coresoftware
Length of output: 1175
🏁 Script executed:
# Find Remove_Pileup() calls and surrounding context
rg -n 'Remove_Pileup' offline/packages/mbd/MbdSig.cc -B5 -A2Repository: sPHENIX-Collaboration/coresoftware
Length of output: 534
🏁 Script executed:
# Check SetY implementation around the mentioned line 235
sed -n '225,245p' offline/packages/mbd/MbdSig.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 588
🏁 Script executed:
# Check SetXY implementation around the mentioned line 309
sed -n '300,320p' offline/packages/mbd/MbdSig.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 658
🏁 Script executed:
# Search for sampmax-6 or y_min6 in Remove_Pileup
rg -n 'y_min6|sampmax.*-.*6' offline/packages/mbd/MbdSig.cc -B2 -A2Repository: sPHENIX-Collaboration/coresoftware
Length of output: 396
🏁 Script executed:
# Get full Remove_Pileup function to check all details
sed -n '325,470p' offline/packages/mbd/MbdSig.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 3776
🏁 Script executed:
# Check what determines ispileup and when it's set
rg -n 'ispileup' offline/packages/mbd/MbdSig.cc -B2 -A2 | head -50Repository: sPHENIX-Collaboration/coresoftware
Length of output: 1132
🏁 Script executed:
# Check MbdEvent for ispileup logic
rg -n 'ispileup' offline/packages/mbd/MbdEvent.cc -B2 -A2Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
# Check what CalcEventPed0_PreSamp returns and when ispileup is set
rg -n 'CalcEventPed0_PreSamp|int.*ispileup' offline/packages/mbd/MbdSig.cc -A3 | head -40Repository: sPHENIX-Collaboration/coresoftware
Length of output: 1559
🏁 Script executed:
# Verify the exact line numbers of Remove_Pileup charge vs time channel logic
sed -n '343,350p' offline/packages/mbd/MbdSig.cc
sed -n '383,395p' offline/packages/mbd/MbdSig.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 578
Add null guard for template_fcn in charge-channel pileup path.
template_fcn is only initialized in SetTemplate(), which is called from MbdEvent.cc only when do_templatefit && type==1 (line 204). In the online path (do_templatefit=0), the pointer remains null. Unlike SetY() at line 235 which guards Remove_Pileup() with !std::isnan(_pileup_p0), SetXY() at line 309 calls Remove_Pileup() with only ispileup==1. This allows charge channels with pileup to reach the code at lines 393, 399, 404, and 455, where template_fcn is dereferenced without a null check. On online data where templates are not loaded, this will segfault.
Add a null guard before using template_fcn in the x_at_max != 0 branch:
if ( x_at_max != 0 )
{
if ( template_fcn == nullptr )
{
return; // No template available; skip pileup correction
}
// Fit a pulse in prev crossing
template_fcn->SetParameters(ymax, x_at_max);
...
}There was a problem hiding this comment.
I don't think the rabbit is right on this one. We use the fit of MBD_SHAPE in all situations.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for all SetTemplate calls to understand when template_fcn is initialized
rg -n 'SetTemplate\(' --type=cc --type=h -C3
Length of output: 26
🏁 Script executed:
#!/bin/bash
# Search for MBD_SHAPE usage to understand template loading
rg -n 'MBD_SHAPE|do_templatefit' offline/packages/mbd/ -C3
Length of output: 5007
🏁 Script executed:
#!/bin/bash
# Check if there are any other places where template_fcn is initialized
rg -n 'template_fcn.*=.*new|template_fcn.*nullptr' offline/packages/mbd/MbdSig.cc -C2
Length of output: 1037
🏁 Script executed:
#!/bin/bash
# Look for the initialization flow in MbdEvent to understand the setup order
rg -n 'Init|SetTemplate|MBD_SHAPE' offline/packages/mbd/MbdEvent.cc -C2
Length of output: 716
@mchiu-bnl Thank you for the clarification. I see that MBD_SHAPES is loaded when do_templatefit==1, and SetTemplate() initializes template_fcn accordingly.
Could you help me understand how template_fcn is guaranteed to be initialized before Remove_Pileup() is called? I'm seeing:
- In online mode,
do_templatefit=0(line 60 of MbdEvent.cc) SetTemplate()is only called whendo_templatefit && type==1(line 204)- The new charge-channel pileup path uses
template_fcnat line 393
Is there a different initialization path for online mode, or does online mode not encounter this particular pileup scenario? I want to make sure I understand the complete picture correctly.
🧬 Code Graph Analysis Results
[No direct code snippet blocks identified]
[If you prefer, I can provide exact verbatim blocks, but none of the provided snippets from the other files appear to directly cover MbdSig.cc's TemplateFcn/SetTemplate/FitTemplate without summarization.]
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/mbd/MbdSig.cc (1)
140-166:⚠️ Potential issue | 🟠 MajorMemory leak:
fit_pileupis never deleted.
fit_pileupis heap-allocated inRemove_Pileup()(line 352 for time channels, line 430 for charge channels) but is not freed in the destructor. Every channel that encounters pileup will leak aTF1.Proposed fix
delete h_chi2ndf; + delete fit_pileup; if ( _pedstudyflag ) { delete gPedvsEvent; } }
Build & test reportReport for commit 70b8bd9b95015b806190b0e29dd06b7e90da6d60:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit a07fca0c6093a63463753a2ebf05e00f9f4cc6de:
Automatically generated by sPHENIX Jenkins continuous integration |
|
clang-tidy warnings are from recent tracking PR, will be fixed in my next PR |
pinkenburg
merged commit 9f22933
into
sPHENIX-Collaboration:master
2 participants
comment: changed running ped to 8 samples, added ped study, handle contamination to waveform due to events from prior crossings better
Types of changes
What kind of change does this PR introduce? (Bug fix, feature, ...)
comment: Did ped study, found that it's optimal to use event-by-event pedestals, and where that is impossible, such as when there is an event from prior crossings contaminating the pedestal, use a short running mean (set to 8 samples, whereas previously it was 100 samples). Also when doing the study I noticed a few cases where the contamination from prior crossings needed to be handled better. Should be all good now.
TODOs (if applicable)
Links to other PRs in macros and calibration repositories (if applicable)
Summary: Optimized MBD Pedestal Calculation and Improved Pileup Contamination Handling
Motivation / context
Reduce pedestal bias and mitigate waveform contamination from prior bunch crossings (pileup) to improve MBD charge and time reconstruction fidelity, informed by a dedicated pedestal study.
Key changes
Potential risk areas
Possible future improvements
Note on AI-generated content
This summary was assembled from the provided diffs and AI-generated summaries; AI can make mistakes or miss subtle control-flow details. Please review the code paths (especially Remove_Pileup(), memory ownership changes, and calibration parameter semantics) directly during code review.