Skip to content

Schema evolution for RooDoubleCBFast #1235

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 3 commits into
base: main
Choose a base branch
from
Draft

Schema evolution for RooDoubleCBFast #1235

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
840f650
Schema evolution for RooDoubleCBFast
runtingt Feb 23, 2026
0acdb44
Fix schema evolution read rule for RooDoubleCBFast to use placement new
maxgalli Feb 20, 2026
fe83000
implement Tom's comments
maxgalli Mar 19, 2026
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
28 changes: 4 additions & 24 deletions interface/RooDoubleCBFast.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
#ifndef ROODOUBLECB
#define ROODOUBLECB

#include "RooAbsPdf.h"
#include "RooRealProxy.h"
#include "RooCrystalBall.h"
#include "RooAbsReal.h"

class RooDoubleCBFast : public RooAbsPdf {
class RooDoubleCBFast : public RooCrystalBall {
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Feb 23, 2026

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

🌐 Web query:

ROOT RooCrystalBall final declaration

💡 Result:

In ROOT/RooFit, RooCrystalBall is declared as a final C++ class in its header:

class RooCrystalBall final : public RooAbsPdf {

This final specifier means you cannot derive/inherit from RooCrystalBall (i.e., class MyPdf : public RooCrystalBall { ... } is not allowed). [1], [2]

Sources: ROOT v6.34.10 reference + header source. [1], [2]

References

  • [1] ROOT docs: RooCrystalBall header source (RooCrystalBall.h, line 11) (root.cern.ch)
  • [2] ROOT docs: RooCrystalBall class reference (shows “Class Reference final”) (root.cern.ch)

Citations:

🏁 Script executed:

# Check if RooCrystalBall.h exists in the repository
find . -name "RooCrystalBall.h" -o -name "*RooCrystalBall*" 2>/dev/null | head -20

Repository: cms-analysis/HiggsAnalysis-CombinedLimit

Length of output: 66

RooCrystalBall is declared final — inheritance is illegal.

RooCrystalBall is marked final in ROOT (v6.34.10+), so class RooDoubleCBFast : public RooCrystalBall will not compile. This blocks the PR.

Possible paths forward:

