Add codegen support for `VerticalInterpPdf` #1060

runtingt · 2025-03-31T15:22:19Z

Refactors the mathematical details of vertical interpolation into free functions and implements the required translate and impl functions to support the codegen backend

To be merged after #1019

src/VerticalInterpHistPdf.cc

src/classes.h

vgvassilev · 2025-04-07T15:09:27Z

@runtingt, can you rebase this PR?

vgvassilev · 2025-04-08T13:39:02Z

@mcbarton, do you have a clue why clang-tidy does not comment in this PR?

runtingt · 2025-04-08T13:44:54Z

@mcbarton, do you have a clue why clang-tidy does not comment in this PR?

If it helps (as I can't actually seem to find it linked to this PR anywhere under Checks), here's the run: https://github.com/cms-analysis/HiggsAnalysis-CombinedLimit/actions/runs/14328764810/job/40159694791#step:3:654

vgvassilev · 2025-04-08T13:48:56Z

Ok, looks like we hit ZedThree/clang-tidy-review#144

vgvassilev · 2025-04-08T15:25:53Z

@runtingt it was recently fixed and released under 0.21 -- can you update

HiggsAnalysis-CombinedLimit/.github/workflows/clang-tidy-review-post.yml

Line 24 in 228c0a0

uses: ZedThree/clang-tidy-review/[email protected]

as part of this PR to see if it works?

mcbarton · 2025-04-08T15:32:08Z

Not entirely sure why its not commenting from a quick look, since it worked in my dummy PR. One thing which does need to be fixed about the workflow is to activate it for more file changes. If you look here https://github.com/cms-analysis/HiggsAnalysis-CombinedLimit/blob/228c0a0bd2d7729c9d550915e0418f997a4d97ea/.github/workflows/clang-tidy-review.yml#L6C1-L7C16 it only activates when PRs change .cc and .h files, but not for .cxx files.

.github/workflows/clang-tidy-review-post.yml

mcbarton · 2025-04-08T15:51:34Z

The comments are clearly created here https://github.com/cms-analysis/HiggsAnalysis-CombinedLimit/actions/runs/14337808458/job/40189300120?pr=1060#step:7:1168 . No idea why they are not posting. After I allowed actions to run on my fork, all I needed to do was open up the PR for it to work, as can be seen in the dummy PR here mcbarton#2 (review) . My only guess is that the something with the settings of the repo is not quite right. Mine were whatever the default was, plus allowing actions to run.

runtingt · 2025-04-08T15:57:24Z

@mcbarton In the case of your dummy PR, the workflow already exists on the target branch. I think in order to use the patched v0.21.0, this needs to be present on the target branch already

mcbarton · 2025-04-08T16:01:21Z

@mcbarton In the case of your dummy PR, the workflow already exists on the target branch. I think in order to use the patched v0.21.0, this needs to be present on the target branch already

@runtingt The action already exists on the target branch, its just not the latest version. Only way to find out if the target branch has to have the new version for it to work would be to open up a separate PR with the clang tidy fixes, get it merged, and then rebase this PR. I'd also suggest making this change in the clang tidy PR #1060 (comment) , since its another issue with the workflow.

vgvassilev · 2025-04-09T16:18:35Z

interface/CombineMathFuncs.h

+
+   if (fabs(coeff) >= quadraticRegion) {
+      return coeff * (coeff > 0 ? fUp - central : central - fDn);
+   } else {


You can drop the else part because of the return.

Likewise for the other else parts that the if-conditional has a return.

github-actions

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 10 out of 37. Check the log or trigger a new build to see more.

github-actions · 2025-04-09T16:54:54Z

interface/CombineCodegenImpl.h

@@ -2,17 +2,28 @@
 #define HiggsAnalysis_CombinedLimit_CombineCodegenImpl_h

 #include <ROOT/RConfig.hxx> // for ROOT_VERSION
+#include <RooAbsReal.h>
+#include <string>


warning: included header RooAbsReal.h is not used directly [misc-include-cleaner]

Suggested change

#include <string>

#include <string>

github-actions · 2025-04-09T16:54:54Z

interface/CombineCodegenImpl.h

@@ -2,17 +2,28 @@
 #define HiggsAnalysis_CombinedLimit_CombineCodegenImpl_h

 #include <ROOT/RConfig.hxx> // for ROOT_VERSION
+#include <RooAbsReal.h>
+#include <string>



warning: included header string is not used directly [misc-include-cleaner]

Suggested change

github-actions · 2025-04-09T16:54:54Z

interface/CombineMathFuncs.h

@@ -145,6 +150,161 @@ inline double processNormalization(double nominalValue, std::size_t nThetas, std
   return norm;
 }

+// Interpolation  (from VerticalInterpPdf)
+inline Double_t interpolate(Double_t const coeff, Double_t const central, Double_t const fUp, 


warning: no header providing "Double_t" is directly included [misc-include-cleaner]

interface/CombineMathFuncs.h:7:

+ #include <RtypesCore.h>

github-actions · 2025-04-09T16:54:55Z

interface/CombineMathFuncs.h

@@ -145,6 +150,161 @@ inline double processNormalization(double nominalValue, std::size_t nThetas, std
   return norm;
 }

+// Interpolation  (from VerticalInterpPdf)
+inline Double_t interpolate(Double_t const coeff, Double_t const central, Double_t const fUp, 
+                            Double_t const fDn, Double_t const quadraticRegion, Int_t const quadraticAlgo)


warning: no header providing "Int_t" is directly included [misc-include-cleaner]

Double_t const fDn, Double_t const quadraticRegion, Int_t const quadraticAlgo) ^

github-actions · 2025-04-09T16:54:55Z

interface/CombineMathFuncs.h

+      Double_t c_up  = + coeff * (quadraticRegion + coeff) / (2 * quadraticRegion);
+      Double_t c_dn  = - coeff * (quadraticRegion - coeff) / (2 * quadraticRegion);
+      Double_t c_cen = - coeff * coeff / quadraticRegion;
+      return c_up * fUp + c_dn * fDn + c_cen * central;


warning: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]

Suggested change

return c_up * fUp + c_dn * fDn + c_cen * central;

return (c_up * fUp) + c_dn * fDn + c_cen * central;

github-actions · 2025-04-09T16:54:55Z

interface/CombineMathFuncs.h

+      Double_t c_up  = + coeff * (quadraticRegion + coeff) / (2 * quadraticRegion);
+      Double_t c_dn  = - coeff * (quadraticRegion - coeff) / (2 * quadraticRegion);
+      Double_t c_cen = - coeff * coeff / quadraticRegion;
+      return c_up * fUp + c_dn * fDn + c_cen * central;


warning: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]

