Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions interface/CombineCodegenImpl.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ class FastVerticalInterpHistPdf2;
class FastVerticalInterpHistPdf2D2;
class ProcessNormalization;
class VerticalInterpPdf;
class RooParametricHist;

namespace RooFit::Experimental {

Expand All @@ -18,8 +19,10 @@ namespace RooFit::Experimental {
void codegenImpl(FastVerticalInterpHistPdf2D2& arg, CodegenContext& ctx);
void codegenImpl(ProcessNormalization& arg, CodegenContext& ctx);
void codegenImpl(VerticalInterpPdf& arg, CodegenContext& ctx);
void codegenImpl(RooParametricHist& arg, CodegenContext& ctx);

std::string codegenIntegralImpl(VerticalInterpPdf& arg, int code, const char* rangeName, CodegenContext& ctx);
std::string codegenIntegralImpl(RooParametricHist& arg, int code, const char* rangeName, CodegenContext& ctx);

} // namespace RooFit::Experimental

Expand Down
161 changes: 161 additions & 0 deletions interface/CombineMathFuncs.h
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,167 @@
return result > 0. ? result : integralFloorVal;
}

inline int parametricHistFindBin(const int N_bins, const double* bins, const double x) {
if (x < bins[0] || x >= bins[N_bins])
return -1;

Check warning on line 311 in interface/CombineMathFuncs.h

View check run for this annotation

Codecov / codecov/patch

interface/CombineMathFuncs.h#L311

Added line #L311 was not covered by tests

// Search for the bin
for (int i = 0; i < N_bins; ++i) {
if (x >= bins[i] && x < bins[i + 1])
return i;
}
return -1;

Check warning on line 318 in interface/CombineMathFuncs.h

View check run for this annotation

Codecov / codecov/patch

interface/CombineMathFuncs.h#L318

Added line #L318 was not covered by tests
}

inline int parametricHistFindBin(const int N_bins, std::vector<double> const& bins, const double x) {
return parametricHistFindBin(N_bins, bins.data(), x);
}

inline Double_t parametricHistMorphScale(const double parVal,

Check warning on line 325 in interface/CombineMathFuncs.h

View check run for this annotation

Codecov / codecov/patch

interface/CombineMathFuncs.h#L325

Added line #L325 was not covered by tests
const int nMorphs,
const double* morphCoeffs,
const double* morphDiffs,
const double* morphSums,
double smoothRegion) {
double morphScale = 1.0;

Check warning on line 331 in interface/CombineMathFuncs.h

View check run for this annotation

Codecov / codecov/patch

interface/CombineMathFuncs.h#L331

Added line #L331 was not covered by tests
if (!morphDiffs || !morphSums)
return morphScale;

Check warning on line 333 in interface/CombineMathFuncs.h

View check run for this annotation

Codecov / codecov/patch

interface/CombineMathFuncs.h#L333

Added line #L333 was not covered by tests
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]);

Check warning on line 338 in interface/CombineMathFuncs.h

View check run for this annotation

Codecov / codecov/patch

interface/CombineMathFuncs.h#L335-L338

Added lines #L335 - L338 were not covered by tests
}
return morphScale;

Check warning on line 340 in interface/CombineMathFuncs.h

View check run for this annotation

Codecov / codecov/patch

interface/CombineMathFuncs.h#L340

Added line #L340 was not covered by tests
}
Comment on lines +325 to +341
Copy link
Copy Markdown
Contributor

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:

# Search for morph-related functions and usage patterns
rg -n "parametricHistMorphScale|parametricHistEvaluate|parVal" interface/CombineMathFuncs.h -A 3 -B 1

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

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

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

Suggested change
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.