  1. Composition over inheritance: Inherit from RooAbsPdf and internally delegate to a RooCrystalBall instance for evaluation and integration.
  2. Inherit from a non-final ancestor: Inherit directly from RooAbsPdf and replicate or call into the crystal ball logic.
  3. Upstream change: Request ROOT to remove final from RooCrystalBall (outside your control).
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@interface/RooDoubleCBFast.h` at line 7, RooCrystalBall is final so
RooDoubleCBFast cannot inherit from it; change RooDoubleCBFast to inherit from
RooAbsPdf instead and implement/override the required RooAbsPdf interface
methods (ctor, copy ctor, evaluate, clone,
getAnalyticalIntegral/analyticalIntegral or evaluateOnGrid as needed), and add
one or two internal RooCrystalBall member(s) (e.g., cb1, cb2) to which you
delegate the actual PDF evaluation and integration logic; update constructors to
initialize these internal RooCrystalBall instances and forward parameter/state
changes to them so RooDoubleCBFast composes RooCrystalBall rather than
inheriting from it.

public:
RooDoubleCBFast();
RooDoubleCBFast() = default;
RooDoubleCBFast(const char *name, const char *title,
RooAbsReal& _x,
RooAbsReal& _mean,
Expand All @@ -17,26 +16,7 @@ class RooDoubleCBFast : public RooAbsPdf {
RooAbsReal& _alpha2,
RooAbsReal& _n2
);
RooDoubleCBFast(const RooDoubleCBFast& other, const char* name=0) ;
TObject* clone(const char* newname) const override { return new RooDoubleCBFast(*this,newname); }
inline ~RooDoubleCBFast() override { }
Int_t getAnalyticalIntegral(RooArgSet& allVars, RooArgSet& analVars, const char* rangeName=0) const override ;
Double_t analyticalIntegral(Int_t code, const char* rangeName=0) const override ;

protected:

RooRealProxy x ;
RooRealProxy mean;
RooRealProxy width;
RooRealProxy alpha1;
RooRealProxy n1;
RooRealProxy alpha2;
RooRealProxy n2;

Double_t evaluate() const override ;

private:

ClassDefOverride(RooDoubleCBFast,1)
ClassDefOverride(RooDoubleCBFast, 2)
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Feb 23, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

ClassDefOverride fails as a consequence of the final base class.

ClassDefOverride expands to override on internal virtual methods inherited from the base. Since RooCrystalBall is final (no derivation allowed), these override specifiers have no valid virtual base methods to override. Once the inheritance issue on Line 7 is resolved, this line should be revisited: if the new base class is RooAbsPdf (or another non-final class), ClassDefOverride is correct; if RooDoubleCBFast itself becomes a root of its hierarchy, use ClassDef instead.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@interface/RooDoubleCBFast.h` at line 20, The ClassDefOverride macro on
RooDoubleCBFast is invalid if its base (currently RooCrystalBall) is final;
decide whether RooDoubleCBFast will inherit from a non-final base (e.g.
RooAbsPdf) or be a root class, then replace ClassDefOverride(RooDoubleCBFast, 2)
accordingly: if the new base is non-final keep ClassDefOverride, otherwise
change to ClassDef(RooDoubleCBFast, 2); ensure the chosen macro matches the
inheritance of RooDoubleCBFast and update any related includes or forward
declarations for RooCrystalBall/RooAbsPdf as needed.

};
#endif
196 changes: 3 additions & 193 deletions src/RooDoubleCBFast.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,11 @@
#include <iostream>
#include <math.h>
#include "TMath.h"

#include "../interface/RooDoubleCBFast.h"
#include "RooRealVar.h"
#include "RooRealConstant.h"
#include "./MathHeaders.h"
#include "RooAbsReal.h"
#include "RooAbsPdf.h"

using namespace RooFit;

ClassImp(RooDoubleCBFast)

RooDoubleCBFast::RooDoubleCBFast(){}

RooDoubleCBFast::RooDoubleCBFast(const char *name, const char *title,
RooAbsReal& _x,
RooAbsReal& _mean,
Expand All @@ -22,191 +15,8 @@ using namespace RooFit;
RooAbsReal& _alpha2,
RooAbsReal& _n2
) :
RooAbsPdf(name,title),
x("x","x",this,_x),
mean("mean","mean",this,_mean),
width("width","width",this,_width),
alpha1("alpha1","alpha1",this,_alpha1),
n1("n1","n1",this,_n1),
alpha2("alpha2","alpha2",this,_alpha2),
n2("n2","n2",this,_n2)
{
}


RooDoubleCBFast::RooDoubleCBFast(const RooDoubleCBFast& other, const char* name) :
RooAbsPdf(other,name),
x("x",this,other.x),
mean("mean",this,other.mean),
width("width",this,other.width),
alpha1("alpha1",this,other.alpha1),
n1("n1",this,other.n1),
alpha2("alpha2",this,other.alpha2),
n2("n2",this,other.n2)

{
}

double RooDoubleCBFast::evaluate() const
RooCrystalBall(name, title, _x, _mean, _width, _alpha1, _n1, _alpha2, _n2)
{
double t = (x-mean)*my_inv(width);
double val = -99.;
if(t>-alpha1 && t<alpha2){
val = my_exp(-0.5*t*t);
}else if(t<=-alpha1){
double alpha1invn1 = alpha1*my_inv(n1);
val = my_exp(-0.5*alpha1*alpha1)*gbrmath::fast_pow(1. - alpha1invn1*(alpha1+t), -n1);

// double n1invalpha1 = n1*vdt::fast_inv(fabs(alpha1));
// double A1 = gbrmath::fast_pow(n1invalpha1,n1)*vdt::fast_exp(-alpha1*alpha1/2.);
// double B1 = n1invalpha1-fabs(alpha1);
// val = A1*gbrmath::fast_pow(B1-t,-n1);
}else if(t>=alpha2){
double alpha2invn2 = alpha2*my_inv(n2);
val = my_exp(-0.5*alpha2*alpha2)*gbrmath::fast_pow(1. - alpha2invn2*(alpha2-t), -n2);

// double n2invalpha2 = n2*vdt::fast_inv(fabs(alpha2));
// double A2 = gbrmath::fast_pow(n2invalpha2,n2)*vdt::fast_exp(-alpha2*alpha2/2.);
// double B2 = n2invalpha2-fabs(alpha2);
// val = A2*gbrmath::fast_pow(B2+t,-n2);
}//else{
//cout << "ERROR evaluating range..." << endl;

if (!std::isnormal(val)) {
printf("bad val: x = %5f, t = %5f, mean = %5f, sigma = %5f, alpha1 = %5f, n1 = %5f, alpha2 = %5f, n2 = %5f\n",double(x), t, double(mean),double(width),double(alpha1),double(n1),double(alpha2), double(n2));
printf("val = %5f\n",val);
}

return val;
//return std::max(double(std::numeric_limits<float>::min()),val);
//return std::max(1e-3,val);
//}

}

Int_t RooDoubleCBFast::getAnalyticalIntegral(RooArgSet& allVars, RooArgSet& analVars, const char* range) const
{
if (matchArgs(allVars,analVars,x)) return 1;
return 0;
}

Double_t RooDoubleCBFast::analyticalIntegral(Int_t code, const char* rangeName) const
{
assert(code==1) ;

double central=0;
double left=0;
double right=0;

double xmin = x.min(rangeName);
double xmax = x.max(rangeName);

//printf("xmin = %5e, xmax = %5e\n",xmin,xmax);

//bool isfullrange = xmin<=-RooNumber::infinity() && xmax>=RooNumber::infinity();

static const double rootPiBy2 = sqrt(atan2(0.0,-1.0)/2.0);
static const double invRoot2 = 1.0/sqrt(2);

double invwidth = my_inv(width);

double tmin = (xmin-mean)*invwidth;
double tmax = (xmax-mean)*invwidth;

bool isfullrange = (tmin<-1000. && tmax>1000.);

//compute gaussian contribution
double central_low =std::max(xmin,mean - alpha1*width );
double central_high=std::min(xmax,mean + alpha2*width );

double tcentral_low = (central_low-mean)*invwidth;
double tcentral_high = (central_high-mean)*invwidth;
if(central_low < central_high) {// is the gaussian part in range?
central = rootPiBy2*width*(TMath::Erf(tcentral_high*invRoot2)-TMath::Erf(tcentral_low*invRoot2));
}
//compute left tail;
if (isfullrange && (n1-1.0)>1.e-5) {
left = width*my_exp(-0.5*alpha1*alpha1)*n1*my_inv(alpha1*(n1-1.));
}
else {

double left_low=xmin;
double left_high=std::min(xmax,mean - alpha1*width);
double thigh = (left_high-mean)*invwidth;

if(left_low < left_high){ //is the left tail in range?
double n1invalpha1 = n1*my_inv(fabs(alpha1));
if(fabs(n1-1.0)>1.e-5) {
double invn1m1 = my_inv(n1-1.);
double leftpow = gbrmath::fast_pow(n1invalpha1,-n1*invn1m1);
double left0 = width*my_exp(-0.5*alpha1*alpha1)*invn1m1;
double left1, left2;

if (xmax>(mean-alpha1*width)) left1 = n1invalpha1;
else left1 = gbrmath::fast_pow( leftpow*(n1invalpha1 - alpha1 - thigh), 1.-n1);

if (tmin<-1000.) left2 = 0.;
else left2 = gbrmath::fast_pow( leftpow*(n1invalpha1 - alpha1 - tmin ), 1.-n1);

left = left0*(left1-left2);

//left = width*vdt::fast_exp(-0.5*alpha1*alpha1)*invn1m1*(n1invalpha1 - gbrmath::fast_pow( gbrmath::fast_pow(n1invalpha1,-n1*invn1m1)*(n1invalpha1 - alpha1 - tmin), 1.-n1)) ;
//left = width*vdt::fast_exp(-0.5*alpha1*alpha1)*invn1m1*(n1invalpha1 - gbrmath::fast_pow( gbrmath::fast_pow(n1invalpha1,-n1*invn1m1)*(n1invalpha1 - alpha1 - tmin), 1.-n1)) ;
//left = A1*vdt::fast_inv(-n1+1.0)*width*(gbrmath::fast_pow(B1-(left_low-mean)*invwidth,-n1+1.)-gbrmath::fast_pow(B1-(left_high-mean)*invwidth,-n1+1.));
}
else {
double A1 = gbrmath::fast_pow(n1invalpha1,n1)*my_exp(-0.5*alpha1*alpha1);
double B1 = n1invalpha1-fabs(alpha1);
left = A1*width*(my_log(B1-(left_low-mean)*invwidth) - my_log(B1-(left_high-mean)*invwidth) );
}
}
}

//compute right tail;
if (isfullrange && (n2-1.0)>1.e-5) {
right = width*my_exp(-0.5*alpha2*alpha2)*n2*my_inv(alpha2*(n2-1.));
}
else {
double right_low=std::max(xmin,mean + alpha2*width);
double right_high=xmax;
double tlow = (right_low - mean)*invwidth;

if(right_low < right_high){ //is the right tail in range?
double n2invalpha2 = n2*my_inv(fabs(alpha2));
if(fabs(n2-1.0)>1.e-5) {
double invn2m2 = my_inv(n2-1.);
double rightpow = gbrmath::fast_pow(n2invalpha2,-n2*invn2m2);
double right0 = width*my_exp(-0.5*alpha2*alpha2)*invn2m2;
double right1, right2;

if (xmin<(mean+alpha2*width)) right1 = n2invalpha2;
else right1 = gbrmath::fast_pow( rightpow*(n2invalpha2 - alpha2 + tlow), 1.-n2);

if (tmax>1000.) right2 = 0.;
else right2 = gbrmath::fast_pow( rightpow*(n2invalpha2 - alpha2 + tmax), 1.-n2);

right = right0*(right1-right2);

//right = A2*vdt::fast_inv(-n2+1.0)*width*(gbrmath::fast_pow(B2+(right_high-mean)*invwidth,-n2+1.)-gbrmath::fast_pow(B2+(right_low-mean)*invwidth,-n2+1.));
}
else {
double A2 = gbrmath::fast_pow(n2invalpha2,n2)*my_exp(-0.5*alpha2*alpha2);
double B2 = n2invalpha2-fabs(alpha2);
right = A2*width*(my_log(B2+(right_high-mean)*invwidth) - my_log(B2+(right_low-mean)*invwidth) );
}
}
}

double sum = left + central + right;

//if (!std::isnormal(left) || !std::isnormal(central) || !std::isnormal(right)) {
if (!std::isnormal(sum)) {
printf("bad int: mean = %5f, sigma = %5f, alpha1 = %5f, n1 = %5f, alpha2 = %5f, n2 = %5f\n",double(mean),double(width),double(alpha1),double(n1),double(alpha2), double(n2));
//printf("left = %5f, central = %5f, right = %5f, A1 = %5f, B1 = %5f, A2 = %5f, B2 = %5f, integral = %5f\n",left,central,right,A1,B1,A2,B2,left+central+right);
printf("left = %5f, central = %5f, right = %5f, integral = %5f\n",left,central,right,sum);
}

return sum;

}

20 changes: 19 additions & 1 deletion src/classes_def.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,24 @@
<class name="RooExpPdf" />
<class name="RooSumTwoExpPdf" />
<class name="RooPowerLawPdf" />
<class name="RooSumTwoPowerLawPdf" />
<class name="CombineUtils" />
Copy link
Copy Markdown
Contributor

@runtingt runtingt Feb 23, 2026

Choose a reason for hiding this comment

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

I think this needs to be kept - its removal here I think comes from the fact that it was added in the time between now and the first attempt at this schema evolution a few hackathons ago

<class name="RooSumTwoPowerLawPdf" />
<read sourceClass="RooDoubleCBFast" targetClass="RooDoubleCBFast" version="[1]"
source="RooRealProxy x; RooRealProxy mean; RooRealProxy width; RooRealProxy alpha1; RooRealProxy n1; RooRealProxy alpha2; RooRealProxy n2"
target="">
<![CDATA[
std::cout << \"Translating RooDoubleCBFast using proper constructor\" << std::endl;
std::string newName(newObj->GetName());
std::string newTitle(newObj->GetTitle());
newObj->~RooDoubleCBFast();
new (newObj) RooDoubleCBFast(newName.c_str(), newTitle.c_str(),
const_cast<RooAbsReal&>(onfile.x.arg()),
const_cast<RooAbsReal&>(onfile.mean.arg()),
const_cast<RooAbsReal&>(onfile.width.arg()),
const_cast<RooAbsReal&>(onfile.alpha1.arg()),
const_cast<RooAbsReal&>(onfile.n1.arg()),
const_cast<RooAbsReal&>(onfile.alpha2.arg()),
const_cast<RooAbsReal&>(onfile.n2.arg()));
]]>
</read>
Comment on lines +224 to +241
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Feb 23, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Escaped quotes inside CDATA will produce invalid C++ — use plain " instead.

Inside a <![CDATA[...]]> section, content is passed through verbatim with no XML entity processing. The \" on Line 228 will appear as literal backslash-quote in the generated C++ code, which is a syntax error outside of a string literal context. Use unescaped " inside CDATA.

Additionally, the std::cout diagnostic on Line 228 will fire on every deserialization of a version-1 object. Consider using oocoutI (RooFit info-level logging) or guarding it behind a verbosity flag, or removing it entirely before merge so that batch processing doesn't produce excessive noise.

Proposed fix for the escaped quotes 
         <![CDATA[
-			std::cout << \"Translating RooDoubleCBFast using proper constructor\" << std::endl;
+			std::cout << "Translating RooDoubleCBFast using proper constructor" << std::endl;
 			std::string newName(newObj->GetName());
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/classes_def.xml` around lines 224 - 241, The CDATA block for translating
RooDoubleCBFast contains escaped quotes (\"), which will emit backslashes into
the generated C++ and break compilation; replace all escaped quotes with plain "
inside the CDATA so string literals like "Translating RooDoubleCBFast using
proper constructor" are emitted correctly, and also remove or replace the
unconditional std::cout diagnostic (inside the same CDATA) with an
RooFit/info-style logger (e.g., oocoutI) or guard it behind a verbosity flag so
deserialization of newObj (and the constructor call for RooDoubleCBFast using
onfile.x.arg(), onfile.mean.arg(), etc.) does not spam batch logs.

</lcgdict>
Loading