Schema evolution for RooDoubleCBFast#1235
Schema evolution for RooDoubleCBFast#1235maxgalli wants to merge 2 commits intocms-analysis:mainfrom
Conversation
📝 WalkthroughWalkthroughRooDoubleCBFast is refactored to inherit from RooCrystalBall instead of RooAbsPdf. Internal data members, evaluation logic, and analytical integral methods are removed and delegated to the parent class. A serialization read directive is added to handle migration of previously-stored instances. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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
🧹 Nitpick comments (2)
src/classes_def.xml (1)
205-205: Newcmsmath::SequentialMinimizertransient class registration — appears unrelated to this PR's schema evolution changes.This addition seems orthogonal to the RooDoubleCBFast schema evolution. Consider splitting it into a separate commit or PR for cleaner history, unless it's intentionally bundled.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/classes_def.xml` at line 205, The new transient class registration for cmsmath::SequentialMinimizer is unrelated to the RooDoubleCBFast schema evolution changes and should be separated; remove or revert the <class name="cmsmath::SequentialMinimizer" transient="true" /> addition from this change and put it into a dedicated commit/PR (or amend the current commit to exclude it) so the RooDoubleCBFast schema-only PR remains focused and history stays clean.src/RooDoubleCBFast.cc (1)
2-3:RooAbsPdf.hinclude appears unnecessary.With the class now delegating entirely to
RooCrystalBall,RooAbsPdf.his no longer directly used in this translation unit.RooAbsReal.his used for the constructor parameter types, butRooAbsPdf.hcan likely be dropped. (This is secondary to thefinal-class blocker.)Proposed diff
`#include` "../interface/RooDoubleCBFast.h" `#include` "RooAbsReal.h" -#include "RooAbsPdf.h"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/RooDoubleCBFast.cc` around lines 2 - 3, The include for "RooAbsPdf.h" is unused now that RooDoubleCBFast delegates entirely to RooCrystalBall; remove the `#include` "RooAbsPdf.h" line from RooDoubleCBFast.cc, keep `#include` "RooAbsReal.h" for the constructor signatures, and confirm no references to types or symbols from RooAbsPdf (e.g., any RooAbsPdf-derived usage) remain in the file; rebuild to ensure no missing dependencies.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@interface/RooDoubleCBFast.h`:
- 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.
- 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.
In `@src/classes_def.xml`:
- Around line 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.
---
Nitpick comments:
In `@src/classes_def.xml`:
- Line 205: The new transient class registration for
cmsmath::SequentialMinimizer is unrelated to the RooDoubleCBFast schema
evolution changes and should be separated; remove or revert the <class
name="cmsmath::SequentialMinimizer" transient="true" /> addition from this
change and put it into a dedicated commit/PR (or amend the current commit to
exclude it) so the RooDoubleCBFast schema-only PR remains focused and history
stays clean.
In `@src/RooDoubleCBFast.cc`:
- Around line 2-3: The include for "RooAbsPdf.h" is unused now that
RooDoubleCBFast delegates entirely to RooCrystalBall; remove the `#include`
"RooAbsPdf.h" line from RooDoubleCBFast.cc, keep `#include` "RooAbsReal.h" for the
constructor signatures, and confirm no references to types or symbols from
RooAbsPdf (e.g., any RooAbsPdf-derived usage) remain in the file; rebuild to
ensure no missing dependencies.
| #include "RooAbsReal.h" | ||
|
|
||
| class RooDoubleCBFast : public RooAbsPdf { | ||
| class RooDoubleCBFast : public RooCrystalBall { |
There was a problem hiding this comment.
🧩 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:
RooCrystalBallheader source (RooCrystalBall.h, line 11) (root.cern.ch) - [2] ROOT docs:
RooCrystalBallclass reference (shows “Class Reference final”) (root.cern.ch)
Citations:
- 1: https://root.cern.ch/doc/v634/RooCrystalBall_8h_source.html
- 2: https://root.cern.ch/doc/v634/classRooCrystalBall.html
🏁 Script executed:
# Check if RooCrystalBall.h exists in the repository
find . -name "RooCrystalBall.h" -o -name "*RooCrystalBall*" 2>/dev/null | head -20Repository: 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:
- Composition over inheritance: Inherit from
RooAbsPdfand internally delegate to aRooCrystalBallinstance for evaluation and integration. - Inherit from a non-final ancestor: Inherit directly from
RooAbsPdfand replicate or call into the crystal ball logic. - Upstream change: Request ROOT to remove
finalfromRooCrystalBall(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.
| private: | ||
|
|
||
| ClassDefOverride(RooDoubleCBFast,1) | ||
| ClassDefOverride(RooDoubleCBFast, 2) |
There was a problem hiding this comment.
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.
| <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> |
There was a problem hiding this comment.
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
<
| <class name="RooSumTwoExpPdf" /> | ||
| <class name="RooPowerLawPdf" /> | ||
| <class name="RooSumTwoPowerLawPdf" /> | ||
| <class name="CombineUtils" /> |
There was a problem hiding this comment.
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="RooDoubleCBFast" /> | ||
| <class name="CombDataSetFactory" transient="true" /> | ||
| <class name="DebugProposal" transient="true" /> | ||
| <class name="cmsmath::SequentialMinimizer" transient="true" /> |
There was a problem hiding this comment.
And maybe this is also added for the same reason as CombineUtils is removed - it's noted as unused in the build at least
2 participants
Patch from @runtingt implementing schema evolution for
RooDoubleCBFast(more details in this presentation.There was still a segfault when reading a in an object created with the old version of the class from a workspace, which was fixed in a debugging session with Claude in this commit (i.e., it would be worth crosschecking the fix).
Summary by CodeRabbit
Refactor
New Features
Chores