Skip to content

Updates to Goodness of Fit #1248

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
DennRoy wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Updates to Goodness of Fit #1248

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1a8a4df
Make nicer GoF plots
DennRoy Apr 22, 2026
4305ee0
Bugfixes for GoF
DennRoy Apr 22, 2026
6bec4b1
linting
DennRoy Apr 22, 2026
3c83a1f
Fixes in plotting text
DennRoy Apr 22, 2026
c5565e2
Small formatting changes
DennRoy Apr 29, 2026
cda4221
Fix for warning messages
DennRoy Apr 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
122 changes: 67 additions & 55 deletions scripts/plotGof.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,47 @@ def DrawAxisHists(pads, axis_hists, def_pad=None):
def_pad.cd()


def DrawWarning(arrowrange, underflow, overflow):
warningtext1 = ROOT.TPaveText(0.48, 0.73, 0.60, 0.77, "NDC")
warningtext1.SetBorderSize(0)
warningtext1.SetFillStyle(0)
warningtext1.SetTextAlign(22)
warningtext1.SetTextSize(0.04)
warningtext1.SetTextColor(2)
warningtext1.SetTextFont(62)

if arrowrange and ((underflow != 0) or (overflow != 0)):
warningstrings = []
if underflow != 0:
warningstrings.append("%d underflow" % underflow)
if overflow != 0:
warningstrings.append("%d overflow" % overflow)
warningtext1.AddText(", ".join(warningstrings))
warningtext1.Draw()

warningtext2 = ROOT.TPaveText(0.48, 0.73, 0.60, 0.77, "NDC")
warningtext2.SetBorderSize(0)
warningtext2.SetFillStyle(0)
warningtext2.SetTextAlign(22)
warningtext2.SetTextSize(0.04)
warningtext2.SetTextColor(2)
warningtext2.SetTextFont(62)
warningtext2.AddText(f"observed value not in range, at {obs.GetX()[0]:.2f}")
return warningtext2
else:
if (underflow != 0) or (overflow != 0):
warningstrings = []
if underflow != 0:
warningstrings.append("%d underflow" % underflow)
if overflow != 0:
warningstrings.append("%d overflow" % overflow)
warningtext1.AddText(", ".join(warningstrings))
elif arrowrange:
warningtext1.AddText(f"observed value not in range, at {obs.GetX()[0]:.2f}")
return warningtext1
return None

@coderabbitai coderabbitai Bot Apr 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's examine the actual DrawWarning function and surrounding context
head -100 scripts/plotGof.py | tail -60

Repository: cms-analysis/HiggsAnalysis-CombinedLimit

Length of output: 2128

🏁 Script executed:

# Also check the call sites of DrawWarning
rg -n "DrawWarning" scripts/plotGof.py -A 2 -B 2

Repository: cms-analysis/HiggsAnalysis-CombinedLimit

Length of output: 829

🏁 Script executed:

# Look for where 'obs' is defined in the file
rg -n "^obs\s*=" scripts/plotGof.py

Repository: cms-analysis/HiggsAnalysis-CombinedLimit

Length of output: 66

🌐 Web query:

PyROOT TPaveText ownership Python garbage collection lifetime

💡 Result:

In PyROOT, TPaveText objects created in Python are owned by Python by default. When the Python reference count reaches zero, Python's garbage collector destroys the proxy, and since Python owns the underlying C++ TPaveText object, it calls the C++ destructor to delete it. Key points: - Use ROOT.SetOwnership(pavetext, False) after creation to transfer ownership to ROOT/C++ side (e.g., if drawn on a canvas/pad, which takes ownership via its primitive list). - TPaveText created via pt = ROOT.TPaveText(...) owns the TText objects returned by pt.AddText(...); they are stored in pt.fLines (TList) and deleted when pt is deleted. - If TPaveText is drawn on a TPad/TCanvas (pt.Draw), ROOT adds it to the pad's list of primitives. If the kCanDelete bit is set (default for many drawn objects), the pad owns it and deletes it on Clear or pad destruction. Otherwise, Python deletion occurs when the variable goes out of scope. - To check ownership: ROOT.GetOwnership(obj) (though not explicitly found, implied by SetOwnership usage). - Common pitfall: Python loop variables can overwrite references, triggering premature GC (e.g., for x in list: pass where x held a ROOT object). - For long-lived display: SetOwnership(pt, False) after Draw to let canvas manage lifetime, preventing double-delete or premature GC. Example safe usage: import ROOT pt = ROOT.TPaveText(0.1,0.1,0.5,0.5) pt.AddText("Hello") c = ROOT.TCanvas pt.Draw ROOT.SetOwnership(pt, False) # Canvas now owns it # pt Python ref can be discarded safely