Suggested change

return c_up * fUp + c_dn * fDn + c_cen * central;

return c_up * fUp + (c_dn * fDn) + c_cen * central;

github-actions · 2025-04-09T16:54:55Z

interface/CombineMathFuncs.h

+      Double_t c_up  = + coeff * (quadraticRegion + coeff) / (2 * quadraticRegion);
+      Double_t c_dn  = - coeff * (quadraticRegion - coeff) / (2 * quadraticRegion);
+      Double_t c_cen = - coeff * coeff / quadraticRegion;
+      return c_up * fUp + c_dn * fDn + c_cen * central;


warning: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]

Suggested change

return c_up * fUp + c_dn * fDn + c_cen * central;

return c_up * fUp + c_dn * fDn + (c_cen * central);

github-actions · 2025-04-09T16:54:55Z

interface/CombineMathFuncs.h

+      Double_t c_up  = (quadraticRegion + coeff) * (quadraticRegion + coeff) / (4 * quadraticRegion);
+      Double_t c_dn  = (quadraticRegion - coeff) * (quadraticRegion - coeff) / (4 * quadraticRegion);
+      Double_t c_cen = - c_up - c_dn;
+      return c_up * fUp + c_dn * fDn + c_cen * central;


warning: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]

return c_up * fUp + c_dn * fDn + c_cen * central; ^

