WIP: First implementation of ATLAS-style global impacts#1197
WIP: First implementation of ATLAS-style global impacts#1197ajgilbert wants to merge 3 commits intocms-analysis:mainfrom
Conversation
- Added option to combineTool.py -M Impacts to calculate the global impacts both for the standard (asymmetric) impacts (what ATLAS call the shifted global-observable method), and the approximate (symmetric) impacts (what ATLAS call the post-fit nuisance-parameter covariance method) - Enabled by adding `--globalImpacts` or `-g` to all three of the combineTool.py steps. At the moment, runs in addition to (not instead of) the standard impacts calculation. Anticipate support run only global impacts eventually. - plotImpacts.py also has an option `--show-global` or `-g` to draw the global impacts too.
📝 WalkthroughWalkthroughAdds optional global-impact computation and visualization: extracts constant-vars from RooFitResults, propagates them into prefit construction, runs fixed-point global fits (hi/lo) when requested, and emits Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Impacts as "Impacts Tool"
participant WS as "prefit_from_workspace"
participant RooFit as "RooFit / Minuit"
participant RFR as "RooFitResult (file)"
participant Plot as "plotImpacts.py"
User->>Impacts: Run with --globalImpacts
Impacts->>RFR: Read prior RooFitResults (extract constVars)
RFR-->>Impacts: constVarValues
Impacts->>WS: prefit_from_workspace(..., constPars=constVarValues)
WS->>RooFit: Run constrained prefit
RooFit-->>WS: prefit observables
WS->>RooFit: Run secondary constrained fit for globalobs
RooFit-->>WS: globalobs (val, +err, -err)
WS-->>Impacts: prefit + globalobs
Impacts->>RooFit: Launch fixed-point global fits (hi/lo) per parameter
RooFit-->>Impacts: global fixed-fit results
Impacts->>Impacts: Merge global impact estimates into output JSON
User->>Plot: Run with --show-global
Plot->>Impacts: Read impacts JSON (local + global_ fields)
Plot->>Plot: ComputeImpact(prefix="", prefix="global_")
Plot-->>User: Render impacts including global curves
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/tool_base/Impacts.py (1)
275-302: Add null check for global fit results.
utils.get_fixed_resultscan returnNoneif the file is missing or invalid (see lines 188-190 in utils.py). If eitherglobalFitHiResorglobalFitLoResisNone, line 302 will raise aTypeErrorwhen accessing["fixedpoint"][p].Suggested fix
if self.args.globalImpacts and pres["type"] != "Unconstrained": globalFitHiRes = utils.get_fixed_results( f"higgsCombine_globalFit_{name}_{param}_hi.MultiDimFit.mH{mh}.root", poiList) globalFitLoRes = utils.get_fixed_results( f"higgsCombine_globalFit_{name}_{param}_lo.MultiDimFit.mH{mh}.root", poiList) + if globalFitHiRes is None or globalFitLoRes is None: + print(f"Warning: Missing global fit results for {param}, skipping global impacts") + globalFitHiRes = None + globalFitLoRes = None if paramScanRes is None: missing.append(param) continue pres["fit"] = paramScanRes[param][param] for p in poiList: pres.update( { p: paramScanRes[param][p], "impact_" + p: max(list(map(abs, (x - paramScanRes[param][p][1] for x in (paramScanRes[param][p][2], paramScanRes[param][p][0]))))), } ) if self.args.globalImpacts and pres["type"] != "Unconstrained": if self.args.approx is not None: # print(param) # print(prefit) symm_prefit = (prefit[param]["prefit"][2] - prefit[param]["prefit"][0]) / 2. symm_postfit = (pres["fit"][2] - pres["fit"][0]) / 2. red_factor = symm_postfit / symm_prefit imp_hi = ((paramScanRes[param][p][2] - paramScanRes[param][p][1]) * red_factor) + paramScanRes[param][p][1] imp_lo = ((paramScanRes[param][p][0] - paramScanRes[param][p][1]) * red_factor) + paramScanRes[param][p][1] pres.update({f"global_{p}": [imp_lo, paramScanRes[param][p][1], imp_hi]}) - else: + elif globalFitHiRes is not None and globalFitLoRes is not None: pres.update({f"global_{p}": [globalFitLoRes["fixedpoint"][p], paramScanRes[param][p][1], globalFitHiRes["fixedpoint"][p]]})
🤖 Fix all issues with AI agents
In @python/tool_base/utils.py:
- Around line 216-223: The function get_rfr_constvars opens a ROOT.TFile (f =
ROOT.TFile(filename)) but never closes it, causing a resource leak; update
get_rfr_constvars to ensure the file is closed after reading (e.g., use a
context manager or call f.Close() in a finally/after the loop) so the ROOT file
handle is released once constPars are extracted and res is returned.
In @scripts/plotImpacts.py:
- Around line 600-610: Replace the hardcoded showGlobalImpacts = True with the
boolean value coming from the parsed CLI flag for --show-global (e.g.
showGlobalImpacts = args.show_global or options.show_global depending on your
argparse/optparse variable), so the legend entries for g_glob_impacts_hi and
g_glob_impacts_lo and the legend.SetNColumns(...) call reflect the actual
command-line toggle rather than always showing global impacts.
🧹 Nitpick comments (3)
python/tool_base/utils.py (2)
56-56: Remove debug print statement.This appears to be debug output that should be removed or converted to a conditional/logging statement to avoid cluttering user output during normal operation.
Suggested fix
- print(updatePars) + # Optionally enable verbose logging if needed: + # print(updatePars)
96-117: Consider restoring variable state after globalobs computation.The primary variable
varis set constant at line 100 andgobsis set non-constant at line 101. While this may not affect the current usage since each parameter gets its ownvarreference, the underlying workspace objects remain modified. Consider restoring their original constant state after the computation for defensive programming.Suggested fix
+ # For non-Gaussian, the best fit and uncertainties of the gobs (given x), + # may not be the same as the best fit and uncertainties on x. + # Let's calculate these here in case we want them later + var_was_const = var.isConstant() + gobs_was_const = gobs.isConstant() var.setConstant(True) gobs.setConstant(False) nll2 = ROOT.RooConstraintSum("NLL", "", ROOT.RooArgSet(pdf), ROOT.RooArgSet(gobs)) minim = ROOT.RooMinimizer(nll2) minim.setEps(0.001) # Might as well get some better precision... minim.setErrorLevel(0.5) # Unlike for a RooNLLVar we must set this explicitly minim.setPrintLevel(-1) minim.setVerbose(False) # Run the fit then run minos for the error minim.minimize("Minuit2", "migrad") minim.minos(ROOT.RooArgSet(gobs)) # Should really have checked that these converged ok... # var.Print() # pdf.Print() val = gobs.getVal() errlo = -1 * gobs.getErrorLo() errhi = +1 * gobs.getErrorHi() res[p]["globalobs"] = [val - errlo, val, val + errhi] + # Restore original state + var.setConstant(var_was_const) + gobs.setConstant(gobs_was_const)scripts/plotImpacts.py (1)
109-129: Remove commented-out dead code.This large block of commented-out code (the previous implementation) should be removed. Version control preserves the history if needed for reference.
Suggested fix
ComputeImpact(ele, POI, POI_fit, prefix="", checkRelative=args.relative) if args.show_global: ComputeImpact(ele, POI, POI_fit, prefix="global_", checkRelative=args.relative) - # # Calculate impacts and relative impacts. Note that here the impacts are signed. - # ele["impact_hi"] = ele[POI][2] - ele[POI][1] - # ele["impact_lo"] = ele[POI][0] - ele[POI][1] - # g_entry = f"global_{POI}" - # if g_entry in ele: - # # The global impact may or may not be available - # ele["global_impact_hi"] = ele[g_entry][2] - ele[g_entry][1] - # ele["global_impact_lo"] = ele[g_entry][0] - ele[g_entry][1] - # # Some care needed with the relative ones, since we don't know the signs of hi and lo. - # # We want to divide any positive impact by the positive uncert. on the POI, and similar for negative. - # # We also need to be careful in case the uncertainties on the POI came out as zero (shouldn't happen...) - # if (POI_fit[2] - POI_fit[1]) > 0.0 and (POI_fit[1] - POI_fit[0]) > 0.0: - # ele["impact_rel_hi"] = ele["impact_hi"] / ((POI_fit[2] - POI_fit[1]) if ele["impact_hi"] >= 0 else (POI_fit[1] - POI_fit[0])) - # ele["impact_rel_lo"] = ele["impact_lo"] / ((POI_fit[2] - POI_fit[1]) if ele["impact_lo"] >= 0 else (POI_fit[1] - POI_fit[0])) - # else: - # ele["impact_rel_hi"] = 0.0 - # ele["impact_rel_lo"] = 0.0 - # if args.relative: - # # Now we have a real problem, best throw: - # raise RuntimeError("Relative impacts requested (--relative), but uncertainty on the POI is zero") -
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
python/tool_base/Impacts.pypython/tool_base/utils.pyscripts/plotImpacts.pysrc/MultiDimFit.cc
🧰 Additional context used
🧬 Code graph analysis (2)
python/tool_base/utils.py (1)
interface/CascadeMinimizer.h (1)
setErrorLevel(31-31)
scripts/plotImpacts.py (1)
src/HybridNew.cc (1)
params(751-751)
🪛 Ruff (0.14.10)
scripts/plotImpacts.py
100-100: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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: CMSSW_11_3_4 - ROOT 6.22.09
- 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: dev3/latest - ROOT LCG master
- GitHub Check: CMSSW_14_0_0_pre1 - ROOT 6.26.11
- GitHub Check: Compile (py3.10, root6.32.2)
- GitHub Check: Compile (py3.10, root6.26.4)
- GitHub Check: Compile (py3.12, root6.34.4)
🔇 Additional comments (11)
src/MultiDimFit.cc (1)
200-200: LGTM! Correct fix for Hessian computation decision.This properly ensures the Hessian is computed when running the
Impactalgorithm, which is essential for the global impacts feature. The logic now correctly triggersdoHessefor Singles, Impact, or whensaveFitResult_is enabled.Minor style note: the outer parentheses around
saveFitResult_are redundant, but this doesn't affect correctness.python/tool_base/utils.py (1)
41-65: LGTM!The refactored parameter handling cleanly combines
setParsandconstParsinto a unifiedupdateParsdictionary, with appropriate type checking for RooRealVar vs category variables.python/tool_base/Impacts.py (5)
84-89: LGTM!The new
--globalImpactsargument is well-defined with appropriate short flag-gand helpful description.
127-128: LGTM!Adding
--saveFitResultto the combine commands enables extraction of constant variable values from fit results, which is necessary for the global impacts feature.Also applies to: 143-144, 147-150
154-185: LGTM!The
constVarValuesextraction logic correctly handles all fit modes (hesse, robust, splitInitial, and regular) and properly initializes the dictionary before use.
256-262: LGTM!The global fit job generation correctly uses prefit values for the global observable shifts and includes appropriate guards for the parameter type.
234-237: LGTM!The TODO comment appropriately documents future cleanup potential, and the
constVarValuesparameter is correctly passed to enable const-var-aware prefit construction.scripts/plotImpacts.py (4)
53-53: LGTM!The new
--show-globalargument is well-defined with appropriate short flag-gand clear help text.
86-101: LGTM!The
ComputeImpacthelper function is well-structured with proper guards for missing data and division by zero. The static analysis hint about exception message length (TRY003) is a minor style preference and can be safely ignored.
434-435: LGTM!The global impact graphs are correctly initialized for all parameters and conditionally populated with error bars only when global impact data is available.
Also applies to: 487-500
579-597: LGTM!The visual styling provides good distinction between regular impacts (filled with reduced alpha when global is shown) and global impacts (hollow with line borders). The drawing logic is correct.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1197 +/- ##
=======================================
Coverage 20.69% 20.69%
=======================================
Files 195 195
Lines 26156 26156
Branches 3923 3923
=======================================
Hits 5413 5413
Misses 20743 20743
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @scripts/plotImpacts.py:
- Around line 498-500: The conditional is checking for the wrong static key
("global_impact_hi") while the code uses the dynamic impt_prefix (e.g.,
"impact_rel") when --relative is used; update the existence check to test for
the exact key used later (use f"global_{impt_prefix}_hi" or otherwise check both
the static and dynamic variants) so that the subsequent accesses to
par[f"global_{impt_prefix}_hi"] and par[f"global_{impt_prefix}_lo"] cannot raise
a KeyError.
🧹 Nitpick comments (3)
scripts/plotImpacts.py (3)
109-128: Remove commented-out code before merging.This block contains the old implementation that has been refactored into
ComputeImpact. Since the PR is marked as WIP, this may be intentional for reference, but should be removed before the final merge to keep the codebase clean.
434-435: Consider conditional creation/drawing of global impact graphs.The global impact graphs are created and initialized unconditionally, but the error bars are only populated when
"global_impact_hi"exists in the parameter data (lines 498-500). When--show-globalis not used, these graphs will still be styled and drawn (lines 590-597), though with zero-width bars sinceFillStyle=0.This works correctly but adds minor overhead. Consider wrapping the drawing logic (lines 590-597) in an
if args.show_global:block for clarity and to avoid drawing empty graphs.♻️ Suggested improvement
- g_glob_impacts_hi.SetLineColor(hi_color[method]) - g_glob_impacts_hi.SetFillStyle(0) - g_glob_impacts_hi.SetLineWidth(2) - g_glob_impacts_lo.SetLineColor(lo_color[method]) - g_glob_impacts_lo.SetLineWidth(2) - g_glob_impacts_lo.SetFillStyle(0) - g_glob_impacts_hi.Draw("5SAME") - g_glob_impacts_lo.Draw("5SAME") + if args.show_global: + g_glob_impacts_hi.SetLineColor(hi_color[method]) + g_glob_impacts_hi.SetFillStyle(0) + g_glob_impacts_hi.SetLineWidth(2) + g_glob_impacts_lo.SetLineColor(lo_color[method]) + g_glob_impacts_lo.SetLineWidth(2) + g_glob_impacts_lo.SetFillStyle(0) + g_glob_impacts_hi.Draw("5SAME") + g_glob_impacts_lo.Draw("5SAME")Also applies to: 487-488
604-609: Legend entry style may not match graph appearance.The global impact graphs are styled with
FillStyle=0(outline only, line 591/595), but the legend entries use"F"(filled box) which will render as a solid fill in the legend. This could create a visual mismatch where the legend shows filled boxes but the actual plot shows outlines.Consider using
"L"for line style in the legend to match the outline appearance, or keep"F"if the intent is to show the color clearly in the legend despite the visual difference.♻️ Suggested change for consistent legend styling
if args.show_global: - legend.AddEntry(g_glob_impacts_hi, "+1#sigma Global", "F") + legend.AddEntry(g_glob_impacts_hi, "+1#sigma Global", "L") legend.AddEntry(g_pull, "Pull", "P") legend.AddEntry(g_impacts_lo, "-1#sigma Impact", "F") if args.show_global: - legend.AddEntry(g_glob_impacts_lo, "-1#sigma Global", "F") + legend.AddEntry(g_glob_impacts_lo, "-1#sigma Global", "L")
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
python/tool_base/utils.pyscripts/plotImpacts.py
🚧 Files skipped from review as they are similar to previous changes (1)
- python/tool_base/utils.py
🧰 Additional context used
🧬 Code graph analysis (1)
scripts/plotImpacts.py (1)
src/HybridNew.cc (1)
params(751-751)
🪛 Ruff (0.14.10)
scripts/plotImpacts.py
100-100: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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.12, root6.34.4)
- GitHub Check: Compile (py3.10, root6.26.4)
- GitHub Check: Compile (py3.10, root6.32.2)
- GitHub Check: dev3/latest - ROOT LCG master
- GitHub Check: LCG_102 - ROOT 6.26.04
- GitHub Check: LCG_106 - ROOT 6.32.02
- GitHub Check: LCG_108 - ROOT 6.36.02
- GitHub Check: CMSSW_14_1_0_pre4 - ROOT 6.30.07
- GitHub Check: CMSSW_14_0_0_pre1 - ROOT 6.26.11
- GitHub Check: CMSSW_11_3_4 - ROOT 6.22.09
🔇 Additional comments (4)
scripts/plotImpacts.py (4)
53-53: LGTM!The new
--show-globalCLI option follows the existing argparse conventions and is clearly documented.
86-100: LGTM with a minor observation.The function correctly handles missing entries by returning early, which is appropriate since global impacts may not be available for all parameters. The in-place modification of
eleis consistent with the existing code style.One consideration: the function returns
Noneimplicitly when the entry is not found, which is fine since the caller doesn't check the return value. However, for clarity, you could add an explicitreturnat the end or change toreturn False/return Trueto signal whether the computation succeeded. This is optional.
579-579: LGTM!Reducing the alpha for local impact fills when global impacts are shown is a good design choice that improves visual clarity.
590-597: LGTM!The styling for global impact graphs (outlined boxes with matching colors) provides clear visual distinction from the filled local impact bars.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@python/tool_base/utils.py`:
- Around line 102-119: The code sets var.setConstant(True) and
gobs.setConstant(False) before building nll2 and running minim/minos but never
restores their original constant states, which can poison later operations;
capture the original states (e.g. orig_var_const = var.isConstant(),
orig_gobs_const = gobs.isConstant()) before changing them, perform the
RooConstraintSum/minimizer steps (nll2, minim.minimize, minim.minos, then read
gobs.getVal/gobs.getErrorLo/getErrorHi into res[p]["globalobs"]), and finally
restore the originals with var.setConstant(orig_var_const) and
gobs.setConstant(orig_gobs_const) in a finally block so they are always reset
even on errors.
- Around line 57-67: The loop over updatePars assumes allParams.find(par)
returns an object but it can be None; add a guard after calling
allParams.find(par) (in the loop over updatePars) to check if tmp is None and
raise a clear, descriptive exception (e.g., ValueError or RuntimeError) naming
the missing parameter `par`; then proceed with the existing logic that checks
tmp.IsA().InheritsFrom(ROOT.RooRealVar.Class()) and calls tmp.setVal(...) or
tmp.setIndex(...).
In `@scripts/groupGlobalImpacts.py`:
- Around line 9-16: The CLI currently allows running without an input file which
causes open(args.input) to fail; update the argparse call that defines the
--input/-i option (the parser.add_argument call) to make the input parameter
required (use required=True) so args.input is always set, and keep references to
args.input and the subsequent open(args.input) unchanged.
- Around line 55-67: The branch intended to detect "both negative" impacts has a
typo repeating impact_lo in the condition; change the condition from "elif
impact_lo <= 0. and impact_lo <= 0.:" to "elif impact_hi <= 0. and impact_lo <=
0.:" so both impact_hi and impact_lo are checked, leaving the existing
assignment contrib_lo = min(impact_hi, impact_lo) unchanged; ensure you update
the condition near the blocks that reference impact_hi, impact_lo, contrib_hi
and contrib_lo in scripts/groupGlobalImpacts.py.
- Around line 73-77: The calculation for err_hi["STAT"][POI] and
err_lo["STAT"][POI] can pass a tiny negative value into math.sqrt due to
rounding/aggregation; change the computation in the POIs loop (where err_hi,
err_lo, err_tot_hi, err_tot_lo and keys "STAT" and "TOTAL" are used) to compute
the radicand first, clamp it to zero (e.g., value = max(0.0, computed_value))
before calling math.sqrt, and optionally emit a warning/log when clamping occurs
so small negative artifacts are noted.
| for par, (val, verbose) in updatePars.items(): | ||
| tmp = allParams.find(par) | ||
| isrvar = tmp.IsA().InheritsFrom(ROOT.RooRealVar.Class()) | ||
| if isrvar: | ||
| if verbose: | ||
| print(f"Setting parameter {par} to {float(val):g}") | ||
| tmp.setVal(float(val)) | ||
| else: | ||
| tmp.setVal(float(val)) | ||
| else: | ||
| if verbose: | ||
| print(f"Setting index {par} to {float(val):g}") | ||
| tmp.setIndex(int(val)) | ||
| tmp.setIndex(int(val)) |
There was a problem hiding this comment.
Guard against unknown parameters in updatePars.
allParams.find(par) can return None, leading to an AttributeError. Add a clear error instead.
🧯 Proposed fix
- tmp = allParams.find(par)
+ tmp = allParams.find(par)
+ if tmp is None:
+ raise RuntimeError(f"Parameter '{par}' not found in workspace '{workspace}'")
isrvar = tmp.IsA().InheritsFrom(ROOT.RooRealVar.Class())🤖 Prompt for AI Agents
In `@python/tool_base/utils.py` around lines 57 - 67, The loop over updatePars
assumes allParams.find(par) returns an object but it can be None; add a guard
after calling allParams.find(par) (in the loop over updatePars) to check if tmp
is None and raise a clear, descriptive exception (e.g., ValueError or
RuntimeError) naming the missing parameter `par`; then proceed with the existing
logic that checks tmp.IsA().InheritsFrom(ROOT.RooRealVar.Class()) and calls
tmp.setVal(...) or tmp.setIndex(...).
| var.setConstant(True) | ||
| gobs.setConstant(False) | ||
| nll2 = ROOT.RooConstraintSum("NLL", "", ROOT.RooArgSet(pdf), ROOT.RooArgSet(gobs)) | ||
| minim = ROOT.RooMinimizer(nll2) | ||
| minim.setEps(0.001) # Might as well get some better precision... | ||
| minim.setErrorLevel(0.5) # Unlike for a RooNLLVar we must set this explicitly | ||
| minim.setPrintLevel(-1) | ||
| minim.setVerbose(False) | ||
| # Run the fit then run minos for the error | ||
| minim.minimize("Minuit2", "migrad") | ||
| minim.minos(ROOT.RooArgSet(gobs)) | ||
| # Should really have checked that these converged ok... | ||
| # var.Print() | ||
| # pdf.Print() | ||
| val = gobs.getVal() | ||
| errlo = -1 * gobs.getErrorLo() | ||
| errhi = +1 * gobs.getErrorHi() | ||
| res[p]["globalobs"] = [val - errlo, val, val + errhi] |
There was a problem hiding this comment.
Restore constant flags after the globalobs fit.
var.setConstant(True) and gobs.setConstant(False) persist in the workspace and can affect subsequent operations if the workspace is reused.
🔧 Proposed fix
- var.setConstant(True)
- gobs.setConstant(False)
- nll2 = ROOT.RooConstraintSum("NLL", "", ROOT.RooArgSet(pdf), ROOT.RooArgSet(gobs))
- minim = ROOT.RooMinimizer(nll2)
- minim.setEps(0.001) # Might as well get some better precision...
- minim.setErrorLevel(0.5) # Unlike for a RooNLLVar we must set this explicitly
- minim.setPrintLevel(-1)
- minim.setVerbose(False)
- # Run the fit then run minos for the error
- minim.minimize("Minuit2", "migrad")
- minim.minos(ROOT.RooArgSet(gobs))
- # Should really have checked that these converged ok...
- # var.Print()
- # pdf.Print()
- val = gobs.getVal()
- errlo = -1 * gobs.getErrorLo()
- errhi = +1 * gobs.getErrorHi()
- res[p]["globalobs"] = [val - errlo, val, val + errhi]
+ var_was_const = var.isConstant()
+ gobs_was_const = gobs.isConstant()
+ try:
+ var.setConstant(True)
+ gobs.setConstant(False)
+ nll2 = ROOT.RooConstraintSum("NLL", "", ROOT.RooArgSet(pdf), ROOT.RooArgSet(gobs))
+ minim = ROOT.RooMinimizer(nll2)
+ minim.setEps(0.001) # Might as well get some better precision...
+ minim.setErrorLevel(0.5) # Unlike for a RooNLLVar we must set this explicitly
+ minim.setPrintLevel(-1)
+ minim.setVerbose(False)
+ # Run the fit then run minos for the error
+ minim.minimize("Minuit2", "migrad")
+ minim.minos(ROOT.RooArgSet(gobs))
+ # Should really have checked that these converged ok...
+ # var.Print()
+ # pdf.Print()
+ val = gobs.getVal()
+ errlo = -1 * gobs.getErrorLo()
+ errhi = +1 * gobs.getErrorHi()
+ res[p]["globalobs"] = [val - errlo, val, val + errhi]
+ finally:
+ var.setConstant(var_was_const)
+ gobs.setConstant(gobs_was_const)📝 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.
| var.setConstant(True) | |
| gobs.setConstant(False) | |
| nll2 = ROOT.RooConstraintSum("NLL", "", ROOT.RooArgSet(pdf), ROOT.RooArgSet(gobs)) | |
| minim = ROOT.RooMinimizer(nll2) | |
| minim.setEps(0.001) # Might as well get some better precision... | |
| minim.setErrorLevel(0.5) # Unlike for a RooNLLVar we must set this explicitly | |
| minim.setPrintLevel(-1) | |
| minim.setVerbose(False) | |
| # Run the fit then run minos for the error | |
| minim.minimize("Minuit2", "migrad") | |
| minim.minos(ROOT.RooArgSet(gobs)) | |
| # Should really have checked that these converged ok... | |
| # var.Print() | |
| # pdf.Print() | |
| val = gobs.getVal() | |
| errlo = -1 * gobs.getErrorLo() | |
| errhi = +1 * gobs.getErrorHi() | |
| res[p]["globalobs"] = [val - errlo, val, val + errhi] | |
| var_was_const = var.isConstant() | |
| gobs_was_const = gobs.isConstant() | |
| try: | |
| var.setConstant(True) | |
| gobs.setConstant(False) | |
| nll2 = ROOT.RooConstraintSum("NLL", "", ROOT.RooArgSet(pdf), ROOT.RooArgSet(gobs)) | |
| minim = ROOT.RooMinimizer(nll2) | |
| minim.setEps(0.001) # Might as well get some better precision... | |
| minim.setErrorLevel(0.5) # Unlike for a RooNLLVar we must set this explicitly | |
| minim.setPrintLevel(-1) | |
| minim.setVerbose(False) | |
| # Run the fit then run minos for the error | |
| minim.minimize("Minuit2", "migrad") | |
| minim.minos(ROOT.RooArgSet(gobs)) | |
| # Should really have checked that these converged ok... | |
| # var.Print() | |
| # pdf.Print() | |
| val = gobs.getVal() | |
| errlo = -1 * gobs.getErrorLo() | |
| errhi = +1 * gobs.getErrorHi() | |
| res[p]["globalobs"] = [val - errlo, val, val + errhi] | |
| finally: | |
| var.setConstant(var_was_const) | |
| gobs.setConstant(gobs_was_const) |
🤖 Prompt for AI Agents
In `@python/tool_base/utils.py` around lines 102 - 119, The code sets
var.setConstant(True) and gobs.setConstant(False) before building nll2 and
running minim/minos but never restores their original constant states, which can
poison later operations; capture the original states (e.g. orig_var_const =
var.isConstant(), orig_gobs_const = gobs.isConstant()) before changing them,
perform the RooConstraintSum/minimizer steps (nll2, minim.minimize, minim.minos,
then read gobs.getVal/gobs.getErrorLo/getErrorHi into res[p]["globalobs"]), and
finally restore the originals with var.setConstant(orig_var_const) and
gobs.setConstant(orig_gobs_const) in a finally block so they are always reset
even on errors.
| parser = argparse.ArgumentParser() | ||
| parser.add_argument("--input", "-i", help="input json file") | ||
| args = parser.parse_args() | ||
|
|
||
| # Load the json output of combineTool.py -M Impacts | ||
| data = {} | ||
| with open(args.input) as jsonfile: | ||
| data = json.load(jsonfile) |
There was a problem hiding this comment.
Require --input to avoid cryptic failures.
Without required=True, open(None) will raise a TypeError. Make the CLI enforce the input file.
✅ Proposed fix
-parser.add_argument("--input", "-i", help="input json file")
+parser.add_argument("--input", "-i", required=True, help="input json file")📝 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.
| parser = argparse.ArgumentParser() | |
| parser.add_argument("--input", "-i", help="input json file") | |
| args = parser.parse_args() | |
| # Load the json output of combineTool.py -M Impacts | |
| data = {} | |
| with open(args.input) as jsonfile: | |
| data = json.load(jsonfile) | |
| parser = argparse.ArgumentParser() | |
| parser.add_argument("--input", "-i", required=True, help="input json file") | |
| args = parser.parse_args() | |
| # Load the json output of combineTool.py -M Impacts | |
| data = {} | |
| with open(args.input) as jsonfile: | |
| data = json.load(jsonfile) |
🤖 Prompt for AI Agents
In `@scripts/groupGlobalImpacts.py` around lines 9 - 16, The CLI currently allows
running without an input file which causes open(args.input) to fail; update the
argparse call that defines the --input/-i option (the parser.add_argument call)
to make the input parameter required (use required=True) so args.input is always
set, and keep references to args.input and the subsequent open(args.input)
unchanged.
| if impact_hi >= 0. and impact_lo <= 0.: | ||
| contrib_hi = impact_hi | ||
| contrib_lo = impact_lo | ||
| elif impact_hi <=0. and impact_lo >= 0.: | ||
| contrib_lo = impact_hi | ||
| contrib_hi = impact_lo | ||
| elif impact_hi >= 0. and impact_lo >= 0.: | ||
| print(f"Warning, parameter {name} has global impact on {POI} that are both positive: ({impact_hi},{impact_lo}), we will take the max of the two") | ||
| contrib_hi = max(impact_hi, impact_lo) | ||
| elif impact_lo <= 0. and impact_lo <= 0.: | ||
| print(f"Warning, parameter {name} has global impact on {POI} that are both negative: ({impact_hi},{impact_lo}), we will take the min of the two") | ||
|
|
||
| contrib_lo = min(impact_hi, impact_lo) |
There was a problem hiding this comment.
Fix the sign-logic condition typo.
The “both negative” branch repeats impact_lo and never checks impact_hi. This reads as a bug and is easy to misinterpret later.
🧩 Proposed fix
- elif impact_lo <= 0. and impact_lo <= 0.:
+ elif impact_hi <= 0. and impact_lo <= 0.:📝 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.
| if impact_hi >= 0. and impact_lo <= 0.: | |
| contrib_hi = impact_hi | |
| contrib_lo = impact_lo | |
| elif impact_hi <=0. and impact_lo >= 0.: | |
| contrib_lo = impact_hi | |
| contrib_hi = impact_lo | |
| elif impact_hi >= 0. and impact_lo >= 0.: | |
| print(f"Warning, parameter {name} has global impact on {POI} that are both positive: ({impact_hi},{impact_lo}), we will take the max of the two") | |
| contrib_hi = max(impact_hi, impact_lo) | |
| elif impact_lo <= 0. and impact_lo <= 0.: | |
| print(f"Warning, parameter {name} has global impact on {POI} that are both negative: ({impact_hi},{impact_lo}), we will take the min of the two") | |
| contrib_lo = min(impact_hi, impact_lo) | |
| if impact_hi >= 0. and impact_lo <= 0.: | |
| contrib_hi = impact_hi | |
| contrib_lo = impact_lo | |
| elif impact_hi <=0. and impact_lo >= 0.: | |
| contrib_lo = impact_hi | |
| contrib_hi = impact_lo | |
| elif impact_hi >= 0. and impact_lo >= 0.: | |
| print(f"Warning, parameter {name} has global impact on {POI} that are both positive: ({impact_hi},{impact_lo}), we will take the max of the two") | |
| contrib_hi = max(impact_hi, impact_lo) | |
| elif impact_hi <= 0. and impact_lo <= 0.: | |
| print(f"Warning, parameter {name} has global impact on {POI} that are both negative: ({impact_hi},{impact_lo}), we will take the min of the two") | |
| contrib_lo = min(impact_hi, impact_lo) |
🤖 Prompt for AI Agents
In `@scripts/groupGlobalImpacts.py` around lines 55 - 67, The branch intended to
detect "both negative" impacts has a typo repeating impact_lo in the condition;
change the condition from "elif impact_lo <= 0. and impact_lo <= 0.:" to "elif
impact_hi <= 0. and impact_lo <= 0.:" so both impact_hi and impact_lo are
checked, leaving the existing assignment contrib_lo = min(impact_hi, impact_lo)
unchanged; ensure you update the condition near the blocks that reference
impact_hi, impact_lo, contrib_hi and contrib_lo in
scripts/groupGlobalImpacts.py.
| err_hi["STAT"] = dict(proto) | ||
| err_lo["STAT"] = dict(proto) | ||
| for POI in POIs: | ||
| err_hi["STAT"][POI] = math.sqrt(pow(err_tot_hi[POI], 2) - err_hi["TOTAL"][POI]) | ||
| err_lo["STAT"][POI] = math.sqrt(pow(err_tot_lo[POI], 2) - err_lo["TOTAL"][POI]) |
There was a problem hiding this comment.
Guard against negative under the square root for STAT.
Rounding or over-aggregation can make the term slightly negative, which will crash with a math domain error. Clamp at zero and optionally warn.
🧪 Proposed fix
for POI in POIs:
- err_hi["STAT"][POI] = math.sqrt(pow(err_tot_hi[POI], 2) - err_hi["TOTAL"][POI])
- err_lo["STAT"][POI] = math.sqrt(pow(err_tot_lo[POI], 2) - err_lo["TOTAL"][POI])
+ stat_hi_sq = pow(err_tot_hi[POI], 2) - err_hi["TOTAL"][POI]
+ stat_lo_sq = pow(err_tot_lo[POI], 2) - err_lo["TOTAL"][POI]
+ err_hi["STAT"][POI] = math.sqrt(max(0.0, stat_hi_sq))
+ err_lo["STAT"][POI] = math.sqrt(max(0.0, stat_lo_sq))🤖 Prompt for AI Agents
In `@scripts/groupGlobalImpacts.py` around lines 73 - 77, The calculation for
err_hi["STAT"][POI] and err_lo["STAT"][POI] can pass a tiny negative value into
math.sqrt due to rounding/aggregation; change the computation in the POIs loop
(where err_hi, err_lo, err_tot_hi, err_tot_lo and keys "STAT" and "TOTAL" are
used) to compute the radicand first, clamp it to zero (e.g., value = max(0.0,
computed_value)) before calling math.sqrt, and optionally emit a warning/log
when clamping occurs so small negative artifacts are noted.
1 participant
--globalImpactsor-gto all three of the combineTool.py steps. At the moment, runs in addition to (not instead of) the standard impacts calculation. Anticipate support run only global impacts eventually.--show-globalor-gto draw the global impacts too.python3 scripts/groupGlobalImpacts.py -i impacts.jsonTODO:
= - Some discussion needed on treatment of NPs with non-Gaussian constraints. Right now we vary global obs by prefit uncertainty on the nuisance parameter (which in this case may not be the same as the "uncertainty" on the global observable). Can see a potential inconsistency with what one would get throwing frequentist toys instead, but this needs to be tested.
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.