Citations:

DrawWarning has several issues worth addressing.

  1. Overlapping NDC coordinates (visible bug). warningtext1 (line 47) and warningtext2 (line 64) use the exact same NDC rectangle (0.48, 0.73, 0.60, 0.77). When both the out-of-range arrow and non-zero under/overflow conditions occur simultaneously, the two labels render on top of each other and become unreadable. Move one of them to a distinct NDC region.

  2. PyROOT garbage collection risk. In the if arrowrange and ((underflow != 0) or (overflow != 0)) branch, warningtext1.Draw() is called inside the function, but the Python reference is then discarded when the function returns. By default, PyROOT owns the object; when the Python reference goes out of scope, the garbage collector can delete the underlying C++ object even though it was added to the pad's primitive list. This causes the drawn label to vanish from the canvas. Either return both objects (as a list) so the caller keeps references alive, or call ROOT.SetOwnership(warningtext1, False) immediately after creation to transfer ownership to the canvas. Additionally, adopt a consistent convention: don't mix calling Draw() inside the function with returning objects for the caller to draw.

  3. obs is an implicit free variable. Lines 71 and 82 reference obs.GetX()[0], but obs is not a parameter—it is picked up from module scope. Pass the observed x-value (or obs itself) as an explicit parameter to make the function self-contained and robust against future refactors.

  4. Unreachable code. Line 84 (return None) is dead code; both the if and else branches above already return.

  5. Duplicated string-building logic. The underflow/overflow string construction is repeated identically at lines 56–61 and 74–80. Compute it once before branching.

♻️ Proposed refactor addressing 1–5 
-def DrawWarning(arrowrange, underflow, overflow):
-    warningtext1 = ROOT.TPaveText(0.48, 0.73, 0.60, 0.77, "NDC")
-    warningtext1.SetBorderSize(0)
-    warningtext1.SetFillStyle(0)
-    warningtext1.SetTextAlign(22)
-    warningtext1.SetTextSize(0.04)
-    warningtext1.SetTextColor(2)
-    warningtext1.SetTextFont(62)
-
-    if arrowrange and ((underflow != 0) or (overflow != 0)):
-        warningstrings = []
-        if underflow != 0:
-            warningstrings.append("%d underflow" % underflow)
-        if overflow != 0:
-            warningstrings.append("%d overflow" % overflow)
-        warningtext1.AddText(", ".join(warningstrings))
-        warningtext1.Draw()
-
-        warningtext2 = ROOT.TPaveText(0.48, 0.73, 0.60, 0.77, "NDC")
-        warningtext2.SetBorderSize(0)
-        warningtext2.SetFillStyle(0)
-        warningtext2.SetTextAlign(22)
-        warningtext2.SetTextSize(0.04)
-        warningtext2.SetTextColor(2)
-        warningtext2.SetTextFont(62)
-        warningtext2.AddText(f"observed value not in range, at {obs.GetX()[0]:.2f}")
-        return warningtext2
-    else:
-        if (underflow != 0) or (overflow != 0):
-            warningstrings = []
-            if underflow != 0:
-                warningstrings.append("%d underflow" % underflow)
-            if overflow != 0:
-                warningstrings.append("%d overflow" % overflow)
-            warningtext1.AddText(", ".join(warningstrings))
-        elif arrowrange:
-            warningtext1.AddText(f"observed value not in range, at {obs.GetX()[0]:.2f}")
-        return warningtext1
-    return None
+def _makePave(y1, y2):
+    pave = ROOT.TPaveText(0.48, y1, 0.60, y2, "NDC")
+    pave.SetBorderSize(0)
+    pave.SetFillStyle(0)
+    pave.SetTextAlign(22)
+    pave.SetTextSize(0.04)
+    pave.SetTextColor(2)
+    pave.SetTextFont(62)
+    ROOT.SetOwnership(pave, False)
+    return pave
+
+
+def DrawWarning(arrowrange, underflow, overflow, obs_x=None):
+    paves = []
+    flow_parts = []
+    if underflow != 0:
+        flow_parts.append("%d underflow" % underflow)
+    if overflow != 0:
+        flow_parts.append("%d overflow" % overflow)
+
+    if flow_parts:
+        p = _makePave(0.73, 0.77)
+        p.AddText(", ".join(flow_parts))
+        paves.append(p)
+    if arrowrange and obs_x is not None:
+        # offset in y so it does not overlap the flow warning
+        y1, y2 = (0.68, 0.72) if flow_parts else (0.73, 0.77)
+        p = _makePave(y1, y2)
+        p.AddText(f"observed value not in range, at {obs_x:.2f}")
+        paves.append(p)
+    return paves

