From 1c49b286a6b8d964eac22107dce465184863932c Mon Sep 17 00:00:00 2001 From: Phmonski Date: Wed, 1 Jul 2026 09:35:43 +0200 Subject: [PATCH 1/5] [HS3] Minor patch to further align HS3 with the spec --- roofit/hs3/src/JSONFactories_HistFactory.cxx | 32 +-- roofit/hs3/src/JSONFactories_RooFitCore.cxx | 98 +++++++-- roofit/hs3/src/RooJSONFactoryWSTool.cxx | 4 +- roofit/hs3/test/testRooFitHS3.cxx | 197 +++++++++++++++++++ 4 files changed, 299 insertions(+), 32 deletions(-) diff --git a/roofit/hs3/src/JSONFactories_HistFactory.cxx b/roofit/hs3/src/JSONFactories_HistFactory.cxx index eb1844d49ebd3..9510b26897588 100644 --- a/roofit/hs3/src/JSONFactories_HistFactory.cxx +++ b/roofit/hs3/src/JSONFactories_HistFactory.cxx @@ -255,7 +255,11 @@ const JSONNode &findStaterror(const JSONNode &comp) RooAbsPdf & getOrCreateConstraint(RooJSONFactoryWSTool &tool, const JSONNode &mod, RooRealVar ¶m, const std::string &sample) { - if (auto constrName = mod.find("constraint_name")) { + JSONNode const *constrName = mod.find("constraint"); + if (!constrName) { + constrName = mod.find("constraint_name"); + } + if (constrName) { auto constraint_name = constrName->val(); auto constraint = tool.workspace()->pdf(constraint_name); if (!constraint) { @@ -366,7 +370,7 @@ bool importHistSample(RooJSONFactoryWSTool &tool, RooDataHist &dh, RooArgSet con RooRealVar &constrParam = getOrCreate(ws, sysname, 1., -3, 5); constrParam.setError(0.0); normElems.add(constrParam); - if (mod.has_child("constraint_name") || mod.has_child("constraint_type")) { + if (mod.has_child("constraint") || mod.has_child("constraint_name") || mod.has_child("constraint_type")) { // for norm factors, constraints are optional constraints.add(getOrCreateConstraint(tool, mod, constrParam, sampleName)); } @@ -429,9 +433,16 @@ bool importHistSample(RooJSONFactoryWSTool &tool, RooDataHist &dh, RooArgSet con RooJSONFactoryWSTool::error("unable to instantiate shapesys '" + sysname + "' with neither values nor parameters!"); } - std::string constraint(mod.has_child("constraint_type") ? mod["constraint_type"].val() - : mod.has_child("constraint") ? mod["constraint"].val() - : "unknown"); + std::string constraint = "unknown"; + if (mod.has_child("constraint_type")) { + constraint = mod["constraint_type"].val(); + } else if (mod.has_child("constraint")) { + std::string constraintValue = mod["constraint"].val(); + if (constraintValue == "Gauss" || constraintValue == "Poisson" || constraintValue == "Const" || + constraintValue == "Lognormal") { + constraint = constraintValue; + } + } shapeElems.add(createPHF(funcName, sysname, parnames, vals, tool, constraints, varlist, constraint, defaultGammaMin, defaultShapeSysGammaMax, minShapeUncertainty)); } else if (modtype == "custom") { @@ -1165,13 +1176,11 @@ void configureStatError(Channel &channel) bool exportChannel(RooJSONFactoryWSTool *tool, const Channel &channel, JSONNode &elem) { - // Write the constraint reference (either by name or by type) for any - // modifier that supports an external Gaussian/Poisson/etc. constraint. + // Write the constraint reference for any modifier that supports an + // external Gaussian/Poisson/etc. constraint. auto writeConstraint = [](JSONNode &mod, auto const &sys) { if (sys.constraint) { - mod["constraint_name"] << sys.constraint->GetName(); - } else if (sys.constraintType) { - mod["constraint_type"] << toString(sys.constraintType); + mod["constraint"] << sys.constraint->GetName(); } }; @@ -1192,7 +1201,7 @@ bool exportChannel(RooJSONFactoryWSTool *tool, const Channel &channel, JSONNode mod["parameter"] << nf.param->GetName(); mod["type"] << "normfactor"; if (nf.constraint) { - mod["constraint_name"] << nf.constraint->GetName(); + mod["constraint"] << nf.constraint->GetName(); tool->queueExport(*nf.constraint); } } @@ -1264,7 +1273,6 @@ bool exportChannel(RooJSONFactoryWSTool *tool, const Channel &channel, JSONNode mod["name"] << ::Literals::staterror; mod["type"] << ::Literals::staterror; optionallyExportGammaParameters(mod, "stat_" + channel.name, sample.staterrorParameters); - mod["constraint_type"] << toString(sample.barlowBeestonLightConstraintType); } if (!observablesWritten) { diff --git a/roofit/hs3/src/JSONFactories_RooFitCore.cxx b/roofit/hs3/src/JSONFactories_RooFitCore.cxx index 6a9fd7ffad156..e062dab212fa9 100644 --- a/roofit/hs3/src/JSONFactories_RooFitCore.cxx +++ b/roofit/hs3/src/JSONFactories_RooFitCore.cxx @@ -63,6 +63,8 @@ #include #include +#include +#include using RooFit::Detail::JSONNode; @@ -71,6 +73,11 @@ using RooFit::Detail::JSONNode; /////////////////////////////////////////////////////////////////////////////////////////////////////// namespace { +bool isReservedExpressionIdentifier(const std::string &arg) +{ + return arg == "PI" || arg == "EULER" || arg == "TMath"; +} + /** * Extracts arguments from a mathematical expression. * @@ -110,16 +117,52 @@ std::set extractArguments(std::string expr) } std::string arg(expr.substr(startidx, i - startidx)); startidx = expr.size(); - arguments.insert(arg); + if (!isReservedExpressionIdentifier(arg)) { + arguments.insert(arg); + } } } } if (startidx < expr.size()) { - arguments.insert(expr.substr(startidx)); + std::string arg(expr.substr(startidx)); + if (!isReservedExpressionIdentifier(arg)) { + arguments.insert(arg); + } } return arguments; } +void replaceIdentifier(TString &expr, std::string_view identifier, std::string_view replacement) +{ + std::string in(expr.Data()); + std::string out; + out.reserve(in.size()); + + for (std::size_t pos = 0; pos < in.size();) { + const bool matches = in.compare(pos, identifier.size(), identifier) == 0; + const bool beforeIdentifier = + pos > 0 && (std::isalnum(static_cast(in[pos - 1])) || in[pos - 1] == '_'); + const std::size_t end = pos + identifier.size(); + const bool afterIdentifier = + end < in.size() && (std::isalnum(static_cast(in[end])) || in[end] == '_'); + if (matches && !beforeIdentifier && !afterIdentifier) { + out.append(replacement); + pos = end; + } else { + out.push_back(in[pos]); + ++pos; + } + } + + expr = out.c_str(); +} + +void translateImportedExpression(TString &expr) +{ + replaceIdentifier(expr, "PI", "TMath::Pi()"); + replaceIdentifier(expr, "EULER", "TMath::E()"); +} + template class RooFormulaArgFactory : public RooFit::JSONIO::Importer { public: @@ -130,6 +173,7 @@ class RooFormulaArgFactory : public RooFit::JSONIO::Importer { RooJSONFactoryWSTool::error("no expression given for '" + name + "'"); } TString formula(p["expression"].val()); + translateImportedExpression(formula); RooArgList dependents; for (const auto &d : extractArguments(formula.Data())) { dependents.add(*tool->request(d, name)); @@ -202,13 +246,14 @@ class RooAddModelFactory : public RooFit::JSONIO::Importer { } }; +template class RooBinWidthFunctionFactory : public RooFit::JSONIO::Importer { public: bool importArg(RooJSONFactoryWSTool *tool, const JSONNode &p) const override { std::string name(RooJSONFactoryWSTool::name(p)); RooHistFunc *hf = static_cast(tool->request(p["histogram"].val(), name)); - tool->wsEmplace(name, *hf, p["divideByBinWidth"].val_bool()); + tool->wsEmplace(name, *hf, DivideByBinWidth); return true; } }; @@ -265,6 +310,7 @@ class RooRealSumFuncFactory : public RooFit::JSONIO::Importer { return true; } }; + template class RooPolynomialFactory : public RooFit::JSONIO::Importer { public: @@ -550,6 +596,9 @@ class ParamHistFuncFactory : public RooFit::JSONIO::Importer { public: bool importArg(RooJSONFactoryWSTool *tool, const JSONNode &p) const override { + if (!p.has_child("parameters")) { + return false; + } std::string name(RooJSONFactoryWSTool::name(p)); RooArgList varList = tool->requestArgList(p, "variables"); if (!p.has_child("axes")) { @@ -733,7 +782,7 @@ class RooHistFactory : public RooFit::JSONIO::Importer { { std::string name(RooJSONFactoryWSTool::name(p)); if (!p.has_child("data")) { - RooJSONFactoryWSTool::error("function '" + name + "' is of histogram type, but does not define a 'data' key"); + return false; } std::unique_ptr dataHist = RooJSONFactoryWSTool::readBinnedData(p["data"], name, RooJSONFactoryWSTool::readAxes(p["data"])); @@ -762,9 +811,8 @@ class RooBinWidthFunctionStreamer : public RooFit::JSONIO::Exporter { bool exportObject(RooJSONFactoryWSTool *, const RooAbsArg *func, JSONNode &elem) const override { const RooBinWidthFunction *pdf = static_cast(func); - elem["type"] << key(); + elem["type"] << (pdf->divideByBinWidth() ? "inverse_binvolume" : "binvolume"); elem["histogram"] << pdf->histFunc().GetName(); - elem["divideByBinWidth"] << pdf->divideByBinWidth(); return true; } }; @@ -806,6 +854,17 @@ class RooFormulaArgStreamer : public RooFit::JSONIO::Exporter { expr.ReplaceAll("TMath::Sqrt", "sqrt"); expr.ReplaceAll("TMath::Power", "pow"); expr.ReplaceAll("TMath::Erf", "erf"); + expr.ReplaceAll("TMath::Floor", "floor"); + expr.ReplaceAll("TMath::Ceil", "ceil"); + expr.ReplaceAll("TMath::Abs", "abs"); + expr.ReplaceAll("TMath::Tan", "tan"); + expr.ReplaceAll("TMath::ASin", "asin"); + expr.ReplaceAll("TMath::ACos", "acos"); + expr.ReplaceAll("TMath::ATan", "atan"); + expr.ReplaceAll("TMath::Pi()", "PI"); + expr.ReplaceAll("TMath::Pi", "PI"); + expr.ReplaceAll("TMath::E()", "EULER"); + expr.ReplaceAll("TMath::E", "EULER"); } }; // Write the "x" reference and the coefficient list for polynomial-like @@ -1176,27 +1235,27 @@ class RooWrapperPdfStreamer : public RooFit::JSONIO::Exporter { template <> DEFINE_EXPORTER_KEY(RooAddPdfStreamer, "mixture_dist"); template <> -DEFINE_EXPORTER_KEY(RooAddPdfStreamer, "mixture_model"); +DEFINE_EXPORTER_KEY(RooAddPdfStreamer, "mixture_resolution_model"); DEFINE_EXPORTER_KEY(RooBinSamplingPdfStreamer, "binsampling"); DEFINE_EXPORTER_KEY(RooWrapperPdfStreamer, "density_function_dist"); -DEFINE_EXPORTER_KEY(RooBinWidthFunctionStreamer, "binwidth"); +DEFINE_EXPORTER_KEY(RooBinWidthFunctionStreamer, "binvolume"); template <> DEFINE_EXPORTER_KEY(RooPolynomialStreamer, "legacy_exp_poly_dist"); DEFINE_EXPORTER_KEY(RooExponentialStreamer, "exponential_dist"); template <> -DEFINE_EXPORTER_KEY(RooFormulaArgStreamer, "generic_function"); +DEFINE_EXPORTER_KEY(RooFormulaArgStreamer, "generic"); template <> DEFINE_EXPORTER_KEY(RooFormulaArgStreamer, "generic_dist"); template <> -DEFINE_EXPORTER_KEY(RooHistStreamer, "histogram"); +DEFINE_EXPORTER_KEY(RooHistStreamer, "step"); template <> DEFINE_EXPORTER_KEY(RooHistStreamer, "histogram_dist"); DEFINE_EXPORTER_KEY(RooLogNormalStreamer, "lognormal_dist"); DEFINE_EXPORTER_KEY(RooMultiVarGaussianStreamer, "multivariate_normal_dist"); DEFINE_EXPORTER_KEY(RooPoissonStreamer, "poisson_dist"); DEFINE_EXPORTER_KEY(RooDecayStreamer, "decay_dist"); -DEFINE_EXPORTER_KEY(RooTruthModelStreamer, "truth_model_function"); -DEFINE_EXPORTER_KEY(RooGaussModelStreamer, "gauss_model_function"); +DEFINE_EXPORTER_KEY(RooTruthModelStreamer, "delta_resolution_model"); +DEFINE_EXPORTER_KEY(RooGaussModelStreamer, "gauss_resolution_model"); template <> DEFINE_EXPORTER_KEY(RooPolynomialStreamer, "polynomial_dist"); template <> @@ -1206,7 +1265,7 @@ DEFINE_EXPORTER_KEY(RooRealSumPdfStreamer, "weighted_sum_dist"); DEFINE_EXPORTER_KEY(RooTFnBindingStreamer, "generic_function"); DEFINE_EXPORTER_KEY(RooRealIntegralStreamer, "integral"); DEFINE_EXPORTER_KEY(RooDerivativeStreamer, "derivative"); -DEFINE_EXPORTER_KEY(RooFFTConvPdfStreamer, "fft_conv_pdf"); +DEFINE_EXPORTER_KEY(RooFFTConvPdfStreamer, "fft_convolution_dist"); DEFINE_EXPORTER_KEY(RooExtendPdfStreamer, "rate_extended_dist"); DEFINE_EXPORTER_KEY(ParamHistFuncStreamer, "step"); DEFINE_EXPORTER_KEY(RooSplineStreamer, "spline"); @@ -1224,28 +1283,31 @@ STATIC_EXECUTE([]() { registerImporter("product_dist", false); registerImporter("sum", false); registerImporter("mixture_dist", false); - registerImporter("mixture_model", false); + registerImporter("mixture_resolution_model", false); registerImporter("binsampling_dist", false); - registerImporter("binwidth", false); + registerImporter>("binvolume", false); + registerImporter>("inverse_binvolume", false); registerImporter>("legacy_exp_poly_dist", false); registerImporter("exponential_dist", false); + registerImporter>("generic", false); registerImporter>("generic_function", false); registerImporter>("generic_dist", false); registerImporter>("histogram", false); + registerImporter>("step", false); registerImporter>("histogram_dist", false); registerImporter("lognormal_dist", false); registerImporter("multivariate_normal_dist", false); registerImporter("poisson_dist", false); registerImporter("decay_dist", false); - registerImporter("truth_model_function", false); - registerImporter("gauss_model_function", false); + registerImporter("delta_resolution_model", false); + registerImporter("gauss_resolution_model", false); registerImporter>("polynomial_dist", false); registerImporter>("polynomial", false); registerImporter("weighted_sum_dist", false); registerImporter("weighted_sum", false); registerImporter("integral", false); registerImporter("derivative", false); - registerImporter("fft_conv_pdf", false); + registerImporter("fft_convolution_dist", false); registerImporter("extend_pdf", false); registerImporter("step", false); registerImporter("spline", false); diff --git a/roofit/hs3/src/RooJSONFactoryWSTool.cxx b/roofit/hs3/src/RooJSONFactoryWSTool.cxx index 0d93c738aae54..7681f5da7c735 100644 --- a/roofit/hs3/src/RooJSONFactoryWSTool.cxx +++ b/roofit/hs3/src/RooJSONFactoryWSTool.cxx @@ -1090,8 +1090,8 @@ void RooJSONFactoryWSTool::exportVariable(const RooAbsArg *v, JSONNode &node, bo var["const"] << true; } else if (rrv) { var["value"] << rrv->getVal(); - if (rrv->isConstant() && storeConstant) { - var["const"] << rrv->isConstant(); + if (storeConstant && (rrv->isConstant() || rrv->getMin() >= rrv->getMax())) { + var["const"] << true; } else if (storeBins) { var["min"] << rrv->getMin(); var["max"] << rrv->getMax(); diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index 3fcf10dc044de..f9b4af4c0ed74 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -29,6 +30,7 @@ #include #include #include +#include #include @@ -417,6 +419,30 @@ TEST(RooFitHS3, ParameterStepWidthsFallbackExcludesDataAxes) EXPECT_FALSE(ws2.var("x")->hasError()); } +TEST(RooFitHS3, FixedRangeParameterExportsConst) +{ + RooWorkspace ws{"ws_fixed_range"}; + RooRealVar x{"x", "x", 0.0, -5.0, 5.0}; + RooRealVar fixed{"fixed", "fixed", 1.0, 1.0, 1.0}; + RooRealVar sigma{"sigma", "sigma", 1.0, 0.1, 10.0}; + RooGaussian gauss{"gauss", "gauss", x, fixed, sigma}; + fixed.setConstant(false); + ws.import(gauss, RooFit::Silence()); + + const std::string json = RooJSONFactoryWSTool{ws}.exportJSONtoString(); + const auto fixedPos = json.find("\"const\":true,\"name\":\"fixed\""); + ASSERT_NE(fixedPos, std::string::npos) << json; + const auto fixedBegin = json.rfind("{", fixedPos); + ASSERT_NE(fixedBegin, std::string::npos) << json; + const auto fixedEnd = json.find("}", fixedPos); + ASSERT_NE(fixedEnd, std::string::npos) << json; + const std::string fixedNode = json.substr(fixedBegin, fixedEnd - fixedBegin); + + EXPECT_NE(fixedNode.find("\"const\":true"), std::string::npos) << fixedNode; + EXPECT_EQ(fixedNode.find("\"min\""), std::string::npos) << fixedNode; + EXPECT_EQ(fixedNode.find("\"max\""), std::string::npos) << fixedNode; +} + TEST(RooFitHS3, ParameterStepWidthsImportAfterDefaultSnapshot) { const std::string json = R"({ @@ -681,6 +707,33 @@ TEST(RooFitHS3, RooGenericPdf) EXPECT_EQ(status, 0); } +TEST(RooFitHS3, GenericExpressionCleanup) +{ + RooRealVar x{"x", "x", 0.5, -1.0, 1.0}; + RooFormulaVar formula{"formula", + "formula", + "TMath::Floor(x) + TMath::Ceil(x) + TMath::Abs(x) + TMath::Tan(x) + " + "TMath::ASin(x / 2.) + TMath::ACos(x / 2.) + TMath::ATan(x) + TMath::Pi() + TMath::E()", + RooArgList{x}}; + + RooWorkspace ws1{"ws_expr_cleanup"}; + ws1.import(formula, RooFit::Silence()); + const std::string json = RooJSONFactoryWSTool{ws1}.exportJSONtoString(); + + for (const char *token : {"floor", "ceil", "abs", "tan", "asin", "acos", "atan", "PI", "EULER"}) { + EXPECT_NE(json.find(token), std::string::npos) << json; + } + EXPECT_EQ(json.find("TMath::Pi"), std::string::npos) << json; + EXPECT_EQ(json.find("TMath::E"), std::string::npos) << json; + + RooWorkspace ws2{"ws_expr_cleanup_2"}; + ASSERT_TRUE(RooJSONFactoryWSTool{ws2}.importJSONfromString(json)); + auto *imported = ws2.function("formula"); + ASSERT_NE(imported, nullptr); + ws2.var("x")->setVal(0.5); + EXPECT_DOUBLE_EQ(imported->getVal(), formula.getVal()); +} + TEST(RooFitHS3, RooHistPdf) { RooHelpers::LocalChangeMsgLevel changeMsgLvl(RooFit::WARNING); @@ -696,6 +749,73 @@ TEST(RooFitHS3, RooHistPdf) EXPECT_EQ(status, 0); } +TEST(RooFitHS3, RooBinWidthFunctionUsesBinVolumeKeys) +{ + RooRealVar x{"x", "x", 0.0, 2.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}; + RooBinWidthFunction binVolume{"binVolume", "binVolume", histFunc, false}; + RooBinWidthFunction inverseBinVolume{"inverseBinVolume", "inverseBinVolume", histFunc, true}; + + RooWorkspace ws1{"ws_binvolume"}; + ws1.import(binVolume, RooFit::Silence()); + ws1.import(inverseBinVolume, RooFit::Silence(), RooFit::RecycleConflictNodes()); + + const std::string json = RooJSONFactoryWSTool{ws1}.exportJSONtoString(); + EXPECT_NE(json.find("\"type\":\"binvolume\""), std::string::npos) << json; + EXPECT_NE(json.find("\"type\":\"inverse_binvolume\""), std::string::npos) << json; + EXPECT_EQ(json.find("divideByBinWidth"), std::string::npos) << json; + EXPECT_EQ(json.find("\"type\":\"binwidth\""), std::string::npos) << json; + + RooWorkspace ws2{"ws_binvolume_2"}; + ASSERT_TRUE(RooJSONFactoryWSTool{ws2}.importJSONfromString(json)); + auto *importedBinVolume = dynamic_cast(ws2.function("binVolume")); + auto *importedInverseBinVolume = dynamic_cast(ws2.function("inverseBinVolume")); + ASSERT_NE(importedBinVolume, nullptr); + ASSERT_NE(importedInverseBinVolume, nullptr); + EXPECT_FALSE(importedBinVolume->divideByBinWidth()); + EXPECT_TRUE(importedInverseBinVolume->divideByBinWidth()); +} + +TEST(RooFitHS3, StepDispatchesToRooHistFuncAndParamHistFunc) +{ + RooRealVar x{"x", "x", 0.0, 2.0}; + x.setBins(2); + + RooDataHist dataHist{"dataHist", "dataHist", x}; + dataHist.set(0, 3.0, -1); + dataHist.set(1, 5.0, -1); + + RooHistFunc histFunc{"histFunc", "histFunc", x, dataHist}; + RooRealVar p0{"p0", "p0", 1.0}; + RooRealVar p1{"p1", "p1", 2.0}; + ParamHistFunc paramHistFunc{"paramHistFunc", "paramHistFunc", RooArgList{x}, RooArgList{p0, p1}}; + + RooWorkspace ws1{"ws_step"}; + ws1.import(histFunc, RooFit::Silence()); + ws1.import(paramHistFunc, RooFit::Silence()); + + const std::string json = RooJSONFactoryWSTool{ws1}.exportJSONtoString(); + EXPECT_NE(json.find("\"name\":\"histFunc\",\"type\":\"step\""), std::string::npos) << json; + EXPECT_EQ(json.find("\"name\":\"histFunc\",\"type\":\"histogram\""), std::string::npos) << json; + const auto paramHistFuncPos = json.find("\"name\":\"paramHistFunc\""); + ASSERT_NE(paramHistFuncPos, std::string::npos) << json; + const auto paramHistFuncEnd = json.find("}", paramHistFuncPos); + ASSERT_NE(paramHistFuncEnd, std::string::npos) << json; + const std::string paramHistFuncNode = json.substr(paramHistFuncPos, paramHistFuncEnd - paramHistFuncPos); + EXPECT_NE(paramHistFuncNode.find("\"type\":\"step\""), std::string::npos) << paramHistFuncNode; + + RooWorkspace ws2{"ws_step_2"}; + ASSERT_TRUE(RooJSONFactoryWSTool{ws2}.importJSONfromString(json)); + EXPECT_NE(dynamic_cast(ws2.function("histFunc")), nullptr); + EXPECT_NE(dynamic_cast(ws2.function("paramHistFunc")), nullptr); +} + TEST(RooFitHS3, RooLandau) { int status = validate({"Landau::landau(x[0, 10], mean[5], sigma[1.0, 0.1, 10])"}); @@ -1159,6 +1279,83 @@ TEST(RooFitHS3, HistFactoryZeroYieldBin) } } +TEST(RooFitHS3, HistFactoryConstraintKeyMigration) +{ + const std::string jsonStr = R"({ + "metadata": {"hs3_version": "0.1.90"}, + "domains": [ + { + "name": "default_domain", + "type": "product_domain", + "axes": [ + {"name": "obs_channel0", "min": 0.0, "max": 2.0, "nbins": 2}, + {"name": "nom_mu", "min": 1.0, "max": 1.0}, + {"name": "sigma_mu", "min": 1.0, "max": 1.0} + ] + } + ], + "parameter_points": [ + { + "name": "default_values", + "parameters": [ + {"name": "nom_mu", "value": 1.0, "const": true}, + {"name": "sigma_mu", "value": 1.0, "const": true} + ] + } + ], + "distributions": [ + { + "name": "model_channel0", + "type": "histfactory_dist", + "axes": [ + {"name": "obs_channel0", "min": 0.0, "max": 2.0, "nbins": 2} + ], + "samples": [ + { + "name": "sig", + "data": {"contents": [10.0, 20.0]}, + "modifiers": [ + { + "name": "mu", + "parameter": "mu", + "type": "normfactor", + "constraint_name": "muConstraint" + } + ] + } + ] + }, + { + "name": "muConstraint", + "type": "gaussian_dist", + "x": "mu", + "mean": "nom_mu", + "sigma": "sigma_mu" + } + ], + "data": [ + { + "name": "obsData_channel0", + "type": "binned", + "axes": [ + {"name": "obs_channel0", "min": 0.0, "max": 2.0, "nbins": 2} + ], + "contents": [10.0, 20.0] + } + ] + })"; + + RooHelpers::LocalChangeMsgLevel changeMsgLvl(RooFit::WARNING); + + RooWorkspace ws{"ws_hf_constraint"}; + ASSERT_TRUE(RooJSONFactoryWSTool{ws}.importJSONfromString(jsonStr)); + + const std::string exported = RooJSONFactoryWSTool{ws}.exportJSONtoString(); + EXPECT_NE(exported.find("\"constraint\":\"muConstraint\""), std::string::npos) << exported; + EXPECT_EQ(exported.find("constraint_name"), std::string::npos) << exported; + EXPECT_EQ(exported.find("constraint_type"), std::string::npos) << exported; +} + // Snapshot export must keep all variables that any pdf depends on, even when // the variable is not in the set of separately exported objects. Global // observables of HistFactory constraint pdfs (the nominal "nom_*" parameters) From 88cc3451c1f1915d7e128bf30b068248587d810d Mon Sep 17 00:00:00 2001 From: Phmonski Date: Fri, 3 Jul 2026 09:54:59 +0200 Subject: [PATCH 2/5] [HS3] fix bug with importing legacy workspaces --- roofit/hs3/src/JSONFactories_HistFactory.cxx | 91 +++++++++++++++----- roofit/hs3/test/testRooFitHS3.cxx | 66 ++++++++++++++ 2 files changed, 137 insertions(+), 20 deletions(-) diff --git a/roofit/hs3/src/JSONFactories_HistFactory.cxx b/roofit/hs3/src/JSONFactories_HistFactory.cxx index 9510b26897588..79cfad2824cd2 100644 --- a/roofit/hs3/src/JSONFactories_HistFactory.cxx +++ b/roofit/hs3/src/JSONFactories_HistFactory.cxx @@ -191,6 +191,43 @@ std::string constraintName(std::string const ¶mName) return paramName + "Constraint"; } +bool isLegacyConstraintType(std::string const &value) +{ + return value == "Gauss" || value == "Poisson" || value == "Const" || value == "Lognormal"; +} + +RooAbsPdf *findNamedConstraint(RooJSONFactoryWSTool &tool, std::string const &constraintName, + std::string const &sample) +{ + if (auto *constraint = tool.workspace()->pdf(constraintName)) { + return constraint; + } + + try { + return tool.request(constraintName, sample); + } catch (RooJSONFactoryWSTool::DependencyMissingError const &err) { + if (err.child() != constraintName) { + throw; + } + } + + return nullptr; +} + +RooAbsPdf &createLegacyConstraint(RooJSONFactoryWSTool &tool, const JSONNode &mod, RooRealVar ¶m, + std::string const &constraintType) +{ + if (constraintType == "Gauss") { + param.setError(1.0); + return getOrCreate(*tool.workspace(), constraintName(param.GetName()), param, + *tool.workspace()->var(std::string("nom_") + param.GetName()), 1.); + } + + RooJSONFactoryWSTool::error("legacy constraint value '" + constraintType + "' for modifier '" + + RooJSONFactoryWSTool::name(mod) + + "' is a known constraint type, but it cannot be resolved in this context"); +} + ParamHistFunc &createPHF(const std::string &phfname, std::string const &sysname, const std::vector &parnames, const std::vector &vals, RooJSONFactoryWSTool &tool, RooAbsCollection &constraints, const RooArgSet &observables, @@ -255,16 +292,10 @@ const JSONNode &findStaterror(const JSONNode &comp) RooAbsPdf & getOrCreateConstraint(RooJSONFactoryWSTool &tool, const JSONNode &mod, RooRealVar ¶m, const std::string &sample) { - JSONNode const *constrName = mod.find("constraint"); - if (!constrName) { - constrName = mod.find("constraint_name"); - } + JSONNode const *constrName = mod.find("constraint_name"); if (constrName) { auto constraint_name = constrName->val(); - auto constraint = tool.workspace()->pdf(constraint_name); - if (!constraint) { - constraint = tool.request(constrName->val(), sample); - } + auto constraint = findNamedConstraint(tool, constraint_name, sample); if (!constraint) { RooJSONFactoryWSTool::error("unable to find definition of of constraint '" + constraint_name + "' for modifier '" + RooJSONFactoryWSTool::name(mod) + "'"); @@ -273,19 +304,35 @@ getOrCreateConstraint(RooJSONFactoryWSTool &tool, const JSONNode &mod, RooRealVa param.setError(gauss->getSigma().getVal()); } return *constraint; - } else { - std::string constraint_type = "Gauss"; - if (auto constrType = mod.find("constraint_type")) { - constraint_type = constrType->val(); + } + + if (auto constr = mod.find("constraint")) { + std::string constraintValue = constr->val(); + if (auto *constraint = findNamedConstraint(tool, constraintValue, sample)) { + if (auto gauss = dynamic_cast(constraint)) { + param.setError(gauss->getSigma().getVal()); + } + return *constraint; } - if (constraint_type == "Gauss") { - param.setError(1.0); - return getOrCreate(*tool.workspace(), constraintName(param.GetName()), param, - *tool.workspace()->var(std::string("nom_") + param.GetName()), 1.); + + if (isLegacyConstraintType(constraintValue)) { + return createLegacyConstraint(tool, mod, param, constraintValue); } - RooJSONFactoryWSTool::error("unknown or invalid constraint for modifier '" + RooJSONFactoryWSTool::name(mod) + - "'"); + + RooJSONFactoryWSTool::error("unable to resolve constraint value '" + constraintValue + "' for modifier '" + + RooJSONFactoryWSTool::name(mod) + + "': this looks like a legacy workspace where the 'constraint' field is neither a " + "constraint pdf name nor a supported legacy constraint type"); } + + std::string constraint_type = "Gauss"; + if (auto constrType = mod.find("constraint_type")) { + constraint_type = constrType->val(); + } + if (isLegacyConstraintType(constraint_type)) { + return createLegacyConstraint(tool, mod, param, constraint_type); + } + RooJSONFactoryWSTool::error("unknown or invalid constraint for modifier '" + RooJSONFactoryWSTool::name(mod) + "'"); } double poissonTau(RooPoisson const &constraint, RooAbsArg const &gamma) { @@ -438,9 +485,13 @@ bool importHistSample(RooJSONFactoryWSTool &tool, RooDataHist &dh, RooArgSet con constraint = mod["constraint_type"].val(); } else if (mod.has_child("constraint")) { std::string constraintValue = mod["constraint"].val(); - if (constraintValue == "Gauss" || constraintValue == "Poisson" || constraintValue == "Const" || - constraintValue == "Lognormal") { + if (isLegacyConstraintType(constraintValue)) { constraint = constraintValue; + } else { + RooJSONFactoryWSTool::error("unable to resolve constraint value '" + constraintValue + + "' for modifier '" + RooJSONFactoryWSTool::name(mod) + + "': this looks like a legacy workspace where the 'constraint' field is " + "not a supported legacy constraint type"); } } shapeElems.add(createPHF(funcName, sysname, parnames, vals, tool, constraints, varlist, constraint, diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index f9b4af4c0ed74..e5a2cf42e6e99 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -1356,6 +1356,72 @@ TEST(RooFitHS3, HistFactoryConstraintKeyMigration) EXPECT_EQ(exported.find("constraint_type"), std::string::npos) << exported; } +TEST(RooFitHS3, HistFactoryLegacyConstraintTypeInConstraintKey) +{ + const std::string jsonStr = R"({ + "metadata": {"hs3_version": "0.1.90"}, + "domains": [ + { + "name": "default_domain", + "type": "product_domain", + "axes": [ + {"name": "obs_channel0", "min": 0.0, "max": 2.0, "nbins": 2} + ] + } + ], + "distributions": [ + { + "name": "model_channel0", + "type": "histfactory_dist", + "axes": [ + {"name": "obs_channel0", "min": 0.0, "max": 2.0, "nbins": 2} + ], + "samples": [ + { + "name": "sig", + "data": {"contents": [10.0, 20.0]}, + "modifiers": [ + { + "name": "lumi", + "type": "normsys", + "constraint": "Gauss", + "data": {"lo": 0.95, "hi": 1.05} + } + ] + } + ] + } + ], + "data": [ + { + "name": "obsData_channel0", + "type": "binned", + "axes": [ + {"name": "obs_channel0", "min": 0.0, "max": 2.0, "nbins": 2} + ], + "contents": [10.0, 20.0] + } + ] + })"; + + RooHelpers::LocalChangeMsgLevel changeMsgLvl(RooFit::WARNING); + + RooWorkspace ws{"ws_hf_legacy_constraint_type"}; + ASSERT_TRUE(RooJSONFactoryWSTool{ws}.importJSONfromString(jsonStr)); + + auto *constraint = dynamic_cast(ws.pdf("alpha_lumiConstraint")); + ASSERT_NE(constraint, nullptr); + auto *alpha = ws.var("alpha_lumi"); + ASSERT_NE(alpha, nullptr); + EXPECT_DOUBLE_EQ(alpha->getError(), 1.0); + + const std::string exported = RooJSONFactoryWSTool{ws}.exportJSONtoString(); + EXPECT_NE(exported.find("\"constraint\":\"alpha_lumiConstraint\""), std::string::npos) << exported; + EXPECT_EQ(exported.find("\"constraint\":\"Gauss\""), std::string::npos) << exported; + EXPECT_EQ(exported.find("constraint_name"), std::string::npos) << exported; + EXPECT_EQ(exported.find("constraint_type"), std::string::npos) << exported; +} + // Snapshot export must keep all variables that any pdf depends on, even when // the variable is not in the set of separately exported objects. Global // observables of HistFactory constraint pdfs (the nominal "nom_*" parameters) From 23dd4fc389869a6d5c27009e0d67f2021fad7bbe Mon Sep 17 00:00:00 2001 From: Phmonski Date: Tue, 7 Jul 2026 11:24:59 +0200 Subject: [PATCH 3/5] [HS3] Fix Histfactory ShapeSys constraints Export --- roofit/hs3/src/JSONFactories_HistFactory.cxx | 81 +++++++++++++++----- 1 file changed, 60 insertions(+), 21 deletions(-) diff --git a/roofit/hs3/src/JSONFactories_HistFactory.cxx b/roofit/hs3/src/JSONFactories_HistFactory.cxx index 79cfad2824cd2..6935597873e7a 100644 --- a/roofit/hs3/src/JSONFactories_HistFactory.cxx +++ b/roofit/hs3/src/JSONFactories_HistFactory.cxx @@ -136,23 +136,6 @@ RooAbsPdf *findConstraint(RooAbsArg *g) return nullptr; } -std::string toString(TClass *c) -{ - if (!c) { - return "Const"; - } - if (c == RooPoisson::Class()) { - return "Poisson"; - } - if (c == RooGaussian::Class()) { - return "Gauss"; - } - if (c == RooLognormal::Class()) { - return "Lognormal"; - } - return "unknown"; -} - inline std::string defaultGammaName(std::string const &sysname, std::size_t i) { return "gamma_" + sysname + "_bin_" + std::to_string(i); @@ -231,7 +214,8 @@ RooAbsPdf &createLegacyConstraint(RooJSONFactoryWSTool &tool, const JSONNode &mo ParamHistFunc &createPHF(const std::string &phfname, std::string const &sysname, const std::vector &parnames, const std::vector &vals, RooJSONFactoryWSTool &tool, RooAbsCollection &constraints, const RooArgSet &observables, - const std::string &constraintType, double gammaMin, double gammaMax, double minSigma) + const std::string &constraintType, double gammaMin, double gammaMax, double minSigma, + bool createConstraints = true) { RooWorkspace &ws = *tool.workspace(); @@ -249,7 +233,9 @@ ParamHistFunc &createPHF(const std::string &phfname, std::string const &sysname, auto &phf = tool.wsEmplace(phfname, observables, gammas); if (vals.size() > 0) { - if (constraintType != "Const") { + if (!createConstraints) { + configureConstrainedGammas(gammas, vals, minSigma); + } else if (constraintType != "Const") { auto constraintsInfo = createGammaConstraints( gammas, vals, minSigma, constraintType == "Poisson" ? Constraint::Poisson : Constraint::Gaussian); for (auto const &term : constraintsInfo.constraints) { @@ -481,7 +467,30 @@ bool importHistSample(RooJSONFactoryWSTool &tool, RooDataHist &dh, RooArgSet con "' with neither values nor parameters!"); } std::string constraint = "unknown"; - if (mod.has_child("constraint_type")) { + std::vector constraintPdfs; + bool const hasConstraintList = mod.has_child("constraints"); + if (hasConstraintList) { + for (const auto &v : mod["constraints"].children()) { + if (v.is_null()) { + constraintPdfs.push_back(nullptr); + } else { + std::string constraintName = v.val(); + auto *constraintPdf = findNamedConstraint(tool, constraintName, sampleName); + if (!constraintPdf) { + RooJSONFactoryWSTool::error("unable to find definition of constraint '" + constraintName + + "' for modifier '" + RooJSONFactoryWSTool::name(mod) + "'"); + } + constraintPdfs.push_back(constraintPdf); + } + } + std::size_t const nGammas = std::max(vals.size(), parnames.size()); + if (constraintPdfs.size() != nGammas) { + std::stringstream ss; + ss << "modifier '" << RooJSONFactoryWSTool::name(mod) << "' has " << constraintPdfs.size() + << " constraints, but " << nGammas << " parameters"; + RooJSONFactoryWSTool::error(ss.str()); + } + } else if (mod.has_child("constraint_type")) { constraint = mod["constraint_type"].val(); } else if (mod.has_child("constraint")) { std::string constraintValue = mod["constraint"].val(); @@ -495,7 +504,13 @@ bool importHistSample(RooJSONFactoryWSTool &tool, RooDataHist &dh, RooArgSet con } } shapeElems.add(createPHF(funcName, sysname, parnames, vals, tool, constraints, varlist, constraint, - defaultGammaMin, defaultShapeSysGammaMax, minShapeUncertainty)); + defaultGammaMin, defaultShapeSysGammaMax, minShapeUncertainty, + /*createConstraints=*/!hasConstraintList)); + for (auto *constraintPdf : constraintPdfs) { + if (constraintPdf) { + constraints.add(*constraintPdf); + } + } } else if (modtype == "custom") { RooAbsReal *obj = ws.function(sysname); if (!obj) { @@ -788,6 +803,7 @@ struct HistoSys { struct ShapeSys { std::string name; std::vector constraints; + std::vector constraintPdfs; std::vector parameters; RooAbsPdf const *constraint = nullptr; TClass *constraintType = RooGaussian::Class(); @@ -1180,16 +1196,22 @@ Channel readChannel(RooJSONFactoryWSTool *tool, const std::string &pdfname, cons } if (!constraint) { sys.constraints.push_back(0.0); + sys.constraintPdfs.push_back(nullptr); } else if (auto constraint_p = dynamic_cast(constraint)) { sys.constraints.push_back(1. / std::sqrt(poissonTau(*constraint_p, *g))); + sys.constraintPdfs.push_back(constraint_p); if (!sys.constraint) { sys.constraintType = RooPoisson::Class(); } } else if (auto constraint_g = dynamic_cast(constraint)) { sys.constraints.push_back(constraint_g->getSigma().getVal() / constraint_g->getMean().getVal()); + sys.constraintPdfs.push_back(constraint_g); if (!sys.constraint) { sys.constraintType = RooGaussian::Class(); } + } else { + RooJSONFactoryWSTool::error( + "currently, only RooPoisson and RooGaussian are supported as constraint types"); } } sample.shapesys.emplace_back(std::move(sys)); @@ -1297,6 +1319,16 @@ bool exportChannel(RooJSONFactoryWSTool *tool, const Channel &channel, JSONNode mod["type"] << "shapesys"; optionallyExportGammaParameters(mod, sys.name, sys.parameters); writeConstraint(mod, sys); + if (std::any_of(sys.constraintPdfs.begin(), sys.constraintPdfs.end(), [](auto *pdf) { return pdf != nullptr; })) { + auto &constraintNames = mod["constraints"].set_seq(); + for (auto *constraint : sys.constraintPdfs) { + if (constraint) { + constraintNames.append_child() << constraint->GetName(); + } else { + constraintNames.append_child().set_null(); + } + } + } auto &vals = mod["data"].set_map()["vals"]; if (sys.constraint || sys.constraintType) { vals.fill_seq(sys.constraints); @@ -1464,6 +1496,13 @@ bool tryExportHistFactory(RooJSONFactoryWSTool *tool, const std::string &pdfname tool->queueExport(*modifier.constraint); } } + for (auto &modifier : sample.shapesys) { + for (auto *constraint : modifier.constraintPdfs) { + if (constraint) { + tool->queueExport(*constraint); + } + } + } } // Export all the custom modifiers From 09ae02b13ee0febdf9e6b55393c7354538887583 Mon Sep 17 00:00:00 2001 From: Phmonski Date: Tue, 7 Jul 2026 12:59:24 +0200 Subject: [PATCH 4/5] [HS3] Update HS3TestSuit pull command to latest fixtures --- roofit/hs3/test/CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/roofit/hs3/test/CMakeLists.txt b/roofit/hs3/test/CMakeLists.txt index f5a1fea4a18b5..02c32bf467d4a 100644 --- a/roofit/hs3/test/CMakeLists.txt +++ b/roofit/hs3/test/CMakeLists.txt @@ -6,7 +6,7 @@ if(test_roofit_hs3testsuite) FetchContent_Declare( hs3testsuite GIT_REPOSITORY https://github.com/hep-statistics-serialization-standard/HS3TestSuite.git - GIT_TAG 9d04e321ae6fddd283a35507f14ecf852eb7df61 + GIT_TAG b2e5f19023025d03b7d2239154a02e69f43a1374 ) # suppress git interactive prompt so fetch fails fast if the repo is gone or unreachable set(ENV{GIT_TERMINAL_PROMPT} "0") From 0a57adab2242234e4c037e3a0cddbbaf183b79dc Mon Sep 17 00:00:00 2001 From: Phmonski Date: Thu, 9 Jul 2026 09:21:19 +0200 Subject: [PATCH 5/5] fix clang-format error --- roofit/hs3/src/JSONFactories_HistFactory.cxx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/roofit/hs3/src/JSONFactories_HistFactory.cxx b/roofit/hs3/src/JSONFactories_HistFactory.cxx index 6935597873e7a..35802e2e8e876 100644 --- a/roofit/hs3/src/JSONFactories_HistFactory.cxx +++ b/roofit/hs3/src/JSONFactories_HistFactory.cxx @@ -179,8 +179,7 @@ bool isLegacyConstraintType(std::string const &value) return value == "Gauss" || value == "Poisson" || value == "Const" || value == "Lognormal"; } -RooAbsPdf *findNamedConstraint(RooJSONFactoryWSTool &tool, std::string const &constraintName, - std::string const &sample) +RooAbsPdf *findNamedConstraint(RooJSONFactoryWSTool &tool, std::string const &constraintName, std::string const &sample) { if (auto *constraint = tool.workspace()->pdf(constraintName)) { return constraint; @@ -1319,7 +1318,8 @@ bool exportChannel(RooJSONFactoryWSTool *tool, const Channel &channel, JSONNode mod["type"] << "shapesys"; optionallyExportGammaParameters(mod, sys.name, sys.parameters); writeConstraint(mod, sys); - if (std::any_of(sys.constraintPdfs.begin(), sys.constraintPdfs.end(), [](auto *pdf) { return pdf != nullptr; })) { + if (std::any_of(sys.constraintPdfs.begin(), sys.constraintPdfs.end(), + [](auto *pdf) { return pdf != nullptr; })) { auto &constraintNames = mod["constraints"].set_seq(); for (auto *constraint : sys.constraintPdfs) { if (constraint) {