Skip to content
Closed
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
174 changes: 49 additions & 125 deletions interface/RooBernsteinFast.h
Original file line number Diff line number Diff line change
@@ -1,146 +1,70 @@
/*****************************************************************************
* RooBernsteinFast
* Josh Bendavid (CERN)
* Fast templated version of RooBernstein class using SMatrices
*
*
*
*
*****************************************************************************/
#ifndef ROO_BERNSTEINFAST
#define ROO_BERNSTEINFAST

#include "RooAbsPdf.h"
#include "RooRealProxy.h"
#include "RooBernstein.h"
#include "RooListProxy.h"
#include "RooChangeTracker.h"
#include "TMath.h"
#include "Math/SMatrix.h"
#include "RooRealConstant.h"
#include "RooAbsReal.h"
#include "TClass.h"
#include "TDataMember.h"

class RooRealVar;
class RooArgList ;

template<int N> class RooBernsteinFast : public RooAbsPdf {
template<int N> class RooBernsteinFast : public RooBernstein {
public:

RooBernsteinFast() = default;


RooBernsteinFast() {}

RooBernsteinFast(const char *name, const char *title,
RooAbsReal& x, const RooArgList& coefList) :
RooAbsPdf(name, title),
_x("x", "Dependent", this, x),
_coefList("coefList","List of coefficients",this)
{
_coefList.add(coefList);


//precompute coefficients for integral
for (int ipow=0; ipow<=N; ++ipow) {
_rvector(ipow) = 1.0/((double)ipow+1.0);
}
RooAbsReal& x, const RooArgList& coefList) :
RooBernstein(name, title,
dynamic_cast<RooAbsRealLValue&>(x),
buildFullList(coefList))
{}
Comment on lines 16 to +21

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:

find . -name "RooBernsteinFast.h" -type f

Repository: cms-analysis/HiggsAnalysis-CombinedLimit

Length of output: 115


🏁 Script executed:

find . -name "RooBernstein.h" -type f

Repository: cms-analysis/HiggsAnalysis-CombinedLimit

Length of output: 66


🏁 Script executed:

cat -n ./interface/RooBernsteinFast.h

Repository: cms-analysis/HiggsAnalysis-CombinedLimit

Length of output: 2698


🏁 Script executed:

rg "RooBernsteinFast\s*<" --type cpp --type h -A 3 -B 1

Repository: cms-analysis/HiggsAnalysis-CombinedLimit

Length of output: 399


🏁 Script executed:

rg "RooBernsteinFast\(" --type cpp --type h -B 2 -A 5

Repository: cms-analysis/HiggsAnalysis-CombinedLimit

Length of output: 666


🏁 Script executed:

find . -name "*.cc" -o -name "*.cpp" | head -20

Repository: cms-analysis/HiggsAnalysis-CombinedLimit

Length of output: 574


🏁 Script executed:

rg "new RooBernsteinFast|make.*RooBernsteinFast" --type cpp

Repository: cms-analysis/HiggsAnalysis-CombinedLimit

Length of output: 66


🏁 Script executed:

rg "RooBernsteinFast" --type cpp --type h

Repository: cms-analysis/HiggsAnalysis-CombinedLimit

Length of output: 1091


🏁 Script executed:

cat -n ./src/RooBernsteinFast.cc

Repository: cms-analysis/HiggsAnalysis-CombinedLimit

Length of output: 223


Tighten the observable type instead of failing with std::bad_cast.

This constructor accepts any RooAbsReal&, but Line 19 immediately requires RooAbsRealLValue&. That turns an API mismatch into a runtime failure for callers that pass a derived expression rather than an lvalue observable. Change the signature to RooAbsRealLValue& and remove the unnecessary dynamic_cast.

Suggested fix
   RooBernsteinFast(const char *name, const char *title,
-                   RooAbsReal& x, const RooArgList& coefList) :
+                   RooAbsRealLValue& x, const RooArgList& coefList) :
     RooBernstein(name, title,
-                 dynamic_cast<RooAbsRealLValue&>(x),
+                 x,
                  buildFullList(coefList))
   {}
📝 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
RooBernsteinFast(const char *name, const char *title,
RooAbsReal& x, const RooArgList& coefList) :
RooAbsPdf(name, title),
_x("x", "Dependent", this, x),
_coefList("coefList","List of coefficients",this)
{
_coefList.add(coefList);
//precompute coefficients for integral
for (int ipow=0; ipow<=N; ++ipow) {
_rvector(ipow) = 1.0/((double)ipow+1.0);
}
RooAbsReal& x, const RooArgList& coefList) :
RooBernstein(name, title,
dynamic_cast<RooAbsRealLValue&>(x),
buildFullList(coefList))
{}
RooBernsteinFast(const char *name, const char *title,
RooAbsRealLValue& x, const RooArgList& coefList) :
RooBernstein(name, title,
x,
buildFullList(coefList))
{}
🤖 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/RooBernsteinFast.h` around lines 16 - 21, The constructor
RooBernsteinFast currently takes RooAbsReal& but immediately dynamic_casts to
RooAbsRealLValue&, risking std::bad_cast; change the constructor signature to
accept RooAbsRealLValue& (i.e., RooBernsteinFast(const char* name, const char*
title, RooAbsRealLValue& x, const RooArgList& coefList)) and remove the
dynamic_cast in the initializer where RooBernstein is called (use x directly) so
the API requires an lvalue observable and avoids the runtime cast.


//precompute coefficients for conversion from bernstein basis to power basis
for (int ibern=0; ibern<=N; ++ibern) {
for (int ipow=0; ipow<ibern; ++ipow) {
_cmatrix(ipow,ibern) = 0.;
}
for (int ipow=ibern; ipow<=N; ++ipow) {
_cmatrix(ipow, ibern) = pow(-1.,ipow-ibern)*TMath::Binomial(N,ipow)*TMath::Binomial(ipow,ibern);
}
}

}

ClassDefOverride(RooBernsteinFast, 2)
Comment on lines +11 to +23

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:

# First, locate the RooBernstein class definition
find . -name "*.h" -o -name "*.cpp" | head -20

Repository: cms-analysis/HiggsAnalysis-CombinedLimit

Length of output: 638


🏁 Script executed:

# Search for RooBernstein class definition
rg "class RooBernstein" -A 5

Repository: cms-analysis/HiggsAnalysis-CombinedLimit

Length of output: 419


🏁 Script executed:

# Look for clone() method in RooBernstein
rg "clone\(" --type-list | head -5
rg -t cpp -t h "clone\(" -B 2 -A 2 | grep -E "(RooBernstein|clone)" | head -30

Repository: cms-analysis/HiggsAnalysis-CombinedLimit

Length of output: 3479


🏁 Script executed:

# Find where RooBernstein might be defined
fd "RooBernstein" --type f

Repository: cms-analysis/HiggsAnalysis-CombinedLimit

Length of output: 137


🏁 Script executed:

# Check the implementation file
cat -n src/RooBernsteinFast.cc

Repository: cms-analysis/HiggsAnalysis-CombinedLimit

Length of output: 223


🏁 Script executed:

# Search for usages of RooBernsteinFast to understand workflow
rg "RooBernsteinFast" -B 2 -A 2

Repository: cms-analysis/HiggsAnalysis-CombinedLimit

Length of output: 3462


🏁 Script executed:

# Check if there are other templated RooFit classes and how they handle clone()
rg "template.*class.*Roo" -A 15 interface/ | head -80

Repository: cms-analysis/HiggsAnalysis-CombinedLimit

Length of output: 2182


Restore clone() so copies stay RooBernsteinFast<N>.

Dropping the override here means RooFit copy/import paths will fall back to RooBernstein::clone(), which produces a plain RooBernstein. Since RooBernsteinFast is registered for ROOT I/O persistence with custom Streamer logic for schema migration, type preservation is critical—returning the base type breaks type fidelity during cloning and serialization. Reintroducing the copy ctor + clone() override keeps the derived type intact, consistent with patterns used elsewhere in the codebase (e.g., FastTemplateFunc_t).

Possible localized fix
 public:
   RooBernsteinFast() = default;
+
+  RooBernsteinFast(const RooBernsteinFast& other, const char *name = nullptr)
+      : RooBernstein(other, name) {}
 
   RooBernsteinFast(const char *name, const char *title,
                    RooAbsReal& x, const RooArgList& coefList) :
     RooBernstein(name, title,
                  dynamic_cast<RooAbsRealLValue&>(x),
                  buildFullList(coefList))
   {}
+
+  TObject *clone(const char *newname) const override {
+    return new RooBernsteinFast(*this, newname);
+  }
 
   ClassDefOverride(RooBernsteinFast, 2)
📝 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
template<int N> class RooBernsteinFast : public RooBernstein {
public:
RooBernsteinFast() = default;
RooBernsteinFast() {}
RooBernsteinFast(const char *name, const char *title,
RooAbsReal& x, const RooArgList& coefList) :
RooAbsPdf(name, title),
_x("x", "Dependent", this, x),
_coefList("coefList","List of coefficients",this)
{
_coefList.add(coefList);
//precompute coefficients for integral
for (int ipow=0; ipow<=N; ++ipow) {
_rvector(ipow) = 1.0/((double)ipow+1.0);
}
RooAbsReal& x, const RooArgList& coefList) :
RooBernstein(name, title,
dynamic_cast<RooAbsRealLValue&>(x),
buildFullList(coefList))
{}
//precompute coefficients for conversion from bernstein basis to power basis
for (int ibern=0; ibern<=N; ++ibern) {
for (int ipow=0; ipow<ibern; ++ipow) {
_cmatrix(ipow,ibern) = 0.;
}
for (int ipow=ibern; ipow<=N; ++ipow) {
_cmatrix(ipow, ibern) = pow(-1.,ipow-ibern)*TMath::Binomial(N,ipow)*TMath::Binomial(ipow,ibern);
}
}
}
ClassDefOverride(RooBernsteinFast, 2)
template<int N> class RooBernsteinFast : public RooBernstein {
public:
RooBernsteinFast() = default;
RooBernsteinFast(const RooBernsteinFast& other, const char *name = nullptr)
: RooBernstein(other, name) {}
RooBernsteinFast(const char *name, const char *title,
RooAbsReal& x, const RooArgList& coefList) :
RooBernstein(name, title,
dynamic_cast<RooAbsRealLValue&>(x),
buildFullList(coefList))
{}
TObject *clone(const char *newname) const override {
return new RooBernsteinFast(*this, newname);
}
ClassDefOverride(RooBernsteinFast, 2)
🤖 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/RooBernsteinFast.h` around lines 11 - 23, The RooBernsteinFast<N>
class must reintroduce a copy constructor and override clone() so copies
preserve the RooBernsteinFast<N> dynamic type; add a copy ctor (e.g.,
RooBernsteinFast(const RooBernsteinFast& other)) and implement RooAbsArg*
clone(const char* newname) const override to return new
RooBernsteinFast<N>(*this) (with renamed instance if needed), ensuring the
class-specific Streamer/IO behavior is retained instead of falling back to
RooBernstein::clone; target the RooBernsteinFast template class and its
clone/copy ctor symbols when making the change.


RooBernsteinFast(const RooBernsteinFast& other, const char* name = 0) :
RooAbsPdf(other, name),
_x("x", this, other._x),
_coefList("coefList",this,other._coefList),
_cmatrix(other._cmatrix),
_rvector(other._rvector),
_bernvector(other._bernvector),
_powvector(other._powvector),
_xvector(other._xvector) {}


TObject* clone(const char* newname) const override { return new RooBernsteinFast(*this, newname); }
~RooBernsteinFast() override { }


Int_t getAnalyticalIntegral(RooArgSet& allVars, RooArgSet& analVars, const char* rangeName=0) const override
{

// No analytical calculation available (yet) of integrals over subranges (as for standard RooBernstein)
if (rangeName && strlen(rangeName)) {
return 0 ;
}

if (matchArgs(allVars, analVars, _x)) return 1;
return 0;
}


Double_t analyticalIntegral(Int_t code, const char* rangeName=0) const override
{
private:

_bernvector[0] = 1.0;
for (int ipow=1; ipow<=N; ++ipow) {
_bernvector[ipow] = static_cast<RooAbsReal*>(_coefList.at(ipow-1))->getVal();
}

_powvector = _cmatrix*_bernvector;

double xmin = _x.min();
double xmax = _x.max();
return (xmax-xmin)*ROOT::Math::Dot(_powvector,_rvector);
// RooBernstein expects N+1 coefficients; RooBernsteinFast fixed c0=1 internally,
// so we prepend a constant 1.0 to the user-supplied N coefficients.
static RooArgList buildFullList(const RooArgList& coefList) {
RooArgList full;
full.add(RooRealConstant::value(1.0));
full.add(coefList);
return full;
}

}
};

