[RF] Make RooBinWidthFunction depend directly on observables

roofit/hs3/src/JSONFactories_RooFitCore.cxx

@@ -341,6 +341,11 @@ template <bool DivideByBinWidth>
 bool importBinWidthFunction(RooJSONFactoryWSTool *tool, const JSONNode &p)
 {
    std::string name(RooJSONFactoryWSTool::name(p));
+   if (p.has_child("variables")) {
+      tool->wsEmplace<RooBinWidthFunction>(name, tool->requestArgList<RooAbsReal>(p, "variables"), DivideByBinWidth);
+      return true;
+   }
+   // Accept the legacy histogram-reference representation.
    RooHistFunc *hf = dynamic_cast<RooHistFunc *>(tool->request<RooAbsReal>(p["histogram"].val(), name));
    if (!hf) {
       RooJSONFactoryWSTool::error("histogram '" + p["histogram"].val() + "' of '" + name + "' is not a RooHistFunc");
@@ -779,7 +784,7 @@ bool exportBinWidthFunction(RooJSONFactoryWSTool *, const RooAbsArg *func, JSONN
 {
    const RooBinWidthFunction *pdf = static_cast<const RooBinWidthFunction *>(func);
    elem["type"] << (pdf->divideByBinWidth() ? "inverse_binvolume" : "binvolume");
-   elem["histogram"] << pdf->histFunc().GetName();
+   elem["variables"].fill_seq(pdf->variables(), [](auto *arg) { return arg->GetName(); });
    return true;
 }
 

roofit/hs3/test/testRooFitHS3.cxx

@@ -1294,14 +1294,17 @@ TEST(RooFitHS3, RooHistPdf)
 
 TEST(RooFitHS3, RooBinWidthFunctionUsesBinVolumeKeys)
 {
-   RooRealVar x{"x", "x", 0.0, 2.0};
+   RooRealVar x{"x", "x", 0.0, 6.0};
    x.setBins(2);
 
    RooDataHist dataHist{"dataHist", "dataHist", x};
    dataHist.set(0, 2.0, -1);
    dataHist.set(1, 4.0, -1);
 
    RooHistFunc histFunc{"histFunc", "histFunc", x, dataHist};
+   // The function must use the live variable's binning, not the data histogram's.
+   const double edges[] = {0., 1., 3., 6.};
+   x.setBinning(RooBinning{3, edges});
    RooBinWidthFunction binVolume{"binVolume", "binVolume", histFunc, false};
    RooBinWidthFunction inverseBinVolume{"inverseBinVolume", "inverseBinVolume", histFunc, true};
 
@@ -1323,6 +1326,99 @@ TEST(RooFitHS3, RooBinWidthFunctionUsesBinVolumeKeys)
    ASSERT_NE(importedInverseBinVolume, nullptr);
    EXPECT_FALSE(importedBinVolume->divideByBinWidth());
    EXPECT_TRUE(importedInverseBinVolume->divideByBinWidth());
+   EXPECT_EQ(json.find("\"histogram\""), std::string::npos) << json;
+   EXPECT_EQ(ws2.function("histFunc"), nullptr);
+   auto tree = RooFit::Detail::JSONTree::create(json);
+   const auto *function = RooJSONFactoryWSTool::findNamedChild(tree->rootnode()["functions"], "binVolume");
+   ASSERT_NE(function, nullptr);
+   ASSERT_EQ((*function)["variables"].num_children(), 1u);
+   EXPECT_EQ((*function)["variables"].child(0).val(), "x");
+   EXPECT_FALSE(function->has_child("edges"));
+   EXPECT_FALSE(function->has_child("nbins"));
+   const auto *domain = RooJSONFactoryWSTool::findNamedChild(tree->rootnode()["domains"], "default_domain");
+   ASSERT_NE(domain, nullptr);
+   const auto *axis = RooJSONFactoryWSTool::findNamedChild((*domain)["axes"], "x");
+   ASSERT_NE(axis, nullptr);
+   ASSERT_EQ((*axis)["edges"].num_children(), 4u);
+   for (int i = 0; i < 4; ++i) EXPECT_DOUBLE_EQ((*axis)["edges"].child(i).val_double(), edges[i]);
+   for (int i = 0; i < 3; ++i) {
+      ws1.var("x")->setBin(i);
+      ws2.var("x")->setBin(i);
+      EXPECT_DOUBLE_EQ(ws1.function("binVolume")->getVal(), edges[i + 1] - edges[i]);
+      EXPECT_DOUBLE_EQ(importedBinVolume->getVal(), edges[i + 1] - edges[i]);
+      EXPECT_DOUBLE_EQ(importedInverseBinVolume->getVal(), 1. / (edges[i + 1] - edges[i]));
+   }
+}
+
+// This fixture carries binning exclusively in domains, with no histogram or
+// function-local binning metadata. Exercise both import and re-export.
+TEST(RooFitHS3, RooBinWidthFunctionBinningFromDomains)
+{
+   const std::string input = R"({
+  "metadata": {"hs3_version": "0.1.90"},
+  "domains": [{
+    "name": "default_domain",
+    "type": "product_domain",
+    "axes": [
+      {"name": "x", "min": 0.0, "max": 6.0, "edges": [0.0, 1.0, 3.0, 6.0]},
+      {"name": "y", "min": 0.0, "max": 10.0, "nbins": 2}
+    ]
+  }],
+  "parameter_points": [{
+    "name": "default_values",
+    "parameters": [{"name": "x", "value": 0.5}, {"name": "y", "value": 2.0}]
+  }],
+  "functions": [
+    {"name": "volume", "type": "binvolume", "variables": ["x", "y"]},
+    {"name": "inverse", "type": "inverse_binvolume", "variables": ["x", "y"]}
+  ]
+})";
+   RooWorkspace original;
+   ASSERT_TRUE(RooJSONFactoryWSTool{original}.importJSONfromString(input));
+   const std::string json = RooJSONFactoryWSTool{original}.exportJSONtoString();
+   RooWorkspace restored;
+   ASSERT_TRUE(RooJSONFactoryWSTool{restored}.importJSONfromString(json));
+   for (auto *ws : {&original, &restored}) {
+      ASSERT_NE(ws->function("volume"), nullptr);
+      ASSERT_NE(ws->function("inverse"), nullptr);
+      ASSERT_NE(ws->var("x"), nullptr);
+      ASSERT_NE(ws->var("y"), nullptr);
+      EXPECT_EQ(ws->var("x")->numBins(), 3);
+      EXPECT_EQ(ws->var("y")->numBins(), 2);
+      for (int bin = 0; bin < 3; ++bin) {
+         ws->var("x")->setBin(bin);
+         for (int ybin = 0; ybin < 2; ++ybin) {
+            ws->var("y")->setBin(ybin);
+            EXPECT_DOUBLE_EQ(ws->function("volume")->getVal(), (bin + 1.) * 5.);
+            EXPECT_DOUBLE_EQ(ws->function("inverse")->getVal(), 1. / ((bin + 1.) * 5.));
+         }
+      }
+   }
+}
+
+TEST(RooFitHS3, RooBinWidthFunctionLegacyHistogramReference)
+{
+   RooRealVar x{"x", "x", 0., 6.};
+   const double edges[] = {0., 1., 3., 6.};
+   x.setBinning(RooBinning{3, edges});
+   RooDataHist data{"data", "data", x};
+   RooHistFunc hist{"hist", "hist", x, data};
+   RooWorkspace ws;
+   ws.import(hist, RooFit::Silence());
+   auto tree = RooFit::Detail::JSONTree::create(RooJSONFactoryWSTool{ws}.exportJSONtoString());
+   for (auto *type : {"binvolume", "inverse_binvolume"}) {
+      auto &function = tree->rootnode()["functions"].append_child().set_map();
+      function["name"] << type;
+      function["type"] << type;
+      function["histogram"] << "hist";
+   }
+   RooWorkspace restored;
+   ASSERT_TRUE(RooJSONFactoryWSTool{restored}.importJSONfromString(jsonString(*tree)));
+   for (int bin = 0; bin < 3; ++bin) {
+      restored.var("x")->setBin(bin);
+      EXPECT_DOUBLE_EQ(restored.function("binvolume")->getVal(), bin + 1.);
+      EXPECT_DOUBLE_EQ(restored.function("inverse_binvolume")->getVal(), 1. / (bin + 1.));
+   }
 }
 
 TEST(RooFitHS3, StepDispatchesToRooHistFuncAndParamHistFunc)

roofit/roofitcore/inc/LinkDef.h

@@ -343,7 +343,13 @@
 #pragma link C++ options=nomap class std::map<string,TH1*>+ ;
 #pragma link off class RooErrorHandler+ ;
 #pragma link C++ class RooBinSamplingPdf+;
-#pragma link C++ class RooBinWidthFunction+;
+#pragma link C++ class RooBinWidthFunction-;
+#pragma read sourceClass="RooBinWidthFunction" targetClass="RooBinWidthFunction" version="[1]" \
+             source="RooTemplateProxy<const RooHistFunc> _histFunc" target="_observables" \
+             include="RooHistFunc.h,RooTemplateProxy.h" \
+             code="{ \
+                 _observables.RooArgList::add(onfile._histFunc->variables()); \
+             }"
 #pragma link C++ class RooFit::Detail::RooNLLVarNew+;
 #pragma link C++ class RooFit::Detail::RooNormalizedPdf+ ;
 

roofit/roofitcore/inc/RooBinWidthFunction.h

@@ -19,8 +19,13 @@
 #define ROOFIT_ROOFITCORE_INC_BINWIDTHFUNCTION_H_
 
 #include "RooAbsReal.h"
-#include "RooTemplateProxy.h"
-#include "RooHistFunc.h"
+#include "RooListProxy.h"
+
+#include <utility>
+#include <vector>
+
+class RooHistFunc;
+class RooAbsBinning;
 
 class RooBinWidthFunction : public RooAbsReal {
   static bool _enabled;
@@ -31,16 +36,16 @@ class RooBinWidthFunction : public RooAbsReal {
   static bool isClassEnabled();
 
   /// Create an empty instance.
-  RooBinWidthFunction() :
-    _histFunc("HistFuncForBinWidth", "Handle to a RooHistFunc, whose bin volumes should be returned.", this,
-        /*valueServer=*/true, /*shapeServer=*/true) { }
+  RooBinWidthFunction() = default;
+
+  RooBinWidthFunction(const char* name, const char* title, const RooArgList& observables, bool divideByBinWidth);
 
   RooBinWidthFunction(const char* name, const char* title, const RooHistFunc& histFunc, bool divideByBinWidth);
 
   /// Copy an existing object.
   RooBinWidthFunction(const RooBinWidthFunction& other, const char* newname = nullptr) :
     RooAbsReal(other, newname),
-    _histFunc("HistFuncForBinWidth", this, other._histFunc),
+    _observables(this, other._observables),
     _divideByBinWidth(other._divideByBinWidth) { }
 
   std::unique_ptr<RooAbsArg> compileForNormSet(RooArgSet const &normSet, RooFit::Detail::CompileContext & ctx) const override;
@@ -51,29 +56,31 @@ class RooBinWidthFunction : public RooAbsReal {
   }
 
   // Plotting and binning hints
-  /// Test if internal RooHistFunc is binned.
-  bool isBinnedDistribution(const RooArgSet& obs) const override {
-    return _histFunc->isBinnedDistribution(obs);
-  }
-  /// Return bin boundaries of internal RooHistFunc.
-  std::list<double>* binBoundaries(RooAbsRealLValue& obs, double xlo, double xhi) const override {
-    return _histFunc->binBoundaries(obs, xlo, xhi);
-  }
-  /// Return plotSamplingHint of internal RooHistFunc.
-  std::list<double>* plotSamplingHint(RooAbsRealLValue& obs, double xlo, double xhi) const override {
-    return _histFunc->plotSamplingHint(obs, xlo, xhi);
-  }
+  bool isBinnedDistribution(const RooArgSet&) const override { return true; }
+  std::list<double>* binBoundaries(RooAbsRealLValue& obs, double xlo, double xhi) const override;
+  std::list<double>* plotSamplingHint(RooAbsRealLValue& obs, double xlo, double xhi) const override;
 
   bool divideByBinWidth() const { return _divideByBinWidth; }
-  const RooHistFunc& histFunc() const { return (*_histFunc); }
+  const RooArgList& variables() const { return _observables; }
+  double getValV(const RooArgSet* nset = nullptr) const override;
   double evaluate() const override;
   void doEval(RooFit::EvalContext &) const override;
 
+protected:
+  void ioStreamerPass2() override;
+
 private:
-  RooTemplateProxy<const RooHistFunc> _histFunc;
+  void finalizeIO();
+  bool updateCache() const;
+
+  RooListProxy _observables{"observables", "Observables defining the bin volume", this, true, true};
+  mutable std::vector<double> _binVolumes; //! Cached bin volumes, in observable order
+  mutable std::vector<int> _binCounts; //! Number of bins per observable
+  mutable std::vector<const RooAbsBinning*> _binnings; //! Binnings used to build the cache
+  mutable std::vector<std::pair<double, double>> _binRanges; //! Ranges used to build the cache
   bool _divideByBinWidth{false};
 
-  ClassDefOverride(RooBinWidthFunction, 1);
+  ClassDefOverride(RooBinWidthFunction, 2);
 };
 
 #endif