inline Double_t parametricHistEvaluate(const int bin_i,
const double* parVals,
const double* bins,
const int N_bins,
const double* morphCoeffs,
const int nMorphs,
const double* morphDiffs,
const double* morphSums,
const double* widths,
const double smoothRegion) {
if (bin_i < 0)
return 0.0;

Check warning on line 354 in interface/CombineMathFuncs.h

View check run for this annotation

Codecov / codecov/patch

interface/CombineMathFuncs.h#L354

Added line #L354 was not covered by tests
// Morphing case
if (morphCoeffs != nullptr && nMorphs > 0) {
// morphDiffs and morphSums are flattened arrays of size N_bins * nMorphs
const double* binMorphDiffs = nullptr;
const double* binMorphSums = nullptr;

Check warning on line 359 in interface/CombineMathFuncs.h

View check run for this annotation

Codecov / codecov/patch

interface/CombineMathFuncs.h#L358-L359

Added lines #L358 - L359 were not covered by tests
if (morphDiffs) {
binMorphDiffs = morphDiffs + bin_i * nMorphs;

Check warning on line 361 in interface/CombineMathFuncs.h

View check run for this annotation

Codecov / codecov/patch

interface/CombineMathFuncs.h#L361

Added line #L361 was not covered by tests
}
if (morphSums) {
binMorphSums = morphSums + bin_i * nMorphs;

Check warning on line 364 in interface/CombineMathFuncs.h

View check run for this annotation

Codecov / codecov/patch

interface/CombineMathFuncs.h#L364

Added line #L364 was not covered by tests
}
double parVal = parVals[bin_i];
double scale = parametricHistMorphScale(parVal, nMorphs, morphCoeffs, binMorphDiffs, binMorphSums, smoothRegion);
return (parVal * scale) / widths[bin_i];

Check warning on line 368 in interface/CombineMathFuncs.h

View check run for this annotation

Codecov / codecov/patch

interface/CombineMathFuncs.h#L366-L368

Added lines #L366 - L368 were not covered by tests
}
// No morphing case
return parVals[bin_i] / widths[bin_i];
}

inline Double_t parametricMorphFunction(const int j,

Check warning on line 374 in interface/CombineMathFuncs.h

View check run for this annotation

Codecov / codecov/patch

interface/CombineMathFuncs.h#L374

Added line #L374 was not covered by tests
const double parVal,
const bool hasMorphs,
const int nMorphs,
const double* morphCoeffs,
const double* morphDiffs,
const double* morphSums,
double smoothRegion) {
double morphScale = 1.0;

Check warning on line 382 in interface/CombineMathFuncs.h

View check run for this annotation

Codecov / codecov/patch

interface/CombineMathFuncs.h#L382

Added line #L382 was not covered by tests
if (!hasMorphs)
return morphScale;

Check warning on line 384 in interface/CombineMathFuncs.h

View check run for this annotation

Codecov / codecov/patch

interface/CombineMathFuncs.h#L384

Added line #L384 was not covered by tests

int ndim = nMorphs;

Check warning on line 386 in interface/CombineMathFuncs.h

View check run for this annotation

Codecov / codecov/patch

interface/CombineMathFuncs.h#L386

Added line #L386 was not covered by tests
// apply all morphs one by one to the bin
// almost certaintly a faster way to do this in a vectorized way ....
for (int i = 0; i < ndim; ++i) {
double x = morphCoeffs[i];
double a = 0.5 * x, b = smoothStepFunc(x, smoothRegion);
morphScale *= 1 + (1. / parVal) * a * (morphDiffs[j * nMorphs + i] + b * morphSums[j * nMorphs + i]);

Check warning on line 392 in interface/CombineMathFuncs.h

View check run for this annotation

Codecov / codecov/patch

interface/CombineMathFuncs.h#L390-L392

Added lines #L390 - L392 were not covered by tests
}
return morphScale;

Check warning on line 394 in interface/CombineMathFuncs.h

View check run for this annotation

Codecov / codecov/patch

interface/CombineMathFuncs.h#L394

Added line #L394 was not covered by tests
}

inline Double_t parametricHistFullSum(const double* parVals,
const int nBins,
const bool hasMorphs,
const int nMorphs,
const double* morphCoeffs,
const double* morphDiffs,
const double* morphSums,
double smoothRegion) {
double sum = 0;
for (int i = 0; i < nBins; ++i) {
double thisVal = parVals[i];
if (hasMorphs) {
// Apply morphing to this bin, just like in RooParametricHist::evaluate
thisVal *=
parametricMorphFunction(i, thisVal, hasMorphs, nMorphs, morphCoeffs, morphDiffs, morphSums, smoothRegion);

Check warning on line 411 in interface/CombineMathFuncs.h

View check run for this annotation

Codecov / codecov/patch

interface/CombineMathFuncs.h#L410-L411

Added lines #L410 - L411 were not covered by tests
}
sum += thisVal;
}
return sum;
}

