[RF] Add RooFit::makeResidHist() and makePullHist() with bin integration by guitargeek · Pull Request #23357 · root-project/root

guitargeek · 2026-09-14T08:31:53Z

Residual and pull histograms of binned data are currently created from a plotted RooCurve (RooPlot::residHist()/pullHist()), which interpolates or averages a sampled polyline of the pdf. For strongly peaked pdfs, this produces a systematic 'wiggle' pattern in residual panels and unrealistically large chi-square values when binned data is presented against the result of an unbinned fit, even when the fit is perfectly fine. See this ROOT forum thread for a study of the problem:

https://root-forum.cern.ch/t/comparison-of-binned-data-to-results-of-unbinned-fit/64567

The new free functions take the fit model (any RooAbsReal) and the data (RooAbsData) directly and integrate the model exactly over each bin with RooAbsReal::createIntegral(), avoiding the curve-based bias. The expectation is normalized to the data weight, also correctly restricted to the range if Range() is passed. Unbinned input data is binned internally, and the usual Binning(), Normalization(), and DataError() command arguments are supported. For projections or components of composite models, project the model and data first or pass the component pdf with Normalization().

A warning is printed once per process when the curve-based RooPlot::residHist()/pullHist() are used, pointing to the new functions. The RooPlot functions are scheduled for deprecation in ROOT v6.44.

The rf109 tutorial and the RooHist unit tests are updated/extended to the new interface.

This direction was also discussed with @alread2223 over email.

🤖 Done with the help of AI

Residual and pull histograms of binned data are currently created from a plotted RooCurve (RooPlot::residHist()/pullHist()), which interpolates or averages a sampled polyline of the pdf. For strongly peaked pdfs, this produces a systematic 'wiggle' pattern in residual panels and unrealistically large chi-square values when binned data is presented against the result of an unbinned fit, even when the fit is perfectly fine. See this ROOT forum thread for a study of the problem: https://root-forum.cern.ch/t/comparison-of-binned-data-to-results-of-unbinned-fit/64567 The new free functions take the fit model (any RooAbsReal) and the data (RooAbsData) directly and integrate the model exactly over each bin with RooAbsReal::createIntegral(), avoiding the curve-based bias. The expectation is normalized to the data weight, also correctly restricted to the range if Range() is passed. Unbinned input data is binned internally, and the usual Binning(), Normalization(), and DataError() command arguments are supported. For projections or components of composite models, project the model and data first or pass the component pdf with Normalization(). A warning is printed once per process when the curve-based RooPlot::residHist()/pullHist() are used, pointing to the new functions. The RooPlot functions are scheduled for deprecation in ROOT v6.44. The rf109 tutorial and the RooHist unit tests are updated/extended to the new interface. 🤖 Done with the help of AI

github-actions · 2026-09-14T13:14:01Z

Test Results

23 files 23 suites 3d 18h 20m 38s ⏱️
3 878 tests 3 878 ✅ 0 💤 0 ❌
79 174 runs 79 174 ✅ 0 💤 0 ❌

Results for commit 0c0dfe6.

hageboeck

Hello, below a few things I was wondering about on the first pass.

hageboeck · 2026-09-16T13:42:37Z