protected:
// Custom Streamer: handles reading old (v1) files where RooBernsteinFast
// inherited from RooAbsPdf and stored N coefficients (c0=1 was implicit).
template<int N>
void RooBernsteinFast<N>::Streamer(TBuffer &R__b) {
if (R__b.IsReading()) {
UInt_t R__s, R__c;
Version_t R__v = R__b.ReadVersion(&R__s, &R__c);
R__b.ReadClassBuffer(RooBernsteinFast::Class(), this, R__v, R__s, R__c);

typedef ROOT::Math::SMatrix<double,N+1,N+1,ROOT::Math::MatRepStd<double,N+1,N+1> > MType;
typedef ROOT::Math::SVector<double,N+1> VType;

RooRealProxy _x;
RooListProxy _coefList ;
MType _cmatrix; //conversion matrix between bernstein and power bases
VType _rvector; //vector of integration coefficients
mutable VType _bernvector; //coefficients in bernstein basis
mutable VType _powvector; //coefficients in power basis
mutable VType _xvector; //vector of powers of x variable

Double_t evaluate() const override
{
if (R__v < 2) {
// Old version stored N coefficients (c0=1 was implicit).
// RooBernstein expects N+1 explicit coefficients. Prepend 1.0.
TClass *cl = TClass::GetClass("RooBernstein");
TDataMember *dm = cl ? cl->GetDataMember("_coefList") : nullptr;
if (dm) {
auto &proxy = *reinterpret_cast<RooListProxy *>(
reinterpret_cast<char *>(static_cast<RooBernstein *>(this)) + dm->GetOffset());

_bernvector[0] = 1.0;
bool changed = false;
for (int ipow=1; ipow<=N; ++ipow) {
double rval = static_cast<RooAbsReal*>(_coefList.at(ipow-1))->getVal();
if (_bernvector[ipow] != rval) {
_bernvector[ipow] = rval;
changed = true;
if (proxy.size() == static_cast<std::size_t>(N)) {
RooArgList updated;
updated.add(RooRealConstant::value(1.0));
updated.add(proxy);
proxy.removeAll();
proxy.add(updated);
}
}

if (changed) {
_powvector = _cmatrix*_bernvector;
}

double xmin = _x.min();
double xmax = _x.max();
double x = (_x - xmin) / (xmax - xmin); // rescale to [0,1]
_xvector[0] = 1.;
for (int ipow=1; ipow<=N; ++ipow) {
_xvector[ipow] = x*_xvector[ipow-1];
}

return ROOT::Math::Dot(_powvector,_xvector);

}

ClassDefOverride(RooBernsteinFast,1) // Polynomial PDF
};
} else {
R__b.WriteClassBuffer(RooBernsteinFast::Class(), this);
}
}

#endif
41 changes: 1 addition & 40 deletions src/RooBernsteinFast.cc
Original file line number Diff line number Diff line change
@@ -1,44 +1,5 @@
/*****************************************************************************
* Project: RooFit *
* Package: RooFitModels *
* @(#)root/roofit:$Id: RooBernsteinFast.cxx 44507 2012-06-04 12:30:41Z axel $
* Authors: *
* WV, Wouter Verkerke, UC Santa Barbara, verkerke@slac.stanford.edu *
* DK, David Kirkby, UC Irvine, dkirkby@uci.edu *
* *
* Copyright (c) 2000-2005, Regents of the University of California *
* and Stanford University. All rights reserved. *
* *
* Redistribution and use in source and binary forms, *
* with or without modification, are permitted according to the terms *
* listed in LICENSE (http://roofit.sourceforge.net/license.txt) *
*****************************************************************************/

//////////////////////////////////////////////////////////////////////////////
//
// BEGIN_HTML
// RooBernsteinFast implements a polynomial p.d.f of the form
// <pre>
// f(x) = sum_i a_i * x^i
//</pre>
// By default coefficient a_0 is chosen to be 1, as polynomial
// probability density functions have one degree of freedome
// less than polynomial functions due to the normalization condition
// END_HTML
//

#include "RooFit.h"

#include "Riostream.h"
#include "Riostream.h"
#include "TMath.h"

#include "../interface/RooBernsteinFast.h"
#include "RooAbsReal.h"
#include "RooRealVar.h"
#include "RooArgList.h"

using namespace std;
using namespace RooFit;

templateClassImp(RooBernsteinCoeffs)
templateClassImp(RooBernsteinFast)
Loading