Skip to content

Add codegen implementations of CMSHist classes (2) #1252

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
maxgalli wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Add codegen implementations of CMSHist classes (2) #1252

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6ac692b
Consider a bin edge to fall in the next bin like in ROOT
PetroZarytskyi Nov 25, 2025
1742f37
[WIP] Add codegen implementations of CMSHist classes
guitargeek Nov 4, 2025
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: 2 additions & 1 deletion interface/CMSHistErrorPropagator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,8 @@ class CMSHistErrorPropagator : public RooAbsReal {
RooArgList wrapperList() const;
RooArgList const& coefList() const { return coeffs_; }
RooArgList const& funcList() const { return funcs_; }

RooAbsReal const& getXVar() const { return *x_; }

std::map<std::string, Double_t> getProcessNorms() const;

friend class CMSHistV<CMSHistErrorPropagator>;
Expand Down
2 changes: 2 additions & 0 deletions interface/CMSHistSum.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ class CMSHistSum : public RooAbsReal {

RooAbsReal const& getXVar() const { return x_.arg(); }

std::vector<double> getFuncValList(std::size_t fnIdx);

static void EnableFastVertical();
friend class CMSHistV<CMSHistSum>;

Expand Down
9 changes: 9 additions & 0 deletions interface/CombineCodegenImpl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
#include <string>

class AsymPow;
class CMSHistErrorPropagator;
class CMSHistFunc;
class CMSHistSum;
class FastVerticalInterpHistPdf2;
class FastVerticalInterpHistPdf2D2;
class ProcessNormalization;
Expand All @@ -15,12 +18,18 @@ namespace RooFit::Experimental {
class CodegenContext;

void codegenImpl(AsymPow& arg, CodegenContext& ctx);
void codegenImpl(CMSHistErrorPropagator& arg, CodegenContext& ctx);
void codegenImpl(CMSHistFunc& arg, CodegenContext& ctx);
void codegenImpl(CMSHistSum& arg, CodegenContext& ctx);
void codegenImpl(FastVerticalInterpHistPdf2& arg, CodegenContext& ctx);
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(CMSHistErrorPropagator& arg, int code, const char* rangeName, CodegenContext& ctx);
std::string codegenIntegralImpl(CMSHistFunc& arg, int code, const char* rangeName, CodegenContext& ctx);
std::string codegenIntegralImpl(CMSHistSum& arg, int code, const char* rangeName, CodegenContext& ctx);
Comment on lines +30 to +32

@coderabbitai coderabbitai Bot May 19, 2026

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 | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify declaration/definition presence for CMSHistFunc integral codegen overload.
# Expected: one declaration in header + one definition in a .cxx file.
rg -nP 'codegenIntegralImpl\s*\(\s*CMSHistFunc\s*&' -C2

Repository: cms-analysis/HiggsAnalysis-CombinedLimit

Length of output: 699

🏁 Script executed:

#!/bin/bash
# Search for all definitions of codegenIntegralImpl in implementation files
# Use a broader pattern to catch different formatting styles
rg 'codegenIntegralImpl.*CMSHistFunc' --type cpp -A 2

Repository: cms-analysis/HiggsAnalysis-CombinedLimit

Length of output: 503

🏁 Script executed:

#!/bin/bash
# List all codegenIntegralImpl definitions in .cxx files to see what's implemented
fd '\.(cxx|cpp)$' --type f | xargs rg -l 'codegenIntegralImpl' | head -5

Repository: cms-analysis/HiggsAnalysis-CombinedLimit

Length of output: 111

🏁 Script executed:

#!/bin/bash
# Check the main implementation file for all CMSHistFunc overload definitions
rg -n 'CMSHistFunc' src/CombineCodegenImpl.cxx | grep -i 'integral\|codegen'

Repository: cms-analysis/HiggsAnalysis-CombinedLimit

Length of output: 168

🏁 Script executed:

#!/bin/bash
# Find all codegenIntegralImpl definitions in the .cxx file
rg -n '^.*codegenIntegralImpl' src/CombineCodegenImpl.cxx

Repository: cms-analysis/HiggsAnalysis-CombinedLimit

Length of output: 410

🏁 Script executed:

#!/bin/bash
# Get context around codegenIntegralImpl definitions to see all overloads
rg -B2 -A5 '^\s*std::string.*codegenIntegralImpl' src/CombineCodegenImpl.cxx | head -80

Repository: cms-analysis/HiggsAnalysis-CombinedLimit

Length of output: 1556

Define the CMSHistFunc integral overload implementation.

The declaration at line 31 declares codegenIntegralImpl(CMSHistFunc&, ...), but this overload has no matching definition in src/CombineCodegenImpl.cxx. The other three overloads (CMSHistErrorPropagator, CMSHistSum, VerticalInterpPdf) are all properly defined. This missing definition will cause a linker error if the overload is dispatched to.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@interface/CombineCodegenImpl.h` around lines 30 - 32, Add a definition for
std::string codegenIntegralImpl(CMSHistFunc& arg, int code, const char*
rangeName, CodegenContext& ctx) that matches the declared signature and mirrors
the pattern used by the existing overloads (codegenIntegralImpl for
CMSHistErrorPropagator, CMSHistSum and VerticalInterpPdf): generate the proper
unique name/identifier, emit the same kind of codegen plumbing (handling
CMSHistFunc-specific members/parameters and any interpolation or parameter
bindings), and return the produced code string; ensure the function uses the
same helper utilities and conventions from the other overloads so linking
succeeds when this overload is dispatched.

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

Expand Down
28 changes: 28 additions & 0 deletions interface/CombineMathFuncs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
#include <RooConstVar.h>
#include <RtypesCore.h>

#include <RooFit/Detail/MathFuncs.h>

#include <cmath>

namespace RooFit {
Expand Down Expand Up @@ -467,6 +469,32 @@ inline Double_t parametricHistIntegral(const double* parVals,
return sum;
}

inline double cmsHistFunc(double x, std::size_t nBins, double const* binEdges, double const *values) {
// I guess a "CMS hist func" is just looking up some values in some bin?
unsigned int binIdx = RooFit::Detail::MathFuncs::binNumber(x, 1.0, binEdges, nBins + 1, nBins, 0);
return values[binIdx];
}

inline double cmsHistErrorPropagator(double x, std::size_t nFuncs, double const* coefList, double const* funcList) {
// My naive understanding of the logic: multiply functions with coefficients and sum up
double out = 0.;
for (std::size_t i = 0; i < nFuncs; ++i) {
out += coefList[i] * funcList[i];
}
return out;
}

inline double cmsHistSum(
double x, std::size_t nBins, std::size_t nSamples, double* coeffs, double const* binEdges, double const* values) {
unsigned int binIdx = RooFit::Detail::MathFuncs::binNumber(x, 1.0, binEdges, nBins + 1, nBins, 0);
double val = 0.;
for (std::size_t iSample = 0; iSample < nSamples; ++iSample)
val += coeffs[iSample] * values[iSample * nBins + binIdx];

return val;
}


} // namespace MathFuncs
} // namespace Detail
} // namespace RooFit
Expand Down
6 changes: 5 additions & 1 deletion interface/FastTemplate_Old.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,13 +76,15 @@ class FastTemplate {

void Dump() const ;

AT const &GetValues() const { return values_; }

protected:
unsigned int size_;
AT values_;
};
class FastHisto : public FastTemplate {
public:
FastHisto() : FastTemplate(), binEdges_(), binWidths_() {}
FastHisto() = default;
FastHisto(const TH1 &hist) ;
FastHisto(const FastHisto &other) ;
FastHisto & operator=(const FastHisto &other) {
Expand Down Expand Up @@ -124,6 +126,8 @@ class FastHisto : public FastTemplate {
const T & GetEdge(unsigned int i) const { return binEdges_[i]; }
const T & GetWidth(unsigned int i) const { return binWidths_[i]; }

AT const &GetBinEdges() const { return binEdges_; }

private:
AT binEdges_;
AT binWidths_;
Expand Down
18 changes: 18 additions & 0 deletions src/CMSHistSum.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,24 @@ void CMSHistSum::updateCache() const {
}
}

std::vector<double> CMSHistSum::getFuncValList(std::size_t fnIdx) {
staging_ = compcache_[fnIdx];
if (vtype_[fnIdx] == CMSHistFunc::VerticalSetting::LogQuadLinear) {
staging_.Exp();
staging_.Scale(storage_[process_fields_[fnIdx]].Integral() / staging_.Integral());
}
staging_.CropUnderflows();
std::vector<double> result = staging_.GetValues();
for (unsigned j = 0; j < bintypes_.size(); ++j) {
if (bintypes_[j][0] == 1) {
double x = vbinpars_[j][0]->getVal();
result[j] += binerrors_[fnIdx][j] * x;
} else if (bintypes_[j][0] == 2 || bintypes_[j][0] == 3)
result[j] += scaledbinmods_[fnIdx][j];
}
return result;
}
Comment on lines +415 to +431

@coderabbitai coderabbitai Bot May 19, 2026

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 | ⚡ Quick win

Harden getFuncValList against invalid index and stale/zero-normalization state.

This method currently assumes valid fnIdx, initialized caches, and non-zero staging_.Integral(). Add explicit guards to prevent out-of-bounds and inf/nan results.

Proposed fix 
 std::vector<double> CMSHistSum::getFuncValList(std::size_t fnIdx) {
+  updateCache();
+  if (fnIdx >= compcache_.size() || fnIdx >= binerrors_.size() || fnIdx >= scaledbinmods_.size()) {
+    throw std::out_of_range("CMSHistSum::getFuncValList: fnIdx out of range");
+  }
+
   staging_ = compcache_[fnIdx];
   if (vtype_[fnIdx] == CMSHistFunc::VerticalSetting::LogQuadLinear) {
     staging_.Exp();
-    staging_.Scale(storage_[process_fields_[fnIdx]].Integral() / staging_.Integral());
+    const double denom = staging_.Integral();
+    if (denom > 0.) {
+      staging_.Scale(storage_[process_fields_[fnIdx]].Integral() / denom);
+    }
   }
   staging_.CropUnderflows();
   std::vector<double> result = staging_.GetValues();


void CMSHistSum::runBarlowBeeston() const {
updateCache();
const unsigned n = bb_.use.size();
Expand Down
67 changes: 67 additions & 0 deletions src/CombineCodegenImpl.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
#if ROOT_VERSION_CODE >= ROOT_VERSION(6, 36, 0)

#include "../interface/AsymPow.h"
#include "../interface/CMSHistErrorPropagator.h"
#include "../interface/CMSHistFunc.h"
#include "../interface/CMSHistSum.h"
#include "../interface/ProcessNormalization.h"
#include "../interface/VerticalInterpHistPdf.h"
#include "../interface/VerticalInterpPdf.h"
Expand Down Expand Up @@ -312,4 +315,68 @@ std::string RooFit::Experimental::codegenIntegralImpl(RooParametricHist& arg,
xMax);
}

void RooFit::Experimental::codegenImpl(CMSHistFunc& arg, CodegenContext& ctx) {
arg.evaluate(); // trigger cache() creation
std::vector<double> const& edges = arg.cache().GetBinEdges();

// I don't know if these values are actually constant and we can take them
// here to hardcode into the generated code...
auto const& values = arg.cache().GetValues();

ctx.addResult(&arg,
ctx.buildCall("RooFit::Detail::MathFuncs::cmsHistFunc",
arg.getXVar(),
edges.size() - 1,
edges,
values
));
}

void RooFit::Experimental::codegenImpl(CMSHistErrorPropagator& arg, CodegenContext& ctx) {
ctx.addResult(&arg,
ctx.buildCall("RooFit::Detail::MathFuncs::cmsHistErrorPropagator",
arg.getXVar(),
arg.coefList().size(),
arg.coefList(),
arg.funcList()));
}

std::string RooFit::Experimental::codegenIntegralImpl(CMSHistErrorPropagator& arg,
int code,
const char* rangeName,
CodegenContext& ctx) {
return "2.0"; // TODO: dummy for now
}
Comment on lines +344 to +349

@coderabbitai coderabbitai Bot May 19, 2026

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 | ⚡ Quick win

Replace dummy integral constants with fail-fast behavior.

Returning "2.0"/"3.0" silently produces wrong integrals if these paths are used.

Proposed minimal safe fix 
 std::string RooFit::Experimental::codegenIntegralImpl(CMSHistErrorPropagator& arg,
                                                       int code,
                                                       const char* rangeName,
                                                       CodegenContext& ctx) {
-  return "2.0";  // TODO: dummy for now
+  throw std::runtime_error("codegenIntegralImpl(CMSHistErrorPropagator) not implemented");
 }
@@
 std::string RooFit::Experimental::codegenIntegralImpl(CMSHistSum& arg,
                                                       int code,
                                                       const char* rangeName,
                                                       CodegenContext& ctx) {
-  return "3.0";  // TODO: dummy for now
+  throw std::runtime_error("codegenIntegralImpl(CMSHistSum) not implemented");
 }

Also applies to: 375-380

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/CombineCodegenImpl.cxx` around lines 344 - 349, The function
RooFit::Experimental::codegenIntegralImpl currently returns a dummy string
"2.0"; replace this with fail-fast behavior by throwing a std::runtime_error (or
using LOG and abort) that clearly states the integral code path is unimplemented
and includes the parameters (code, rangeName, maybe ctx.name) for debugging; do
the same replacement for the other similar stubbed function(s) around lines
375-380 so any use of these paths fails loudly instead of silently returning
wrong constants.


void RooFit::Experimental::codegenImpl(CMSHistSum& arg, CodegenContext& ctx) {
arg.evaluate();
std::vector<double> const& edges = arg.cache().GetBinEdges();
std::size_t nBins = edges.size() - 1;
RooArgList const& coefs = arg.coefList();
std::size_t nSamples = coefs.size();
std::vector<double> values(nBins * nSamples);
for (std::size_t iSamples = 0; iSamples < nSamples; ++iSamples) {
std::vector<double> sampleValues = arg.getFuncValList(iSamples);
for (std::size_t iBin = 0; iBin < nBins; ++iBin)
values[iBin + iSamples * nBins] = sampleValues[iBin];
}

ctx.addResult(&arg,
ctx.buildCall("RooFit::Detail::MathFuncs::cmsHistSum",
arg.getXVar(),
nBins,
nSamples,
coefs,
edges,
values
));
}

std::string RooFit::Experimental::codegenIntegralImpl(CMSHistSum& arg,
int code,
const char* rangeName,
CodegenContext& ctx) {
return "3.0"; // TODO: dummy for now
}
Comment on lines +318 to +380

@coderabbitai coderabbitai Bot May 19, 2026

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 | 🟡 Minor | ⚡ Quick win

Run clang-format on the new codegen overload block.

CI is already failing formatting checks for this region (clang-format / precheckin).

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/CombineCodegenImpl.cxx` around lines 318 - 380, Formatting in the new
codegen overload block is not clang-formatted; run the project's
clang-format/precheckin style on the edited region (the functions
RooFit::Experimental::codegenImpl(CMSHistFunc&),
codegenImpl(CMSHistErrorPropagator&),
codegenIntegralImpl(CMSHistErrorPropagator&), codegenImpl(CMSHistSum&), and
codegenIntegralImpl(CMSHistSum&)) and commit the formatting-only changes so the
CI formatting check passes, ensuring spacing, line breaks, and trailing commas
match the repository's clang-format rules.


#endif // ROOT_VERSION_CODE >= ROOT_VERSION(6,36,0)
18 changes: 9 additions & 9 deletions src/FastTemplate_Old.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,15 +73,15 @@ FastHisto::FastHisto(const FastHisto &other) :
}

int FastHisto::FindBin(const T &x) const {
auto match = std::lower_bound(binEdges_.begin(), binEdges_.end(), x);
auto match = std::upper_bound(binEdges_.begin(), binEdges_.end(), x);

@coderabbitai coderabbitai Bot May 19, 2026

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 | ⚡ Quick win

Fix clang-format violations in this file before merge.

CI is currently failing on formatting for this file; please run the project formatter on the touched ranges and re-push.

Also applies to: 84-84, 143-143, 146-146, 189-189, 200-200, 250-250, 253-253, 256-256

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/FastTemplate_Old.cc` at line 76, Run the project formatter (clang-format)
over the edited ranges in FastTemplate_Old.cc and reformat the affected
statements (e.g., the std::upper_bound call that assigns to match using
binEdges_, x) so they conform to the repository style; ensure spacing,
indentation, and line breaks around expressions like auto match =
std::upper_bound(binEdges_.begin(), binEdges_.end(), x); (and the other similar
occurrences referenced by CI) are corrected, then re-run the formatter and push
the updated file.

if (match == binEdges_.begin()) return -1;
if (match == binEdges_.end()) return values_.size();
return match - binEdges_.begin() - 1;
}


FastHisto::T FastHisto::GetAt(const T &x) const {
auto match = std::lower_bound(binEdges_.begin(), binEdges_.end(), x);
auto match = std::upper_bound(binEdges_.begin(), binEdges_.end(), x);
if (match == binEdges_.begin() || match == binEdges_.end()) return T(0.0);
return values_[match - binEdges_.begin() - 1];
}
Expand Down Expand Up @@ -140,10 +140,10 @@ FastHisto2D::FastHisto2D(const FastHisto2D &other) :
}

FastHisto2D::T FastHisto2D::GetAt(const T &x, const T &y) const {
auto matchx = std::lower_bound(binEdgesX_.begin(), binEdgesX_.end(), x);
auto matchx = std::upper_bound(binEdgesX_.begin(), binEdgesX_.end(), x);
if (matchx == binEdgesX_.begin() || matchx == binEdgesX_.end()) return T(0.0);
int ix = (matchx - binEdgesX_.begin() - 1);
auto matchy = std::lower_bound(binEdgesY_.begin(), binEdgesY_.end(), y);
auto matchy = std::upper_bound(binEdgesY_.begin(), binEdgesY_.end(), y);
if (matchy == binEdgesY_.begin() || matchy == binEdgesY_.end()) return T(0.0);
int iy = (matchy - binEdgesY_.begin() - 1);
return values_[ix * binY_ + iy];
Expand Down Expand Up @@ -186,7 +186,7 @@ FastHisto2D::T FastHisto2D::GetMaxOnXY() const {


FastHisto2D::T FastHisto2D::GetMaxOnX(const T &y) const {
auto matchy = std::lower_bound(binEdgesY_.begin(), binEdgesY_.end(), y);
auto matchy = std::upper_bound(binEdgesY_.begin(), binEdgesY_.end(), y);
if (matchy == binEdgesY_.begin() || matchy == binEdgesY_.end()) return T(0.0);
int iy = (matchy - binEdgesY_.begin() - 1);
T ret = 0.0;
Expand All @@ -197,7 +197,7 @@ FastHisto2D::T FastHisto2D::GetMaxOnX(const T &y) const {
}

FastHisto2D::T FastHisto2D::GetMaxOnY(const T &x) const {
auto matchx = std::lower_bound(binEdgesX_.begin(), binEdgesX_.end(), x);
auto matchx = std::upper_bound(binEdgesX_.begin(), binEdgesX_.end(), x);
if (matchx == binEdgesX_.begin() || matchx == binEdgesX_.end()) return T(0.0);
int ix = (matchx - binEdgesX_.begin() - 1);
return *std::max( &values_[ix * binY_], &values_[(ix+1) * binY_] );
Expand Down Expand Up @@ -247,13 +247,13 @@ FastHisto3D::FastHisto3D(const FastHisto3D &other) :
}

FastHisto3D::T FastHisto3D::GetAt(const T &x, const T &y, const T &z) const {
auto matchx = std::lower_bound(binEdgesX_.begin(), binEdgesX_.end(), x);
auto matchx = std::upper_bound(binEdgesX_.begin(), binEdgesX_.end(), x);
if (matchx == binEdgesX_.begin() || matchx == binEdgesX_.end()) return T(0.0);
int ix = (matchx - binEdgesX_.begin() - 1);
auto matchy = std::lower_bound(binEdgesY_.begin(), binEdgesY_.end(), y);
auto matchy = std::upper_bound(binEdgesY_.begin(), binEdgesY_.end(), y);
if (matchy == binEdgesY_.begin() || matchy == binEdgesY_.end()) return T(0.0);
int iy = (matchy - binEdgesY_.begin() - 1);
auto matchz = std::lower_bound(binEdgesZ_.begin(), binEdgesZ_.end(), z);
auto matchz = std::upper_bound(binEdgesZ_.begin(), binEdgesZ_.end(), z);
if (matchz == binEdgesZ_.begin() || matchz == binEdgesZ_.end()) return T(0.0);
int iz = (matchz - binEdgesZ_.begin() - 1);
return values_[ix * binY_ *binZ_ +binZ_*iy + iz];
Expand Down
Loading
Loading