From 9bb87f7c575242d95853bed2df233bdd0806fddd Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Thu, 25 Jun 2026 21:06:48 +0200 Subject: [PATCH 1/2] [RF] Fix binning used in 2D RooDataHist weight interpolation weightInterpolated() took the y-dimension binning from the caller's variable instead of the one owned by the RooDataHist. These can differ (e.g. RooHistPdf passes its own observable clones), and since the bin indexing relies on _idxMult and centralIdx (expressed in the internal binning), a mismatch yields out-of-range bins and a wrong (often zero) weight. Use the internal binning, like calcTreeIndex() and interpolateDim() already do. --- roofit/roofitcore/src/RooDataHist.cxx | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/roofit/roofitcore/src/RooDataHist.cxx b/roofit/roofitcore/src/RooDataHist.cxx index c54b845d9fd88..52a8b914c87a2 100644 --- a/roofit/roofitcore/src/RooDataHist.cxx +++ b/roofit/roofitcore/src/RooDataHist.cxx @@ -1485,7 +1485,14 @@ double RooDataHist::weightInterpolated(const RooArgSet& bin, int intOrder, bool double xval = realX.getVal() ; double yval = realY.getVal() ; - RooAbsBinning const& binningY = realY.getBinning(); + // Use the internal binning of the y variable, not the binning of the + // variable passed in `bin`. The latter may be a different object than the + // one owned by this RooDataHist (e.g. the histogram observable clone of a + // RooHistPdf), with an unrelated default binning. The bin indexing below + // relies on `_idxMult` and `centralIdx`, which are both expressed in terms + // of the internal binning, so the y binning must match it too. This mirrors + // what calcTreeIndex() and interpolateDim() do for the other dimensions. + RooAbsBinning const& binningY = static_cast(*_vars[varInfo.realVarIdx2]).getBinning(); int ybinC = binningY.binNumber(yval) ; int ybinLo = ybinC-intOrder/2 - ((yval Date: Thu, 25 Jun 2026 17:43:43 +0200 Subject: [PATCH 2/2] [RF] Default RooFit variables to zero bins instead of 100 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A freshly-constructed RooRealVar (and RooErrorVar) used to have a default binning of 100 bins. Since 100 is also a valid user choice, there was no way to tell whether a variable's bin count was deliberately set or just left at the default. This caused redundant "nbins" fields to be written when serializing workspaces to HS3 JSON. RooAbsRealLValue::getBins() now returns 0 until a binning is explicitly set, so HS3 can write "nbins" only when getBins() != 0 (previously the heuristic was getBins() != 100, which both missed a deliberate 100 and churned on the default). The historical default is re-injected, via the new constant RooAbsRealLValue::DefaultNBins (100), wherever code legitimately relied on it: - unbinned-dataset plotting (RooAbsRealLValue::frame and overloads) - generateBinned / direct RooDataHist construction (materialized on the dataset's own variable clone in RooDataHist::initialize, so the user's variable is untouched) - createHistogram (RooAbsRealLValue and RooAbsData) - the RooHist "P" draw-option path So plotting, generating binned data and creating histograms from a default-constructed variable behave exactly as before. Also fixes a latent bug where RooUniformBinning's default constructor left its members uninitialized, and guards RooUniformBinning::setRange against division by zero for a zero-bin binning. ROOT I/O is unaffected: binnings are restored from the persisted object, so only newly-constructed variables default to zero bins. 🤖 Done with the help of AI. --- README/ReleaseNotes/v642/index.md | 15 ++++++++++ roofit/hs3/src/RooJSONFactoryWSTool.cxx | 2 +- roofit/roofitcore/inc/RooAbsRealLValue.h | 6 ++++ roofit/roofitcore/inc/RooUniformBinning.h | 10 +++---- roofit/roofitcore/src/RooAbsData.cxx | 4 ++- roofit/roofitcore/src/RooAbsRealLValue.cxx | 30 +++++++++++++------ roofit/roofitcore/src/RooDataHist.cxx | 11 +++++++ roofit/roofitcore/src/RooErrorVar.cxx | 2 +- roofit/roofitcore/src/RooHist.cxx | 18 ++++++++--- roofit/roofitcore/src/RooRealVar.cxx | 10 +++---- roofit/roofitcore/src/RooUniformBinning.cxx | 2 +- roofit/roofitcore/test/stressRooFit_tests.h | 2 ++ .../roofitcore/test/testRooSimultaneous.cxx | 5 +++- roofit/roostats/src/AsymptoticCalculator.cxx | 19 ++++++++++++ .../roofit/roofit/rf306_condpereventerrors.C | 2 ++ .../roofit/roofit/rf306_condpereventerrors.py | 2 ++ 16 files changed, 112 insertions(+), 28 deletions(-) diff --git a/README/ReleaseNotes/v642/index.md b/README/ReleaseNotes/v642/index.md index 284cce2292028..a7b02dc535212 100644 --- a/README/ReleaseNotes/v642/index.md +++ b/README/ReleaseNotes/v642/index.md @@ -88,6 +88,21 @@ Users are strongly encouraged to switch to the vectorized CPU backend if they ar If the vectorized backend does not work for a given use case, **please report it by opening an issue on the ROOT GitHub repository**. +### Default binning of RooFit variables changed to zero bins + +A freshly-constructed `RooRealVar` (or `RooErrorVar`) no longer has a default binning of 100 bins. +Instead, `RooAbsRealLValue::getBins()` now returns `0` until a binning is explicitly set (e.g. via `RooRealVar::setBins()`). +This makes it possible to distinguish a variable whose binning was deliberately chosen from one that was left at the default, which avoids +writing redundant `nbins` fields when serializing workspaces to HS3 JSON. + +For the cases that previously relied on the default of 100 bins, that value is now injected by the relevant routine: +unbinned-dataset plotting (`RooAbsRealLValue::frame()`), `RooAbsPdf::generateBinned()`, and `RooAbsRealLValue::createHistogram()` +all fall back to `RooAbsRealLValue::DefaultNBins` (100) when the variable has no binning set. +As a result, plotting, generating binned data and creating histograms from a default-constructed variable behave exactly as before. + +Code that reads `getBins()`/`numBins()` of a bare variable and expected the value `100` should either set the binning explicitly, +or read the bin count from the relevant histogram or plot frame instead. + ## Graphics and GUI ### Store canvas as HTML file diff --git a/roofit/hs3/src/RooJSONFactoryWSTool.cxx b/roofit/hs3/src/RooJSONFactoryWSTool.cxx index 65634d6da2a40..b069097820e48 100644 --- a/roofit/hs3/src/RooJSONFactoryWSTool.cxx +++ b/roofit/hs3/src/RooJSONFactoryWSTool.cxx @@ -1076,7 +1076,7 @@ void RooJSONFactoryWSTool::exportVariable(const RooAbsArg *v, JSONNode &node, bo var["min"] << rrv->getMin(); var["max"] << rrv->getMax(); } - if (rrv->getBins() != 100 && storeBins) { + if (rrv->getBins() != 0 && storeBins) { var["nbins"] << rrv->getBins(); } _domains->readVariable(*rrv); diff --git a/roofit/roofitcore/inc/RooAbsRealLValue.h b/roofit/roofitcore/inc/RooAbsRealLValue.h index 849a33509d47d..33b1e066b58ab 100644 --- a/roofit/roofitcore/inc/RooAbsRealLValue.h +++ b/roofit/roofitcore/inc/RooAbsRealLValue.h @@ -76,6 +76,12 @@ class RooAbsRealLValue : public RooAbsReal, public RooAbsLValue { /// Get number of bins of currently defined range. /// \param name Optionally, request number of bins for range with given name. virtual Int_t getBins(const char* name=nullptr) const { return getBinning(name).numBins(); } + + /// Historical default number of bins, injected by routines that need a + /// concrete bin count when a variable has no binning explicitly set + /// (i.e. when getBins() returns 0). Used for unbinned-data plotting, + /// generateBinned() and createHistogram(). + static constexpr int DefaultNBins = 100; /// Get minimum of currently defined range. /// \param name Optionally, request minimum of range with given name. virtual double getMin(const char* name=nullptr) const { return getBinning(name).lowBound(); } diff --git a/roofit/roofitcore/inc/RooUniformBinning.h b/roofit/roofitcore/inc/RooUniformBinning.h index b30ff9b78da02..76f77241b9b20 100644 --- a/roofit/roofitcore/inc/RooUniformBinning.h +++ b/roofit/roofitcore/inc/RooUniformBinning.h @@ -23,7 +23,7 @@ class RooUniformBinning : public RooAbsBinning { public: RooUniformBinning(const char* name=nullptr) : RooAbsBinning{name} {} - RooUniformBinning(double xlo, double xhi, Int_t nBins, const char* name=nullptr) ; + RooUniformBinning(double xlo, double xhi, Int_t nBins=0, const char* name=nullptr) ; RooUniformBinning(const RooUniformBinning& other, const char* name=nullptr) ; RooAbsBinning* clone(const char* name=nullptr) const override { return new RooUniformBinning(*this,name?name:GetName()) ; } @@ -48,10 +48,10 @@ class RooUniformBinning : public RooAbsBinning { protected: mutable std::vector _array; ///numBins() == 0) { + binning[0] = new RooUniformBinning(getMin(), getMax(), DefaultNBins) ; + ownBinning[0] = true ; + } } if (pc.hasProcessed("YVar")) { @@ -617,6 +621,10 @@ TH1* RooAbsRealLValue::createHistogram(const char *name, const RooLinkedList& cm ownBinning[1] = true ; } else { binning[1] = &yvar.getBinning() ; + if (binning[1]->numBins() == 0) { + binning[1] = new RooUniformBinning(yvar.getMin(), yvar.getMax(), DefaultNBins) ; + ownBinning[1] = true ; + } } } @@ -634,6 +642,10 @@ TH1* RooAbsRealLValue::createHistogram(const char *name, const RooLinkedList& cm ownBinning[2] = true ; } else { binning[2] = &zvar.getBinning() ; + if (binning[2]->numBins() == 0) { + binning[2] = new RooUniformBinning(zvar.getMin(), zvar.getMax(), DefaultNBins) ; + ownBinning[2] = true ; + } } } @@ -668,7 +680,7 @@ TH1F *RooAbsRealLValue::createHistogram(const char *name, const char *yAxisLabel RooArgList list(*this) ; double xlo = getMin() ; double xhi = getMax() ; - Int_t nbins = getBins() ; + Int_t nbins = getBins()!=0 ? getBins() : DefaultNBins ; // coverity[ARRAY_VS_SINGLETON] return static_cast(createHistogram(name, list, yAxisLabel, &xlo, &xhi, &nbins)); @@ -755,8 +767,8 @@ TH2F *RooAbsRealLValue::createHistogram(const char *name, const RooAbsRealLValue } if (!nBins2) { - nbins_fit[0] = getBins() ; - nbins_fit[1] = yvar.getBins() ; + nbins_fit[0] = getBins()!=0 ? getBins() : DefaultNBins ; + nbins_fit[1] = yvar.getBins()!=0 ? yvar.getBins() : DefaultNBins ; nBins2 = nbins_fit ; } @@ -836,9 +848,9 @@ TH3F *RooAbsRealLValue::createHistogram(const char *name, const RooAbsRealLValue } if (!nBins2) { - nbins_fit[0] = getBins() ; - nbins_fit[1] = yvar.getBins() ; - nbins_fit[2] = zvar.getBins() ; + nbins_fit[0] = getBins()!=0 ? getBins() : DefaultNBins ; + nbins_fit[1] = yvar.getBins()!=0 ? yvar.getBins() : DefaultNBins ; + nbins_fit[2] = zvar.getBins()!=0 ? zvar.getBins() : DefaultNBins ; nBins2 = nbins_fit ; } diff --git a/roofit/roofitcore/src/RooDataHist.cxx b/roofit/roofitcore/src/RooDataHist.cxx index 52a8b914c87a2..5a91943a5f9df 100644 --- a/roofit/roofitcore/src/RooDataHist.cxx +++ b/roofit/roofitcore/src/RooDataHist.cxx @@ -798,6 +798,17 @@ void RooDataHist::initialize(const char* binningName, bool fillTree) } } + // If the variable has no binning explicitly set (the default for a + // freshly-constructed RooRealVar, which reports zero bins), materialize the + // historical default binning. _vars holds this dataset's own clones (see + // RooAbsData::initializeVars, which addClone's the input variables), so this + // does not affect the user's original variable. + if (RooRealVar* rrv = dynamic_cast(_vars[i])) { + if (rrv->getBins() == 0) { + rrv->setBinning(RooUniformBinning(rrv->getMin(), rrv->getMax(), RooAbsRealLValue::DefaultNBins)); + } + } + auto lvarg = dynamic_cast(_vars[i]); assert(lvarg); _lvvars.push_back(lvarg); diff --git a/roofit/roofitcore/src/RooErrorVar.cxx b/roofit/roofitcore/src/RooErrorVar.cxx index af83a963f229e..d8abbcfe06207 100644 --- a/roofit/roofitcore/src/RooErrorVar.cxx +++ b/roofit/roofitcore/src/RooErrorVar.cxx @@ -44,7 +44,7 @@ RooErrorVar::RooErrorVar(const char *name, const char *title, const RooRealVar& RooAbsRealLValue(name,title), _realVar("realVar","RooRealVar with error",this,(RooAbsReal&)input) { - _binning = std::make_unique(-1,1,100) ; + _binning = std::make_unique(-1,1) ; } diff --git a/roofit/roofitcore/src/RooHist.cxx b/roofit/roofitcore/src/RooHist.cxx index 661ca5d3e1bdc..9d12d66dc53a8 100644 --- a/roofit/roofitcore/src/RooHist.cxx +++ b/roofit/roofitcore/src/RooHist.cxx @@ -28,6 +28,7 @@ a RooPlot. #include "RooHist.h" #include "RooAbsRealLValue.h" +#include "RooUniformBinning.h" #include "RooHistError.h" #include "RooCurve.h" #include "RooMsgService.h" @@ -302,13 +303,22 @@ RooHist::RooHist(const RooAbsReal &f, RooAbsRealLValue &x, double xErrorFrac, do RooProduct scaledFunc{"scaled_func", "scaled_func", {f, RooFit::RooConst(scaleFactor)}}; std::unique_ptr funcPtr{scaledFunc.bindVars(x, normVars, true)}; - // calculate the points to add to our curve - int xbins = x.numBins(); + // calculate the points to add to our curve. A variable with no binning + // explicitly set reports zero bins; in that case fall back to the historical + // default bin count for plotting, using a local binning so that the user's + // variable is not mutated. + const RooAbsBinning *xbinning = &x.getBinning(); + std::unique_ptr defaultBinning; + if (x.numBins() == 0) { + defaultBinning = std::make_unique(x.getMin(), x.getMax(), RooAbsRealLValue::DefaultNBins); + xbinning = defaultBinning.get(); + } + int xbins = xbinning->numBins(); RooArgSet nset; if(normVars) nset.add(*normVars); for(int i=0; ibinCenter(i); + double xwidth = xbinning->binWidth(i); Axis_t xval_ax = xval; double yval = (*funcPtr)(&xval); double yerr = std::sqrt(yval); diff --git a/roofit/roofitcore/src/RooRealVar.cxx b/roofit/roofitcore/src/RooRealVar.cxx index 589691d803595..58458da80fa08 100644 --- a/roofit/roofitcore/src/RooRealVar.cxx +++ b/roofit/roofitcore/src/RooRealVar.cxx @@ -104,7 +104,7 @@ RooRealVar::RooRealVar() : _error(0), _asymErrLo(0), _asymErrHi(0), _binning(n RooRealVar::RooRealVar(const char *name, const char *title, double value, const char *unit) : RooAbsRealLValue(name, title, unit), _error(-1), _asymErrLo(1), _asymErrHi(-1), - _binning(new RooUniformBinning(-1,1,100)) + _binning(new RooUniformBinning(-1,1)) { _value = value ; _fast = true ; @@ -122,7 +122,7 @@ RooRealVar::RooRealVar(const char *name, const char *title, double minValue, double maxValue, const char *unit) : RooAbsRealLValue(name, title, unit), _error(-1), _asymErrLo(1), _asymErrHi(-1), - _binning(new RooUniformBinning(minValue,maxValue,100)) + _binning(new RooUniformBinning(minValue,maxValue)) { _fast = true ; @@ -157,7 +157,7 @@ RooRealVar::RooRealVar(const char *name, const char *title, double value, double minValue, double maxValue, const char *unit) : RooAbsRealLValue(name, title, unit), _error(-1), _asymErrLo(1), _asymErrHi(-1), - _binning(new RooUniformBinning(minValue,maxValue,100)) + _binning(new RooUniformBinning(minValue,maxValue)) { _fast = true ; setRange(minValue,maxValue) ; @@ -815,7 +815,7 @@ void RooRealVar::writeToStream(ostream &os, bool compact) const os << " - +INF) "; } - if (getBins() != 100) { + if (getBins() != 0) { os << "B(" << getBins() << ") "; } @@ -868,7 +868,7 @@ void RooRealVar::printExtras(ostream& os) const } os << ") " ; - if (getBins()!=100) { + if (getBins()!=0) { os << "B(" << getBins() << ") " ; } diff --git a/roofit/roofitcore/src/RooUniformBinning.cxx b/roofit/roofitcore/src/RooUniformBinning.cxx index af874a4cb48ef..6829f67678b04 100644 --- a/roofit/roofitcore/src/RooUniformBinning.cxx +++ b/roofit/roofitcore/src/RooUniformBinning.cxx @@ -69,7 +69,7 @@ void RooUniformBinning::setRange(double xlo, double xhi) _xlo = xlo ; _xhi = xhi ; - _binw = (xhi-xlo)/_nbins ; + _binw = _nbins>0 ? (xhi-xlo)/_nbins : 0.0 ; // Delete any out-of-date boundary arrays at this point _array.clear(); diff --git a/roofit/roofitcore/test/stressRooFit_tests.h b/roofit/roofitcore/test/stressRooFit_tests.h index a29918b92c2f5..6f7ab4ca0a854 100644 --- a/roofit/roofitcore/test/stressRooFit_tests.h +++ b/roofit/roofitcore/test/stressRooFit_tests.h @@ -1722,6 +1722,8 @@ class TestBasic306 : public RooUnitTest { // Plot decay_gm(dt|dterr) at various values of dterr RooPlot *frame = dt.frame(Title("Slices of decay(dt|dterr) at various dterr")); + // Sample dterr in 100 bins to select the slice values below with setBin() + dterr.setBins(100); for (int ibin = 0; ibin < 100; ibin += 20) { dterr.setBin(ibin); decay_gm.plotOn(frame, Normalization(5.), Name(Form("curve_slice_%d", ibin))); diff --git a/roofit/roofitcore/test/testRooSimultaneous.cxx b/roofit/roofitcore/test/testRooSimultaneous.cxx index a9b2f34ccb880..31948fb7d2657 100644 --- a/roofit/roofitcore/test/testRooSimultaneous.cxx +++ b/roofit/roofitcore/test/testRooSimultaneous.cxx @@ -682,7 +682,10 @@ TEST(RooSimultaneous, PlotProjWDataExtended) auto integrateLastCurve = [](RooPlot *plot) { const double xmin = plot->getPlotVar()->getMin(); const double xmax = plot->getPlotVar()->getMax(); - return plot->getCurve()->average(xmin, xmax) * plot->getPlotVar()->numBins(); + // Multiply by the number of bins of the plot frame (not the plot + // variable, which no longer carries a default binning) to turn the + // average events-per-bin of the curve back into a total event count. + return plot->getCurve()->average(xmin, xmax) * plot->GetNbinsX(); }; constexpr double tol = 0.01; // tolerate 1 % sampling error diff --git a/roofit/roostats/src/AsymptoticCalculator.cxx b/roofit/roostats/src/AsymptoticCalculator.cxx index 899c604faeb1d..12c9f8a611589 100644 --- a/roofit/roostats/src/AsymptoticCalculator.cxx +++ b/roofit/roostats/src/AsymptoticCalculator.cxx @@ -1111,6 +1111,20 @@ RooAbsData * GenerateAsimovDataSinglePdf(const RooAbsPdf & pdf, const RooArgSet RooArgList obsList(*obs); + // The Asimov data set is built by looping over the bins of the + // observables. A freshly-constructed RooRealVar now defaults to zero bins, + // so temporarily materialize the historical default binning for any + // observable that has no explicit binning set. The change is undone right + // after filling the bins so that the model observables are left untouched. + std::vector obsWithDefaultBinning; + for (auto *arg : obsList) { + auto *rrv = dynamic_cast(arg); + if (rrv && rrv->getBins() == 0) { + rrv->setBins(RooRealVar::DefaultNBins); + obsWithDefaultBinning.push_back(rrv); + } + } + // loop on observables and on the bins if (printLevel >= 2) { oocoutI(nullptr,Generation) << "Generating Asimov data for pdf " << pdf.GetName() << std::endl; @@ -1122,6 +1136,11 @@ RooAbsData * GenerateAsimovDataSinglePdf(const RooAbsPdf & pdf, const RooArgSet double binVolume = 1; int nbins = 0; FillBins(pdf, obsList, *asimovData, obsIndex, binVolume, nbins); + + // restore the zero-bins default on observables that had no explicit binning + for (auto *rrv : obsWithDefaultBinning) { + rrv->setBins(0); + } if (printLevel >= 2) oocoutI(nullptr,Generation) << "filled from " << pdf.GetName() << " " << nbins << " nbins " << " volume is " << binVolume << std::endl; diff --git a/tutorials/roofit/roofit/rf306_condpereventerrors.C b/tutorials/roofit/roofit/rf306_condpereventerrors.C index 091eacb267f41..d068e34f8aa10 100644 --- a/tutorials/roofit/roofit/rf306_condpereventerrors.C +++ b/tutorials/roofit/roofit/rf306_condpereventerrors.C @@ -68,6 +68,8 @@ void rf306_condpereventerrors() // Plot decay_gm(dt|dterr) at various values of dterr RooPlot *frame = dt.frame(Title("Slices of decay(dt|dterr) at various dterr")); + // Sample dterr in 100 bins to select the slice values below with setBin() + dterr.setBins(100); for (Int_t ibin = 0; ibin < 100; ibin += 20) { dterr.setBin(ibin); decay_gm.plotOn(frame, Normalization(5.)); diff --git a/tutorials/roofit/roofit/rf306_condpereventerrors.py b/tutorials/roofit/roofit/rf306_condpereventerrors.py index aac4ab0a9dd74..3139d63a650a2 100644 --- a/tutorials/roofit/roofit/rf306_condpereventerrors.py +++ b/tutorials/roofit/roofit/rf306_condpereventerrors.py @@ -58,6 +58,8 @@ # Plot decay_gm(dt|dterr) at various values of dterr frame = dt.frame(Title="Slices of decay(dt|dterr) at various dterr") +# Sample dterr in 100 bins to select the slice values below with setBin() +dterr.setBins(100) for ibin in range(0, 100, 20): dterr.setBin(ibin) decay_gm.plotOn(frame, Normalization=5.0)