Add codegen to RooParametricHist#1217
Conversation
📝 WalkthroughWalkthroughThis pull request extends RooFit's codegen and math infrastructure to support RooParametricHist by introducing centralized math utilities, refactoring the class's evaluation logic, and adding public data accessors while maintaining existing behavior through new utility functions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (41.62%) is below the target coverage (98.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #1217 +/- ##
==========================================
+ Coverage 20.86% 20.96% +0.10%
==========================================
Files 195 195
Lines 26217 26345 +128
Branches 3932 3947 +15
==========================================
+ Hits 5469 5523 +54
- Misses 20748 20822 +74
🚀 New features to boost your workflow:
|
|
@guitargeek that should be the bits from today minus the proxy problems |
runtingt
force-pushed
the
RooParametricHist_AD
branch
from
a24c6aa to
314d6ac
Compare
runtingt
force-pushed
the
RooParametricHist_AD
branch
from
f25ddb4 to
eb35ad1
Compare
|
@anigamova could we get a |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
interface/CombineMathFuncs.h (1)
374-395: Near-duplicate ofparametricHistMorphScalewith different indexing.
parametricMorphFunctionandparametricHistMorphScalecompute the same morph scale but use different array indexing conventions.parametricHistMorphScaletakes pre-offset pointers while this function indexes into the full flattened array. Consider unifying them to avoid divergent maintenance (e.g., the same division-by-zero risk onparValexists here at Line 392).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/CombineMathFuncs.h` around lines 374 - 395, parametricMorphFunction duplicates logic from parametricHistMorphScale (but uses different indexing) and also repeats the division-by-parVal risk; refactor by extracting the common morph-scaling loop into a single helper (e.g., computeMorphScale or similar) that accepts either a base pointer+stride or an index-mapper so both parametricMorphFunction and parametricHistMorphScale call the same implementation, update parametricMorphFunction to pass the appropriate offsets/stride instead of reimplementing the loop, and add a guard against parVal == 0 (or handle small/zero parVal consistently with parametricHistMorphScale) before performing (1./parVal) to eliminate the division-by-zero risk; preserve existing parameters morphDiffs, morphSums, nMorphs, hasMorphs when wiring to the new helper.src/RooParametricHist.cxx (2)
214-230: Stale-cache risk:getFlattenedMorphsonly flattens once, butaddMorphscan mutate_diffs/_sums.The
!diffs_flat.empty()guard at Line 218 means subsequentaddMorphs()calls won't be reflected. If the calling order is always setup-then-evaluate this is fine, but the cache has no invalidation mechanism. Consider clearingdiffs_flat_/sums_flat_inaddMorphs()to avoid silent staleness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/RooParametricHist.cxx` around lines 214 - 230, getFlattenedMorphs currently skips rebuilding the flattened cache when diffs_flat/sums_flat are non-empty, but addMorphs can mutate _diffs/_sums causing a stale cache; update addMorphs (the method that updates _diffs/_sums) to invalidate the flattened caches by clearing any stored diffs_flat_/sums_flat (or setting a boolean dirty flag) so that getFlattenedMorphs (which checks diffs_flat.empty()) will rebuild after morphs are added; reference the methods/fields getFlattenedMorphs, addMorphs, _diffs, _sums, and the cached diffs_flat_/sums_flat in your change.
205-212: Inconsistent cast:RooRealVar*here vs.RooAbsReal*ingetParVals().Line 209 uses
static_cast<RooRealVar*>while the analogousgetParVals()at Line 200 usesstatic_cast<RooAbsReal*>. Since onlygetVal()is needed (defined onRooAbsReal), prefer the wider type to avoid UB if a non-RooRealVaris ever added to_coeffList.Suggested fix
- coeffs_.push_back(static_cast<RooRealVar*>(_coeffList.at(i))->getVal()); + coeffs_.push_back(static_cast<RooAbsReal*>(_coeffList.at(i))->getVal());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/RooParametricHist.cxx` around lines 205 - 212, The getCoeffs() implementation casts list entries to RooRealVar* which is inconsistent with getParVals() and can be UB if _coeffList contains non-RooRealVar objects; change the cast in RooParametricHist::getCoeffs to static_cast<RooAbsReal*> (same type used in getParVals()) and call getVal() on that RooAbsReal pointer when iterating _coeffList to safely retrieve coefficient values from the list.interface/RooParametricHist.h (1)
48-48: Add explanatory comment for publicevaluate()override.
RooAbsPdf::evaluate()is conventionally protected in RooFit, but this implementation appears intentionally public given the supporting public accessors (getX(), getParVal(), getCoeffs(), etc.). A brief comment explaining this design choice would clarify intent for future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/RooParametricHist.h` at line 48, The public override of Double_t evaluate() in class RooParametricHist should have a short explanatory comment clarifying why it is public (because this PDF exposes public accessors like getX(), getParVal(), getCoeffs() and is intended to be evaluated directly by external code) and noting that this deviates from the RooFit convention where RooAbsPdf::evaluate() is protected; add that comment immediately above the public evaluate() declaration in RooParametricHist to document the design intent and avoid future confusion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@interface/CombineMathFuncs.h`:
- Around line 325-341: Add a guard for parVal == 0 in both
parametricHistMorphScale and parametricMorphFunction to avoid division by zero:
in parametricHistMorphScale (function parametricHistMorphScale) check if parVal
== 0 at the start and return the default morphScale (1.0) immediately; in
parametricMorphFunction (function parametricMorphFunction) check if parVal == 0
and return an appropriate zero/default morph value (e.g., 0.0) immediately so no
(1.0/parVal) or other division by parVal occurs later in the function. Ensure
these checks come before any use of parVal in divisions.
- Around line 439-467: The ranged-integration branch is inconsistently
normalized: the "domain fully contained in this bin" path returns sum/fullSum
but the "upper domain boundary is in bin" path and the final fallthrough return
unnormalized sum. Make normalization consistent by computing fullSum via
parametricHistFullSum (same arguments used earlier) and dividing sum by fullSum
in the upper-bound early return (the branch with comment "Upper domain boundary
is in bin") and in the final return after the loop; keep use of parVals, N_bins,
morphCoeffs, nMorphs, morphDiffs, morphSums, smoothRegion and
parametricMorphFunction unchanged.
In `@src/CombineCodegenImpl.cxx`:
- Around line 246-271: The code currently appends a full statement (including
the trailing ";\n") to code.str() and passes that to ctx.addResult, which yields
a statement where an expression is expected; in
RooFit::Experimental::codegenImpl for RooParametricHist replace the pattern that
builds code (code and code.str()) with: generate a unique temporary variable
name, produce an assignment statement via ctx.addToCodeBody that assigns the
result of ctx.buildCall("RooFit::Detail::MathFuncs::parametricHistEvaluate",
...) to that temp (so keep bin_i and the same argument list), then call
ctx.addResult(&arg, tempVarName) passing only the bare variable/expression (no
semicolon). Ensure you still add the bin-finding call to the body (bin_i usage)
and preserve morphs handling (diffs_flat/sums_flat) when forming the buildCall.
In `@src/RooParametricHist.cxx`:
- Around line 163-192: In RooParametricHist::evaluate(), the call to
RooFit::Detail::MathFuncs::parametricHistFindBin(bin_i...) can dereference bins
when it's empty, so move the guard that checks bins.empty() || widths.empty() to
before the parametricHistFindBin call (or assert/ensure bins and widths are
non-empty on construction), resize/prepare pars_vals_ after the empty check, and
only call parametricHistFindBin and subsequent logic when bins and widths are
valid; keep the existing logic for morphs/getFlattenedMorphs/getCoeffs unchanged
but invoked after the empty-vector guard.
---
Nitpick comments:
In `@interface/CombineMathFuncs.h`:
- Around line 374-395: parametricMorphFunction duplicates logic from
parametricHistMorphScale (but uses different indexing) and also repeats the
division-by-parVal risk; refactor by extracting the common morph-scaling loop
into a single helper (e.g., computeMorphScale or similar) that accepts either a
base pointer+stride or an index-mapper so both parametricMorphFunction and
parametricHistMorphScale call the same implementation, update
parametricMorphFunction to pass the appropriate offsets/stride instead of
reimplementing the loop, and add a guard against parVal == 0 (or handle
small/zero parVal consistently with parametricHistMorphScale) before performing
(1./parVal) to eliminate the division-by-zero risk; preserve existing parameters
morphDiffs, morphSums, nMorphs, hasMorphs when wiring to the new helper.
In `@interface/RooParametricHist.h`:
- Line 48: The public override of Double_t evaluate() in class RooParametricHist
should have a short explanatory comment clarifying why it is public (because
this PDF exposes public accessors like getX(), getParVal(), getCoeffs() and is
intended to be evaluated directly by external code) and noting that this
deviates from the RooFit convention where RooAbsPdf::evaluate() is protected;
add that comment immediately above the public evaluate() declaration in
RooParametricHist to document the design intent and avoid future confusion.
In `@src/RooParametricHist.cxx`:
- Around line 214-230: getFlattenedMorphs currently skips rebuilding the
flattened cache when diffs_flat/sums_flat are non-empty, but addMorphs can
mutate _diffs/_sums causing a stale cache; update addMorphs (the method that
updates _diffs/_sums) to invalidate the flattened caches by clearing any stored
diffs_flat_/sums_flat (or setting a boolean dirty flag) so that
getFlattenedMorphs (which checks diffs_flat.empty()) will rebuild after morphs
are added; reference the methods/fields getFlattenedMorphs, addMorphs, _diffs,
_sums, and the cached diffs_flat_/sums_flat in your change.
- Around line 205-212: The getCoeffs() implementation casts list entries to
RooRealVar* which is inconsistent with getParVals() and can be UB if _coeffList
contains non-RooRealVar objects; change the cast in RooParametricHist::getCoeffs
to static_cast<RooAbsReal*> (same type used in getParVals()) and call getVal()
on that RooAbsReal pointer when iterating _coeffList to safely retrieve
coefficient values from the list.
| inline Double_t parametricHistMorphScale(const double parVal, | ||
| const int nMorphs, | ||
| const double* morphCoeffs, | ||
| const double* morphDiffs, | ||
| const double* morphSums, | ||
| double smoothRegion) { | ||
| double morphScale = 1.0; | ||
| if (!morphDiffs || !morphSums) | ||
| return morphScale; | ||
| for (int i = 0; i < nMorphs; ++i) { | ||
| double coeff = morphCoeffs[i]; | ||
| double a = 0.5 * coeff; | ||
| double b = smoothStepFunc(coeff, smoothRegion); | ||
| morphScale *= 1 + (1.0 / parVal) * a * (morphDiffs[i] + b * morphSums[i]); | ||
| } | ||
| return morphScale; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for morph-related functions and usage patterns
rg -n "parametricHistMorphScale|parametricHistEvaluate|parVal" interface/CombineMathFuncs.h -A 3 -B 1Repository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 3350
🏁 Script executed:
# Check if there are any guards for parVal == 0 in related code
rg -n "parVal.*==.*0|parVal.*!=" interface/CombineMathFuncs.hRepository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 311
🏁 Script executed:
# Look at the full parametricHistEvaluate function to understand context
sed -n '343,372p' interface/CombineMathFuncs.hRepository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 1386
Add guard for parVal == 0 in parametric morph functions.
The functions parametricHistMorphScale (line 338) and parametricMorphFunction (line 392) both divide by parVal without checking if it is zero. When parVal == 0, this produces inf, and subsequent calculations like (parVal * scale) / widths[bin_i] result in NaN. Add guards to both functions:
inline Double_t parametricHistMorphScale(const double parVal,
const int nMorphs,
const double* morphCoeffs,
const double* morphDiffs,
const double* morphSums,
double smoothRegion) {
double morphScale = 1.0;
if (!morphDiffs || !morphSums)
return morphScale;
+ if (parVal == 0.0)
+ return morphScale;
for (int i = 0; i < nMorphs; ++i) {Apply the same guard to parametricMorphFunction (line 374-395) at line 390.
📝 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.
| inline Double_t parametricHistMorphScale(const double parVal, | |
| const int nMorphs, | |
| const double* morphCoeffs, | |
| const double* morphDiffs, | |
| const double* morphSums, | |
| double smoothRegion) { | |
| double morphScale = 1.0; | |
| if (!morphDiffs || !morphSums) | |
| return morphScale; | |
| for (int i = 0; i < nMorphs; ++i) { | |
| double coeff = morphCoeffs[i]; | |
| double a = 0.5 * coeff; | |
| double b = smoothStepFunc(coeff, smoothRegion); | |
| morphScale *= 1 + (1.0 / parVal) * a * (morphDiffs[i] + b * morphSums[i]); | |
| } | |
| return morphScale; | |
| } | |
| inline Double_t parametricHistMorphScale(const double parVal, | |
| const int nMorphs, | |
| const double* morphCoeffs, | |
| const double* morphDiffs, | |
| const double* morphSums, | |
| double smoothRegion) { | |
| double morphScale = 1.0; | |
| if (!morphDiffs || !morphSums) | |
| return morphScale; | |
| if (parVal == 0.0) | |
| return morphScale; | |
| for (int i = 0; i < nMorphs; ++i) { | |
| double coeff = morphCoeffs[i]; | |
| double a = 0.5 * coeff; | |
| double b = smoothStepFunc(coeff, smoothRegion); | |
| morphScale *= 1 + (1.0 / parVal) * a * (morphDiffs[i] + b * morphSums[i]); | |
| } | |
| return morphScale; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@interface/CombineMathFuncs.h` around lines 325 - 341, Add a guard for parVal
== 0 in both parametricHistMorphScale and parametricMorphFunction to avoid
division by zero: in parametricHistMorphScale (function
parametricHistMorphScale) check if parVal == 0 at the start and return the
default morphScale (1.0) immediately; in parametricMorphFunction (function
parametricMorphFunction) check if parVal == 0 and return an appropriate
zero/default morph value (e.g., 0.0) immediately so no (1.0/parVal) or other
division by parVal occurs later in the function. Ensure these checks come before
any use of parVal in divisions.
| for (i = 1; i <= N_bins; i++) { | ||
| // Get maybe-morphed bin value | ||
| double binVal = parVals[i - 1] / widths[i - 1]; | ||
| if (morphCoeffs != nullptr) { | ||
| binVal *= parametricMorphFunction( | ||
| i - 1, parVals[i - 1], true, nMorphs, morphCoeffs, morphDiffs, morphSums, smoothRegion); | ||
| } | ||
|
|
||
| if (bins[i - 1] >= xmin && bins[i] <= xmax) { | ||
| // Bin fully in integration domain | ||
| sum += (bins[i] - bins[i - 1]) * binVal; | ||
| } else if (bins[i - 1] < xmin && bins[i] > xmax) { | ||
| // Domain is fully contained in this bin | ||
| sum += (xmax - xmin) * binVal; | ||
| // Exit here, this is the last bin to be processed by construction | ||
| double fullSum = parametricHistFullSum( | ||
| parVals, N_bins, morphCoeffs != nullptr, nMorphs, morphCoeffs, morphDiffs, morphSums, smoothRegion); | ||
| return sum / fullSum; | ||
| } else if (bins[i - 1] < xmin && bins[i] <= xmax && bins[i] > xmin) { | ||
| // Lower domain boundary is in bin | ||
| sum += (bins[i] - xmin) * binVal; | ||
| } else if (bins[i - 1] >= xmin && bins[i] > xmax && bins[i - 1] < xmax) { | ||
| // Upper domain boundary is in bin | ||
| // Exit here, this is the last bin to be processed by construction | ||
| sum += (xmax - bins[i - 1]) * binVal; | ||
| return sum; | ||
| } | ||
| } | ||
| return sum; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and read the full parametricHistIntegral function
find . -name "CombineMathFuncs.h" -o -name "CombineMathFuncs.cxx" | head -20Repository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 114
🏁 Script executed:
# Search for the parametricHistIntegral function to understand its full context
rg "parametricHistIntegral" --type=cpp -n -B 5 -A 60 | head -150Repository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 10752
🏁 Script executed:
# Search for analyticalIntegral and related implementations to check the original pattern
rg "analyticalIntegral|getFullSum" --type=cpp -n -B 3 -A 15 | head -200Repository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 12230
🏁 Script executed:
# Search for test cases that use parametricHistIntegral with ranges
rg -n "parametricHistIntegral|RooParametricHist" --type=cpp -B 2 -A 2 | grep -A 5 -B 5 "range\|integral"Repository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 5005
🏁 Script executed:
# Look for git history or comments explaining the normalization logic
rg -n "fullSum|normalize|parametricHistIntegral" --type=h -B 3 -A 3Repository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 7983
🏁 Script executed:
# Check if there are test files or expected behavior documentation
fd -e "test" -o -name "*Test*" -o -name "*test*" | head -20Repository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 314
🏁 Script executed:
# Look at the parametricHistFullSum function to understand what it computes
rg -n "parametricHistFullSum" --type=cpp -B 5 -A 15 | head -80Repository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 6053
Inconsistent normalization in ranged integral computation.
Line 456 normalizes the result by dividing by fullSum when the domain is fully contained within a single bin, but lines 464 and 467 return unnormalized values for all other ranged cases (including when the upper boundary falls in a bin and the general fallthrough). Both exit cases share identical comments ("this is the last bin to be processed by construction"), making this asymmetry appear unintentional. Either all ranged paths should normalize, or none should—clarify the intended behavior and make the implementation consistent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@interface/CombineMathFuncs.h` around lines 439 - 467, The ranged-integration
branch is inconsistently normalized: the "domain fully contained in this bin"
path returns sum/fullSum but the "upper domain boundary is in bin" path and the
final fallthrough return unnormalized sum. Make normalization consistent by
computing fullSum via parametricHistFullSum (same arguments used earlier) and
dividing sum by fullSum in the upper-bound early return (the branch with comment
"Upper domain boundary is in bin") and in the final return after the loop; keep
use of parVals, N_bins, morphCoeffs, nMorphs, morphDiffs, morphSums,
smoothRegion and parametricMorphFunction unchanged.
| void RooFit::Experimental::codegenImpl(RooParametricHist& arg, CodegenContext& ctx) { | ||
| std::vector<double> diffs_flat; | ||
| std::vector<double> sums_flat; | ||
| if (arg.hasMorphs()) { | ||
| arg.getFlattenedMorphs(diffs_flat, sums_flat); | ||
| } | ||
|
|
||
| std::string xName = ctx.getResult(arg.observable()); | ||
| std::stringstream bin_i; | ||
| bin_i << ctx.buildCall("RooFit::Detail::MathFuncs::parametricHistFindBin", arg.getNBins(), arg.getBins(), xName); | ||
| std::stringstream code; | ||
| code << ctx.buildCall("RooFit::Detail::MathFuncs::parametricHistEvaluate", | ||
| bin_i.str(), | ||
| arg.getPars(), | ||
| arg.getBins(), | ||
| arg.getNBins(), | ||
| arg.getCoeffList(), | ||
| static_cast<int>(arg.getCoeffList().size()), | ||
| arg.hasMorphs() ? diffs_flat : std::vector<double>{}, | ||
| arg.hasMorphs() ? sums_flat : std::vector<double>{}, | ||
| arg.getWidths(), | ||
| arg.getSmoothRegion()) + | ||
| ";\n"; | ||
| ctx.addToCodeBody(code.str()); | ||
| ctx.addResult(&arg, code.str()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the file
find . -name "CombineCodegenImpl.cxx" -type fRepository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 112
🏁 Script executed:
# Check how other codegen implementations use addResult
ast-grep --pattern 'ctx.addResult($_, $$$)'Repository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 2238
🏁 Script executed:
# Also search for the specific file and examine the context around the mentioned lines
rg -n "codegenImpl.*RooParametricHist" --type cpp -A 30Repository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 3093
🏁 Script executed:
# Let's examine the FastVerticalInterpHistPdf2 implementation around lines 120-121
sed -n '100,140p' src/CombineCodegenImpl.cxxRepository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 1464
🏁 Script executed:
# Let's also check if there are other patterns where code is added to body and result separately
rg "ctx.addToCodeBody|ctx.addResult" src/CombineCodegenImpl.cxx -B 3 -A 1Repository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 1164
🏁 Script executed:
# Look at the exact sequence in the problematic function
sed -n '246,271p' src/CombineCodegenImpl.cxxRepository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 1222
addResult receives a statement (with ;\n) instead of an expression.
code.str() at Line 270 includes the trailing ";\n" appended at Line 268. Using a full statement as a codegen result will break the generated code when it's substituted into expression contexts. All other codegen implementations follow the correct pattern: FastVerticalInterpHistPdf2 (Line 121) passes just the variable name expression to addResult, while VerticalInterpPdf (Line 235–243) passes the bare buildCall result without the statement terminator.
Store the buildCall result in a temporary variable via addToCodeBody, then pass the variable name to addResult.
Proposed fix sketch
+ std::string callExpr = ctx.buildCall("RooFit::Detail::MathFuncs::parametricHistEvaluate",
+ bin_i.str(),
+ arg.getPars(),
+ arg.getBins(),
+ arg.getNBins(),
+ arg.getCoeffList(),
+ static_cast<int>(arg.getCoeffList().size()),
+ arg.hasMorphs() ? diffs_flat : std::vector<double>{},
+ arg.hasMorphs() ? sums_flat : std::vector<double>{},
+ arg.getWidths(),
+ arg.getSmoothRegion());
+
+ std::string tmpVar = ctx.getTmpVarName();
std::stringstream code;
- code << ctx.buildCall("RooFit::Detail::MathFuncs::parametricHistEvaluate",
- bin_i.str(),
- arg.getPars(),
- arg.getBins(),
- arg.getNBins(),
- arg.getCoeffList(),
- static_cast<int>(arg.getCoeffList().size()),
- arg.hasMorphs() ? diffs_flat : std::vector<double>{},
- arg.hasMorphs() ? sums_flat : std::vector<double>{},
- arg.getWidths(),
- arg.getSmoothRegion()) +
- ";\n";
- ctx.addToCodeBody(code.str());
- ctx.addResult(&arg, code.str());
+ code << "double " << tmpVar << " = " << callExpr << ";\n";
+ ctx.addToCodeBody(code.str());
+ ctx.addResult(&arg, tmpVar);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/CombineCodegenImpl.cxx` around lines 246 - 271, The code currently
appends a full statement (including the trailing ";\n") to code.str() and passes
that to ctx.addResult, which yields a statement where an expression is expected;
in RooFit::Experimental::codegenImpl for RooParametricHist replace the pattern
that builds code (code and code.str()) with: generate a unique temporary
variable name, produce an assignment statement via ctx.addToCodeBody that
assigns the result of
ctx.buildCall("RooFit::Detail::MathFuncs::parametricHistEvaluate", ...) to that
temp (so keep bin_i and the same argument list), then call ctx.addResult(&arg,
tempVarName) passing only the bare variable/expression (no semicolon). Ensure
you still add the bin-finding call to the body (bin_i usage) and preserve morphs
handling (diffs_flat/sums_flat) when forming the buildCall.
| Double_t RooParametricHist::evaluate() const { | ||
| // Find which bin we are in and lookup the parameter value | ||
| double xVal = getX(); | ||
| int bin_i = RooFit::Detail::MathFuncs::parametricHistFindBin(N_bins, bins, xVal); | ||
| if (bin_i < 0) | ||
| return 0.0; // Out of range | ||
| pars_vals_.resize(N_bins); | ||
| pars_vals_[bin_i] = getParVal(bin_i); | ||
| if (bins.empty() || widths.empty()) | ||
| return 0.0; | ||
| const double* bins_ptr = bins.data(); | ||
| const double* widths_ptr = widths.data(); | ||
| int nMorphs = _coeffList.getSize(); | ||
| getFlattenedMorphs(diffs_flat_, sums_flat_); | ||
|
|
||
| if (_hasMorphs) { | ||
| getCoeffs(); | ||
| } | ||
| size_t bin_i = std::distance(std::begin(bins), it) - 1; | ||
| RooAbsReal *retVar = (RooAbsReal*)pars.at(bin_i); | ||
|
|
||
| double ret = retVar->getVal(); | ||
| ret /= widths[bin_i]; | ||
| return ret; | ||
| return RooFit::Detail::MathFuncs::parametricHistEvaluate(bin_i, | ||
| pars_vals_.data(), | ||
| bins_ptr, | ||
| N_bins, | ||
| _hasMorphs ? coeffs_.data() : nullptr, | ||
| nMorphs, | ||
| _hasMorphs ? diffs_flat_.data() : nullptr, | ||
| _hasMorphs ? sums_flat_.data() : nullptr, | ||
| widths_ptr, | ||
| _smoothRegion); | ||
| } |
There was a problem hiding this comment.
Empty-vector guard at Lines 171–172 is too late — UB already occurred at Line 166.
parametricHistFindBin at Line 166 dereferences bins[0] and bins[N_bins]. If bins is empty, this is undefined behavior before the guard at Line 171 is reached. Move the empty check before the findBin call, or remove it if bins/widths are guaranteed non-empty after construction.
Proposed fix
Double_t RooParametricHist::evaluate() const {
// Find which bin we are in and lookup the parameter value
double xVal = getX();
+ if (bins.empty() || widths.empty())
+ return 0.0;
int bin_i = RooFit::Detail::MathFuncs::parametricHistFindBin(N_bins, bins, xVal);
if (bin_i < 0)
return 0.0; // Out of range
pars_vals_.resize(N_bins);
pars_vals_[bin_i] = getParVal(bin_i);
- if (bins.empty() || widths.empty())
- return 0.0;
const double* bins_ptr = bins.data();📝 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.
| Double_t RooParametricHist::evaluate() const { | |
| // Find which bin we are in and lookup the parameter value | |
| double xVal = getX(); | |
| int bin_i = RooFit::Detail::MathFuncs::parametricHistFindBin(N_bins, bins, xVal); | |
| if (bin_i < 0) | |
| return 0.0; // Out of range | |
| pars_vals_.resize(N_bins); | |
| pars_vals_[bin_i] = getParVal(bin_i); | |
| if (bins.empty() || widths.empty()) | |
| return 0.0; | |
| const double* bins_ptr = bins.data(); | |
| const double* widths_ptr = widths.data(); | |
| int nMorphs = _coeffList.getSize(); | |
| getFlattenedMorphs(diffs_flat_, sums_flat_); | |
| if (_hasMorphs) { | |
| getCoeffs(); | |
| } | |
| size_t bin_i = std::distance(std::begin(bins), it) - 1; | |
| RooAbsReal *retVar = (RooAbsReal*)pars.at(bin_i); | |
| double ret = retVar->getVal(); | |
| ret /= widths[bin_i]; | |
| return ret; | |
| return RooFit::Detail::MathFuncs::parametricHistEvaluate(bin_i, | |
| pars_vals_.data(), | |
| bins_ptr, | |
| N_bins, | |
| _hasMorphs ? coeffs_.data() : nullptr, | |
| nMorphs, | |
| _hasMorphs ? diffs_flat_.data() : nullptr, | |
| _hasMorphs ? sums_flat_.data() : nullptr, | |
| widths_ptr, | |
| _smoothRegion); | |
| } | |
| Double_t RooParametricHist::evaluate() const { | |
| // Find which bin we are in and lookup the parameter value | |
| double xVal = getX(); | |
| if (bins.empty() || widths.empty()) | |
| return 0.0; | |
| int bin_i = RooFit::Detail::MathFuncs::parametricHistFindBin(N_bins, bins, xVal); | |
| if (bin_i < 0) | |
| return 0.0; // Out of range | |
| pars_vals_.resize(N_bins); | |
| pars_vals_[bin_i] = getParVal(bin_i); | |
| const double* bins_ptr = bins.data(); | |
| const double* widths_ptr = widths.data(); | |
| int nMorphs = _coeffList.getSize(); | |
| getFlattenedMorphs(diffs_flat_, sums_flat_); | |
| if (_hasMorphs) { | |
| getCoeffs(); | |
| } | |
| return RooFit::Detail::MathFuncs::parametricHistEvaluate(bin_i, | |
| pars_vals_.data(), | |
| bins_ptr, | |
| N_bins, | |
| _hasMorphs ? coeffs_.data() : nullptr, | |
| nMorphs, | |
| _hasMorphs ? diffs_flat_.data() : nullptr, | |
| _hasMorphs ? sums_flat_.data() : nullptr, | |
| widths_ptr, | |
| _smoothRegion); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/RooParametricHist.cxx` around lines 163 - 192, In
RooParametricHist::evaluate(), the call to
RooFit::Detail::MathFuncs::parametricHistFindBin(bin_i...) can dereference bins
when it's empty, so move the guard that checks bins.empty() || widths.empty() to
before the parametricHistFindBin call (or assert/ensure bins and widths are
non-empty on construction), resize/prepare pars_vals_ after the empty check, and
only call parametricHistFindBin and subsequent logic when bins and widths are
valid; keep the existing logic for morphs/getFlattenedMorphs/getCoeffs unchanged
but invoked after the empty-vector guard.
guitargeek
left a comment
guitargeek
left a comment
There was a problem hiding this comment.
Great, thank you very much!
|
@runtingt @anigamova, this is ready to be merged right? |
Happy from my side |
3 participants
Refactors
evaluateandanalyticalIntegralto free functions such that they can be used by codegen.Marking this as a draft as I had to pre-compute the parameter values in each bin to materialise them into a
std::vector<double>. This gets around the limitation of codegen requiring stateless functions, but may lead to slowdowns when running with the default backend.@guitargeek is there a nice way out here? Perhaps something I've missed that would allow us to keep the old behaviour (evaluating in a single bin) for both backends?
Summary by CodeRabbit
New Features
Refactor