inline Double_t parametricHistIntegral(const double* parVals,
const double* bins,
const int N_bins,
const double* morphCoeffs,
const int nMorphs,
const double* morphDiffs,
const double* morphSums,
const double* widths,
const double smoothRegion,
const char* rangeName,
const double xmin,
const double xmax) {
// No ranges
if (!rangeName) {
return parametricHistFullSum(
parVals, N_bins, morphCoeffs != nullptr, nMorphs, morphCoeffs, morphDiffs, morphSums, smoothRegion);
}

// Case with ranges, calculate integral explicitly
double sum = 0;

Check warning on line 437 in interface/CombineMathFuncs.h

View check run for this annotation

Codecov / codecov/patch

interface/CombineMathFuncs.h#L437

Added line #L437 was not covered by tests
int i;
for (i = 1; i <= N_bins; i++) {
// Get maybe-morphed bin value
double binVal = parVals[i - 1] / widths[i - 1];

Check warning on line 441 in interface/CombineMathFuncs.h

View check run for this annotation

Codecov / codecov/patch

interface/CombineMathFuncs.h#L441

Added line #L441 was not covered by tests
if (morphCoeffs != nullptr) {
binVal *= parametricMorphFunction(
i - 1, parVals[i - 1], true, nMorphs, morphCoeffs, morphDiffs, morphSums, smoothRegion);

Check warning on line 444 in interface/CombineMathFuncs.h

View check run for this annotation

Codecov / codecov/patch

interface/CombineMathFuncs.h#L443-L444

Added lines #L443 - L444 were not covered by tests
}

if (bins[i - 1] >= xmin && bins[i] <= xmax) {
// Bin fully in integration domain
sum += (bins[i] - bins[i - 1]) * binVal;

Check warning on line 449 in interface/CombineMathFuncs.h

View check run for this annotation

Codecov / codecov/patch

interface/CombineMathFuncs.h#L449

Added line #L449 was not covered by tests
} else if (bins[i - 1] < xmin && bins[i] > xmax) {
// Domain is fully contained in this bin
sum += (xmax - xmin) * binVal;

Check warning on line 452 in interface/CombineMathFuncs.h

View check run for this annotation

Codecov / codecov/patch

interface/CombineMathFuncs.h#L452

Added line #L452 was not covered by tests
// Exit here, this is the last bin to be processed by construction
double fullSum = parametricHistFullSum(

Check warning on line 454 in interface/CombineMathFuncs.h

View check run for this annotation

Codecov / codecov/patch

interface/CombineMathFuncs.h#L454

Added line #L454 was not covered by tests
parVals, N_bins, morphCoeffs != nullptr, nMorphs, morphCoeffs, morphDiffs, morphSums, smoothRegion);
return sum / fullSum;

Check warning on line 456 in interface/CombineMathFuncs.h

View check run for this annotation

Codecov / codecov/patch

interface/CombineMathFuncs.h#L456

Added line #L456 was not covered by tests
} else if (bins[i - 1] < xmin && bins[i] <= xmax && bins[i] > xmin) {
// Lower domain boundary is in bin
sum += (bins[i] - xmin) * binVal;

Check warning on line 459 in interface/CombineMathFuncs.h

View check run for this annotation

Codecov / codecov/patch

interface/CombineMathFuncs.h#L459

Added line #L459 was not covered by tests
} 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;

Check warning on line 464 in interface/CombineMathFuncs.h

View check run for this annotation

Codecov / codecov/patch

interface/CombineMathFuncs.h#L463-L464

Added lines #L463 - L464 were not covered by tests
}
}
return sum;

Check warning on line 467 in interface/CombineMathFuncs.h

View check run for this annotation

Codecov / codecov/patch

interface/CombineMathFuncs.h#L467

Added line #L467 was not covered by tests
Comment on lines +439 to +467
Copy link
Copy Markdown
Contributor

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:

