Schema evolution for RooBernsteinFast (with custom streamers)#1251
Schema evolution for RooBernsteinFast (with custom streamers)#1251maxgalli wants to merge 1 commit into
Conversation
Since the implementation using a linkdef file was triggering segfaults difficult to understand, we implement schema evolution for this class using custom streamers [1]. The main operation consists in prepending 1.0 to the coefficient list, since in ROOT RooBernstein's the first term has to be provided specifically, whereas in the Combine version 1.0 was hardcoded. [1] https://root.cern/manual/io/#schema-evolution-with-custom-streamers
📝 WalkthroughWalkthrough
ChangesRooBernsteinFast Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
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: 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 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.
- Around line 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.
🪄 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: be2eb4b5-e9e2-4dde-b889-e766344d1541
📒 Files selected for processing (2)
interface/RooBernsteinFast.hsrc/RooBernsteinFast.cc
| 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) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the RooBernstein class definition
find . -name "*.h" -o -name "*.cpp" | head -20Repository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 638
🏁 Script executed:
# Search for RooBernstein class definition
rg "class RooBernstein" -A 5Repository: 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 -30Repository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 3479
🏁 Script executed:
# Find where RooBernstein might be defined
fd "RooBernstein" --type fRepository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 137
🏁 Script executed:
# Check the implementation file
cat -n src/RooBernsteinFast.ccRepository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 223
🏁 Script executed:
# Search for usages of RooBernsteinFast to understand workflow
rg "RooBernsteinFast" -B 2 -A 2Repository: 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 -80Repository: 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.
| 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 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)) | ||
| {} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "RooBernsteinFast.h" -type fRepository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 115
🏁 Script executed:
find . -name "RooBernstein.h" -type fRepository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 66
🏁 Script executed:
cat -n ./interface/RooBernsteinFast.hRepository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 2698
🏁 Script executed:
rg "RooBernsteinFast\s*<" --type cpp --type h -A 3 -B 1Repository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 399
🏁 Script executed:
rg "RooBernsteinFast\(" --type cpp --type h -B 2 -A 5Repository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 666
🏁 Script executed:
find . -name "*.cc" -o -name "*.cpp" | head -20Repository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 574
🏁 Script executed:
rg "new RooBernsteinFast|make.*RooBernsteinFast" --type cppRepository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 66
🏁 Script executed:
rg "RooBernsteinFast" --type cpp --type hRepository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 1091
🏁 Script executed:
cat -n ./src/RooBernsteinFast.ccRepository: 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.
| 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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1251 +/- ##
==========================================
- Coverage 20.90% 20.81% -0.09%
==========================================
Files 195 195
Lines 26316 26276 -40
Branches 3947 3938 -9
==========================================
- Hits 5502 5470 -32
+ Misses 20814 20806 -8
... and 1 file with indirect coverage changes
🚀 New features to boost your workflow:
|
|
Closing as it doesn't work as intended |
1 participant
Replaces #1250.
Since the original implementation using I/O customization rules in the xml file was giving segfaults difficult to debug, I helped myself with Claude and implemented the schema evolution using custom streamers [1]. More details about the reason for this and tests can be found in the above-mentioned PR.
[1] https://root.cern/manual/io/#schema-evolution-with-custom-streamers
Summary by CodeRabbit
RooBernsteinFastnow maintains full backward compatibility with objects and files saved in previous software versions, ensuring seamless restoration and data integrity across version updates.