Add codegen implementations of CMSHist classes (2) - #1252
Draft
maxgalli wants to merge 2 commits into
Draft
Conversation
maxgalli
marked this pull request as draft
May 19, 2026 08:59
Contributor
📝 WalkthroughWalkthroughThis PR adds complete code generation support for CMS histogram types ( ChangesHistogram codegen integration and refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
maxgalli
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/CombineCodegenImpl.h`:
- Around line 30-32: Add a definition for std::string
codegenIntegralImpl(CMSHistFunc& arg, int code, const char* rangeName,
CodegenContext& ctx) that matches the declared signature and mirrors the pattern
used by the existing overloads (codegenIntegralImpl for CMSHistErrorPropagator,
CMSHistSum and VerticalInterpPdf): generate the proper unique name/identifier,
emit the same kind of codegen plumbing (handling CMSHistFunc-specific
members/parameters and any interpolation or parameter bindings), and return the
produced code string; ensure the function uses the same helper utilities and
conventions from the other overloads so linking succeeds when this overload is
dispatched.
In `@src/CombineCodegenImpl.cxx`:
- Around line 344-349: The function RooFit::Experimental::codegenIntegralImpl
currently returns a dummy string "2.0"; replace this with fail-fast behavior by
throwing a std::runtime_error (or using LOG and abort) that clearly states the
integral code path is unimplemented and includes the parameters (code,
rangeName, maybe ctx.name) for debugging; do the same replacement for the other
similar stubbed function(s) around lines 375-380 so any use of these paths fails
loudly instead of silently returning wrong constants.
- Around line 318-380: Formatting in the new codegen overload block is not
clang-formatted; run the project's clang-format/precheckin style on the edited
region (the functions RooFit::Experimental::codegenImpl(CMSHistFunc&),
codegenImpl(CMSHistErrorPropagator&),
codegenIntegralImpl(CMSHistErrorPropagator&), codegenImpl(CMSHistSum&), and
codegenIntegralImpl(CMSHistSum&)) and commit the formatting-only changes so the
CI formatting check passes, ensuring spacing, line breaks, and trailing commas
match the repository's clang-format rules.
In `@src/FastTemplate_Old.cc`:
- Line 76: Run the project formatter (clang-format) over the edited ranges in
FastTemplate_Old.cc and reformat the affected statements (e.g., the
std::upper_bound call that assigns to match using binEdges_, x) so they conform
to the repository style; ensure spacing, indentation, and line breaks around
expressions like auto match = std::upper_bound(binEdges_.begin(),
binEdges_.end(), x); (and the other similar occurrences referenced by CI) are
corrected, then re-run the formatter and push the updated file.
In `@test/testCreateNLL.cxx`:
- Around line 291-330: The test currently continues after fatal setup failures
(failed TFile::Open, missing RooWorkspace, missing ModelConfig/pdf/data/POI)
which can cause null dereferences; update test/testCreateNLL.cxx to fail fast by
checking the results of TFile::Open, the dynamic_cast<RooWorkspace*>
(workspace), modelConfig (RooStats::ModelConfig*), pdf (RooAbsPdf*), data
(RooAbsData*), and poi before proceeding: replace or augment the std::cerr lines
with immediate test-failing exits (e.g. GTest ASSERT/FAIL macros or return
non-zero) so execution stops before calling utils::check_inf_parameters,
loadSnapshotIfExists, modelConfig->GetPdf(), workspace->data(), or dereferencing
poi; ensure the checks reference the existing symbols workspace,
modelConfigName, dataName, and poi so the guards are colocated with the current
error messages.
🪄 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: 6f460fc6-b115-46e8-96b6-b71dd28ca56a
📒 Files selected for processing (9)
interface/CMSHistErrorPropagator.hinterface/CMSHistSum.hinterface/CombineCodegenImpl.hinterface/CombineMathFuncs.hinterface/FastTemplate_Old.hsrc/CMSHistSum.ccsrc/CombineCodegenImpl.cxxsrc/FastTemplate_Old.cctest/testCreateNLL.cxx
Comment on lines
+30
to
+32
| std::string codegenIntegralImpl(CMSHistErrorPropagator& arg, int code, const char* rangeName, CodegenContext& ctx); | ||
| std::string codegenIntegralImpl(CMSHistFunc& arg, int code, const char* rangeName, CodegenContext& ctx); | ||
| std::string codegenIntegralImpl(CMSHistSum& arg, int code, const char* rangeName, CodegenContext& ctx); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify declaration/definition presence for CMSHistFunc integral codegen overload.
# Expected: one declaration in header + one definition in a .cxx file.
rg -nP 'codegenIntegralImpl\s*\(\s*CMSHistFunc\s*&' -C2Repository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 699
🏁 Script executed:
#!/bin/bash
# Search for all definitions of codegenIntegralImpl in implementation files
# Use a broader pattern to catch different formatting styles
rg 'codegenIntegralImpl.*CMSHistFunc' --type cpp -A 2Repository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 503
🏁 Script executed:
#!/bin/bash
# List all codegenIntegralImpl definitions in .cxx files to see what's implemented
fd '\.(cxx|cpp)$' --type f | xargs rg -l 'codegenIntegralImpl' | head -5Repository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 111
🏁 Script executed:
#!/bin/bash
# Check the main implementation file for all CMSHistFunc overload definitions
rg -n 'CMSHistFunc' src/CombineCodegenImpl.cxx | grep -i 'integral\|codegen'Repository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 168
🏁 Script executed:
#!/bin/bash
# Find all codegenIntegralImpl definitions in the .cxx file
rg -n '^.*codegenIntegralImpl' src/CombineCodegenImpl.cxxRepository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 410
🏁 Script executed:
#!/bin/bash
# Get context around codegenIntegralImpl definitions to see all overloads
rg -B2 -A5 '^\s*std::string.*codegenIntegralImpl' src/CombineCodegenImpl.cxx | head -80Repository: cms-analysis/HiggsAnalysis-CombinedLimit
Length of output: 1556
Define the CMSHistFunc integral overload implementation.
The declaration at line 31 declares codegenIntegralImpl(CMSHistFunc&, ...), but this overload has no matching definition in src/CombineCodegenImpl.cxx. The other three overloads (CMSHistErrorPropagator, CMSHistSum, VerticalInterpPdf) are all properly defined. This missing definition will cause a linker error if the overload is dispatched to.
🤖 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/CombineCodegenImpl.h` around lines 30 - 32, Add a definition for
std::string codegenIntegralImpl(CMSHistFunc& arg, int code, const char*
rangeName, CodegenContext& ctx) that matches the declared signature and mirrors
the pattern used by the existing overloads (codegenIntegralImpl for
CMSHistErrorPropagator, CMSHistSum and VerticalInterpPdf): generate the proper
unique name/identifier, emit the same kind of codegen plumbing (handling
CMSHistFunc-specific members/parameters and any interpolation or parameter
bindings), and return the produced code string; ensure the function uses the
same helper utilities and conventions from the other overloads so linking
succeeds when this overload is dispatched.
Comment on lines
+415
to
+431
| std::vector<double> CMSHistSum::getFuncValList(std::size_t fnIdx) { | ||
| staging_ = compcache_[fnIdx]; | ||
| if (vtype_[fnIdx] == CMSHistFunc::VerticalSetting::LogQuadLinear) { | ||
| staging_.Exp(); | ||
| staging_.Scale(storage_[process_fields_[fnIdx]].Integral() / staging_.Integral()); | ||
| } | ||
| staging_.CropUnderflows(); | ||
| std::vector<double> result = staging_.GetValues(); | ||
| for (unsigned j = 0; j < bintypes_.size(); ++j) { | ||
| if (bintypes_[j][0] == 1) { | ||
| double x = vbinpars_[j][0]->getVal(); | ||
| result[j] += binerrors_[fnIdx][j] * x; | ||
| } else if (bintypes_[j][0] == 2 || bintypes_[j][0] == 3) | ||
| result[j] += scaledbinmods_[fnIdx][j]; | ||
| } | ||
| return result; | ||
| } |
Contributor
There was a problem hiding this comment.
Harden getFuncValList against invalid index and stale/zero-normalization state.
This method currently assumes valid fnIdx, initialized caches, and non-zero staging_.Integral(). Add explicit guards to prevent out-of-bounds and inf/nan results.
Proposed fix
std::vector<double> CMSHistSum::getFuncValList(std::size_t fnIdx) {
+ updateCache();
+ if (fnIdx >= compcache_.size() || fnIdx >= binerrors_.size() || fnIdx >= scaledbinmods_.size()) {
+ throw std::out_of_range("CMSHistSum::getFuncValList: fnIdx out of range");
+ }
+
staging_ = compcache_[fnIdx];
if (vtype_[fnIdx] == CMSHistFunc::VerticalSetting::LogQuadLinear) {
staging_.Exp();
- staging_.Scale(storage_[process_fields_[fnIdx]].Integral() / staging_.Integral());
+ const double denom = staging_.Integral();
+ if (denom > 0.) {
+ staging_.Scale(storage_[process_fields_[fnIdx]].Integral() / denom);
+ }
}
staging_.CropUnderflows();
std::vector<double> result = staging_.GetValues();
Comment on lines
+318
to
+380
| void RooFit::Experimental::codegenImpl(CMSHistFunc& arg, CodegenContext& ctx) { | ||
| arg.evaluate(); // trigger cache() creation | ||
| std::vector<double> const& edges = arg.cache().GetBinEdges(); | ||
|
|
||
| // I don't know if these values are actually constant and we can take them | ||
| // here to hardcode into the generated code... | ||
| auto const& values = arg.cache().GetValues(); | ||
|
|
||
| ctx.addResult(&arg, | ||
| ctx.buildCall("RooFit::Detail::MathFuncs::cmsHistFunc", | ||
| arg.getXVar(), | ||
| edges.size() - 1, | ||
| edges, | ||
| values | ||
| )); | ||
| } | ||
|
|
||
| void RooFit::Experimental::codegenImpl(CMSHistErrorPropagator& arg, CodegenContext& ctx) { | ||
| ctx.addResult(&arg, | ||
| ctx.buildCall("RooFit::Detail::MathFuncs::cmsHistErrorPropagator", | ||
| arg.getXVar(), | ||
| arg.coefList().size(), | ||
| arg.coefList(), | ||
| arg.funcList())); | ||
| } | ||
|
|
||
| std::string RooFit::Experimental::codegenIntegralImpl(CMSHistErrorPropagator& arg, | ||
| int code, | ||
| const char* rangeName, | ||
| CodegenContext& ctx) { | ||
| return "2.0"; // TODO: dummy for now | ||
| } | ||
|
|
||
| void RooFit::Experimental::codegenImpl(CMSHistSum& arg, CodegenContext& ctx) { | ||
| arg.evaluate(); | ||
| std::vector<double> const& edges = arg.cache().GetBinEdges(); | ||
| std::size_t nBins = edges.size() - 1; | ||
| RooArgList const& coefs = arg.coefList(); | ||
| std::size_t nSamples = coefs.size(); | ||
| std::vector<double> values(nBins * nSamples); | ||
| for (std::size_t iSamples = 0; iSamples < nSamples; ++iSamples) { | ||
| std::vector<double> sampleValues = arg.getFuncValList(iSamples); | ||
| for (std::size_t iBin = 0; iBin < nBins; ++iBin) | ||
| values[iBin + iSamples * nBins] = sampleValues[iBin]; | ||
| } | ||
|
|
||
| ctx.addResult(&arg, | ||
| ctx.buildCall("RooFit::Detail::MathFuncs::cmsHistSum", | ||
| arg.getXVar(), | ||
| nBins, | ||
| nSamples, | ||
| coefs, | ||
| edges, | ||
| values | ||
| )); | ||
| } | ||
|
|
||
| std::string RooFit::Experimental::codegenIntegralImpl(CMSHistSum& arg, | ||
| int code, | ||
| const char* rangeName, | ||
| CodegenContext& ctx) { | ||
| return "3.0"; // TODO: dummy for now | ||
| } |
Contributor
There was a problem hiding this comment.
Run clang-format on the new codegen overload block.
CI is already failing formatting checks for this region (clang-format / precheckin).
🤖 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 `@src/CombineCodegenImpl.cxx` around lines 318 - 380, Formatting in the new
codegen overload block is not clang-formatted; run the project's
clang-format/precheckin style on the edited region (the functions
RooFit::Experimental::codegenImpl(CMSHistFunc&),
codegenImpl(CMSHistErrorPropagator&),
codegenIntegralImpl(CMSHistErrorPropagator&), codegenImpl(CMSHistSum&), and
codegenIntegralImpl(CMSHistSum&)) and commit the formatting-only changes so the
CI formatting check passes, ensuring spacing, line breaks, and trailing commas
match the repository's clang-format rules.
Comment on lines
+344
to
+349
| std::string RooFit::Experimental::codegenIntegralImpl(CMSHistErrorPropagator& arg, | ||
| int code, | ||
| const char* rangeName, | ||
| CodegenContext& ctx) { | ||
| return "2.0"; // TODO: dummy for now | ||
| } |
Contributor
There was a problem hiding this comment.
Replace dummy integral constants with fail-fast behavior.
Returning "2.0"/"3.0" silently produces wrong integrals if these paths are used.
Proposed minimal safe fix
std::string RooFit::Experimental::codegenIntegralImpl(CMSHistErrorPropagator& arg,
int code,
const char* rangeName,
CodegenContext& ctx) {
- return "2.0"; // TODO: dummy for now
+ throw std::runtime_error("codegenIntegralImpl(CMSHistErrorPropagator) not implemented");
}
@@
std::string RooFit::Experimental::codegenIntegralImpl(CMSHistSum& arg,
int code,
const char* rangeName,
CodegenContext& ctx) {
- return "3.0"; // TODO: dummy for now
+ throw std::runtime_error("codegenIntegralImpl(CMSHistSum) not implemented");
}Also applies to: 375-380
🤖 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 `@src/CombineCodegenImpl.cxx` around lines 344 - 349, The function
RooFit::Experimental::codegenIntegralImpl currently returns a dummy string
"2.0"; replace this with fail-fast behavior by throwing a std::runtime_error (or
using LOG and abort) that clearly states the integral code path is unimplemented
and includes the parameters (code, rangeName, maybe ctx.name) for debugging; do
the same replacement for the other similar stubbed function(s) around lines
375-380 so any use of these paths fails loudly instead of silently returning
wrong constants.
|
|
||
| int FastHisto::FindBin(const T &x) const { | ||
| auto match = std::lower_bound(binEdges_.begin(), binEdges_.end(), x); | ||
| auto match = std::upper_bound(binEdges_.begin(), binEdges_.end(), x); |
Contributor
There was a problem hiding this comment.
Fix clang-format violations in this file before merge.
CI is currently failing on formatting for this file; please run the project formatter on the touched ranges and re-push.
Also applies to: 84-84, 143-143, 146-146, 189-189, 200-200, 250-250, 253-253, 256-256
🤖 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 `@src/FastTemplate_Old.cc` at line 76, Run the project formatter (clang-format)
over the edited ranges in FastTemplate_Old.cc and reformat the affected
statements (e.g., the std::upper_bound call that assigns to match using
binEdges_, x) so they conform to the repository style; ensure spacing,
indentation, and line breaks around expressions like auto match =
std::upper_bound(binEdges_.begin(), binEdges_.end(), x); (and the other similar
occurrences referenced by CI) are corrected, then re-run the formatter and push
the updated file.
Comment on lines
+291
to
+330
| std::unique_ptr<TFile> file(TFile::Open(fileName.c_str(), "READ")); | ||
| EXPECT_FALSE(!file || file->IsZombie()) << "ERROR: failed to open input file '" << fileName << "'.\n"; | ||
|
|
||
| auto* workspace = dynamic_cast<RooWorkspace*>(file->Get(workspaceName.c_str())); | ||
| if (!workspace) { | ||
| std::cerr << "ERROR: workspace '" << workspaceName << "' not found in '" << fileName << "'.\n"; | ||
| file->ls(); | ||
| //return 2; | ||
| } | ||
|
|
||
| // Matches https://github.com/cms-analysis/HiggsAnalysis-CombinedLimit/blob/master/src/Combine.cc#L505-L603 | ||
| utils::check_inf_parameters(workspace->allVars(), /*verbosity=*/0); | ||
| loadSnapshotIfExists(*workspace, "clean"); | ||
| // Matches https://github.com/cms-analysis/HiggsAnalysis-CombinedLimit/blob/master/src/Combine.cc#L505-L603 | ||
| utils::check_inf_parameters(workspace->allVars(), /*verbosity=*/0); | ||
| loadSnapshotIfExists(*workspace, "clean"); | ||
|
|
||
| auto *modelConfig = | ||
| dynamic_cast<RooStats::ModelConfig *>(workspace->genobj(modelConfigName.c_str())); | ||
| if (!modelConfig) { | ||
| std::cerr << "ERROR: ModelConfig '" << modelConfigName << "' not found in workspace '" | ||
| << workspaceName << "'.\n"; | ||
| //return 2; | ||
| } | ||
| auto* modelConfig = dynamic_cast<RooStats::ModelConfig*>(workspace->genobj(modelConfigName.c_str())); | ||
| if (!modelConfig) { | ||
| std::cerr << "ERROR: ModelConfig '" << modelConfigName << "' not found in workspace '" << workspaceName << "'.\n"; | ||
| //return 2; | ||
| } | ||
|
|
||
| RooAbsPdf *pdf = modelConfig->GetPdf(); | ||
| if (!pdf) { | ||
| std::cerr << "ERROR: ModelConfig '" << modelConfigName << "' does not define a pdf.\n"; | ||
| //return 2; | ||
| } | ||
| RooAbsPdf* pdf = modelConfig->GetPdf(); | ||
| if (!pdf) { | ||
| std::cerr << "ERROR: ModelConfig '" << modelConfigName << "' does not define a pdf.\n"; | ||
| //return 2; | ||
| } | ||
|
|
||
| RooAbsData *data = workspace->data(dataName.c_str()); | ||
| if (!data) { | ||
| std::cerr << "ERROR: dataset '" << dataName << "' not found in workspace '" << workspaceName | ||
| << "'.\n"; | ||
| //return 2; | ||
| } | ||
| RooAbsData* data = workspace->data(dataName.c_str()); | ||
| if (!data) { | ||
| std::cerr << "ERROR: dataset '" << dataName << "' not found in workspace '" << workspaceName << "'.\n"; | ||
| //return 2; | ||
| } | ||
|
|
||
| RooMsgService::instance().setGlobalKillBelow(RooFit::ERROR); | ||
| RooAbsReal::setEvalErrorLoggingMode(RooAbsReal::CountErrors); | ||
| RooMsgService::instance().setGlobalKillBelow(RooFit::ERROR); | ||
| RooAbsReal::setEvalErrorLoggingMode(RooAbsReal::CountErrors); | ||
|
|
||
| const RooArgSet *poi = modelConfig->GetParametersOfInterest(); | ||
| if (!poi || poi->getSize() == 0) { | ||
| std::cerr << "ERROR: ModelConfig '" << modelConfigName << "' has no parameters of interest.\n"; | ||
| //return 2; | ||
| } | ||
| for (RooAbsArg *arg : *poi) { | ||
| auto *var = dynamic_cast<RooRealVar *>(arg); | ||
| if (var) | ||
| var->setConstant(false); | ||
| } | ||
| const RooArgSet* poi = modelConfig->GetParametersOfInterest(); | ||
| if (!poi || poi->getSize() == 0) { | ||
| std::cerr << "ERROR: ModelConfig '" << modelConfigName << "' has no parameters of interest.\n"; | ||
| //return 2; | ||
| } |
Contributor
There was a problem hiding this comment.
Fail fast on missing test inputs to avoid null dereferences.
Line 292 and the subsequent object checks continue execution after fatal setup failures, so later dereferences can crash the test process.
Proposed guard fixes
- EXPECT_FALSE(!file || file->IsZombie()) << "ERROR: failed to open input file '" << fileName << "'.\n";
+ ASSERT_TRUE(file && !file->IsZombie()) << "ERROR: failed to open input file '" << fileName << "'.\n";
auto* workspace = dynamic_cast<RooWorkspace*>(file->Get(workspaceName.c_str()));
- if (!workspace) {
- std::cerr << "ERROR: workspace '" << workspaceName << "' not found in '" << fileName << "'.\n";
- file->ls();
- //return 2;
- }
+ ASSERT_NE(workspace, nullptr) << "ERROR: workspace '" << workspaceName << "' not found in '" << fileName << "'.\n";
@@
auto* modelConfig = dynamic_cast<RooStats::ModelConfig*>(workspace->genobj(modelConfigName.c_str()));
- if (!modelConfig) {
- std::cerr << "ERROR: ModelConfig '" << modelConfigName << "' not found in workspace '" << workspaceName << "'.\n";
- //return 2;
- }
+ ASSERT_NE(modelConfig, nullptr)
+ << "ERROR: ModelConfig '" << modelConfigName << "' not found in workspace '" << workspaceName << "'.\n";
@@
RooAbsPdf* pdf = modelConfig->GetPdf();
- if (!pdf) {
- std::cerr << "ERROR: ModelConfig '" << modelConfigName << "' does not define a pdf.\n";
- //return 2;
- }
+ ASSERT_NE(pdf, nullptr) << "ERROR: ModelConfig '" << modelConfigName << "' does not define a pdf.\n";
@@
RooAbsData* data = workspace->data(dataName.c_str());
- if (!data) {
- std::cerr << "ERROR: dataset '" << dataName << "' not found in workspace '" << workspaceName << "'.\n";
- //return 2;
- }
+ ASSERT_NE(data, nullptr)
+ << "ERROR: dataset '" << dataName << "' not found in workspace '" << workspaceName << "'.\n";
@@
const RooArgSet* poi = modelConfig->GetParametersOfInterest();
- if (!poi || poi->getSize() == 0) {
- std::cerr << "ERROR: ModelConfig '" << modelConfigName << "' has no parameters of interest.\n";
- //return 2;
- }
+ ASSERT_TRUE(poi && poi->getSize() > 0)
+ << "ERROR: ModelConfig '" << modelConfigName << "' has no parameters of interest.\n";📝 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.
Suggested change
| std::unique_ptr<TFile> file(TFile::Open(fileName.c_str(), "READ")); | |
| EXPECT_FALSE(!file || file->IsZombie()) << "ERROR: failed to open input file '" << fileName << "'.\n"; | |
| auto* workspace = dynamic_cast<RooWorkspace*>(file->Get(workspaceName.c_str())); | |
| if (!workspace) { | |
| std::cerr << "ERROR: workspace '" << workspaceName << "' not found in '" << fileName << "'.\n"; | |
| file->ls(); | |
| //return 2; | |
| } | |
| // Matches https://github.com/cms-analysis/HiggsAnalysis-CombinedLimit/blob/master/src/Combine.cc#L505-L603 | |
| utils::check_inf_parameters(workspace->allVars(), /*verbosity=*/0); | |
| loadSnapshotIfExists(*workspace, "clean"); | |
| // Matches https://github.com/cms-analysis/HiggsAnalysis-CombinedLimit/blob/master/src/Combine.cc#L505-L603 | |
| utils::check_inf_parameters(workspace->allVars(), /*verbosity=*/0); | |
| loadSnapshotIfExists(*workspace, "clean"); | |
| auto *modelConfig = | |
| dynamic_cast<RooStats::ModelConfig *>(workspace->genobj(modelConfigName.c_str())); | |
| if (!modelConfig) { | |
| std::cerr << "ERROR: ModelConfig '" << modelConfigName << "' not found in workspace '" | |
| << workspaceName << "'.\n"; | |
| //return 2; | |
| } | |
| auto* modelConfig = dynamic_cast<RooStats::ModelConfig*>(workspace->genobj(modelConfigName.c_str())); | |
| if (!modelConfig) { | |
| std::cerr << "ERROR: ModelConfig '" << modelConfigName << "' not found in workspace '" << workspaceName << "'.\n"; | |
| //return 2; | |
| } | |
| RooAbsPdf *pdf = modelConfig->GetPdf(); | |
| if (!pdf) { | |
| std::cerr << "ERROR: ModelConfig '" << modelConfigName << "' does not define a pdf.\n"; | |
| //return 2; | |
| } | |
| RooAbsPdf* pdf = modelConfig->GetPdf(); | |
| if (!pdf) { | |
| std::cerr << "ERROR: ModelConfig '" << modelConfigName << "' does not define a pdf.\n"; | |
| //return 2; | |
| } | |
| RooAbsData *data = workspace->data(dataName.c_str()); | |
| if (!data) { | |
| std::cerr << "ERROR: dataset '" << dataName << "' not found in workspace '" << workspaceName | |
| << "'.\n"; | |
| //return 2; | |
| } | |
| RooAbsData* data = workspace->data(dataName.c_str()); | |
| if (!data) { | |
| std::cerr << "ERROR: dataset '" << dataName << "' not found in workspace '" << workspaceName << "'.\n"; | |
| //return 2; | |
| } | |
| RooMsgService::instance().setGlobalKillBelow(RooFit::ERROR); | |
| RooAbsReal::setEvalErrorLoggingMode(RooAbsReal::CountErrors); | |
| RooMsgService::instance().setGlobalKillBelow(RooFit::ERROR); | |
| RooAbsReal::setEvalErrorLoggingMode(RooAbsReal::CountErrors); | |
| const RooArgSet *poi = modelConfig->GetParametersOfInterest(); | |
| if (!poi || poi->getSize() == 0) { | |
| std::cerr << "ERROR: ModelConfig '" << modelConfigName << "' has no parameters of interest.\n"; | |
| //return 2; | |
| } | |
| for (RooAbsArg *arg : *poi) { | |
| auto *var = dynamic_cast<RooRealVar *>(arg); | |
| if (var) | |
| var->setConstant(false); | |
| } | |
| const RooArgSet* poi = modelConfig->GetParametersOfInterest(); | |
| if (!poi || poi->getSize() == 0) { | |
| std::cerr << "ERROR: ModelConfig '" << modelConfigName << "' has no parameters of interest.\n"; | |
| //return 2; | |
| } | |
| std::unique_ptr<TFile> file(TFile::Open(fileName.c_str(), "READ")); | |
| ASSERT_TRUE(file && !file->IsZombie()) << "ERROR: failed to open input file '" << fileName << "'.\n"; | |
| auto* workspace = dynamic_cast<RooWorkspace*>(file->Get(workspaceName.c_str())); | |
| ASSERT_NE(workspace, nullptr) << "ERROR: workspace '" << workspaceName << "' not found in '" << fileName << "'.\n"; | |
| // Matches https://github.com/cms-analysis/HiggsAnalysis-CombinedLimit/blob/master/src/Combine.cc#L505-L603 | |
| utils::check_inf_parameters(workspace->allVars(), /*verbosity=*/0); | |
| loadSnapshotIfExists(*workspace, "clean"); | |
| auto* modelConfig = dynamic_cast<RooStats::ModelConfig*>(workspace->genobj(modelConfigName.c_str())); | |
| ASSERT_NE(modelConfig, nullptr) | |
| << "ERROR: ModelConfig '" << modelConfigName << "' not found in workspace '" << workspaceName << "'.\n"; | |
| RooAbsPdf* pdf = modelConfig->GetPdf(); | |
| ASSERT_NE(pdf, nullptr) << "ERROR: ModelConfig '" << modelConfigName << "' does not define a pdf.\n"; | |
| RooAbsData* data = workspace->data(dataName.c_str()); | |
| ASSERT_NE(data, nullptr) | |
| << "ERROR: dataset '" << dataName << "' not found in workspace '" << workspaceName << "'.\n"; | |
| RooMsgService::instance().setGlobalKillBelow(RooFit::ERROR); | |
| RooAbsReal::setEvalErrorLoggingMode(RooAbsReal::CountErrors); | |
| const RooArgSet* poi = modelConfig->GetParametersOfInterest(); | |
| ASSERT_TRUE(poi && poi->getSize() > 0) | |
| << "ERROR: ModelConfig '" << modelConfigName << "' has no parameters of interest.\n"; |
🤖 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/testCreateNLL.cxx` around lines 291 - 330, The test currently continues
after fatal setup failures (failed TFile::Open, missing RooWorkspace, missing
ModelConfig/pdf/data/POI) which can cause null dereferences; update
test/testCreateNLL.cxx to fail fast by checking the results of TFile::Open, the
dynamic_cast<RooWorkspace*> (workspace), modelConfig (RooStats::ModelConfig*),
pdf (RooAbsPdf*), data (RooAbsData*), and poi before proceeding: replace or
augment the std::cerr lines with immediate test-failing exits (e.g. GTest
ASSERT/FAIL macros or return non-zero) so execution stops before calling
utils::check_inf_parameters, loadSnapshotIfExists, modelConfig->GetPdf(),
workspace->data(), or dereferencing poi; ensure the checks reference the
existing symbols workspace, modelConfigName, dataName, and poi so the guards are
colocated with the current error messages.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Attempt to pick up work from here #1191
Summary by CodeRabbit
New Features
Bug Fixes
Tests