+/// bin, instead of interpolating the plotted curve.
 RooHist* RooPlot::residHist(const char* histname, const char* curvename, bool normalize, bool useAverage) const
 {
+  static std::atomic<bool> warned{false};


Is there a particular reason for using a static atomic here?

Although I'm sure it works, I doubt the plotting and integration code is thread safe in the way that it should be used from multiple threads, is it? So this could possibly be done without the atomic and the (small) risk that this warning is seen more than once.

hageboeck · 2026-09-16T13:44:41Z

-   RooHist *hpull = frame1->pullHist();
+   // Construct a histogram with the residuals of the data w.r.t. the model.
+   // The Binning() argument selects how the unbinned dataset is binned.
+   std::unique_ptr<RooHist> hresid{makeResidHist(gauss, *data, Binning(40))};


One could use RooFit::makeResidHist just to clarify where it came from. Similar below.
What do you think?

hageboeck · 2026-09-16T13:50:41Z

+/// the bin (with RooAbsReal::createIntegral()), normalized to the weight of
+/// the data inside the normalization range. Unbinned input data is binned


That it's a weight is an implementation detail. Most users probably think of data as counts.

Suggested change

/// the bin (with RooAbsReal::createIntegral()), normalized to the weight of

/// the data inside the normalization range. Unbinned input data is binned

/// the bin (with RooAbsReal::createIntegral()), normalized to

/// the data inside the normalization range. Unbinned input data is binned

hageboeck · 2026-09-16T13:52:15Z

+///   - `Range(lo, hi)` / `Range("name")` : the range that the model
+///     expectation is normalized in. Only bins inside this range get points


Suggested change

/// - `Range(lo, hi)` / `Range("name")` : the range that the model

/// expectation is normalized in. Only bins inside this range get points

/// - `Range(lo, hi)` / `Range("name")` : compute residuals only in this range.

/// The model expectation is normalized in this range.

hageboeck · 2026-09-16T13:53:37Z

+/// \note For internal use of RooAbsReal::createIntegral(), a scratch named
+/// range is left on the model's observable after the call.


scratch named range?

A range with the name "xxx" is declared?

hageboeck · 2026-09-16T14:01:03Z

+   std::unique_ptr<RooHist> pull{makePullHist(model.gauss, binData)};
+   ASSERT_EQ(resid->GetN(), kNBins);
+   ASSERT_EQ(pull->GetN(), kNBins);
+   expectExactResiduals(*resid, hist, 1e-3);


Why only 1.E-3? Does the algo not manage better?

Well, on the other hand, for a residual plot its accurate enough ...

hageboeck · 2026-09-16T14:03:23Z

+   std::unique_ptr<RooHist> resid{makeResidHist(gauss, *data, Binning(40))};
+   std::unique_ptr<RooHist> pull{makePullHist(gauss, *data, Binning(40))};
+   EXPECT_EQ(resid->GetN(), 40);
+   EXPECT_EQ(pull->GetN(), 40);


On the future, consider declaring the number of bins somewhere, so later changes change all instances.

hageboeck · 2026-09-16T14:03:58Z

+   for (int i = 0; i < pull->GetN(); ++i) {
+      if (pull->GetErrorYhigh(i) == 0.)
+         continue;
+      EXPECT_LT(std::abs(pull->GetPointY(i)), 5.) << "point " << i;


5 is a large pull, isn't it?

hageboeck · 2026-09-16T14:10:04Z

+   // call to not clobber existing ranges of the same name on the user's
+   // variable; they are shared ranges and stay on the variable after the
+   // call.
+   static std::atomic<unsigned long> nScratchRanges{0};


You expect to call this from multiple threads?

As a slightly slower algo that has the advantage of using less global memory and working without static init & destroy (remember that these statics need to be allocated and finalised under lock by the C++ runtime):

Put a local counter

Craft a range name using that number

Check if that range exists for the var. If not, just that, otherwise increment counter and repeat.

hageboeck · 2026-09-16T14:16:24Z

+         continue;
+
+      intVar->setRange(rangeNameBin.c_str(), binLo, binHi);
+      const double fraction = fitModel.createIntegral(obsSet, normSet, rangeNameBin.c_str())->getVal() / normIntegral;


I don't remember this, but can the integral be reused?

guitargeek self-assigned this Sep 14, 2026

guitargeek requested review from couet, dpiparo and hageboeck as code owners September 14, 2026 08:31

guitargeek added new feature in:RooFit AI-Assisted labels Sep 14, 2026

dpiparo approved these changes Sep 15, 2026

View reviewed changes

hageboeck reviewed Sep 16, 2026

View reviewed changes

		/// the bin (with RooAbsReal::createIntegral()), normalized to the weight of
		/// the data inside the normalization range. Unbinned input data is binned

		/// - `Range(lo, hi)` / `Range("name")` : the range that the model
		/// expectation is normalized in. Only bins inside this range get points

		/// \note For internal use of RooAbsReal::createIntegral(), a scratch named
		/// range is left on the model's observable after the call.

Uh oh!

Conversation

guitargeek commented Sep 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Sep 14, 2026

Test Results

Uh oh!

hageboeck left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

guitargeek commented Sep 14, 2026 •

edited

Loading