# First, let's find and read the full parametricHistIntegral function
find . -name "CombineMathFuncs.h" -o -name "CombineMathFuncs.cxx" | head -20

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

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

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

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

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

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

}

} // namespace MathFuncs
} // namespace Detail
} // namespace RooFit
Expand Down
33 changes: 24 additions & 9 deletions interface/RooParametricHist.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,33 @@
RooArgList & getAllBinVars() const ;

RooRealVar & getObs() const { return (RooRealVar&)x; };
RooAbsReal& observable() const { return const_cast<RooAbsReal&>(static_cast<const RooAbsReal&>(x.arg())); }

Check warning on line 34 in interface/RooParametricHist.h

View check run for this annotation

Codecov / codecov/patch

interface/RooParametricHist.h#L34

Added line #L34 was not covered by tests
const std::vector<double> getBins() const { return bins; };
const int getNBins() const { return N_bins; };

Check warning on line 36 in interface/RooParametricHist.h

View check run for this annotation

Codecov / codecov/patch

interface/RooParametricHist.h#L36

Added line #L36 was not covered by tests
const std::vector<double> getWidths() const { return widths; };

const double quickSum() const {return getFullSum() ;}
const RooArgList& getPars() const { return pars; };
const RooArgList& getCoeffList() const { return _coeffList; };

Check warning on line 40 in interface/RooParametricHist.h

View check run for this annotation

Codecov / codecov/patch

interface/RooParametricHist.h#L39-L40

Added lines #L39 - L40 were not covered by tests

const double quickSum() const;
//RooAddition & getYieldVar(){return sum;};

// how can we pass this version? is there a Collection object for RooDataHists?
//void addMorphs(RooArgList &_morphPdfsUp, RooArgList &_morphPdfsDown, RooArgList &_coeffs, double smoothRegion);
void addMorphs(RooDataHist&, RooDataHist&, RooRealVar&, double );
Double_t evaluate() const override;

// Accessors for evaluation data
double getX() const { return x; }
double getSmoothRegion() const { return _smoothRegion; }
bool hasMorphs() const { return _hasMorphs; }

Check warning on line 53 in interface/RooParametricHist.h

View check run for this annotation

Codecov / codecov/patch

interface/RooParametricHist.h#L52-L53

Added lines #L52 - L53 were not covered by tests

double getParVal(int bin_i) const;

// Utility functions for data extraction
const std::vector<double>& getParVals() const;
const std::vector<double>& getCoeffs() const;
void getFlattenedMorphs(std::vector<double>& diffs_flat, std::vector<double>& sums_flat) const;

protected:

Expand All @@ -56,19 +74,16 @@
mutable double _smoothRegion;
mutable bool _hasMorphs;
mutable std::vector<std::vector <double> > _diffs;
mutable std::vector<std::vector <double> > _sums;
double evaluateMorphFunction(int) const;
mutable std::vector<std::vector<double> > _sums;

mutable std::vector<double> pars_vals_; //! Don't serialize me
mutable std::vector<double> coeffs_; //! Don't serialize me
mutable std::vector<double> diffs_flat_; //! Don't serialize me
mutable std::vector<double> sums_flat_; //! Don't serialize me

void initializeBins(const TH1&) const;
//void initializeNorm();


double evaluatePartial() const ;
double evaluateFull() const ;
Double_t evaluate() const override ;
double getFullSum() const ;

mutable double _cval;
void update_cval(double r){_cval=r;};

Expand Down
55 changes: 55 additions & 0 deletions src/CombineCodegenImpl.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "../interface/VerticalInterpHistPdf.h"
#include "../interface/VerticalInterpPdf.h"
#include "../interface/CombineMathFuncs.h"
#include "../interface/RooParametricHist.h"

#include <RooUniformBinning.h>

Expand Down Expand Up @@ -242,6 +243,33 @@
arg.quadraticAlgo()));
}

