From 66347d603a32315319b3bd46d30f0686b6405b91 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Tue, 1 Sep 2026 14:22:19 +0000 Subject: [PATCH 1/2] [RF][HS3] Don't let eval. artifacts demote top-level pdfs in the export MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The HS3 exporter and `RooJSONFactoryWSTool::cleanWS()` used `!hasClients()` as the criterion for "this pdf or function is a top-level object of the workspace". But RooFit also registers objects that live outside the workspace as clients: the normalization integral that `RooAbsPdf` caches after `getVal(normSet)`, or the integral returned by `createIntegral()`. A single normalized evaluation before the export therefore silently produced a document without any distributions, domains or parameter values, while `exportJSON()` still returned `true`. Only count clients that are actually components of the workspace, since these are the ones that make an object a sub-node of a bigger model. Closes #23221. 🤖 Done with the help of AI --- roofit/hs3/src/RooJSONFactoryWSTool.cxx | 25 +++++++++++--- roofit/hs3/test/testRooFitHS3.cxx | 46 +++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 4 deletions(-) diff --git a/roofit/hs3/src/RooJSONFactoryWSTool.cxx b/roofit/hs3/src/RooJSONFactoryWSTool.cxx index 0deff751e7219..5b4b9f6357a7e 100644 --- a/roofit/hs3/src/RooJSONFactoryWSTool.cxx +++ b/roofit/hs3/src/RooJSONFactoryWSTool.cxx @@ -736,6 +736,23 @@ void sortByName(T &coll) std::sort(coll.begin(), coll.end(), [](auto &l, auto &r) { return strcmp(l->GetName(), r->GetName()) < 0; }); } +/// Check whether an object is a top-level object of the workspace, i.e. not a +/// component of some other workspace object. Only clients that are part of the +/// workspace count: RooFit also registers evaluation artifacts as clients, +/// like the normalization integral that RooAbsPdf caches after +/// getVal(normSet), or the integral returned by createIntegral(). These live +/// outside the workspace and are not evidence that the object is a sub-node of +/// a bigger model (see https://github.com/root-project/root/issues/23221). +bool isTopLevel(RooAbsArg const &arg, RooWorkspace const &ws) +{ + for (RooAbsArg const *client : arg.clients()) { + if (ws.components().containsInstance(*client)) { + return false; + } + } + return true; +} + } // namespace RooJSONFactoryWSTool::RooJSONFactoryWSTool(RooWorkspace &ws) : _workspace{ws} {} @@ -1737,7 +1754,7 @@ void RooJSONFactoryWSTool::exportAllObjects(JSONNode &n) // export all toplevel pdfs std::vector allpdfs; for (auto &arg : _workspace.allPdfs()) { - if (!arg->hasClients()) { + if (isTopLevel(*arg, _workspace)) { if (auto *pdf = dynamic_cast(arg)) { allpdfs.push_back(pdf); } @@ -1750,7 +1767,7 @@ void RooJSONFactoryWSTool::exportAllObjects(JSONNode &n) // export all toplevel functions std::vector allfuncs; for (auto &arg : _workspace.allFunctions()) { - if (!arg->hasClients()) { + if (isTopLevel(*arg, _workspace)) { if (auto *func = dynamic_cast(arg)) { allfuncs.push_back(func); } @@ -2257,13 +2274,13 @@ RooWorkspace RooJSONFactoryWSTool::cleanWS(const RooWorkspace &ws, bool onlyMode } else { for (auto *pdf : ws.allPdfs()) { - if (!pdf->hasClients()) { + if (isTopLevel(*pdf, ws)) { tmpWS.import(*pdf, RooFit::RecycleConflictNodes(true)); } } for (auto *func : ws.allFunctions()) { - if (!func->hasClients()) { + if (isTopLevel(*func, ws)) { tmpWS.import(*func, RooFit::RecycleConflictNodes(true)); } } diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index c1ae9e2906128..a972fa14cae3c 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -438,6 +438,52 @@ TEST(RooFitHS3, ParameterPointsDoNotExportRanges) } } +// Evaluating a pdf with a normalization set, or creating an integral over it, +// registers an integral that lives outside the workspace as a client of the +// pdf. This must not demote the pdf from being a top-level object of the +// export. Covers https://github.com/root-project/root/issues/23221. +TEST(RooFitHS3, TopLevelPdfExportAfterNormalizedEvaluation) +{ + auto exportedDistributions = [](RooWorkspace &ws) { + std::vector names; + auto tree = RooFit::Detail::JSONTree::create(RooJSONFactoryWSTool{ws}.exportJSONtoString()); + if (auto const *dists = tree->rootnode().find("distributions")) { + for (auto const &dist : dists->children()) { + names.push_back(dist["name"].val()); + } + } + return names; + }; + + const std::vector expected{"gauss"}; + + { + RooWorkspace ws{"ws"}; + ws.factory("Gaussian::gauss(x[0, -5, 5], mean[1, -5, 5], sigma[2, 0.1, 10])"); + RooAbsPdf &pdf = *ws.pdf("gauss"); + RooArgSet normSet{*ws.var("x")}; + pdf.getVal(normSet); + // Make sure the test covers the scenario from the issue: the cached + // normalization integral is registered as a client of the pdf. + ASSERT_TRUE(pdf.hasClients()); + + EXPECT_EQ(exportedDistributions(ws), expected); + // cleanWS() uses the same top-level criterion + EXPECT_NE(RooJSONFactoryWSTool::cleanWS(ws).pdf("gauss"), nullptr); + // ... and the full round trip should still work after the evaluation + EXPECT_EQ(validate(ws, "gauss"), 0); + } + { + RooWorkspace ws{"ws"}; + ws.factory("Gaussian::gauss(x[0, -5, 5], mean[1, -5, 5], sigma[2, 0.1, 10])"); + std::unique_ptr integral{ws.pdf("gauss")->createIntegral(*ws.var("x"))}; + ASSERT_TRUE(ws.pdf("gauss")->hasClients()); + + EXPECT_EQ(exportedDistributions(ws), expected); + EXPECT_NE(RooJSONFactoryWSTool::cleanWS(ws).pdf("gauss"), nullptr); + } +} + TEST(RooFitHS3, ProductDomainEntriesExportExplicitBounds) { RooRealVar x{"x", "x", 0.0, -10.0, 10.0}; From 7053a317593bb93b239dff2589694f4d1e4912b5 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Tue, 1 Sep 2026 14:50:30 +0000 Subject: [PATCH 2/2] [RF][HS3] Remove duplicated and dead code in the JSON factories MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Behaviour-preserving cleanup of roofit/hs3, motivated by keeping the line count in check after the fix for #23221: * Build error messages with string concatenation instead of a `std::stringstream` whose only purpose is to feed `error()`, and drop the `return`s after `error()`, which is `[[noreturn]]`. * Share the "find the category observable" loop between the two dataset export functions, and the name-index construction between "functions" and "distributions". * HistFactory: factor out the repeated modifier node creation and the constraint queueing loops, drop the unreachable "optionally" branch of `optionallyExportGammaParameters()` (`forceExport` was always true), merge `hasStaterror()` into `findStaterror()`, and drop a `std::map` presence check that `operator[]` already covers. * RooFitCore: `readBinning()` reimplemented `RooJSONFactoryWSTool::readAxes()`, the RealSumPdf/RealSumFunc importer and exporter pairs are now templates, `writePolynomialBody()` is inlined into its single caller, and the manual `push_back` loops use the existing `operator<<` for JSON sequences. * Drop unused includes and dead locals. Verified that the exported JSON for HistFactory, RealSum, Polynomial, Spline and ParamHistFunc models is byte-identical before and after. 🤖 Done with the help of AI --- roofit/hs3/src/JSONFactories_HistFactory.cxx | 143 ++++++------------- roofit/hs3/src/JSONFactories_RooFitCore.cxx | 122 +++++----------- roofit/hs3/src/RooJSONFactoryWSTool.cxx | 123 ++++++---------- 3 files changed, 122 insertions(+), 266 deletions(-) diff --git a/roofit/hs3/src/JSONFactories_HistFactory.cxx b/roofit/hs3/src/JSONFactories_HistFactory.cxx index 1627723fee8f1..bb62724c95cb4 100644 --- a/roofit/hs3/src/JSONFactories_HistFactory.cxx +++ b/roofit/hs3/src/JSONFactories_HistFactory.cxx @@ -403,16 +403,11 @@ RooAbsPdf *findConstraint(RooAbsArg *g) { if (!g) return nullptr; - RooPoisson *constraint_p = findClient(g); - if (constraint_p) - return constraint_p; - RooGaussian *constraint_g = findClient(g); - if (constraint_g) - return constraint_g; - RooLognormal *constraint_l = findClient(g); - if (constraint_l) - return constraint_l; - return nullptr; + if (auto *constraint = findClient(g)) + return constraint; + if (auto *constraint = findClient(g)) + return constraint; + return findClient(g); } inline std::string defaultGammaName(std::string const &sysname, std::size_t i) @@ -420,23 +415,14 @@ inline std::string defaultGammaName(std::string const &sysname, std::size_t i) return "gamma_" + sysname + "_bin_" + std::to_string(i); } -/// Export the names of the gamma parameters to the modifier struct if the -/// names don't match the default gamma parameter names, which is gamma__bin_ -void optionallyExportGammaParameters(JSONNode &mod, std::string const &sysname, std::vector const ¶ms, - bool forceExport = true) +/// Export the names of the gamma parameters to the modifier struct +void exportGammaParameters(JSONNode &mod, std::vector const ¶ms) { std::vector paramNames; - bool needExport = forceExport; - for (std::size_t i = 0; i < params.size(); ++i) { - std::string name(params[i]->GetName()); - paramNames.push_back(name); - if (name != defaultGammaName(sysname, i)) { - needExport = true; - } - } - if (needExport) { - mod["parameters"].fill_seq(paramNames); + for (RooAbsReal *param : params) { + paramNames.emplace_back(param->GetName()); } + mod["parameters"].fill_seq(paramNames); } RooRealVar &createNominal(RooWorkspace &ws, std::string const &parname, double val, double min, double max) @@ -530,27 +516,16 @@ ParamHistFunc &createPHF(const std::string &phfname, std::string const &sysname, return phf; } -bool hasStaterror(const JSONNode &comp) -{ - if (!comp.has_child("modifiers")) - return false; - for (const auto &mod : comp["modifiers"].children()) { - if (mod["type"].val() == ::Literals::staterror) - return true; - } - return false; -} - -const JSONNode &findStaterror(const JSONNode &comp) +/// Find the staterror modifier of a sample, or return nullptr if there is none. +const JSONNode *findStaterror(const JSONNode &comp) { if (comp.has_child("modifiers")) { for (const auto &mod : comp["modifiers"].children()) { if (mod["type"].val() == ::Literals::staterror) - return mod; + return &mod; } } - RooJSONFactoryWSTool::error("sample '" + RooJSONFactoryWSTool::name(comp) + "' does not have a " + - ::Literals::staterror + " modifier!"); + return nullptr; } RooAbsPdf & @@ -667,7 +642,7 @@ bool importHistSample(RooJSONFactoryWSTool &tool, RooDataHist &dh, RooArgSet con shapeElems.add(tool.wsEmplace(prefixedName + "_binWidth", hf, true)); - if (hasStaterror(p)) { + if (findStaterror(p)) { shapeElems.add(*mcStatObject); } @@ -882,7 +857,7 @@ class HistFactoryImporter : public RooFit::JSONIO::Importer { comp["data"], fprefix + "_" + RooJSONFactoryWSTool::name(comp) + "_dataHist", observables); size_t nbins = dh->numEntries(); - if (hasStaterror(comp)) { + if (const JSONNode *staterror = findStaterror(comp)) { if (sumW.empty()) { sumW.resize(nbins); sumW2.resize(nbins); @@ -892,7 +867,7 @@ class HistFactoryImporter : public RooFit::JSONIO::Importer { sumW2[i] += dh->weightSquared(i); } if (gammaParnames.empty()) { - if (auto staterrorParams = findStaterror(comp).find("parameters")) { + if (auto staterrorParams = staterror->find("parameters")) { for (const auto &v : staterrorParams->children()) { gammaParnames.push_back(v.val()); } @@ -1461,10 +1436,6 @@ Channel readChannel(RooJSONFactoryWSTool *tool, const std::string &pdfname, cons sample.staterrorParameters.push_back(static_cast(g)); ++idx; RooAbsPdf *constraint = findConstraint(g); - if (channel.tot_yield.find(idx) == channel.tot_yield.end()) { - channel.tot_yield[idx] = 0; - channel.tot_yield2[idx] = 0; - } channel.tot_yield[idx] += sample.hist[idx - 1]; channel.tot_yield2[idx] += (sample.hist[idx - 1] * sample.hist[idx - 1]); if (constraint) { @@ -1712,6 +1683,13 @@ bool exportChannel(RooJSONFactoryWSTool *tool, const Channel &channel, JSONNode mod["constraint"] << sys.constraint->GetName(); } }; + auto addModifier = [](JSONNode &modifiers, std::string const &name, const char *type) -> JSONNode & { + auto &mod = modifiers.append_child(); + mod.set_map(); + mod["name"] << name; + mod["type"] << type; + return mod; + }; elem["type"] << "histfactory_dist"; const auto channelDefaultInterpolation = defaultInterpolation(channel); @@ -1740,10 +1718,7 @@ bool exportChannel(RooJSONFactoryWSTool *tool, const Channel &channel, JSONNode } for (const auto &sys : sample.normsys) { - auto &mod = modifiers.append_child(); - mod.set_map(); - mod["name"] << sys.name; - mod["type"] << "normsys"; + auto &mod = addModifier(modifiers, sys.name, "normsys"); mod["parameter"] << sys.param->GetName(); if (!channelDefaultInterpolation || sys.interpolation != *channelDefaultInterpolation) { writeInterpolation(mod["interpolation"], sys.interpolation); @@ -1755,10 +1730,7 @@ bool exportChannel(RooJSONFactoryWSTool *tool, const Channel &channel, JSONNode } for (const auto &sys : sample.histosys) { - auto &mod = modifiers.append_child(); - mod.set_map(); - mod["name"] << sys.name; - mod["type"] << "histosys"; + auto &mod = addModifier(modifiers, sys.name, "histosys"); mod["parameter"] << sys.param->GetName(); if (!channelDefaultInterpolation || sys.interpolation != *channelDefaultInterpolation) { writeInterpolation(mod["interpolation"], sys.interpolation); @@ -1766,21 +1738,17 @@ bool exportChannel(RooJSONFactoryWSTool *tool, const Channel &channel, JSONNode writeConstraint(mod, sys); auto &data = mod["data"].set_map(); if (channel.nBins != sys.low.size() || channel.nBins != sys.high.size()) { - std::stringstream ss; - ss << "inconsistent binning: " << channel.nBins << " bins expected, but " << sys.low.size() << "/" - << sys.high.size() << " found in nominal histogram errors!"; - RooJSONFactoryWSTool::error(ss.str().c_str()); + RooJSONFactoryWSTool::error("inconsistent binning: " + std::to_string(channel.nBins) + + " bins expected, but " + std::to_string(sys.low.size()) + "/" + + std::to_string(sys.high.size()) + " found in nominal histogram errors!"); } RooJSONFactoryWSTool::exportArray(channel.nBins, sys.low.data(), data["lo"].set_map()["contents"]); RooJSONFactoryWSTool::exportArray(channel.nBins, sys.high.data(), data["hi"].set_map()["contents"]); } for (const auto &sys : sample.shapesys) { - auto &mod = modifiers.append_child(); - mod.set_map(); - mod["name"] << sys.name; - mod["type"] << "shapesys"; - optionallyExportGammaParameters(mod, sys.name, sys.parameters); + auto &mod = addModifier(modifiers, sys.name, "shapesys"); + exportGammaParameters(mod, sys.parameters); if (std::any_of(sys.constraintPdfs.begin(), sys.constraintPdfs.end(), [](auto *pdf) { return pdf != nullptr; })) { auto &constraintNames = mod["constraints"].set_seq(); @@ -1796,24 +1764,15 @@ bool exportChannel(RooJSONFactoryWSTool *tool, const Channel &channel, JSONNode } for (const auto &other : sample.otherElements) { - auto &mod = modifiers.append_child(); - mod.set_map(); - mod["name"] << other.name; - mod["type"] << "custom"; + addModifier(modifiers, other.name, "custom"); } for (const auto &other : sample.tmpElements) { - auto &mod = modifiers.append_child(); - mod.set_map(); - mod["name"] << other.name; - mod["type"] << "custom"; + addModifier(modifiers, other.name, "custom"); } if (sample.useBarlowBeestonLight) { - auto &mod = modifiers.append_child(); - mod.set_map(); - mod["name"] << ::Literals::staterror; - mod["type"] << ::Literals::staterror; - optionallyExportGammaParameters(mod, "stat_" + channel.name, sample.staterrorParameters); + auto &mod = addModifier(modifiers, ::Literals::staterror, ::Literals::staterror); + exportGammaParameters(mod, sample.staterrorParameters); } if (!observablesWritten) { @@ -1825,18 +1784,15 @@ bool exportChannel(RooJSONFactoryWSTool *tool, const Channel &channel, JSONNode } auto &dataNode = s["data"].set_map(); if (channel.nBins != sample.hist.size()) { - std::stringstream ss; - ss << "inconsistent binning: " << channel.nBins << " bins expected, but " << sample.hist.size() - << " found in nominal histogram!"; - RooJSONFactoryWSTool::error(ss.str().c_str()); + RooJSONFactoryWSTool::error("inconsistent binning: " + std::to_string(channel.nBins) + " bins expected, but " + + std::to_string(sample.hist.size()) + " found in nominal histogram!"); } RooJSONFactoryWSTool::exportArray(channel.nBins, sample.hist.data(), dataNode["contents"]); if (!sample.histError.empty()) { if (channel.nBins != sample.histError.size()) { - std::stringstream ss; - ss << "inconsistent binning: " << channel.nBins << " bins expected, but " << sample.histError.size() - << " found in nominal histogram errors!"; - RooJSONFactoryWSTool::error(ss.str().c_str()); + RooJSONFactoryWSTool::error("inconsistent binning: " + std::to_string(channel.nBins) + + " bins expected, but " + std::to_string(sample.histError.size()) + + " found in nominal histogram errors!"); } RooJSONFactoryWSTool::exportArray(channel.nBins, sample.histError.data(), dataNode["errors"]); } @@ -1930,22 +1886,17 @@ bool tryExportHistFactory(RooJSONFactoryWSTool *tool, const std::string &pdfname } // Export all the regular modifiers - for (const auto &sample : channel.samples) { - for (auto &modifier : sample.normfactors) { - if (modifier.constraint) { - tool->queueExport(*modifier.constraint); - } - } - for (auto &modifier : sample.normsys) { - if (modifier.constraint) { - tool->queueExport(*modifier.constraint); - } - } - for (auto &modifier : sample.histosys) { + auto queueConstraints = [&](auto const &modifiers) { + for (auto &modifier : modifiers) { if (modifier.constraint) { tool->queueExport(*modifier.constraint); } } + }; + for (const auto &sample : channel.samples) { + queueConstraints(sample.normfactors); + queueConstraints(sample.normsys); + queueConstraints(sample.histosys); for (auto &modifier : sample.shapesys) { for (auto *constraint : modifier.constraintPdfs) { if (constraint) { diff --git a/roofit/hs3/src/JSONFactories_RooFitCore.cxx b/roofit/hs3/src/JSONFactories_RooFitCore.cxx index 835827be82ef5..f809bbe07fcc1 100644 --- a/roofit/hs3/src/JSONFactories_RooFitCore.cxx +++ b/roofit/hs3/src/JSONFactories_RooFitCore.cxx @@ -12,13 +12,11 @@ #include -#include #include #include #include #include #include -#include #include #include #include @@ -50,17 +48,17 @@ #include #include #include -#include #include #include #include #include #include -#include #include "JSONIOUtils.h" +#include + #include "static_execute.h" #include @@ -374,24 +372,18 @@ bool importBinSamplingPdf(RooJSONFactoryWSTool *tool, const JSONNode &p) return true; } -bool importRealSumPdf(RooJSONFactoryWSTool *tool, const JSONNode &p) +template +bool importRealSum(RooJSONFactoryWSTool *tool, const JSONNode &p) { std::string name(RooJSONFactoryWSTool::name(p)); - - bool extended = false; - if (p.has_child("extended") && p["extended"].val_bool()) { - extended = true; + RooArgList samples = tool->requestArgList(p, "samples"); + RooArgList coefs = tool->requestArgList(p, "coefficients"); + if constexpr (std::is_same_v) { + const bool extended = p.has_child("extended") && p["extended"].val_bool(); + tool->wsEmplace(name, samples, coefs, extended); + } else { + tool->wsEmplace(name, samples, coefs); } - tool->wsEmplace(name, tool->requestArgList(p, "samples"), - tool->requestArgList(p, "coefficients"), extended); - return true; -} - -bool importRealSumFunc(RooJSONFactoryWSTool *tool, const JSONNode &p) -{ - std::string name(RooJSONFactoryWSTool::name(p)); - tool->wsEmplace(name, tool->requestArgList(p, "samples"), - tool->requestArgList(p, "coefficients")); return true; } @@ -624,9 +616,7 @@ bool importMultiVarGaussian(RooJSONFactoryWSTool *tool, const JSONNode &p) } } else { std::vector variances; - for (const auto &v : p["standard_deviations"].children()) { - variances.push_back(v.val_double()); - } + variances << p["standard_deviations"]; covmat.ResizeTo(variances.size(), variances.size()); int i = 0; for (const auto &row : p["correlations"].children()) { @@ -643,44 +633,14 @@ bool importMultiVarGaussian(RooJSONFactoryWSTool *tool, const JSONNode &p) return true; } +/// Read the binning variables from the "axes" node, ordered like in `varList`. RooArgList readBinning(const JSONNode &topNode, const RooArgList &varList) { - // Temporary map from variable name → RooRealVar - std::map> varMap; - - // Build variables from JSON - for (const JSONNode &node : topNode["axes"].children()) { - const std::string name = node["name"].val(); - std::unique_ptr obs; - - if (node.has_child("edges")) { - std::vector edges; - for (const auto &bound : node["edges"].children()) { - edges.push_back(bound.val_double()); - } - obs = std::make_unique(name.c_str(), name.c_str(), edges.front(), edges.back()); - RooBinning bins(obs->getMin(), obs->getMax()); - for (auto b : edges) - bins.addBoundary(b); - obs->setBinning(bins); - } else { - obs = std::make_unique(name.c_str(), name.c_str(), node["min"].val_double(), - node["max"].val_double()); - obs->setBins(node["nbins"].val_int()); - } - - varMap[name] = std::move(obs); - } - - // Now build the final list following the order in varList + RooArgSet axes = RooJSONFactoryWSTool::readAxes(topNode); RooArgList vars; - for (auto *refVar : dynamic_range_cast(varList)) { - if (!refVar) - continue; - - auto it = varMap.find(refVar->GetName()); - if (it != varMap.end()) { - vars.addOwned(std::move(it->second)); // preserve ownership + for (RooAbsArg *refVar : varList) { + if (RooAbsArg *axis = axes.find(*refVar)) { + vars.addClone(*axis); } } return vars; @@ -737,13 +697,8 @@ bool importSpline(RooJSONFactoryWSTool *tool, const JSONNode &p) // Read knots std::vector x0; std::vector y0; - x0.reserve(p["x0"].num_children()); - y0.reserve(p["y0"].num_children()); - - for (const auto &v : p["x0"].children()) - x0.push_back(v.val_double()); - for (const auto &v : p["y0"].children()) - y0.push_back(v.val_double()); + x0 << p["x0"]; + y0 << p["y0"]; if (x0.size() != y0.size()) { RooJSONFactoryWSTool::error("x0/y0 size mismatch in '" + name + "': x0 has " + std::to_string(x0.size()) + @@ -774,22 +729,16 @@ bool exportAddPdf(RooJSONFactoryWSTool *, const RooAbsArg *func, JSONNode &elem, return true; } -bool exportRealSumPdf(RooJSONFactoryWSTool *, const RooAbsArg *func, JSONNode &elem, std::string const &key) -{ - const RooRealSumPdf *pdf = static_cast(func); - elem["type"] << key; - RooJSONFactoryWSTool::fillSeq(elem["samples"], pdf->funcList()); - RooJSONFactoryWSTool::fillSeq(elem["coefficients"], pdf->coefList()); - elem["extended"] << (pdf->extendMode() != RooAbsPdf::CanNotBeExtended); - return true; -} - -bool exportRealSumFunc(RooJSONFactoryWSTool *, const RooAbsArg *func, JSONNode &elem, std::string const &key) +template +bool exportRealSum(RooJSONFactoryWSTool *, const RooAbsArg *func, JSONNode &elem, std::string const &key) { - const RooRealSumFunc *pdf = static_cast(func); + auto const *pdf = static_cast(func); elem["type"] << key; RooJSONFactoryWSTool::fillSeq(elem["samples"], pdf->funcList()); RooJSONFactoryWSTool::fillSeq(elem["coefficients"], pdf->coefList()); + if constexpr (std::is_same_v) { + elem["extended"] << (pdf->extendMode() != RooAbsPdf::CanNotBeExtended); + } return true; } @@ -910,9 +859,11 @@ bool exportFormulaArg(RooJSONFactoryWSTool *, const RooAbsArg *func, JSONNode &e // Write the "x" reference and the coefficient list for polynomial-like // pdfs/funcs, including the implicit defaults below "lowestOrder" so that the // output is self-documenting. -template -void writePolynomialBody(const Pdf *pdf, JSONNode &elem) +template +bool exportPolynomial(RooJSONFactoryWSTool *, const RooAbsArg *func, JSONNode &elem, std::string const &key) { + auto const *pdf = static_cast(func); + elem["type"] << key; elem["x"] << pdf->x().GetName(); auto &coefs = elem["coefficients"].set_seq(); for (int i = 0; i < pdf->lowestOrder(); ++i) { @@ -921,13 +872,6 @@ void writePolynomialBody(const Pdf *pdf, JSONNode &elem) for (const auto &coef : pdf->coefList()) { coefs.append_child() << coef->GetName(); } -} - -template -bool exportPolynomial(RooJSONFactoryWSTool *, const RooAbsArg *func, JSONNode &elem, std::string const &key) -{ - elem["type"] << key; - writePolynomialBody(static_cast(func), elem); return true; } @@ -1258,8 +1202,8 @@ STATIC_EXECUTE([]() { registerImporter("gauss_resolution_model", false); registerImporter>("polynomial_dist", false); registerImporter>("polynomial", false); - registerImporter("weighted_sum_dist", false); - registerImporter("weighted_sum", false); + registerImporter>("weighted_sum_dist", false); + registerImporter>("weighted_sum", false); registerImporter("integral", false); registerImporter("derivative", false); registerImporter("fft_convolution_dist", false); @@ -1286,8 +1230,8 @@ STATIC_EXECUTE([]() { registerExporter(RooGaussModel::Class(), "gauss_resolution_model", false); registerExporter>(RooPolynomial::Class(), "polynomial_dist", false); registerExporter>(RooPolyVar::Class(), "polynomial", false); - registerExporter(RooRealSumFunc::Class(), "weighted_sum", false); - registerExporter(RooRealSumPdf::Class(), "weighted_sum_dist", false); + registerExporter>(RooRealSumFunc::Class(), "weighted_sum", false); + registerExporter>(RooRealSumPdf::Class(), "weighted_sum_dist", false); registerExporter(RooTFnBinding::Class(), "generic", false); registerExporter(RooRealIntegral::Class(), "integral", false); registerExporter(RooDerivative::Class(), "derivative", false); diff --git a/roofit/hs3/src/RooJSONFactoryWSTool.cxx b/roofit/hs3/src/RooJSONFactoryWSTool.cxx index 5b4b9f6357a7e..1e5a245fcbb82 100644 --- a/roofit/hs3/src/RooJSONFactoryWSTool.cxx +++ b/roofit/hs3/src/RooJSONFactoryWSTool.cxx @@ -17,14 +17,12 @@ #include #include #include -#include -#include +#include #include #include #include #include #include -#include #include #include #include @@ -32,15 +30,12 @@ #include "JSONIOUtils.h" #include "Domains.h" -#include "RooFitImplHelpers.h" - #include #include #include #include #include -#include #include /** \class RooJSONFactoryWSTool @@ -347,9 +342,7 @@ std::string generate(const RooFit::JSONIO::ImportExpression &ex, const JSONNode if (k == "true" || k == "false") { expression << (k == "true" ? "1" : "0"); } else if (!p.has_child(k)) { - std::stringstream errMsg; - errMsg << "node '" << name << "' is missing key '" << k << "'"; - RooJSONFactoryWSTool::error(errMsg.str()); + RooJSONFactoryWSTool::error("node '" + name + "' is missing key '" + k + "'"); } else if (p[k].is_seq()) { bool firstInner = true; expression << "{"; @@ -444,9 +437,7 @@ void getObservables(RooWorkspace const &ws, const JSONNode &node, RooAbsCollecti if (ws.var(name)) { out.add(*ws.var(name)); } else { - std::stringstream errMsg; - errMsg << "The observable \"" << name << "\" could not be found in the workspace!"; - RooJSONFactoryWSTool::error(errMsg.str()); + RooJSONFactoryWSTool::error("The observable \"" + name + "\" could not be found in the workspace!"); } } } @@ -485,9 +476,7 @@ std::unique_ptr loadData(const JSONNode &p, RooWorkspace &workspace) std::size_t i = 0; for (auto const &point : coords.children()) { if (!point.is_seq()) { - std::stringstream errMsg; - errMsg << "coordinate point '" << i << "' is not a list!"; - RooJSONFactoryWSTool::error(errMsg.str()); + RooJSONFactoryWSTool::error("coordinate point '" + std::to_string(i) + "' is not a list!"); } if (point.num_children() != varlist.size()) { RooJSONFactoryWSTool::error("inconsistent number of entries and observables!"); @@ -508,10 +497,7 @@ std::unique_ptr loadData(const JSONNode &p, RooWorkspace &workspace) return data; } - std::stringstream ss; - ss << "RooJSONFactoryWSTool() failed to create dataset " << name << std::endl; - RooJSONFactoryWSTool::error(ss.str()); - return nullptr; + RooJSONFactoryWSTool::error("RooJSONFactoryWSTool() failed to create dataset " + name); } // Import an analysis (likelihood + domains) as one or more ModelConfig objects into the workspace. @@ -753,6 +739,22 @@ bool isTopLevel(RooAbsArg const &arg, RooWorkspace const &ws) return true; } +/// Find the single category observable of a dataset, if any. +RooAbsCategory *findCategoryObservable(RooAbsData const &data) +{ + RooAbsCategory *cat = nullptr; + for (RooAbsArg *obs : *data.get()) { + if (auto *c = dynamic_cast(obs)) { + if (cat) { + RooJSONFactoryWSTool::error("dataset '" + std::string(data.GetName()) + + " has several category observables!"); + } + cat = c; + } + } + return cat; +} + } // namespace RooJSONFactoryWSTool::RooJSONFactoryWSTool(RooWorkspace &ws) : _workspace{ws} {} @@ -1139,19 +1141,13 @@ void RooJSONFactoryWSTool::importFunction(const JSONNode &p, bool importAllDepen } // if the key we found is not a map, it's an error if (!p.is_map()) { - std::stringstream ss; - ss << "RooJSONFactoryWSTool() function node " + name + " is not a map!"; - RooJSONFactoryWSTool::error(ss.str()); - return; + RooJSONFactoryWSTool::error("RooJSONFactoryWSTool() function node " + name + " is not a map!"); } std::string prefix = genPrefix(p, true); if (!prefix.empty()) name = prefix + name; if (!p.has_child("type")) { - std::stringstream ss; - ss << "RooJSONFactoryWSTool() no type given for function '" << name << "', skipping." << std::endl; - RooJSONFactoryWSTool::error(ss.str()); - return; + RooJSONFactoryWSTool::error("RooJSONFactoryWSTool() no type given for function '" + name + "', skipping."); } std::string functype(p["type"].val()); @@ -1215,9 +1211,7 @@ void RooJSONFactoryWSTool::importFunction(const JSONNode &p, bool importAllDepen } RooAbsReal *func = _workspace.function(name); if (!func) { - std::stringstream err; - err << "something went wrong importing function '" << name << "'."; - RooJSONFactoryWSTool::error(err.str()); + RooJSONFactoryWSTool::error("something went wrong importing function '" + name + "'."); } } @@ -1343,17 +1337,7 @@ void RooJSONFactoryWSTool::exportCategory(RooAbsCategory const &cat, JSONNode &n // component-name map. RooJSONFactoryWSTool::CombinedData RooJSONFactoryWSTool::exportCombinedData(RooAbsData const &data) { - // find category observables - RooAbsCategory *cat = nullptr; - for (RooAbsArg *obs : *data.get()) { - if (dynamic_cast(obs)) { - if (cat) { - RooJSONFactoryWSTool::error("dataset '" + std::string(data.GetName()) + - " has several category observables!"); - } - cat = static_cast(obs); - } - } + RooAbsCategory *cat = findCategoryObservable(data); // prepare return value RooJSONFactoryWSTool::CombinedData datamap; @@ -1399,18 +1383,7 @@ RooJSONFactoryWSTool::CombinedData RooJSONFactoryWSTool::exportCombinedData(RooA // Export a single dataset `data` (binned or unbinned) to the output JSON. void RooJSONFactoryWSTool::exportData(RooAbsData const &data) { - // find category observables - - RooAbsCategory *cat = nullptr; - for (RooAbsArg *obs : *data.get()) { - if (dynamic_cast(obs)) { - if (cat) { - RooJSONFactoryWSTool::error("dataset '" + std::string(data.GetName()) + - " has several category observables!"); - } - cat = static_cast(obs); - } - } + RooAbsCategory *cat = findCategoryObservable(data); if (cat) return; @@ -1558,9 +1531,8 @@ RooJSONFactoryWSTool::readBinnedData(const JSONNode &n, const std::string &name, auto bins = generateBinIndices(vars); if (contents.num_children() != bins.size()) { - std::stringstream errMsg; - errMsg << "inconsistent bin numbers: contents=" << contents.num_children() << ", bins=" << bins.size(); - RooJSONFactoryWSTool::error(errMsg.str()); + RooJSONFactoryWSTool::error("inconsistent bin numbers: contents=" + std::to_string(contents.num_children()) + + ", bins=" + std::to_string(bins.size())); } auto dh = std::make_unique(name, name, vars); std::vector contentVals; @@ -1592,9 +1564,8 @@ void RooJSONFactoryWSTool::importVariable(const JSONNode &p) if (_workspace.arg(name)) return; if (!p.is_map()) { - std::stringstream ss; - ss << "RooJSONFactoryWSTool() node '" << name << "' is not a map, skipping."; - oocoutE(nullptr, InputArguments) << ss.str() << std::endl; + oocoutE(nullptr, InputArguments) << "RooJSONFactoryWSTool() node '" << name << "' is not a map, skipping." + << std::endl; return; } if (config().importNoDomainParametersAsRooConstVars && !_domains->hasVariable(name.c_str())) { @@ -2000,24 +1971,18 @@ void RooJSONFactoryWSTool::importAllNodes(const JSONNode &n) // arguments) triggers a linear scan over all sibling nodes via // findNamedChild(), which becomes O(N^2) on workspaces with thousands of // entries. Populating the maps up-front turns each lookup into O(1). - _functionsByName.clear(); - _distributionsByName.clear(); - if (auto seq = n.find("functions")) { - if (seq->is_seq()) { - _functionsByName.reserve(seq->num_children()); - for (const auto &p : seq->children()) { - _functionsByName.emplace(RooJSONFactoryWSTool::name(p), &p); - } - } - } - if (auto seq = n.find("distributions")) { - if (seq->is_seq()) { - _distributionsByName.reserve(seq->num_children()); - for (const auto &p : seq->children()) { - _distributionsByName.emplace(RooJSONFactoryWSTool::name(p), &p); - } + auto buildIndex = [&n](const char *key, auto &index) { + index.clear(); + auto seq = n.find(key); + if (!seq || !seq->is_seq()) + return; + index.reserve(seq->num_children()); + for (const auto &p : seq->children()) { + index.emplace(RooJSONFactoryWSTool::name(p), &p); } - } + }; + buildIndex("functions", _functionsByName); + buildIndex("distributions", _distributionsByName); this->importDependants(n); @@ -2181,13 +2146,9 @@ void RooJSONFactoryWSTool::importVariableElement(const JSONNode &elementNode) importVariable(p); auto paramPointsNode = n.find("parameter_points"); - const auto &snsh = paramPointsNode->child(0); - std::string name = RooJSONFactoryWSTool::name(snsh); - RooArgSet vars; - const auto &var = snsh["parameters"].child(0); + const auto &var = paramPointsNode->child(0)["parameters"].child(0); if (RooRealVar *rrv = _workspace.var(RooJSONFactoryWSTool::name(var))) { configureVariable(*_domains, var, *rrv); - vars.add(*rrv); } // Import attributes