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 c54b845d9fd88..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); @@ -1485,7 +1496,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(-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)