Schema evolution for RooBernsteinFast#1250
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughRooBernsteinFast is converted into a thin RooBernstein subclass, with buildFullList prepending a constant, a migrateFromV1 helper to repair v1 persisted objects, classes_def.xml migration entries for variants 1–7, a reduced translation unit, and a Python test plus CMake wiring validating schema evolution. ChangesRooBernsteinFast refactor and schema evolution
Sequence Diagram(s)sequenceDiagram
participant RootFile as ROOT File (v1)
participant Deserializer as ROOT Deserializer
participant ioStreamerPass2 as RooBernsteinFast::migrateFromV1
participant RooBernstein as RooBernstein instance
RootFile->>Deserializer: load persisted RooBernsteinFast v1
Deserializer->>ioStreamerPass2: onfile proxies resolved (old _x, _coefList)
ioStreamerPass2->>RooBernstein: reflectively access/replace _coefList and _x
ioStreamerPass2->>RooBernstein: write new coefList [1.0, c1..cN] and set proxy name "x"
Deserializer->>RooBernstein: object usable as v2 RooBernsteinFast
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 3
🤖 Prompt for all review comments with 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.
Inline comments:
In `@interface/RooBernsteinFast.h`:
- Around line 29-33: The buildFullList helper in RooBernsteinFast must enforce
that the provided coefList length matches the template parameter N; add a check
at the start of RooBernsteinFast::buildFullList that compares coefList.getSize()
(or coefList.size()) to N and if they differ, throw a descriptive exception
(e.g. std::invalid_argument) or use the RooFit error mechanism with a message
containing N and the provided size, so invalid lists are rejected instead of
silently forwarded to RooBernstein.
- Around line 8-23: Add a clone override to RooBernsteinFast so cloning
preserves the derived template type: declare and define a virtual RooAbsArg*
clone(const char* newname) const override in RooBernsteinFast that returns a new
RooBernsteinFast<N> (e.g. by calling a copy constructor that accepts a name or
by copying and calling setName(newname)), replacing use of RooBernstein::clone()
which would slice the type; reference RooBernsteinFast, clone(),
RooBernstein::clone(), RooAbsArg::clone(), and
ClassDefOverride(RooBernsteinFast, 2) when locating where to add the method.
In `@src/classes_def.xml`:
- Around line 224-283: The read-schema needs matching version-1 blocks for
RooBernsteinFast<6> and RooBernsteinFast<7> like the existing <1>-<5> entries;
add two <read> entries for sourceClass="RooBernsteinFast<6>" and
"RooBernsteinFast<7>" using the same body as the others (capture
newName/newTitle, explicitly call the destructor
newObj->RooBernsteinFast<N>::~RooBernsteinFast(); and placement-new a
RooBernsteinFast<N>(newName.c_str(), newTitle.c_str(),
const_cast<RooAbsReal&>(onfile._x.arg()), onfile._coefList)). Ensure the source
attributes (source="RooRealProxy _x; RooListProxy _coefList") and version="[1]"
match the existing entries so old workspaces for RooBernsteinFast<6> and <7>
have proper migration rules.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2e850136-6408-4129-9729-cc5a9999d3e6
📒 Files selected for processing (3)
interface/RooBernsteinFast.hsrc/RooBernsteinFast.ccsrc/classes_def.xml
| 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); | ||
| } | ||
|
|
||
| //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); | ||
| } | ||
| } | ||
|
|
||
| } | ||
|
|
||
|
|
||
| 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 | ||
| { | ||
|
|
||
| _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); | ||
|
|
||
| } | ||
|
|
||
| protected: | ||
|
|
||
| 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 | ||
| { | ||
|
|
||
| _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 (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); | ||
|
|
||
| } | ||
| RooAbsReal& x, const RooArgList& coefList) : | ||
| // NOTE: RooBernstein takes RooAbsRealLValue& for x, while this constructor | ||
| // keeps RooAbsReal& for backward compatibility. The dynamic_cast will throw | ||
| // at runtime if x is not actually a RooAbsRealLValue. | ||
| RooBernstein(name, title, | ||
| dynamic_cast<RooAbsRealLValue&>(x), | ||
| buildFullList(coefList)) | ||
| {} | ||
|
|
||
| ClassDefOverride(RooBernsteinFast, 2) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if RooBernsteinFast.h exists and see its current state
find . -name "RooBernsteinFast.h" -type fRepository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 115
🏁 Script executed:
# Check the current content of RooBernsteinFast.h to verify the code snippet and look for clone()
cat -n interface/RooBernsteinFast.hRepository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 1450
🏁 Script executed:
# Check if RooBernstein has a clone() method
find . -name "RooBernstein.h" -type fRepository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 66
🏁 Script executed:
# Check RooBernstein implementation for clone() method
cat -n interface/RooBernstein.h | head -100Repository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 141
🏁 Script executed:
# Search for other clone() implementations in the interface directory to understand the pattern
rg "clone\(" interface/ -A 3 | head -50Repository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 3353
🏁 Script executed:
# Search for RooBernstein to understand its source (ROOT library or local)
rg "RooBernstein" --type cpp --type h | head -20Repository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 1325
🏁 Script executed:
# Check if there's a copy constructor pattern in similar classes
rg "const.*other.*const char\*" interface/ -A 2 | head -40Repository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 3258
🏁 Script executed:
# Verify ROOT patterns - check RooSpline1D implementation since it has clone()
rg -A 10 "class RooSpline1D" interface/RooSpline1D.h | head -30Repository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 874
Add a clone() override to preserve the RooBernsteinFast<N> type.
Without this override, RooAbsArg::clone() will use RooBernstein::clone(), creating a base-class instance and dropping the derived type. This breaks schema versioning since ClassDefOverride(RooBernsteinFast, 2) creates a per-specialization schema version that will be lost on clone.
🛠️ Suggested fix
template<int N> class RooBernsteinFast : public RooBernstein {
public:
RooBernsteinFast() = default;
+ RooBernsteinFast(const RooBernsteinFast& other, const char* name = nullptr)
+ : RooBernstein(other, name) {}
+
+ TObject* clone(const char* newname) const override {
+ return new RooBernsteinFast(*this, newname);
+ }🤖 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 8 - 23, Add a clone override to
RooBernsteinFast so cloning preserves the derived template type: declare and
define a virtual RooAbsArg* clone(const char* newname) const override in
RooBernsteinFast that returns a new RooBernsteinFast<N> (e.g. by calling a copy
constructor that accepts a name or by copying and calling setName(newname)),
replacing use of RooBernstein::clone() which would slice the type; reference
RooBernsteinFast, clone(), RooBernstein::clone(), RooAbsArg::clone(), and
ClassDefOverride(RooBernsteinFast, 2) when locating where to add the method.
| static RooArgList buildFullList(const RooArgList& coefList) { | ||
| RooArgList full; | ||
| full.add(RooRealConstant::value(1.0)); | ||
| full.add(coefList); | ||
| return full; |
There was a problem hiding this comment.
Enforce that the coefficient list size matches the template order.
N is no longer used anywhere in the implementation, so RooBernsteinFast<3> will currently accept any coefList length and silently forward that order to RooBernstein. The wrapper should reject lists whose size is not exactly N, otherwise the specialization name and the actual polynomial order can diverge.
🛠️ Suggested fix
+#include <stdexcept>
+
static RooArgList buildFullList(const RooArgList& coefList) {
+ if (coefList.getSize() != N) {
+ throw std::invalid_argument("RooBernsteinFast coefficient count does not match template order");
+ }
RooArgList full;
full.add(RooRealConstant::value(1.0));
full.add(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.
| static RooArgList buildFullList(const RooArgList& coefList) { | |
| RooArgList full; | |
| full.add(RooRealConstant::value(1.0)); | |
| full.add(coefList); | |
| return full; | |
| `#include` <stdexcept> | |
| static RooArgList buildFullList(const RooArgList& coefList) { | |
| if (coefList.getSize() != N) { | |
| throw std::invalid_argument("RooBernsteinFast coefficient count does not match template order"); | |
| } | |
| RooArgList full; | |
| full.add(RooRealConstant::value(1.0)); | |
| full.add(coefList); | |
| return full; | |
| } |
🤖 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 29 - 33, The buildFullList helper
in RooBernsteinFast must enforce that the provided coefList length matches the
template parameter N; add a check at the start of
RooBernsteinFast::buildFullList that compares coefList.getSize() (or
coefList.size()) to N and if they differ, throw a descriptive exception (e.g.
std::invalid_argument) or use the RooFit error mechanism with a message
containing N and the provided size, so invalid lists are rejected instead of
silently forwarded to RooBernstein.
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (93.75%) 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 #1250 +/- ##
==========================================
- Coverage 20.90% 20.85% -0.05%
==========================================
Files 195 195
Lines 26316 26290 -26
Branches 3947 3942 -5
==========================================
- Hits 5502 5484 -18
+ Misses 20814 20806 -8
🚀 New features to boost your workflow:
|
maxgalli
force-pushed
the
roo_bernstein_deprecation
branch
from
076007d to
bf51ef4
Compare
maxgalli
force-pushed
the
roo_bernstein_deprecation
branch
from
bf51ef4 to
73224f6
Compare
maxgalli
changed the title
maxgalli
force-pushed
the
roo_bernstein_deprecation
branch
2 times, most recently
from
9538158 to
5d62792
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/test_roo_bernstein_fast_schema_evolution.py (1)
20-20: ⚡ Quick winAssert the wrapper type as well as the base type.
This check still passes if readback accidentally materializes a plain
RooBernstein, so it misses regressions around preserving theRooBernsteinFast<N>wrapper during schema evolution.Suggested assertion
assert pdf.InheritsFrom("RooBernstein"), f"Expected 'pdf' to inherit from RooBernstein after schema evolution, got {pdf.ClassName()}" +assert pdf.ClassName().startswith("RooBernsteinFast"), ( + f"Expected RooBernsteinFast wrapper after schema evolution, got {pdf.ClassName()}" +)🤖 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 `@test/test_roo_bernstein_fast_schema_evolution.py` at line 20, The test currently only checks that pdf.InheritsFrom("RooBernstein") which can miss loss of the RooBernsteinFast<N> wrapper; update the assertion to also verify the concrete wrapper type by asserting pdf.ClassName() matches the RooBernsteinFast wrapper (e.g. assert pdf.ClassName().startswith("RooBernsteinFast") or a regex like r"RooBernsteinFast<.*>"), and keep the existing InheritsFrom check (use pdf.InheritsFrom("RooBernstein") and an additional assertion on pdf.ClassName() to ensure the RooBernsteinFast<N> wrapper is preserved).
🤖 Prompt for all review comments with 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.
Inline comments:
In `@interface/RooBernsteinFast.h`:
- Around line 16-23: The RooBernsteinFast constructor currently accepts
RooAbsReal& but immediately dynamic_casts to RooAbsRealLValue&, which can throw
std::bad_cast; either make the constructor signature accept RooAbsRealLValue&
directly (remove the dynamic_cast) to enforce compile-time safety, or keep the
RooAbsReal& signature for backward compatibility but replace the raw
dynamic_cast in
RooBernsteinFast(name,title,dynamic_cast<RooAbsRealLValue&>(x),buildFullList(coefList))
with a safe check that catches the bad_cast and logs/throws a clearer
domain-specific error message before forwarding to RooBernstein, referencing the
RooBernsteinFast constructor, the dynamic_cast usage, and RooAbsRealLValue to
locate the change.
---
Nitpick comments:
In `@test/test_roo_bernstein_fast_schema_evolution.py`:
- Line 20: The test currently only checks that pdf.InheritsFrom("RooBernstein")
which can miss loss of the RooBernsteinFast<N> wrapper; update the assertion to
also verify the concrete wrapper type by asserting pdf.ClassName() matches the
RooBernsteinFast wrapper (e.g. assert
pdf.ClassName().startswith("RooBernsteinFast") or a regex like
r"RooBernsteinFast<.*>"), and keep the existing InheritsFrom check (use
pdf.InheritsFrom("RooBernstein") and an additional assertion on pdf.ClassName()
to ensure the RooBernsteinFast<N> wrapper is preserved).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c37d115-1a58-48db-8773-367678f47696
📒 Files selected for processing (5)
data/ci/test_bernstein_fast_v1.rootinterface/RooBernsteinFast.hsrc/RooBernsteinFast.cctest/CMakeLists.txttest/test_roo_bernstein_fast_schema_evolution.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/RooBernsteinFast.cc
| 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) : | ||
| // NOTE: RooBernstein takes RooAbsRealLValue& for x, while this constructor | ||
| // keeps RooAbsReal& for backward compatibility. The dynamic_cast will throw | ||
| // at runtime if x is not actually a RooAbsRealLValue. | ||
| RooBernstein(name, title, | ||
| dynamic_cast<RooAbsRealLValue&>(x), | ||
| buildFullList(coefList)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and examine the RooBernsteinFast.h file
find . -name "RooBernsteinFast.h" -type fRepository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 115
🏁 Script executed:
# Also search for the RooBernstein class definition to understand the parent requirement
find . -name "*.h" -o -name "*.cpp" | head -20 | xargs grep -l "class RooBernstein"Repository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 68
🏁 Script executed:
# Search for usages of RooBernsteinFast constructor
rg "RooBernsteinFast\s*\(" --type cpp --type h -A 2Repository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 462
🏁 Script executed:
# Read the full RooBernsteinFast.h file
cat -n ./interface/RooBernsteinFast.hRepository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 2871
🏁 Script executed:
# Search for RooBernstein class definition in this repository
rg "class RooBernstein[^F]" --type cpp --type hRepository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 66
🏁 Script executed:
# Search for instantiations/calls to RooBernsteinFast constructor
rg "RooBernsteinFast\s*\(" . --type cpp --type hRepository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 234
🏁 Script executed:
# Search for actual instantiations - look for patterns like "RooBernsteinFast<...>(...)" in src files
rg "RooBernsteinFast\s*<" --type cpp --type h -A 3Repository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 66
🏁 Script executed:
# Look for any files that might create RooBernsteinFast objects
find . -name "*.cpp" -type f -exec grep -l "RooBernsteinFast" {} \;Repository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 66
🏁 Script executed:
# Check git log for any mentions of backward compatibility in this file
git log --oneline --all -- ./interface/RooBernsteinFast.h | head -20Repository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 395
🏁 Script executed:
# Examine the commit that modified RooBernsteinFast
git show 73224f6e --statRepository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 450
🏁 Script executed:
# Get the full diff for context
git show 73224f6eRepository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 9148
Consider clarifying the backward compatibility requirement or enforcing type safety in the constructor signature.
This constructor signature advertises RooAbsReal& but the implementation requires RooAbsRealLValue&, relying on a runtime dynamic_cast that throws std::bad_cast for incompatible arguments. While the in-code comment documents this intentional trade-off for backward compatibility, the choice deserves explicit re-evaluation:
- If backward compatibility with old callers passing arbitrary
RooAbsReal&is still required, the current approach (with the documented runtime check) is acceptable, though thedynamic_castfailure mode could be improved with a domain-specific error message. - If backward compatibility is no longer needed post-refactoring, update the signature to
RooAbsRealLValue&and remove the cast to enforce type safety at compile time.
🤖 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 - 23, The RooBernsteinFast
constructor currently accepts RooAbsReal& but immediately dynamic_casts to
RooAbsRealLValue&, which can throw std::bad_cast; either make the constructor
signature accept RooAbsRealLValue& directly (remove the dynamic_cast) to enforce
compile-time safety, or keep the RooAbsReal& signature for backward
compatibility but replace the raw dynamic_cast in
RooBernsteinFast(name,title,dynamic_cast<RooAbsRealLValue&>(x),buildFullList(coefList))
with a safe check that catches the bad_cast and logs/throws a clearer
domain-specific error message before forwarding to RooBernstein, referencing the
RooBernsteinFast constructor, the dynamic_cast usage, and RooAbsRealLValue to
locate the change.
5d62792 to
d329707
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with 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.
Inline comments:
In `@interface/RooBernsteinFast.h`:
- Around line 28-32: The migrateFromV1 migration is still being invoked via the
<read> rules (as documented) which runs too early; update RooBernsteinFast to
override ioStreamerPass2() and call migrateFromV1(RooAbsRealLValue&, const
RooListProxy&) there so the migration runs after proxy resolution, and remove
the <read>-rule hook that invokes migrateFromV1; specifically add an
ioStreamerPass2() method on class RooBernsteinFast that performs the existing
migrateFromV1 logic and ensure classes_def.xml no longer binds migrateFromV1 via
<read> rules.
- Around line 34-68: The code in migrateFromV1 uses
TClass::GetClass()/GetDataMemberOffset() (bernCls, coefOff, xOff, apCls,
ownerOff, argOff, vsOff, ssOff) without validating results; fix by checking each
TClass* is not null and each offset != -1 before doing any
reinterpret_cast/pointer arithmetic, and if any check fails, log a clear error
and abort the migration path (e.g. return/throw) so you never perform the unsafe
pointer writes; apply these guards around the RooBernstein/RooArgProxy offset
lookups and before the coefProxy/xBase manipulation to fail fast and avoid
memory corruption.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2b444ab8-f4da-44e7-94f7-d96cc6ac7dc4
📒 Files selected for processing (6)
data/ci/test_bernstein_fast_v1.rootinterface/RooBernsteinFast.hsrc/RooBernsteinFast.ccsrc/classes_def.xmltest/CMakeLists.txttest/test_roo_bernstein_fast_schema_evolution.py
🚧 Files skipped from review as they are similar to previous changes (2)
- test/test_roo_bernstein_fast_schema_evolution.py
- test/CMakeLists.txt
| // Schema evolution helper: migrate a v1 object (which had its own _x and | ||
| // _coefList with N elements) into the v2 layout (inherits RooBernstein, | ||
| // _coefList has N+1 elements with c0=1 prepended). | ||
| // Called from <read> rules in classes_def.xml. | ||
| void migrateFromV1(RooAbsRealLValue& x, const RooListProxy& oldCoefs) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify where RooBernsteinFast schema evolution is actually hooked.
rg -n -C3 'ioStreamerPass2\s*\(|migrateFromV1\s*\(|<read>|RooBernsteinFast' interface src testRepository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 9246
Migration timing does not match the stated PR objective.
The code still hooks migrateFromV1() via <read> rules in classes_def.xml (lines 224–285), not via an ioStreamerPass2() override. The inline comment (line 31 of RooBernsteinFast.h) documents the <read>-rule path, but the PR objective claims the crash fix was to move this into ioStreamerPass2() for post-proxy-resolution timing. If this override does not exist, the migration runs at the timing already known to crash.
🤖 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 28 - 32, The migrateFromV1
migration is still being invoked via the <read> rules (as documented) which runs
too early; update RooBernsteinFast to override ioStreamerPass2() and call
migrateFromV1(RooAbsRealLValue&, const RooListProxy&) there so the migration
runs after proxy resolution, and remove the <read>-rule hook that invokes
migrateFromV1; specifically add an ioStreamerPass2() method on class
RooBernsteinFast that performs the existing migrateFromV1 logic and ensure
classes_def.xml no longer binds migrateFromV1 via <read> rules.
| static TClass* bernCls = TClass::GetClass("RooBernstein"); | ||
| static Long_t coefOff = bernCls->GetDataMemberOffset("_coefList"); | ||
| static Long_t xOff = bernCls->GetDataMemberOffset("_x"); | ||
|
|
||
| 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 | ||
| { | ||
| static TClass* apCls = TClass::GetClass("RooArgProxy"); | ||
| static Long_t ownerOff = apCls->GetDataMemberOffset("_owner"); | ||
| static Long_t argOff = apCls->GetDataMemberOffset("_arg"); | ||
| static Long_t vsOff = apCls->GetDataMemberOffset("_valueServer"); | ||
| static Long_t ssOff = apCls->GetDataMemberOffset("_shapeServer"); | ||
|
|
||
| _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); | ||
| auto* base = reinterpret_cast<char*>(static_cast<RooBernstein*>(this)); | ||
|
|
||
| } | ||
| // --- _coefList (RooListProxy, private in RooBernstein) --- | ||
| auto& coefProxy = *reinterpret_cast<RooListProxy*>(base + coefOff); | ||
| // initializeAfterIOConstructor sets _owner, copies server-propagation | ||
| // flags, name, and adds elements via the base-class add() (no addServer | ||
| // calls — correct for IO since the server list was already read). | ||
| coefProxy.initializeAfterIOConstructor(this, oldCoefs); | ||
| // coefProxy = [c1 .. cN]. v2 needs [1.0, c1 .. cN]. | ||
| // Use base-class casts to avoid removeServer/addServer side effects. | ||
| RooArgList tmp; | ||
| tmp.add(coefProxy); | ||
| static_cast<RooArgList&>(coefProxy).removeAll(); | ||
| static_cast<RooArgList&>(coefProxy).add(RooRealConstant::value(1.0)); | ||
| static_cast<RooArgList&>(coefProxy).add(tmp); | ||
|
|
||
| protected: | ||
| // --- _x (RooTemplateProxy<RooAbsRealLValue>, private in RooBernstein) --- | ||
| // RooTemplateProxy has no initializeAfterIOConstructor, so we poke the | ||
| // underlying RooArgProxy fields directly. | ||
| auto* xBase = reinterpret_cast<RooArgProxy*>(base + xOff); | ||
| *reinterpret_cast<RooAbsArg**>(reinterpret_cast<char*>(xBase) + ownerOff) = this; | ||
| *reinterpret_cast<RooAbsArg**>(reinterpret_cast<char*>(xBase) + argOff) = &x; | ||
| *reinterpret_cast<bool*>(reinterpret_cast<char*>(xBase) + vsOff) = true; | ||
| *reinterpret_cast<bool*>(reinterpret_cast<char*>(xBase) + ssOff) = false; | ||
| xBase->SetName("x"); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In current ROOT master, do RooBernsteinandRooArgProxystill declare the members_coefList, _x, _owner, _arg, _valueServer, and _shapeServer with those exact names?
💡 Result:
In the current ROOT master branch, the specified members are not all present or named as you listed in the classes RooBernstein and RooArgProxy. For RooBernstein: The members _coefList and _x are present, but they are declared as proxy objects (RooListProxy _coefList and RooTemplateProxy _x) rather than raw pointers or other types [1]. For RooArgProxy: The members _owner, _valueServer, and _shapeServer are present as declared members [2]. However, _arg is not a member of RooArgProxy; instead, the proxied argument is managed through the base class or internal mechanisms, and the class does not contain a member named _arg [3][2]. In summary, while some of these names persist as member variables, others do not exist in the current master branch of these classes [1][3][2].
Citations:
- 1: https://root.cern.ch/doc/v634/RooBernstein_8h_source.html
- 2: https://root.cern.ch/doc/latest-stable/RooArgProxy_8h_source.html
- 3: https://root.cern/doc/master/RooArgProxy_8cxx_source.html
🌐 Web query:
RooArgProxy class members _arg ROOT source code
💡 Result:
In the ROOT framework's RooFit package, the _arg member of the RooArgProxy class is a protected pointer to a RooAbsArg object [1][2][3]. It serves as the primary mechanism for the proxy to store a reference to the specific RooAbsArg object it is managing [4][2][5]. Key details regarding _arg include: Role: It acts as the pointer to the content of the proxy [2][3]. When a RooArgProxy is created, it registers itself with the owning class and adds the proxied object (referenced by _arg) to the owner's server list [4][6]. This allows the owner to receive value and shape dirty flags from the proxied object [4][5]. Access: While _arg is a protected member, it is accessed publicly via the absArg member function, which returns the pointer to the contained argument [1][3]. Lifecycle and Management: - The pointer is initialized to nullptr by default [1][3]. - It is updated during construction or through methods like changePointer, which allows the proxy to redirect its reference to a different RooAbsArg object (e.g., during copy or clone operations) [4][5][7]. - The proxy can optionally take ownership of the object pointed to by _arg if the _ownArg flag is set to true [4][5][6]. Other related protected members in RooArgProxy include _owner (the pointer to the proxy's owner), _valueServer and _shapeServer (booleans indicating if the contents propagate dirty flags), _isFund (indicating if the proxy contains an lvalue), and _ownArg (indicating ownership) [1][2][3].
Citations:
- 1: https://root.cern/doc/master/RooArgProxy_8h_source.html
- 2: https://root.cern.ch/doc/master/classRooArgProxy.html
- 3: https://root.cern.ch/doc/latest-stable/RooArgProxy_8h_source.html
- 4: https://root.cern.ch/doc/v622/RooArgProxy_8cxx_source.html
- 5: https://root.cern/doc/master/RooArgProxy_8cxx_source.html
- 6: https://root.cern.ch/root/html534/RooArgProxy.html
- 7: https://root.cern/doc/v608/RooArgProxy_8cxx_source.html
🏁 Script executed:
find . -name "RooBernsteinFast.h" -type fRepository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 115
🏁 Script executed:
sed -n '34,68p' ./interface/RooBernsteinFast.hRepository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 2116
🏁 Script executed:
sed -n '1,100p' ./interface/RooBernsteinFast.h | head -100Repository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 3707
Guard the reflected ROOT offsets before doing pointer arithmetic.
The migrateFromV1 function uses TClass reflection without validating results. If TClass::GetClass() returns nullptr or GetDataMemberOffset() returns -1 (for missing or renamed members), the code proceeds to pointer arithmetic with invalid offsets, corrupting memory during schema evolution instead of failing cleanly.
Suggested hardening
+#include <stdexcept>
+
void migrateFromV1(RooAbsRealLValue& x, const RooListProxy& oldCoefs) {
// --- locate private members via TClass reflection ---
static TClass* bernCls = TClass::GetClass("RooBernstein");
+ if (!bernCls) {
+ throw std::runtime_error("RooBernsteinFast schema evolution: missing RooBernstein dictionary");
+ }
static Long_t coefOff = bernCls->GetDataMemberOffset("_coefList");
static Long_t xOff = bernCls->GetDataMemberOffset("_x");
+ if (coefOff < 0 || xOff < 0) {
+ throw std::runtime_error("RooBernsteinFast schema evolution: RooBernstein layout changed");
+ }
static TClass* apCls = TClass::GetClass("RooArgProxy");
+ if (!apCls) {
+ throw std::runtime_error("RooBernsteinFast schema evolution: missing RooArgProxy dictionary");
+ }
static Long_t ownerOff = apCls->GetDataMemberOffset("_owner");
static Long_t argOff = apCls->GetDataMemberOffset("_arg");
static Long_t vsOff = apCls->GetDataMemberOffset("_valueServer");
static Long_t ssOff = apCls->GetDataMemberOffset("_shapeServer");
+ if (ownerOff < 0 || argOff < 0 || vsOff < 0 || ssOff < 0) {
+ throw std::runtime_error("RooBernsteinFast schema evolution: RooArgProxy layout changed");
+ }🤖 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 34 - 68, The code in migrateFromV1
uses TClass::GetClass()/GetDataMemberOffset() (bernCls, coefOff, xOff, apCls,
ownerOff, argOff, vsOff, ssOff) without validating results; fix by checking each
TClass* is not null and each offset != -1 before doing any
reinterpret_cast/pointer arithmetic, and if any check fails, log a clear error
and abort the migration path (e.g. return/throw) so you never perform the unsafe
pointer writes; apply these guards around the RooBernstein/RooArgProxy offset
lookups and before the coefProxy/xBase manipulation to fail fast and avoid
memory corruption.
maxgalli
force-pushed
the
roo_bernstein_deprecation
branch
from
d329707 to
9ff798d
Compare
1 participant
This PR follows the same steps done in #1235 for the case of
RooDoubleCBFastapplied to the case ofRooBernsteinFast. The reason for this is that, as reported in #1249, with the latest version of RooFitRooBernsteinseems to be more performant thanRooBernsteinFast(to be checked for other cases than the simple test in the issue).The steps to cross check that this PR works are the following:
RooBernsteinFastEdit
Following a series of tests crashing, I debugged with Claude and decided for a different approach that overrides the function
ioStreamerPass2(link). It is my understanding that it is the earliest point in the code when the workspace has finished reading all objects and resolving proxy references. In here, we create aRooListProxyand prepend 1.0 to it.Summary by CodeRabbit
Refactor
Tests
Chores