mods for final waveform fit pass#4203
Merged
pinkenburg merged 3 commits intosPHENIX-Collaboration:masterfrom Mar 3, 2026
Merged
mods for final waveform fit pass#4203pinkenburg merged 3 commits intosPHENIX-Collaboration:masterfrom
pinkenburg merged 3 commits intosPHENIX-Collaboration:masterfrom
Conversation
…aveforms to check for better alternative fit
Contributor
📝 WalkthroughWalkthroughAdds MBD V2 data types and ROOT dictionaries, migrates reconstruction to use MbdRawContainerV2, extends hit/container APIs for event/clock/fit metadata, reorders calibration loading, propagates event sequence into signals, and significantly refines waveform fitting with chi2/fitinfo recording and tail-based fitting. Changes
Possibly related PRs
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
offline/packages/mbd/Makefile.amoffline/packages/mbd/MbdCalib.ccoffline/packages/mbd/MbdEvent.ccoffline/packages/mbd/MbdRawContainerV2.ccoffline/packages/mbd/MbdRawContainerV2.hoffline/packages/mbd/MbdRawContainerV2LinkDef.hoffline/packages/mbd/MbdRawHit.hoffline/packages/mbd/MbdRawHitV2.ccoffline/packages/mbd/MbdRawHitV2.hoffline/packages/mbd/MbdRawHitV2LinkDef.hoffline/packages/mbd/MbdReco.ccoffline/packages/mbd/MbdReturnCodes.hoffline/packages/mbd/MbdSig.ccoffline/packages/mbd/MbdSig.h
Comment on lines
+53
to
+60
| void set_npmt(const Short_t ival) override | ||
| { | ||
| npmt = ival; | ||
| return; | ||
| } | ||
|
|
||
| /// get Number of Mbd Pmt's | ||
| Short_t get_npmt() const override { return MbdRawHits->GetEntries(); } |
Contributor
There was a problem hiding this comment.
Inconsistent set_npmt/get_npmt behavior — potential API confusion.
set_npmt() assigns to the private member npmt, but get_npmt() returns MbdRawHits->GetEntries(). This means:
set_npmt(128)followed byget_npmt()will not return 128 unless the TClonesArray also happens to have 128 entries.isValid()checksnpmt, but users callingget_npmt()see a different value.
Consider aligning these: either have get_npmt() return the npmt member, or document the semantic difference clearly. This matches V1's pattern, so if intentional, please add a clarifying comment.
Option A: Return the stored member
- Short_t get_npmt() const override { return MbdRawHits->GetEntries(); }
+ Short_t get_npmt() const override { return npmt; }📝 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
| void set_npmt(const Short_t ival) override | |
| { | |
| npmt = ival; | |
| return; | |
| } | |
| /// get Number of Mbd Pmt's | |
| Short_t get_npmt() const override { return MbdRawHits->GetEntries(); } | |
| void set_npmt(const Short_t ival) override | |
| { | |
| npmt = ival; | |
| return; | |
| } | |
| /// get Number of Mbd Pmt's | |
| Short_t get_npmt() const override { return npmt; } |
Comment on lines
+43
to
+68
| virtual Float_t get_chi2ndf() const | ||
| { | ||
| PHOOL_VIRTUAL_WARNING; | ||
| return MbdReturnCodes::MBD_INVALID_FLOAT; | ||
| } | ||
|
|
||
| virtual UShort_t get_fitinfo() const | ||
| { | ||
| PHOOL_VIRTUAL_WARNING; | ||
| return 0; | ||
| } | ||
|
|
||
| virtual void set_pmt(const Short_t /*pmt*/, const Float_t /*adc*/, const Float_t /*ttdc*/, const Float_t /*qtdc*/) | ||
| { | ||
| PHOOL_VIRTUAL_WARNING; | ||
| } | ||
|
|
||
| virtual void set_chi2ndf(const Double_t /*chi2ndf*/) | ||
| { | ||
| PHOOL_VIRTUAL_WARNING; | ||
| } | ||
|
|
||
| virtual void set_fitinfo(const UShort_t /*fitinfo*/) | ||
| { | ||
| PHOOL_VIRTUAL_WARNING; | ||
| } |
Contributor
There was a problem hiding this comment.
Document ABI/downstream impact of the MbdRawHit interface expansion.
Lines 43-68 add new virtual methods on public base class MbdRawHit, which changes the vtable surface. Please include compatibility notes (downstream rebuild expectations and migration impact) in the PR/release notes.
As per coding guidelines: "If interfaces change, ask for compatibility notes and any needed downstream updates."
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
offline/packages/mbd/MbdRawHitV2.h (1)
13-14:⚠️ Potential issue | 🟠 MajorInitialize members in-class to avoid undefined reads before
Clear().Line 13 default-constructs the object, but Line 80–84 members are uninitialized. Then Line 58 and Line 65 do read-modify-write on
fitstat, and Line 75 can readbttdcbefore initialization. This can produce undefined behavior ifClear()is not called first.Proposed fix
private: - Short_t bpmt; - UShort_t fitstat; //waveform fit status - Float_t badc; - Float_t bttdc; - Float_t bqtdc; + Short_t bpmt{-1}; + UShort_t fitstat{0}; // waveform fit status + Float_t badc{std::numeric_limits<Float_t>::quiet_NaN()}; + Float_t bttdc{std::numeric_limits<Float_t>::quiet_NaN()}; + Float_t bqtdc{std::numeric_limits<Float_t>::quiet_NaN()};Also applies to: 58-59, 65-65, 73-76, 80-84
offline/packages/mbd/MbdSig.cc (2)
702-704:⚠️ Potential issue | 🟠 MajorGuard pedestal fit ratio decisions when
NDF <= 0.
ndfis captured at Lines 703–704, but Line 726 still useschi2/ndfunguarded (and Line 717 in verbose mode does the same). This can produce inf/NaN and flip pedestal classification.Proposed fix
double chi2 = ped_fcn->GetChisquare(); double ndf = ped_fcn->GetNDF(); + const double chi2ndf = (ndf > 0.) ? (chi2 / ndf) : std::numeric_limits<double>::infinity(); @@ - double chi2ndf = ped_fcn->GetChisquare()/ped_fcn->GetNDF(); + const double v_ndf = ped_fcn->GetNDF(); + const double chi2ndf = (v_ndf > 0.) ? (ped_fcn->GetChisquare() / v_ndf) : std::numeric_limits<double>::infinity(); @@ - if ( chi2/ndf < 4.0 ) + if ( chi2ndf < 4.0 )
1457-1465:⚠️ Potential issue | 🟠 Major
FitTemplatestill allows invalid chi2/NDF comparisons on zero NDF.
newchi2ndfdefaults to0.whennewndf <= 0(Lines 1457–1461), which can incorrectly treat an invalid fit as better. Also, Line 1527 compares two unguarded ratios that may divide by zero.Proposed fix
- Double_t newchi2ndf = 0.; - if ( newndf>0.) - { - newchi2ndf = newchi2/newndf; - } + Double_t newchi2ndf = std::numeric_limits<double>::infinity(); + if (newndf > 0.) + { + newchi2ndf = newchi2 / newndf; + } @@ - if ( (newchi2/newndf)<f_chi2/f_ndf ) + const Double_t oldchi2ndf = (f_ndf > 0.) ? (f_chi2 / f_ndf) : std::numeric_limits<double>::infinity(); + const Double_t sat_newchi2ndf = (newndf > 0.) ? (newchi2 / newndf) : std::numeric_limits<double>::infinity(); + if (sat_newchi2ndf < oldchi2ndf)Also applies to: 1525-1528
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
offline/packages/mbd/MbdRawContainerV2.hoffline/packages/mbd/MbdRawHitV1.ccoffline/packages/mbd/MbdRawHitV2.ccoffline/packages/mbd/MbdRawHitV2.hoffline/packages/mbd/MbdSig.ccoffline/packages/mbd/MbdSig.h
🚧 Files skipped from review as they are similar to previous changes (2)
- offline/packages/mbd/MbdRawHitV2.cc
- offline/packages/mbd/MbdRawContainerV2.h
Comment on lines
+1248
to
+1254
| // fitmode: | ||
| // 0 - no info or no fit | ||
| // 1 - regular template fit (shortened) | ||
| // 2 - two template fit | ||
| // 3 - two template fit, neg ampl 2nd template | ||
| // 4 - saturated template fit | ||
| // 5 - saturated and shortened template fit |
Contributor
There was a problem hiding this comment.
f_fitmode code 3 semantics are inconsistent with assignment logic.
The fitmode comment says code 3 means “two template fit, neg ampl 2nd template,” but Line 1464’s rejection condition sets f_fitmode = 3 for several unrelated reasons (time2>15, ampl1<0, or newchi2ndf>chi2ndf). Since fitinfo is now exported, this mislabels downstream metadata.
Also applies to: 1464-1472
Build & test reportReport for commit 6e6503f5c5261d3b311e0464287d2133361c2d98:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 979697c730795d74b7f32641729e9562e4d43f57:
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
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.
comment: aborts if calibs don't exist for waveform fits, added info on waveform fit to RawHit output object, added refits to waveforms to check for better alternative fit
Types of changes
What kind of change does this PR introduce? (Bug fix, feature, ...)
TODOs (if applicable)
Links to other PRs in macros and calibration repositories (if applicable)
Motivation and Context
This PR improves MBD (Minimum Bias Detector) waveform reconstruction and data output to support a production waveform-fit pass: it enforces required calibration availability, records detailed fit-quality information with raw hits, and implements more robust/refit-capable waveform fitting to improve signal extraction and allow downstream validation.
Key Changes
Data model / IO
Calibration & workflow control
Event handling & output population
Waveform fitting algorithm and diagnostics
Miscellaneous
Potential Risk Areas
IO format / compatibility (High)
Reconstruction behavior & physics validation (High)
Build / runtime (Medium)
Performance & thread-safety (Medium)
Possible Future Improvements
Note: This summary was generated with AI assistance and may contain errors or omissions; please review the changed files directly (MbdSig*, MbdEvent*, MbdCalib*, MbdRawHit*/ContainerV2*, Makefile.am, LinkDefs) and verify calibration/IO implications before merging.