Updates to Goodness of Fit#1248
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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. Review rate limit: 0/1 reviews remaining, refill in 3 minutes and 7 seconds.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)
src/GoodnessOfFit.cc (1)
295-304:⚠️ Potential issue | 🟠 MajorMap
qVals_by category name, not split order.Line 297 correctly uses the split dataset name for the PDF lookup, but Line 304 still writes to
qVals_[i]. IfdatasetsandbinNames_are ordered differently, the per-category branches created frombinNames_receive the wrong category’s statistic.🐛 Proposed fix
- for (unsigned i = 0; i < binNames_.size(); i++) { - RooAbsData *cat_data = datasets[i].get(); + for (auto const &dataset : datasets) { + RooAbsData *cat_data = dataset.get(); + auto bin_it = std::find(binNames_.begin(), binNames_.end(), cat_data->GetName()); + if (bin_it == binNames_.end()) { + throw std::runtime_error(std::string("Error: missing category label for dataset ") + cat_data->GetName()); + } + auto qval_index = std::distance(binNames_.begin(), bin_it); RooAbsPdf *cat_pdf = sim->getPdf(cat_data->GetName()); + if (cat_pdf == nullptr) { + throw std::runtime_error(std::string("Error: missing PDF for category label ") + cat_data->GetName()); + } 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 " "models with more than one observable\n"; } RooRealVar *x = dynamic_cast<RooRealVar *>(observables->first()); - qVals_[i] = EvaluateADDistance(*cat_pdf, *cat_data, *x, kolmo); + qVals_[qval_index] = EvaluateADDistance(*cat_pdf, *cat_data, *x, kolmo); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/GoodnessOfFit.cc` around lines 295 - 304, qVals_ is being indexed by the loop index i (qVals_[i]) which assumes binNames_ and datasets share the same order; instead, store the per-category statistic keyed by the dataset/PDF category name so branches get the correct value. Change the assignment so it uses the category identifier from the split dataset (use cat_data->GetName() or the PDF name obtained via sim->getPdf(cat_data->GetName())) as the key into qVals_ (e.g., qVals_[categoryName] = EvaluateADDistance(...)) rather than qVals_[i]; ensure the rest of the code that reads qVals_ expects the same key type (string) and adjust any consumers accordingly (symbols: qVals_, binNames_, datasets, cat_data->GetName(), sim->getPdf, EvaluateADDistance).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/plotGof.py`:
- Around line 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.
In `@src/GoodnessOfFit.cc`:
- Around line 405-410: Run clang-format on the shown hunk in GoodnessOfFit.cc to
fix spacing/indentation: reformat the assignment and if/else block around
bin_prob, current_cdf_val, last_cdf_val, distance, s_data and empirical_df so it
matches project style (consistent spaces around operators, braces on same
column, and proper indentation). Ensure the ternary logic remains unchanged
(distance = 0 when current_cdf_val >= 1.0 || current_cdf_val <= 0.0, otherwise
compute distance = s_data * pow((empirical_df - current_cdf_val), 2) /
current_cdf_val / (1. - current_cdf_val) * bin_prob) and commit the formatted
result.
---
Outside diff comments:
In `@src/GoodnessOfFit.cc`:
- Around line 295-304: qVals_ is being indexed by the loop index i (qVals_[i])
which assumes binNames_ and datasets share the same order; instead, store the
per-category statistic keyed by the dataset/PDF category name so branches get
the correct value. Change the assignment so it uses the category identifier from
the split dataset (use cat_data->GetName() or the PDF name obtained via
sim->getPdf(cat_data->GetName())) as the key into qVals_ (e.g.,
qVals_[categoryName] = EvaluateADDistance(...)) rather than qVals_[i]; ensure
the rest of the code that reads qVals_ expects the same key type (string) and
adjust any consumers accordingly (symbols: qVals_, binNames_, datasets,
cat_data->GetName(), sim->getPdf, EvaluateADDistance).
🪄 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: 3803ce15-6446-4b6e-b9fe-7e844e2eeebb
📒 Files selected for processing (2)
scripts/plotGof.pysrc/GoodnessOfFit.cc
| 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 | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the actual DrawWarning function and surrounding context
head -100 scripts/plotGof.py | tail -60Repository: 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 2Repository: 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.pyRepository: 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:
- 1: https://root.cern/manual/python/
- 2: https://roottalk.root.cern.narkive.com/LdrEomW1/pyroot-and-tfile-deletion
- 3: https://wiki.physik.uzh.ch/lhcb/root:ownershippyroot?idx=tt
- 4: https://root.cern/root/html534/guides/users-guide/ObjectOwnership.html
- 5: https://root.cern/manual/object_ownership
- 6: https://root.cern.ch/doc/latest-stable/classTPaveText.html
DrawWarning has several issues worth addressing.
-
Overlapping NDC coordinates (visible bug).
warningtext1(line 47) andwarningtext2(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. -
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 callROOT.SetOwnership(warningtext1, False)immediately after creation to transfer ownership to the canvas. Additionally, adopt a consistent convention: don't mix callingDraw()inside the function with returning objects for the caller to draw. -
obsis an implicit free variable. Lines 71 and 82 referenceobs.GetX()[0], butobsis not a parameter—it is picked up from module scope. Pass the observed x-value (orobsitself) as an explicit parameter to make the function self-contained and robust against future refactors. -
Unreachable code. Line 84 (
return None) is dead code; both theifandelsebranches above already return. -
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 pavesCall 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.
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (0.00%) is below the target coverage (98.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #1248 +/- ##
==========================================
- Coverage 20.89% 20.89% -0.01%
==========================================
Files 195 195
Lines 26310 26311 +1
Branches 3947 3947
==========================================
Hits 5498 5498
- Misses 20812 20813 +1
🚀 New features to boost your workflow:
|
|
Hi, thanks for the contribution! could you please fix this failing test? https://github.com/cms-analysis/HiggsAnalysis-CombinedLimit/actions/runs/24781067248/job/72512036911?pr=1248 |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
scripts/plotGof.py (2)
46-106:⚠️ Potential issue | 🟠 MajorMake
DrawTextself-contained by passing observed x explicitly.
DrawTextstill readsobsfrom outer scope (Line 93, Line 104). This hidden dependency is brittle and can break with refactors or reordered calls. Passobs_xinto the function and use it in both call sites.♻️ Proposed fix
-def DrawText(stat, ntoys, pval, arrowrange, underflow, overflow): +def DrawText(stat, ntoys, pval, arrowrange, underflow, overflow, obs_x=None): @@ - warningtext2.AddText(f"observed value not in range, at {obs.GetX()[0]:.2f}") + warningtext2.AddText(f"observed value not in range, at {obs_x:.2f}") @@ - elif arrowrange: - warningtext1.AddText(f"observed value not in range, at {obs.GetX()[0]:.2f}") + elif arrowrange and obs_x is not None: + warningtext1.AddText(f"observed value not in range, at {obs_x:.2f}") @@ - texts = DrawText(args.statistic, toy_graph.GetN(), pValue, arrow_not_in_range, underflow_count, overflow_count) + texts = DrawText(args.statistic, toy_graph.GetN(), pValue, arrow_not_in_range, underflow_count, overflow_count, obs.GetX()[0]) @@ - texts = DrawText(args.statistic, toy_graph.GetN(), pValue, arrow_not_in_range, underflow_count, overflow_count) + texts = DrawText(args.statistic, toy_graph.GetN(), pValue, arrow_not_in_range, underflow_count, overflow_count, obs.GetX()[0])Also applies to: 231-233, 300-302
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/plotGof.py` around lines 46 - 106, DrawText currently reads obs from outer scope (uses obs.GetX()[0]) making it brittle; change the signature of DrawText to accept an explicit obs_x (float) parameter and replace both occurrences of obs.GetX()[0] with that obs_x (formatted as before), then update every call site that invokes DrawText to pass the observed x value (e.g., obs.GetX()[0]) into the new parameter so DrawText becomes self-contained.
230-230:⚠️ Potential issue | 🟠 MajorUse the real histogram min edge for out-of-range detection.
Line 230 uses
GetBinLowEdge(0)for the lower bound, which is the underflow bin edge (xmin - binWidth), notxmin. This can miss under-range observed values just below the plotted range. Compare againstGetBinLowEdge(1)(orGetXaxis().GetXmin()) instead.♻️ Proposed fix
- arrow_not_in_range = (obs.GetX()[0] > toy_hist.GetBinLowEdge(args.bins + 1)) or (obs.GetX()[0] < toy_hist.GetBinLowEdge(0)) + arrow_not_in_range = (obs.GetX()[0] > toy_hist.GetBinLowEdge(args.bins + 1)) or (obs.GetX()[0] < toy_hist.GetBinLowEdge(1))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/plotGof.py` at line 230, The out-of-range check uses the underflow bin edge via toy_hist.GetBinLowEdge(0); change it to compare obs.GetX()[0] against the real histogram minimum (e.g., toy_hist.GetBinLowEdge(1) or toy_hist.GetXaxis().GetXmin()) so arrow_not_in_range correctly flags values below the plotted range; update the expression that sets arrow_not_in_range (which references obs.GetX()[0] and toy_hist.GetBinLowEdge(...)) to use the correct lower bound.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@scripts/plotGof.py`:
- Around line 46-106: DrawText currently reads obs from outer scope (uses
obs.GetX()[0]) making it brittle; change the signature of DrawText to accept an
explicit obs_x (float) parameter and replace both occurrences of obs.GetX()[0]
with that obs_x (formatted as before), then update every call site that invokes
DrawText to pass the observed x value (e.g., obs.GetX()[0]) into the new
parameter so DrawText becomes self-contained.
- Line 230: The out-of-range check uses the underflow bin edge via
toy_hist.GetBinLowEdge(0); change it to compare obs.GetX()[0] against the real
histogram minimum (e.g., toy_hist.GetBinLowEdge(1) or
toy_hist.GetXaxis().GetXmin()) so arrow_not_in_range correctly flags values
below the plotted range; update the expression that sets arrow_not_in_range
(which references obs.GetX()[0] and toy_hist.GetBinLowEdge(...)) to use the
correct lower bound.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8335d28c-db53-4246-b0b9-23ddf4ee18ed
📒 Files selected for processing (2)
scripts/plotGof.pysrc/GoodnessOfFit.cc
|
IMO it looks safe enough to merge and, in the worst case scenario, correct in case something weird comes up or is spotted by other users. But would like to wait for a second check from @anigamova. |
2 participants
datasetsList.reset(): To avoid random segmentation faults that may happen.RooAbsPdf *cat_pdf = sim->getPdf(cat_data->GetName()): The list of categories indatasetsandbinNames_are not sorted identically, so here the CDFs of different categories may have been compared. Now, make sure the same category is taken from simulation as from data.distance...: The change I made here isn't necessary, but it might be "healthier" like this to ensure we're not dividing by zero.Summary by CodeRabbit
Bug Fixes
Improvements