this fix will not be applied because it overlaps with another fix

github-actions · 2025-04-09T16:54:55Z

interface/CombineMathFuncs.h

+      Double_t c_up  = (quadraticRegion + coeff) * (quadraticRegion + coeff) / (4 * quadraticRegion);
+      Double_t c_dn  = (quadraticRegion - coeff) * (quadraticRegion - coeff) / (4 * quadraticRegion);
+      Double_t c_cen = - c_up - c_dn;
+      return c_up * fUp + c_dn * fDn + c_cen * central;


warning: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]

return c_up * fUp + c_dn * fDn + c_cen * central; ^

this fix will not be applied because it overlaps with another fix

github-actions · 2025-04-09T16:54:55Z

interface/CombineMathFuncs.h

+      Double_t c_up  = (quadraticRegion + coeff) * (quadraticRegion + coeff) / (4 * quadraticRegion);
+      Double_t c_dn  = (quadraticRegion - coeff) * (quadraticRegion - coeff) / (4 * quadraticRegion);
+      Double_t c_cen = - c_up - c_dn;
+      return c_up * fUp + c_dn * fDn + c_cen * central;


warning: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]

return c_up * fUp + c_dn * fDn + c_cen * central; ^

this fix will not be applied because it overlaps with another fix

runtingt · 2025-04-09T16:56:08Z

@vgvassilev @mcbarton Looks like updating main worked - I'll go through these suggestions

github-actions

clang-tidy made some suggestions

github-actions · 2025-04-10T11:21:19Z

interface/CombineMathFuncs.h

+                                  Double_t const pdfFloorVal, Double_t const quadraticRegion, Int_t const quadraticAlgo)
+{
+   // Do running sum of coef/func pairs, calculate lastCoef.
+   Double_t central = funcList[0];


warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]

Double_t central = funcList[0]; ^

github-actions · 2025-04-10T11:21:19Z

interface/CombineMathFuncs.h

+   Double_t value = central;
+
+   for (std::size_t iCoef = 0; iCoef < nCoeffs; ++iCoef) {
+      double coefVal = coefList[iCoef];


warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]

double coefVal = coefList[iCoef]; ^

github-actions · 2025-04-10T11:21:19Z

interface/CombineMathFuncs.h

+
+   for (std::size_t iCoef = 0; iCoef < nCoeffs; ++iCoef) {
+      double coefVal = coefList[iCoef];
+      double funcUp = funcList[(2 * iCoef) + 1];


warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]

double funcUp = funcList[(2 * iCoef) + 1]; ^

github-actions · 2025-04-10T11:21:20Z

interface/CombineMathFuncs.h

+   for (std::size_t iCoef = 0; iCoef < nCoeffs; ++iCoef) {
+      double coefVal = coefList[iCoef];
+      double funcUp = funcList[(2 * iCoef) + 1];
+      double funcDn = funcList[(2 * iCoef) + 2];


warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]

double funcDn = funcList[(2 * iCoef) + 2]; ^

github-actions · 2025-04-10T11:21:20Z

interface/CombineMathFuncs.h

+                                  Double_t const pdfFloorVal, Double_t const quadraticRegion, Int_t const quadraticAlgo)
+{
+   // Do running sum of coef/func pairs, calculate lastCoef.
+   Double_t central = funcList[0];


warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]

Double_t central = funcList[0]; ^

github-actions · 2025-04-10T11:21:20Z

interface/CombineMathFuncs.h

+   Double_t value = central;
+
+   for (std::size_t iCoef = 0; iCoef < nCoeffs; ++iCoef) {
+      double coefVal = coefList[iCoef];


warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]

double coefVal = coefList[iCoef]; ^

github-actions · 2025-04-10T11:21:20Z

interface/CombineMathFuncs.h

+
+   for (std::size_t iCoef = 0; iCoef < nCoeffs; ++iCoef) {
+      double coefVal = coefList[iCoef];
+      double funcUp = funcList[(2 * iCoef) + 1];


warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]

