Add non-uniform binning support for fastVerticalInterpHistPdf2 codegen implementation #1188
Conversation
WalkthroughModified binning logic in code generation to support both uniform and non-uniform distributions. Implemented conditional pathways: uniform binning uses the fast uniformBinNumber path, while non-uniform binning generates static bin edge arrays and integrates with ROOT's rawBinNumber function. Applied changes to 1D and 2D interpolation handlers. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
🔇 Additional comments (2)
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}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/CombineCodegenImpl.cxx (1)
155-155: Critical bug: Incorrect total bin count calculation.
numBinsY * numBinsYshould benumBinsX * numBinsY. This typo will cause incorrect array sizing and out-of-bounds access when X and Y have different bin counts.- int numBins = numBinsY * numBinsY; + int numBins = numBinsX * numBinsY;
🧹 Nitpick comments (2)
src/CombineCodegenImpl.cxx (2)
183-237: Correct dual-path binning for 2D axes; consider extracting helper to reduce duplication.The binning logic for X and Y axes is implemented correctly. However, the bin edge array generation code (constructing static array, formatting to stringstream, calling
addToCodeBody) is now repeated three times across 1D and 2D implementations.Consider extracting a helper function like:
std::string emitBinEdgesArray(CodegenContext& ctx, const RooAbsBinning& binning, int numBins) { std::vector<double> binEdges(numBins + 1); for (int i = 0; i < numBins; ++i) { binEdges[i] = binning.binLow(i); } binEdges[numBins] = binning.binHigh(numBins - 1); std::string arrayName = ctx.getTmpVarName(); std::stringstream code; code << "static const double " << arrayName << "[] = {"; for (int i = 0; i <= numBins; ++i) { if (i > 0) code << ", "; code << binEdges[i]; } code << "};\n"; ctx.addToCodeBody(code.str(), true); return arrayName; }This would reduce duplication across lines 93-109, 192-207, and 220-235.
239-240: Minor inconsistency: UsenumBinsYfor consistency.Line 240 uses
yVar.numBins()while the rest of the function usesnumBinsY(fromcacheNominal.binY()). For consistency and to avoid potential discrepancies, consider using the already-computed variable.- binIdx << "(" << binIdxY << " + " << yVar.numBins() << " * " << binIdxX << ")"; + binIdx << "(" << binIdxY << " + " << numBinsY << " * " << binIdxX << ")";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/CombineCodegenImpl.cxx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: dev3/latest - ROOT LCG master
- GitHub Check: CMSSW_14_1_0_pre4 - ROOT 6.30.07
- GitHub Check: CMSSW_11_3_4 - ROOT 6.22.09
- GitHub Check: CMSSW_14_0_0_pre1 - ROOT 6.26.11
- GitHub Check: LCG_106 - ROOT 6.32.02
- GitHub Check: LCG_102 - ROOT 6.26.04
- GitHub Check: LCG_108 - ROOT 6.36.02
- GitHub Check: Compile (py3.10, root6.32.2)
- GitHub Check: Compile (py3.12, root6.34.4)
- GitHub Check: Compile (py3.10, root6.26.4)
🔇 Additional comments (1)
src/CombineCodegenImpl.cxx (1)
81-113: Implementation correctly uses both binning functions with proper signatures.The dual-path binning approach is sound and verified:
Uniform binning correctly calls
uniformBinNumber(xLow, xHigh, xVar, numBins, 1.0)matching the signatureuniformBinNumber(double low, double high, double val, unsigned int numBins, double coef).Non-uniform binning correctly calls
rawBinNumber(xVar, binEdgesArrayName, numBins + 1)matching the signaturerawBinNumber(double x, double const *boundaries, std::size_t nBoundaries). The static bin edges array is properly generated with all boundary points (bin lows plus the final high), and the variable name is correctly passed as a C++ identifier in the generated code.The bin edge extraction logic correctly captures all boundaries:
binLow(i)for each bin plusbinHigh(numBins - 1)for the final edge.
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (0.00%) 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 #1188 +/- ##
==========================================
- Coverage 22.25% 22.23% -0.02%
==========================================
Files 195 195
Lines 26154 26172 +18
Branches 3884 3887 +3
==========================================
Hits 5820 5820
- Misses 20334 20352 +18
🚀 New features to boost your workflow:
|
src/CombineCodegenImpl.cxx
Outdated
| ctx.addToCodeBody(binEdgesCode.str(), true); | ||
|
|
||
| // Call ROOT's rawBinNumber for non-uniform bin finding | ||
| binIdx = ctx.buildCall("RooFit::Detail::MathFuncs::rawBinNumber", xVar, binEdgesArrayName, numBins + 1); |
There was a problem hiding this comment.
| binIdx = ctx.buildCall("RooFit::Detail::MathFuncs::rawBinNumber", xVar, binEdgesArrayName, numBins + 1); | |
| binIdx = ctx.buildCall("RooFit::Detail::MathFuncs::rawBinNumber", xVar, binEdges, numBins + 1); |
Can you try to use the std::vector<double> binEdges directly to build the function call here?
The code generation context will generate the code with the copied values implicitly then, and also take care of passing the right variable name.
src/CombineCodegenImpl.cxx
Outdated
| binEdgesCodeY << "};\n"; | ||
|
|
||
| ctx.addToCodeBody(binEdgesCodeY.str(), true); | ||
| binIdxY = ctx.buildCall("RooFit::Detail::MathFuncs::rawBinNumber", arg.y(), binEdgesArrayNameY, numBinsY + 1); |
There was a problem hiding this comment.
| binIdxY = ctx.buildCall("RooFit::Detail::MathFuncs::rawBinNumber", arg.y(), binEdgesArrayNameY, numBinsY + 1); | |
| binIdxY = ctx.buildCall("RooFit::Detail::MathFuncs::rawBinNumber", arg.y(), binEdgesY, numBinsY + 1); |
Same here
src/CombineCodegenImpl.cxx
Outdated
| binEdgesCodeX << "};\n"; | ||
|
|
||
| ctx.addToCodeBody(binEdgesCodeX.str(), true); | ||
| binIdxX = ctx.buildCall("RooFit::Detail::MathFuncs::rawBinNumber", arg.x(), binEdgesArrayNameX, numBinsX + 1); |
There was a problem hiding this comment.
| binIdxX = ctx.buildCall("RooFit::Detail::MathFuncs::rawBinNumber", arg.x(), binEdgesArrayNameX, numBinsX + 1); | |
| binIdxX = ctx.buildCall("RooFit::Detail::MathFuncs::rawBinNumber", arg.x(), binEdgesX, numBinsX + 1); |
And here
guitargeek
left a comment
guitargeek
left a comment
There was a problem hiding this comment.
Thank you very much, looks very correct!
I just made a suggestion about avoiding to manually generate some code. You can directly use the standard vectors with the bin edges when building the function call. The code generation context will copy the values to the generated code for you.
maxgalli
force-pushed
the
fast_vertical
branch
from
612c77f to
dc559f6
Compare
guitargeek
left a comment
guitargeek
left a comment
There was a problem hiding this comment.
Thank you very much!
Thanks a lot, I updated it now |
I was thinking about it. In principle, this is already part of the test that runs here: until now, it failed with the error raised in these lines, whereas now it will fail because codegen backend for |
|
OK, I see. We just have to work on the AD tests, and I think ideally we need both combine workflow tests and individual functions, but of course it will take time and probably not super relevant for this PR |
3 participants
This PR extends the codegen implementation of
FastVerticalInterpHistPdf2andFastVerticalInterpHistPdf2D2by providing support for non-uniform binning. This is done using therawBinNumberfunction in Roofit (link).Once this is merged, we will be able to test the codegen backend for objects of type
RooParametricHistwithout using the--use-HistPdfkeyword when runningtext2workspace.py.Summary by CodeRabbit
New Features
Performance
✏️ Tip: You can customize this high-level summary in your review settings.