functional fit option for waveform processing#4131
Merged
pinkenburg merged 5 commits intosPHENIX-Collaboration:masterfrom Jan 21, 2026
Merged
functional fit option for waveform processing#4131pinkenburg merged 5 commits intosPHENIX-Collaboration:masterfrom
pinkenburg merged 5 commits intosPHENIX-Collaboration:masterfrom
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a functional-fit processing path (FUNCFIT): new PowerLawExp and PowerLawDoubleExp signal shapes, a per-channel fitting routine Changes
Sequence Diagram(s)sequenceDiagram
participant Tower as CaloTowerBuilder
participant Proc as CaloWaveformProcessing
participant Fit as CaloWaveformFitting
participant ROOT as ROOT/TF1
Tower->>Proc: initialize_processing(FUNCFIT) + set fit params
Proc->>Fit: set_funcfit_type / set_powerlaw_params / set_doubleexp_params
Tower->>Proc: process_event -> process_waveform()
Proc->>Fit: calo_processing_funcfit(chnlvector)
Fit->>Fit: build per-channel histograms, handle ZS/saturation
Fit->>ROOT: fit histogram (PowerLawExp or PowerLawDoubleExp)
ROOT-->>Fit: fit results (amplitude, time, pedestal, chi2)
Fit-->>Proc: per-channel fit vectors
Proc-->>Tower: processed waveform / tower energies
✨ 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 |
Shuonli
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| std::string m_zs_fieldname{"zs_threshold"}; | ||
|
|
||
| // Functional fit parameters | ||
| int m_funcfit_type{1}; // 0 = PowerLawExp, 1 = PowerLawDoubleExp |
Contributor
There was a problem hiding this comment.
Inconsistent default for m_funcfit_type.
CaloTowerBuilder defaults m_funcfit_type to 1 (POWERLAWDOUBLEEXP), while CaloWaveformFitting and CaloWaveformProcessing default to 0 (POWERLAWEXP). This inconsistency could cause unexpected behavior when users rely on defaults—the fit type used will depend on whether configuration is done through CaloTowerBuilder or directly on the lower-level classes.
Consider aligning all defaults to the same value (likely 0 to match the physics fitting classes).
Suggested fix
// Functional fit parameters
- int m_funcfit_type{1}; // 0 = PowerLawExp, 1 = PowerLawDoubleExp
+ int m_funcfit_type{0}; // 0 = PowerLawExp, 1 = PowerLawDoubleExp📝 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
| int m_funcfit_type{1}; // 0 = PowerLawExp, 1 = PowerLawDoubleExp | |
| int m_funcfit_type{0}; // 0 = PowerLawExp, 1 = PowerLawDoubleExp |
Contributor
There was a problem hiding this comment.
The Rabbit has a point here, the default for the fit type should be identical across classes, otherwise this will confuse everybody. What to use as default is up to you
Contributor
There was a problem hiding this comment.
@pinkenburg Thank you for confirming. @Shuonli, could you please decide which default fit type should be used consistently across CaloTowerBuilder, CaloWaveformFitting, and CaloWaveformProcessing? Currently:
CaloTowerBuilder.h: defaults to1(POWERLAWDOUBLEEXP)CaloWaveformFitting.handCaloWaveformProcessing.h: default to0(POWERLAWEXP)
Once you decide, all three classes should use the same default value to avoid confusion.
🧠 Learnings used
Learnt from: silas-gross
Repo: sPHENIX-Collaboration/coresoftware PR: 4100
File: generators/Herwig/HepMCTrigger/HepMCParticleTrigger.h:93-108
Timestamp: 2026-01-10T07:54:57.021Z
Learning: In HepMCParticleTrigger (generators/Herwig/HepMCTrigger/HepMCParticleTrigger.h), the SetEta* and SetAbsEta* methods intentionally share the same storage variables (_theEtaHigh and _theEtaLow) to emulate the Pythia trigger behavior. This design pattern should be maintained for consistency with existing trigger implementations.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
offline/packages/CaloReco/CaloWaveformFitting.cc (1)
717-729: Guard against out-of-bounds access whennsamples < 7.Line 718 dereferences
v.at(6)and Line 724 usesv.at(1)without checking bounds. This can throw for short waveforms (e.g., 3–6 samples). This is the same issue previously raised for the template-fit path and should be handled here too.✅ Suggested defensive guard
- if ((_bdosoftwarezerosuppression && v.at(6) - v.at(0) < _nsoftwarezerosuppression) || - (_maxsoftwarezerosuppression && maxheight - pedestal < _nsoftwarezerosuppression)) + if ((_bdosoftwarezerosuppression && nsamples > 6 && v.at(6) - v.at(0) < _nsoftwarezerosuppression) || + (_maxsoftwarezerosuppression && maxheight - pedestal < _nsoftwarezerosuppression)) { - amp = v.at(6) - v.at(0); + amp = (nsamples > 6) ? v.at(6) - v.at(0) : maxheight - pedestal; time = std::numeric_limits<float>::quiet_NaN(); ped = v.at(0); - if (v.at(0) != 0 && v.at(1) == 0) + if (nsamples > 1 && v.at(0) != 0 && v.at(1) == 0) { chi2 = 1000000; }
Comment on lines
+770
to
+797
| double risetime = m_powerlaw_power / m_powerlaw_decay; | ||
| double par[5]; | ||
| par[0] = maxheight - pedestal; // Amplitude | ||
| par[1] = maxbin - risetime; // t0 | ||
| if (par[1] < 0) | ||
| { | ||
| par[1] = 0; | ||
| } | ||
| par[2] = m_powerlaw_power; // Power | ||
| par[3] = m_powerlaw_decay; // Decay | ||
| par[4] = pedestal; // Pedestal | ||
|
|
||
| f.SetParameters(par); | ||
| f.SetParLimits(0, (maxheight - pedestal) * 0.5, (maxheight - pedestal) * 10); | ||
| f.SetParLimits(1, 0, nsamples); | ||
| f.SetParLimits(2, 0, 10.0); | ||
| f.SetParLimits(3, 0, 10.0); | ||
| f.SetParLimits(4, pedestal - std::abs(maxheight - pedestal), pedestal + std::abs(maxheight - pedestal)); | ||
|
|
||
| // Perform fit | ||
| h.Fit(&f, "QRN0W", "", 0, nsamples); | ||
|
|
||
| // Calculate peak amplitude and time from fit parameters | ||
| // Peak height is (p0 * Power(p2/p3, p2)) / exp(p2) | ||
| fit_amp = (f.GetParameter(0) * pow(f.GetParameter(2) / f.GetParameter(3), f.GetParameter(2))) / exp(f.GetParameter(2)); | ||
| // Peak time is t0 + power/decay | ||
| fit_time = f.GetParameter(1) + f.GetParameter(2) / f.GetParameter(3); | ||
| fit_ped = f.GetParameter(4); |
Contributor
There was a problem hiding this comment.
Prevent divide-by-zero in POWERLAWEXP risetime and peak calculation.
Line 770 divides by m_powerlaw_decay, and Lines 794–796 divide by the fitted decay. If decay is 0 (or allowed to fit to 0), this yields inf/NaN and contaminates fit outputs. Tighten the lower bound and guard the pre-fit risetime.
✅ Suggested fix
- double risetime = m_powerlaw_power / m_powerlaw_decay;
+ const double decay0 = std::max(m_powerlaw_decay, 1e-6);
+ double risetime = m_powerlaw_power / decay0;
@@
- f.SetParLimits(3, 0, 10.0);
+ f.SetParLimits(3, 1e-6, 10.0);
@@
- fit_amp = (f.GetParameter(0) * pow(f.GetParameter(2) / f.GetParameter(3), f.GetParameter(2))) / exp(f.GetParameter(2));
- fit_time = f.GetParameter(1) + f.GetParameter(2) / f.GetParameter(3);
+ const double decay = f.GetParameter(3);
+ fit_amp = (f.GetParameter(0) * pow(f.GetParameter(2) / decay, f.GetParameter(2))) / exp(f.GetParameter(2));
+ fit_time = f.GetParameter(1) + f.GetParameter(2) / decay;
Build & test reportReport for commit 2f01ab562a041546fab06579c672cbacb2734568:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 5334735d2a18c44a0d1737216fcf1cdb1d36e4fb:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 0d8b5fc4fd281a852503bb9c20df765a061945b6:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 47d0f386183507d42c2bd37a2751c83b354d73b5:
Automatically generated by sPHENIX Jenkins continuous integration |
5 tasks
This was referenced Feb 25, 2026
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
2 participants
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, ...)
added an option to use functional fit(
CaloWaveformProcessing::FUNCFIT) in calotowerbuilder.The functional form is copied from the testbeam processing code.
TODOs (if applicable)
Links to other PRs in macros and calibration repositories (if applicable)
Summary
Motivation / Context
This PR adds a functional-fit option to the calorimeter waveform processing chain to provide an alternative reconstruction path based on the functional forms used in sPHENIX testbeam processing. The new mode enables per-channel parametric fits in CaloWaveformProcessing/CaloWaveformFitting (and wiring in CaloTowerBuilder), which may improve amplitude and timing estimates for pulse shapes not well-modelled by existing TEMPLATE/NYQUIST/ONNX/FAST methods.
Key changes
Potential risk areas
Possible future improvements
Note: This summary was AI-assisted — please verify fit implementation details, default parameter choices, and zero-suppression/saturation handling against the original testbeam code and intended production usage. AI-generated content can contain mistakes; use best judgment when reviewing code and tests.