Make combineCreateNLL available in Python via utility class
#1183
Conversation
WalkthroughRefactors the global combineCreateNLL access into a new CombineUtils class with a static wrapper, updates Python call sites to use ROOT.CombineUtils.combineCreateNLL, adds CombineUtils to build/export metadata, and inserts a FastScan template-analysis CI step. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Py as Python tool (FastScan/TaylorExpand)
participant ROOT as ROOT Python bindings
participant Cpp as C++ global function
Note over Py,ROOT: NLL creation flow (changed)
Py->>ROOT: Call ROOT.CombineUtils.combineCreateNLL(pdf, data, ...)
activate ROOT
ROOT->>Cpp: Invoke ::combineCreateNLL(pdf, data, ...)
activate Cpp
Cpp-->>ROOT: std::unique_ptr<RooAbsReal> (NLL)
deactivate Cpp
ROOT-->>Py: RooAbsReal NLL pointer
deactivate ROOT
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/cvmfs-ci.yml(1 hunks)interface/Combine.h(1 hunks)interface/CombineUtils.h(1 hunks)python/tool_base/FastScan.py(1 hunks)python/tool_base/TaylorExpand.py(1 hunks)src/classes.h(1 hunks)src/classes_def.xml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
python/tool_base/FastScan.py (2)
interface/CombineUtils.h (1)
CombineUtils(11-20)src/Combine.cc (2)
combineCreateNLL(1278-1338)combineCreateNLL(1278-1279)
python/tool_base/TaylorExpand.py (2)
interface/CombineUtils.h (1)
CombineUtils(11-20)src/Combine.cc (2)
combineCreateNLL(1278-1338)combineCreateNLL(1278-1279)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Compile (py3.10, root6.32.2)
- GitHub Check: Compile (py3.12, root6.34.4)
- GitHub Check: Compile (py3.10, root6.26.4)
- GitHub Check: LCG_108 - ROOT 6.36.02
- GitHub Check: CMSSW_14_1_0_pre4 - ROOT 6.30.07
- GitHub Check: LCG_102 - ROOT 6.26.04
- GitHub Check: CMSSW_14_0_0_pre1 - ROOT 6.26.11
- GitHub Check: CMSSW_11_3_4 - ROOT 6.22.09
- GitHub Check: dev3/latest - ROOT LCG master
- GitHub Check: LCG_106 - ROOT 6.32.02
🔇 Additional comments (6)
src/classes.h (1)
66-66: LGTM! Include added correctly.The addition of
CombineUtils.hproperly exposes the new utility class for dictionary generation and Python bindings.src/classes_def.xml (1)
223-223: LGTM! Dictionary export configured correctly.Adding
CombineUtilsto the lcgdict properly exposes the class to Python via ROOT's dictionary generation..github/workflows/cvmfs-ci.yml (1)
286-291: LGTM! CI test validates the new Python API.The new workflow step appropriately tests the FastScan functionality with the updated
CombineUtilsinterface.python/tool_base/FastScan.py (1)
60-60: LGTM! Python API call updated correctly.The call site properly uses the new
CombineUtils.combineCreateNLLinterface while maintaining the same functionality.python/tool_base/TaylorExpand.py (1)
189-189: LGTM! Python API call updated consistently.The call site correctly uses the new
CombineUtils.combineCreateNLLinterface, matching the update pattern in FastScan.py.interface/Combine.h (1)
10-11: LGTM! Header dependency updated correctly.The inclusion of
CombineUtils.hproperly exposes the movedcombineCreateNLLdeclaration and provides the new utility class interface.
guitargeek
force-pushed
the
combineCreateNLL
branch
from
fc08972 to
70c01d9
Compare
This fixes crashes with newer ROOT versions (6.36+).
guitargeek
force-pushed
the
combineCreateNLL
branch
from
70c01d9 to
dfdc8a6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
python/tool_base/FastScan.py (1)
117-118: Improved canvas naming for uniqueness.The canvas name now uses
"par_%s" % par.GetName()instead of the genericself.args.output, which gives each parameter's canvas a unique, descriptive name. This prevents potential canvas name conflicts in ROOT.Consider sanitizing the parameter name to handle any potential special characters:
- canv_name = "par_%s" % par.GetName() + canv_name = "par_%s" % re.sub(r'[^\w]', '_', par.GetName()) canv = ROOT.TCanvas(canv_name, canv_name)This is likely unnecessary since RooFit parameter names are typically alphanumeric with underscores, but provides defensive coding against edge cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/cvmfs-ci.yml(1 hunks)interface/Combine.h(1 hunks)interface/CombineUtils.h(1 hunks)python/tool_base/FastScan.py(3 hunks)python/tool_base/TaylorExpand.py(1 hunks)src/classes.h(1 hunks)src/classes_def.xml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- interface/Combine.h
- python/tool_base/TaylorExpand.py
- .github/workflows/cvmfs-ci.yml
🧰 Additional context used
🧬 Code graph analysis (1)
python/tool_base/FastScan.py (2)
interface/CombineUtils.h (1)
CombineUtils(18-27)src/Combine.cc (2)
combineCreateNLL(1278-1338)combineCreateNLL(1278-1279)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: dev3/latest - ROOT LCG master
- GitHub Check: CMSSW_14_0_0_pre1 - ROOT 6.26.11
- GitHub Check: LCG_108 - ROOT 6.36.02
- GitHub Check: CMSSW_14_1_0_pre4 - ROOT 6.30.07
- GitHub Check: LCG_106 - ROOT 6.32.02
- GitHub Check: LCG_102 - ROOT 6.26.04
- GitHub Check: CMSSW_11_3_4 - ROOT 6.22.09
- GitHub Check: Compile (py3.10, root6.26.4)
- GitHub Check: Compile (py3.12, root6.34.4)
- GitHub Check: Compile (py3.10, root6.32.2)
🔇 Additional comments (7)
src/classes.h (1)
66-66: LGTM! Include added for dictionary generation.The include for
CombineUtils.his correctly placed and necessary for generating Python bindings for the newCombineUtilsclass.src/classes_def.xml (1)
223-223: LGTM! Dictionary entry correctly registered.The
CombineUtilsclass is properly registered in the lcgdict section, enabling Python bindings via ROOT's dictionary generation mechanism.python/tool_base/FastScan.py (2)
60-60: LGTM! NLL creation updated to use CombineUtils wrapper.The change from
ROOT.combineCreateNLLtoROOT.CombineUtils.combineCreateNLLcorrectly uses the new static wrapper method, aligning with the PR's objective to expose this functionality through a utility class.
147-147: Excellent bug fix for output file naming!The old code
canv.Print(".pdf%s" % extra)would create a hidden file namedcanv.Print("%s.pdf%s" % (self.args.output, extra))properly creates a file named<output_name>.pdfwith correct multi-page PDF syntax.interface/CombineUtils.h (3)
1-9: LGTM! Header structure is correct.The include guards, necessary includes, and forward declarations are properly structured. The forward declarations minimize compilation dependencies, and
<memory>is correctly included forstd::unique_ptr.
11-15: LGTM! Global function declaration.The free function declaration with default parameters is clean and well-defined. This function will be called by the static wrapper in the
CombineUtilsclass.
17-27: LGTM! Clean wrapper design for Python bindings.The
CombineUtilsclass provides a stateless facade with a static method that simply delegates to the global function. This approach is cleaner for ROOT's dictionary generation than exposing free functions, as noted in the comment. The parameter signatures match perfectly between the wrapper and the global function.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1183 +/- ##
=======================================
Coverage 22.25% 22.25%
=======================================
Files 195 195
Lines 26154 26154
Branches 3883 3884 +1
=======================================
Hits 5820 5820
Misses 20334 20334
🚀 New features to boost your workflow:
|
|
Thank you @guitargeek! |
2 participants
Alternative to #1182 that works without creating dictionaries for the "Combine" class in
Combine.h, which would imply parsing of the boost headers that doesn't always work.Summary by CodeRabbit
New Features
Refactor
User-facing improvements