double funcUp = funcList[(2 * iCoef) + 1]; ^

github-actions · 2025-04-10T11:21:20Z

interface/CombineMathFuncs.h

+   for (std::size_t iCoef = 0; iCoef < nCoeffs; ++iCoef) {
+      double coefVal = coefList[iCoef];
+      double funcUp = funcList[(2 * iCoef) + 1];
+      double funcDn = funcList[(2 * iCoef) + 2];


warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]

double funcDn = funcList[(2 * iCoef) + 2]; ^

runtingt · 2025-04-10T11:37:27Z

@vgvassilev I fixed up most of the suggestions, with the exception of these do not use pointer arithmetic warnings. I wanted to use a RooArgList instead of these double const* arr arrays, but when creating the NLL object with the codegen backend I get

input_line_152:53:75: error: reference to type 'const RooArgList' could not bind to an lvalue of type 'double[1]'
        const double t15 = RooFit::Detail::MathFuncs::verticalInterpolate(t9, t11, 0.000000, 0.000000, 0);
                                                                          ^~

Am I missing something obvious?

vgvassilev · 2025-04-10T16:19:51Z

@vgvassilev I fixed up most of the suggestions, with the exception of these do not use pointer arithmetic warnings.

Generally a good idea but sometimes it's just too awkward. We have an option to add the // NOLINT(...) clause to suppress them or just suppress them generally in the .clang-tidy file if this is a common development pattern in combine...

I wanted to use a RooArgList instead of these double const* arr arrays, but when creating the NLL object with the codegen backend I get
input_line_152:53:75: error: reference to type 'const RooArgList' could not bind to an lvalue of type 'double[1]'
        const double t15 = RooFit::Detail::MathFuncs::verticalInterpolate(t9, t11, 0.000000, 0.000000, 0);
                                                                          ^~
Am I missing something obvious?

Not obvious to me but may be obvious to @guitargeek...

anigamova · 2025-04-23T15:18:00Z

Hi @runtingt, could you explain a bit why and where were you trying to introduce RooArgLists exactly? Do you mean the funcList and coefList or something else?

runtingt · 2025-04-23T16:50:38Z

Hi @runtingt, could you explain a bit why and where were you trying to introduce RooArgLists exactly? Do you mean the funcList and coefList or something else?

Hi @anigamova, exactly the funcList and coefList (and funcIntList which is functionally the same). These are already RooArgLists (see e.g. here), so it's more a case of trying to use them as part of the AD call.

In order to avoid the above input_line_152:53:75: error: reference to type 'const RooArgList' could not bind to an lvalue of type 'double[1]' (which I believe is from clad), I had to introduce another overload with the more primitive double const* char arrays, done here.

This upsets clang-tidy: warning: do not use pointer arithmetic, but I'm not sure there's a way to avoid both. Does that help at all?

vgvassilev · 2025-04-23T18:05:02Z

In order to avoid the above input_line_152:53:75: error: reference to type 'const RooArgList' could not bind to an lvalue of type 'double[1]' (which I believe is from clad)

Can you open an issue against the clad repository?

cc: @PetroZarytskyi.

runtingt marked this pull request as draft March 31, 2025 15:30

runtingt marked this pull request as ready for review March 31, 2025 15:59

runtingt force-pushed the roofit_ad_dev branch 2 times, most recently from fa79af8 to 73023df Compare April 1, 2025 07:25

vgvassilev reviewed Apr 1, 2025

View reviewed changes

src/VerticalInterpHistPdf.cc Outdated Show resolved Hide resolved

src/classes.h Outdated Show resolved Hide resolved

runtingt added 2 commits April 8, 2025 09:20

Refactor VerticalInterpPdf to enable codegen backend

9ca2150

Remove unused includes

9109701

runtingt force-pushed the roofit_ad_dev branch from eeb8a09 to 9109701 Compare April 8, 2025 08:21

runtingt requested a review from vgvassilev April 8, 2025 11:54