void RooFit::Experimental::codegenImpl(RooParametricHist& arg, CodegenContext& ctx) {
std::vector<double> diffs_flat;
std::vector<double> sums_flat;

Check warning on line 248 in src/CombineCodegenImpl.cxx

View check run for this annotation

Codecov / codecov/patch

src/CombineCodegenImpl.cxx#L246-L248

Added lines #L246 - L248 were not covered by tests
if (arg.hasMorphs()) {
arg.getFlattenedMorphs(diffs_flat, sums_flat);

Check warning on line 250 in src/CombineCodegenImpl.cxx

View check run for this annotation

Codecov / codecov/patch

src/CombineCodegenImpl.cxx#L250

Added line #L250 was not covered by tests
}

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

Check warning on line 258 in src/CombineCodegenImpl.cxx

View check run for this annotation

Codecov / codecov/patch

src/CombineCodegenImpl.cxx#L253-L258

Added lines #L253 - L258 were not covered by tests
arg.getPars(),
arg.getBins(),
arg.getNBins(),

Check warning on line 261 in src/CombineCodegenImpl.cxx

View check run for this annotation

Codecov / codecov/patch

src/CombineCodegenImpl.cxx#L260-L261

Added lines #L260 - L261 were not covered by tests
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());

Check warning on line 270 in src/CombineCodegenImpl.cxx

View check run for this annotation

Codecov / codecov/patch

src/CombineCodegenImpl.cxx#L263-L270

Added lines #L263 - L270 were not covered by tests
}
Comment on lines +246 to +271
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# First, let's locate and examine the file
find . -name "CombineCodegenImpl.cxx" -type f

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

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

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

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

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


std::string RooFit::Experimental::codegenIntegralImpl(VerticalInterpPdf& arg,
int code,
const char* rangeName,
Expand All @@ -257,4 +285,31 @@
arg.quadraticAlgo());
}

std::string RooFit::Experimental::codegenIntegralImpl(RooParametricHist& arg,

Check warning on line 288 in src/CombineCodegenImpl.cxx

View check run for this annotation

Codecov / codecov/patch

src/CombineCodegenImpl.cxx#L288

Added line #L288 was not covered by tests
int code,
const char* rangeName,
CodegenContext& ctx) {
std::vector<double> diffs_flat;
std::vector<double> sums_flat;

Check warning on line 293 in src/CombineCodegenImpl.cxx

View check run for this annotation

Codecov / codecov/patch

src/CombineCodegenImpl.cxx#L292-L293

Added lines #L292 - L293 were not covered by tests
if (arg.hasMorphs()) {
arg.getFlattenedMorphs(diffs_flat, sums_flat);

Check warning on line 295 in src/CombineCodegenImpl.cxx

View check run for this annotation

Codecov / codecov/patch

src/CombineCodegenImpl.cxx#L295

Added line #L295 was not covered by tests
}
double xMin = rangeName ? arg.getObs().getMin(rangeName) : -1;
double xMax = rangeName ? arg.getObs().getMax(rangeName) : -1; // if rangeName is null, these values won't be used

Check warning on line 298 in src/CombineCodegenImpl.cxx

View check run for this annotation

Codecov / codecov/patch

src/CombineCodegenImpl.cxx#L297-L298

Added lines #L297 - L298 were not covered by tests

return ctx.buildCall("RooFit::Detail::MathFuncs::parametricHistIntegral",
arg.getPars(),
arg.getBins(),
arg.getNBins(),

Check warning on line 303 in src/CombineCodegenImpl.cxx

View check run for this annotation

Codecov / codecov/patch

src/CombineCodegenImpl.cxx#L302-L303

Added lines #L302 - L303 were not covered by tests
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(),
rangeName ? std::string("\"") + rangeName + "\"" : std::string("nullptr"),

Check warning on line 310 in src/CombineCodegenImpl.cxx

View check run for this annotation

Codecov / codecov/patch

src/CombineCodegenImpl.cxx#L305-L310

Added lines #L305 - L310 were not covered by tests
xMin,
xMax);

Check warning on line 312 in src/CombineCodegenImpl.cxx

View check run for this annotation

Codecov / codecov/patch

src/CombineCodegenImpl.cxx#L312

Added line #L312 was not covered by tests
}

#endif // ROOT_VERSION_CODE >= ROOT_VERSION(6,36,0)
Loading
Loading