Call sites become:

-        warningtext = DrawWarning(arrow_not_in_range, underflow_count, overflow_count)
-        if warningtext:
-            warningtext.Draw()
+        for w in DrawWarning(arrow_not_in_range, underflow_count, overflow_count, obs.GetX()[0]):
+            w.Draw()
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@scripts/plotGof.py` around lines 46 - 85, DrawWarning currently uses
overlapping NDC rectangles and duplicates logic, uses an implicit module
variable obs, mixes Draw() with returned objects (risking PyROOT GC), and
contains unreachable code; fix it by consolidating the underflow/overflow string
construction into a single block (use warningstrings once), add a new function
parameter obs_x (or obs) and use that instead of the free variable
obs.GetX()[0], move warningtext2 to a different NDC rectangle so it doesn't
overlap warningtext1, choose one ownership/drawing convention (either call
ROOT.SetOwnership(warningtext1, False) and warningtext2 after creation and call
Draw() here, or do not call Draw() and instead return both objects e.g.
[warningtext1, warningtext2] so the caller holds references), remove the final
unreachable "return None", and keep the symbols warningtext1, warningtext2,
arrowrange, underflow, overflow, DrawWarning and ROOT.SetOwnership in mind when
making the edits.


## Boilerplate
ROOT.PyConfig.IgnoreCommandLineOptions = True
ROOT.gROOT.SetBatch(ROOT.kTRUE)
Expand Down Expand Up @@ -125,23 +166,24 @@ def DrawAxisHists(pads, axis_hists, def_pad=None):
for i in range(toy_graph.GetN()):
toy_hist.Fill(toy_graph.GetX()[i])
pValue = js[args.mass][key]["p"]
underflow_count = toy_hist.GetBinContent(0)
overflow_count = toy_hist.GetBinContent(args.bins + 1)
obs = plot.ToyTGraphFromJSON(js, [args.mass, key, "obs"])
arr = ROOT.TArrow(obs.GetX()[0], 0.001, obs.GetX()[0], toy_hist.GetMaximum() / 8, 0.02, "<|")
arr = ROOT.TArrow(obs.GetX()[0], 0.001, obs.GetX()[0], toy_hist.GetMaximum() / 6, 0.03, "<|")
arr.SetLineColor(ROOT.kBlue)
arr.SetFillColor(ROOT.kBlue)
arr.SetFillStyle(1001)
arr.SetLineWidth(6)
arr.SetLineWidth(4)
arr.SetLineStyle(1)
arr.SetAngle(60)
toy_hist.Draw()
arr.Draw("<|same")
pads[0].RedrawAxis()
pads[0].RedrawAxis("g")
pads[0].GetFrame().Draw()

# axis[0].GetYaxis().SetTitle(args.y_title)
# axis[0].GetXaxis().SetTitle(args.x_title)
# axis[0].GetXaxis().SetLabelOffset(axis[0].GetXaxis().GetLabelOffset()*2)
toy_hist.GetYaxis().SetTitle(args.y_title)
toy_hist.GetXaxis().SetTitle(args.x_title)
toy_hist.GetXaxis().SetLabelOffset(toy_hist.GetXaxis().GetLabelOffset()*2)

y_min, y_max = (plot.GetPadYMin(pads[0]), plot.GetPadYMax(pads[0]))
plot.FixBothRanges(pads[0], 0, 0, y_max, 0.25)
Expand All @@ -159,11 +201,11 @@ def DrawAxisHists(pads, axis_hists, def_pad=None):

legend.Draw()

plot.DrawCMSLogo(pads[0], "CMS", args.cms_sub, 11, 0.045, 0.035, 1.2, "", 0.8)
plot.DrawCMSLogo(pads[0], "CMS", args.cms_sub, 11, 0.095, 0.035, 1.2, "", 0.8)
plot.DrawTitle(pads[0], args.title_right, 3)
plot.DrawTitle(pads[0], title, 1)

textlabel = ROOT.TPaveText(0.68, 0.88, 0.80, 0.92, "NDC")
textlabel = ROOT.TPaveText(0.78, 0.88, 0.90, 0.92, "NDC")
textlabel.SetBorderSize(0)
textlabel.SetFillStyle(0)
textlabel.SetTextAlign(32)
Expand All @@ -173,7 +215,7 @@ def DrawAxisHists(pads, axis_hists, def_pad=None):
textlabel.AddText(args.statistic + ", %s Toys" % (toy_graph.GetN()))
textlabel.Draw()

pvalue = ROOT.TPaveText(0.68, 0.83, 0.80, 0.87, "NDC")
pvalue = ROOT.TPaveText(0.78, 0.83, 0.90, 0.87, "NDC")
pvalue.SetBorderSize(0)
pvalue.SetFillStyle(0)
pvalue.SetTextAlign(32)
Expand All @@ -183,6 +225,11 @@ def DrawAxisHists(pads, axis_hists, def_pad=None):
pvalue.AddText("p-value = %0.3f" % pValue)
pvalue.Draw()

arrow_not_in_range = (obs.GetX()[0] > toy_hist.GetBinLowEdge(args.bins + 1)) or (obs.GetX()[0] < toy_hist.GetBinLowEdge(0))
warningtext = DrawWarning(arrow_not_in_range, underflow_count, overflow_count)
if warningtext:
warningtext.Draw()

canv.Print(key + args.output + ".pdf")
canv.Print(key + args.output + ".png")

Expand All @@ -208,24 +255,24 @@ def DrawAxisHists(pads, axis_hists, def_pad=None):
underflow_count = toy_hist.GetBinContent(0)
overflow_count = toy_hist.GetBinContent(args.bins + 1)
obs = plot.ToyTGraphFromJSON(js, [args.mass, "obs"])
arr = ROOT.TArrow(obs.GetX()[0], 0.001, obs.GetX()[0], toy_hist.GetMaximum() / 8, 0.02, "<|")
arr = ROOT.TArrow(obs.GetX()[0], 0.001, obs.GetX()[0], toy_hist.GetMaximum() / 6, 0.03, "<|")
# if axis is None:
# axis = plot.CreateAxisHists(1, graph_sets[-1].values()[0], True)
# DrawAxisHists(pads, axis, pads[0])
arr.SetLineColor(ROOT.kBlue)
arr.SetFillColor(ROOT.kBlue)
arr.SetFillStyle(1001)
arr.SetLineWidth(6)
arr.SetLineWidth(4)
arr.SetLineStyle(1)
arr.SetAngle(60)
toy_hist.Draw()
arr.Draw("<|same")
pads[0].RedrawAxis()
pads[0].RedrawAxis("g")
pads[0].GetFrame().Draw()
# axis[0].GetYaxis().SetTitle(args.y_title)
# axis[0].GetXaxis().SetTitle(args.x_title)
# axis[0].GetXaxis().SetLabelOffset(axis[0].GetXaxis().GetLabelOffset()*2)
toy_hist.GetYaxis().SetTitle(args.y_title)
toy_hist.GetXaxis().SetTitle(args.x_title)
toy_hist.GetXaxis().SetLabelOffset(toy_hist.GetXaxis().GetLabelOffset()*2)

y_min, y_max = (plot.GetPadYMin(pads[0]), plot.GetPadYMax(pads[0]))
plot.FixBothRanges(pads[0], 0, 0, y_max, 0.25)
Expand All @@ -243,11 +290,11 @@ def DrawAxisHists(pads, axis_hists, def_pad=None):

legend.Draw()

plot.DrawCMSLogo(pads[0], "CMS", args.cms_sub, 11, 0.045, 0.035, 1.2, "", 0.8)
plot.DrawCMSLogo(pads[0], "CMS", args.cms_sub, 11, 0.095, 0.035, 1.2, "", 0.8)
plot.DrawTitle(pads[0], args.title_right, 3)
plot.DrawTitle(pads[0], args.title_left, 1)

textlabel = ROOT.TPaveText(0.68, 0.88, 0.80, 0.92, "NDC")
textlabel = ROOT.TPaveText(0.78, 0.88, 0.90, 0.92, "NDC")
textlabel.SetBorderSize(0)
textlabel.SetFillStyle(0)
textlabel.SetTextAlign(32)
Expand All @@ -257,7 +304,7 @@ def DrawAxisHists(pads, axis_hists, def_pad=None):
textlabel.AddText(args.statistic + ", %s Toys" % (toy_graph.GetN()))
textlabel.Draw()

pvalue = ROOT.TPaveText(0.68, 0.83, 0.80, 0.87, "NDC")
pvalue = ROOT.TPaveText(0.78, 0.83, 0.90, 0.87, "NDC")
pvalue.SetBorderSize(0)
pvalue.SetFillStyle(0)
pvalue.SetTextAlign(32)
Expand All @@ -268,44 +315,9 @@ def DrawAxisHists(pads, axis_hists, def_pad=None):
pvalue.Draw()

arrow_not_in_range = (obs.GetX()[0] > toy_hist.GetBinLowEdge(args.bins + 1)) or (obs.GetX()[0] < toy_hist.GetBinLowEdge(0))

warningtext1 = ROOT.TPaveText(0.68, 0.78, 0.80, 0.82, "NDC")
warningtext1.SetBorderSize(0)
warningtext1.SetFillStyle(0)
warningtext1.SetTextAlign(32)
warningtext1.SetTextSize(0.04)
warningtext1.SetTextColor(2)
warningtext1.SetTextFont(62)

if arrow_not_in_range and ((underflow_count != 0) or (overflow_count != 0)):
warningstrings = []
if underflow_count != 0:
warningstrings.append("%d underflow" % underflow_count)
if overflow_count != 0:
warningstrings.append("%d overflow" % overflow_count)
warningtext1.AddText(", ".join(warningstrings))
warningtext1.Draw()

warningtext2 = ROOT.TPaveText(0.68, 0.73, 0.80, 0.77, "NDC")
warningtext2.SetBorderSize(0)
warningtext2.SetFillStyle(0)
warningtext2.SetTextAlign(32)
warningtext2.SetTextSize(0.04)
warningtext2.SetTextColor(2)
warningtext2.SetTextFont(62)
warningtext2.AddText("observed value not in range")
warningtext2.Draw()
else:
if (underflow_count != 0) or (overflow_count != 0):
warningstrings = []
if underflow_count != 0:
warningstrings.append("%d underflow" % underflow_count)
if overflow_count != 0:
warningstrings.append("%d overflow" % overflow_count)
warningtext1.AddText(", ".join(warningstrings))
elif arrow_not_in_range:
warningtext1.AddText("observed value not in range")
warningtext1.Draw()
warningtext = DrawWarning(arrow_not_in_range, underflow_count, overflow_count)
if warningtext:
warningtext.Draw()

canv.Print(".pdf")
canv.Print(".png")
8 changes: 5 additions & 3 deletions src/GoodnessOfFit.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,7 @@
for (int i = 0; i < datasetsList->GetSize(); ++i) {
datasets.emplace_back(dynamic_cast<RooAbsData*>(datasetsList->At(i)));
}
datasetsList.reset();

Check warning on line 286 in src/GoodnessOfFit.cc

View check run for this annotation

Codecov / codecov/patch

src/GoodnessOfFit.cc#L286 

Added line #L286 was not covered by tests
#endif

// Number of categories should always equal the number of datasets
Expand All @@ -293,7 +294,7 @@

for (unsigned i = 0; i < binNames_.size(); i++) {
RooAbsData *cat_data = datasets[i].get();
RooAbsPdf *cat_pdf = sim->getPdf(binNames_[i].c_str());
RooAbsPdf *cat_pdf = sim->getPdf(cat_data->GetName());

Check warning on line 297 in src/GoodnessOfFit.cc

View check run for this annotation

Codecov / codecov/patch

src/GoodnessOfFit.cc#L297 

Added line #L297 was not covered by tests
std::unique_ptr<RooArgSet> observables(cat_pdf->getObservables(cat_data));
if (observables->getSize() > 1) {
std::cout << "Warning, KS and AD statistics are not well defined for "
Expand Down Expand Up @@ -402,9 +403,10 @@
}
}else{
bin_prob = current_cdf_val-last_cdf_val;
distance = s_data*pow((empirical_df-current_cdf_val), 2)/current_cdf_val/(1.-current_cdf_val)*bin_prob;
if (current_cdf_val >= 1.0) {
if (current_cdf_val >= 1.0 || current_cdf_val <= 0.0) {
distance = 0.;
}else{
distance = s_data*pow((empirical_df-current_cdf_val), 2)/current_cdf_val/(1.-current_cdf_val)*bin_prob;

Check warning on line 409 in src/GoodnessOfFit.cc

View check run for this annotation

Codecov / codecov/patch

src/GoodnessOfFit.cc#L409 

Added line #L409 was not covered by tests
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
if (verbose >= 3) {
std::cout << "Observable: " << observableval << "\tdata: " << d->second << "\tedf: " << empirical_df << "\tcdf: " << current_cdf_val << "\tdistance: " << distance << "\n";
Expand Down
Loading