runtingt added 2 commits April 8, 2025 16:31

Merge branch 'cms-analysis:main' into roofit_ad_dev

41415fe

Update clang-tidy-review-post.yml

ccdd6f9

mcbarton reviewed Apr 8, 2025

View reviewed changes

.github/workflows/clang-tidy-review-post.yml Outdated Show resolved Hide resolved

Update clang-tidy-review.yml

d9c40bc

runtingt mentioned this pull request Apr 8, 2025

Update clang-tidy-review version #1068

Merged

vgvassilev reviewed Apr 9, 2025

View reviewed changes

runtingt and others added 2 commits April 9, 2025 17:49

Remove unnecessary else

e4421d1

Merge branch 'cms-analysis:main' into roofit_ad_dev

7af6560

github-actions bot reviewed Apr 9, 2025

View reviewed changes

Apply suggestions from code review

e3cf574

github-actions bot reviewed Apr 10, 2025

View reviewed changes

runtingt mentioned this pull request Apr 23, 2025

Error when trying to differentiate a function which takes RooArgList(s) vgvassilev/clad#1363

Open

	return c_up * fUp + c_dn * fDn + c_cen * central;
	return (c_up * fUp) + c_dn * fDn + c_cen * central;

	return c_up * fUp + c_dn * fDn + c_cen * central;
	return c_up * fUp + (c_dn * fDn) + c_cen * central;

	return c_up * fUp + c_dn * fDn + c_cen * central;
	return c_up * fUp + c_dn * fDn + (c_cen * central);

Add codegen support for VerticalInterpPdf #1060

Are you sure you want to change the base?

Add codegen support for VerticalInterpPdf #1060

Conversation

runtingt commented Mar 31, 2025 • edited Loading

vgvassilev commented Apr 7, 2025

vgvassilev commented Apr 8, 2025

runtingt commented Apr 8, 2025

vgvassilev commented Apr 8, 2025

vgvassilev commented Apr 8, 2025

mcbarton commented Apr 8, 2025

mcbarton commented Apr 8, 2025

runtingt commented Apr 8, 2025

mcbarton commented Apr 8, 2025

vgvassilev Apr 9, 2025

Choose a reason for hiding this comment

vgvassilev Apr 9, 2025

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Apr 9, 2025

Choose a reason for hiding this comment

github-actions bot Apr 9, 2025

Choose a reason for hiding this comment

github-actions bot Apr 9, 2025

Choose a reason for hiding this comment

github-actions bot Apr 9, 2025

Choose a reason for hiding this comment

github-actions bot Apr 9, 2025

Choose a reason for hiding this comment

github-actions bot Apr 9, 2025

Choose a reason for hiding this comment

github-actions bot Apr 9, 2025

Choose a reason for hiding this comment

github-actions bot Apr 9, 2025

Choose a reason for hiding this comment

github-actions bot Apr 9, 2025

Choose a reason for hiding this comment

github-actions bot Apr 9, 2025

Choose a reason for hiding this comment

runtingt commented Apr 9, 2025

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Apr 10, 2025

Choose a reason for hiding this comment

github-actions bot Apr 10, 2025

Choose a reason for hiding this comment

github-actions bot Apr 10, 2025

Choose a reason for hiding this comment

github-actions bot Apr 10, 2025

Choose a reason for hiding this comment

github-actions bot Apr 10, 2025

Choose a reason for hiding this comment

github-actions bot Apr 10, 2025

Choose a reason for hiding this comment

github-actions bot Apr 10, 2025

Choose a reason for hiding this comment

github-actions bot Apr 10, 2025

Choose a reason for hiding this comment

runtingt commented Apr 10, 2025

vgvassilev commented Apr 10, 2025

anigamova commented Apr 23, 2025

runtingt commented Apr 23, 2025

vgvassilev commented Apr 23, 2025

Add codegen support for `VerticalInterpPdf` #1060

Add codegen support for `VerticalInterpPdf` #1060

runtingt commented Mar 31, 2025 •

edited

Loading