From 33e291407cfc2b4675923f303b2a411ad6c560e8 Mon Sep 17 00:00:00 2001 From: Carsten Burgard Date: Sun, 25 Oct 2026 19:40:48 +0100 Subject: [PATCH 1/9] [RF][HS3][after HS3 json-fixes] Write hs3_version "0.3" HS3 PR "json-fixes-0.3" defines `hs3_version` as a `"MAJOR.MINOR"` string and the version of the revised standard as `"0.3"`. RooFit now writes this version. Documents with a newer version are imported with a warning. This commit depends on the HS3 PR and should only be merged together with or after it. Assisted-by: claude-code:claude-opus-5-5 --- roofit/hs3/src/RooJSONFactoryWSTool.cxx | 14 ++++++++++---- roofit/hs3/test/testRooFitHS3.cxx | 7 ++----- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/roofit/hs3/src/RooJSONFactoryWSTool.cxx b/roofit/hs3/src/RooJSONFactoryWSTool.cxx index 9f372161b8e7c..9d490e491cb9b 100644 --- a/roofit/hs3/src/RooJSONFactoryWSTool.cxx +++ b/roofit/hs3/src/RooJSONFactoryWSTool.cxx @@ -106,7 +106,7 @@ tool.writedoc("hs3.tex") ~~~ */ -constexpr auto hs3VersionTag = "0.2"; +constexpr auto hs3VersionTag = "0.3"; using RooFit::Detail::JSONNode; using RooFit::Detail::JSONTree; @@ -825,8 +825,10 @@ void checkHS3Version(std::string const &version) } } -/// HS3 requires the names of all top-level objects to be unique within a document. Throw if a name is used twice, or -/// only warn if not `strict`: RooFit wrote such documents before HS3 v0.3, e.g. for a range and a pdf of one name. +/// HS3 requires the names of all top-level objects to be unique within a document. If `strict`, throw if a name is +/// used twice within one section or by a distribution and a function. Other clashes, and all clashes if not +/// `strict`, only warn: RooFit's own export still produces them, e.g. for a range and a pdf of one name, and it +/// must be able to read its own output. void checkUniqueNames(JSONNode const &rootnode, bool strict) { std::map sectionOfName; @@ -850,7 +852,11 @@ void checkUniqueNames(JSONNode const &rootnode, bool strict) // compatibility: analyses are not referenced by name, and likelihoods only from analyses, so there is no // ambiguity. auto isLegacyNamed = [](std::string const &s) { return s == "likelihoods" || s == "analyses"; }; - if (!strict || isLegacyNamed(firstSection) || isLegacyNamed(section)) { + // Within one section, and between distributions and functions (both can be referenced by name), a clash is + // ambiguous. Other clashes are produced by RooFit itself (a range, snapshot or data set named like a pdf). + auto isReferable = [](std::string const &s) { return s == "distributions" || s == "functions"; }; + const bool ambiguous = firstSection == section || (isReferable(firstSection) && isReferable(section)); + if (!strict || !ambiguous || isLegacyNamed(firstSection) || isLegacyNamed(section)) { RooJSONFactoryWSTool::warning("RooFitHS3: the name '" + name + "' is used in \"" + firstSection + "\" and in \"" + section + "\", which is not allowed by the HS3 standard."); continue; diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index 560aea148a51b..0d5b913cf0d9e 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -3443,12 +3443,9 @@ TEST(RooFitHS3, HistFactoryMissingInterpolation) // RooFit always writes a "default_interpolation" if a modifier relies on it. const std::string exported = RooJSONFactoryWSTool{ws}.exportJSONtoString(); EXPECT_NE(exported.find("default_interpolation"), std::string::npos) << exported; + EXPECT_NE(exported.find(R"("hs3_version":"0.3")"), std::string::npos) << exported; RooWorkspace ws2; - std::string exportedCurrent = exported; - const std::string exportedVersion = R"("hs3_version":"0.2")"; - ASSERT_NE(exportedCurrent.find(exportedVersion), std::string::npos) << exported; - exportedCurrent.replace(exportedCurrent.find(exportedVersion), exportedVersion.size(), R"("hs3_version":"0.3")"); - EXPECT_TRUE(RooJSONFactoryWSTool{ws2}.importJSONfromString(exportedCurrent)); + EXPECT_TRUE(RooJSONFactoryWSTool{ws2}.importJSONfromString(exported)); } // Malformed matrices, error arrays and bin edges are rejected instead of being read out of bounds. From 87748258bcaeca99080b682cf0d97026af30ddaa Mon Sep 17 00:00:00 2001 From: Carsten Burgard Date: Sun, 25 Oct 2026 19:41:12 +0100 Subject: [PATCH 2/9] [RF][HS3][after HS3 json-fixes] Read the legacy singular analysis keys HS3 PR "json-fixes-0.3" renames the analysis keys to `parameters_of_interest` and `domains` (a list of domain names), which is what RooFit already writes, including one entry each for the nuisance parameter, global observable and parameter-of-interest domains. The keys of HS3 v0.2.9, `parameter_of_interest` and `domain` (a single name), are now also read, with a deprecation warning. The plural keys take precedence. Assisted-by: claude-code:claude-opus-5-5 --- roofit/hs3/src/RooJSONFactoryWSTool.cxx | 36 ++++++++++++++++----- roofit/hs3/test/testRooFitHS3.cxx | 42 +++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 8 deletions(-) diff --git a/roofit/hs3/src/RooJSONFactoryWSTool.cxx b/roofit/hs3/src/RooJSONFactoryWSTool.cxx index 9d490e491cb9b..0b80d475be453 100644 --- a/roofit/hs3/src/RooJSONFactoryWSTool.cxx +++ b/roofit/hs3/src/RooJSONFactoryWSTool.cxx @@ -609,15 +609,35 @@ void importAnalysis(const JSONNode &rootnode, const JSONNode &analysisNode, cons if (!extConstraints.empty()) mc->SetExternalConstraints(extConstraints); - auto readArgSet = [&](std::string const &name) { - RooArgSet out; - for (auto const &child : analysisNode[name].children()) { - out.add(*workspace.arg(child.val())); + // HS3 v0.2.9 used the singular keys "parameter_of_interest" (a list) and "domain" (a single name). They are still + // accepted, but the plural keys take precedence. + auto findAnalysisKey = [&](const char *key, const char *legacyKey) -> JSONNode const * { + if (auto node = analysisNode.find(key)) { + return node; } - return out; + if (auto node = analysisNode.find(legacyKey)) { + RooJSONFactoryWSTool::warning(std::string("RooFitHS3: the key \"") + legacyKey + "\" of analysis '" + + analysisName + "' is deprecated, please use \"" + key + "\" instead."); + return node; + } + return nullptr; }; + JSONNode const *poiNode = findAnalysisKey("parameters_of_interest", "parameter_of_interest"); + JSONNode const *domainsNodeOfAnalysis = findAnalysisKey("domains", "domain"); + std::vector analysisDomains; + if (domainsNodeOfAnalysis && domainsNodeOfAnalysis->is_seq()) { + analysisDomains = valsToStringVec(*domainsNodeOfAnalysis); + } else if (domainsNodeOfAnalysis) { + analysisDomains.push_back(domainsNodeOfAnalysis->val()); + } - mc->SetParametersOfInterest(readArgSet("parameters_of_interest")); + RooArgSet pois; + if (poiNode) { + for (auto const &child : poiNode->children()) { + pois.add(*workspace.arg(child.val())); + } + } + mc->SetParametersOfInterest(pois); mc->SetObservables(observables); RooArgSet pars; pdf->getParameters(&observables, pars); @@ -629,8 +649,8 @@ void importAnalysis(const JSONNode &rootnode, const JSONNode &analysisNode, cons pdf->getAllConstraints(observables, mainPars, /*stripDisconnected*/ true); RooArgSet domainPars; - for (auto &domain : analysisNode["domains"].children()) { - const auto &thisDomain = RooJSONFactoryWSTool::findNamedChild(domainsNode, domain.val()); + for (std::string const &domain : analysisDomains) { + const auto &thisDomain = RooJSONFactoryWSTool::findNamedChild(domainsNode, domain); if (!thisDomain || !thisDomain->has_child("axes")) continue; for (auto &var : (*thisDomain)["axes"].children()) { diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index 0d5b913cf0d9e..7240c0c18a6c0 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -3531,6 +3531,48 @@ TEST(RooFitHS3, BinnedDataZeroErrors) EXPECT_DOUBLE_EQ(data->weightSquared(2), 0.0); } +// The analysis keys "parameters_of_interest" and "domains" are written. The +// singular keys of HS3 v0.2.9 are still read, with a deprecation warning. +TEST(RooFitHS3, AnalysisLegacySingularKeys) +{ + RooWorkspace ws1; + ws1.factory("Gaussian::pdf(x[0, -5, 5], mu[0, -5, 5], sigma[1, 0.1, 5])"); + RooFit::ModelConfig mc1{"ModelConfig", &ws1}; + mc1.SetPdf("pdf"); + mc1.SetParametersOfInterest("mu"); + mc1.SetNuisanceParameters("sigma"); + mc1.SetObservables("x"); + ws1.import(mc1); + + const std::string json = RooJSONFactoryWSTool{ws1}.exportJSONtoString(); + EXPECT_NE(json.find(R"("parameters_of_interest":["mu"])"), std::string::npos) << json; + EXPECT_NE(json.find(R"("domains":["ModelConfig_nuisance_parameters","ModelConfig_parameters_of_interest"])"), + std::string::npos) + << json; + + // Turn the analysis into the singular HS3 v0.2.9 form, with only the nuisance parameter domain. + std::string legacyJson = json; + auto replace = [&](std::string const &from, std::string const &to) { + const auto pos = legacyJson.find(from); + ASSERT_NE(pos, std::string::npos) << legacyJson; + legacyJson.replace(pos, from.size(), to); + }; + replace(R"("parameters_of_interest":["mu"])", R"("parameter_of_interest":["mu"])"); + replace(R"("domains":["ModelConfig_nuisance_parameters","ModelConfig_parameters_of_interest"])", + R"("domain":"ModelConfig_nuisance_parameters")"); + + RooWorkspace ws2; + const std::string warnings = captureMessages(RooFit::WARNING, RooFit::ObjectHandling, [&] { + EXPECT_TRUE(RooJSONFactoryWSTool{ws2}.importJSONfromString(legacyJson)); + }); + EXPECT_NE(warnings.find(R"(the key "parameter_of_interest" of analysis)"), std::string::npos) << warnings; + EXPECT_NE(warnings.find(R"(the key "domain" of analysis)"), std::string::npos) << warnings; + auto *mc2 = dynamic_cast(ws2.obj("ModelConfig")); + ASSERT_NE(mc2, nullptr); + EXPECT_NE(mc2->GetParametersOfInterest()->find("mu"), nullptr); + EXPECT_NE(mc2->GetNuisanceParameters()->find("sigma"), nullptr); +} + // A RooRealVar that is constant in all parameter points must stay a RooRealVar, and a RooConstVar must stay a // RooConstVar. In the parameter points both are written the same way (a value and "const": true). The only // difference is that the RooRealVar is listed in a domain and the RooConstVar is not: a parameter without domain is From 4fefd50fb1e6197d2e42f903f90373ffb74b03f0 Mon Sep 17 00:00:00 2001 From: Carsten Burgard Date: Sun, 25 Oct 2026 19:41:39 +0100 Subject: [PATCH 3/9] [RF][HS3][after HS3 json-fixes] Omit infinite domain bounds instead of writing null RooFit wrote infinite bounds of product-domain axes as JSON `null`, which HS3 doesn't allow. HS3 PR "json-fixes-0.3" makes `min` and `max` of domain axes optional, with an absent bound meaning an unbounded range. Infinite bounds are now omitted on export. Since that PR also requires both bounds for a binning, the default binning is only written for bounded axes. Axes without any bound are still written (with only their `name`), which the HS3 PR keeps valid. RooFit needs them to tell an unbounded RooRealVar from a RooConstVar. On import, an absent bound means -inf/+inf, and `null` is still accepted for files written by older ROOT versions. The RooGaussianConstVarSigmaExport and ProductDomainEntriesExportExplicitBounds tests are updated to expect omitted instead of null bounds. Assisted-by: claude-code:claude-opus-5-5 --- roofit/hs3/src/Domains.cxx | 21 +++++++++++++-------- roofit/hs3/test/testRooFitHS3.cxx | 28 +++++++++++++++++++++------- 2 files changed, 34 insertions(+), 15 deletions(-) diff --git a/roofit/hs3/src/Domains.cxx b/roofit/hs3/src/Domains.cxx index a68513335d094..398c7db9e4b04 100644 --- a/roofit/hs3/src/Domains.cxx +++ b/roofit/hs3/src/Domains.cxx @@ -30,12 +30,12 @@ constexpr static auto defaultDomainName = "default_domain"; namespace { -void writeBound(RooFit::Detail::JSONNode &node, double value) +// JSON can't represent infinity, so an infinite bound is expressed by omitting it. An absent bound is read as +// infinite by readBound() in JSONIOUtils. +void writeBound(RooFit::Detail::JSONNode &node, const char *key, double value) { - if (RooNumber::isInfinite(value)) { - node.set_null(); - } else { - node << value; + if (!RooNumber::isInfinite(value)) { + node[key] << value; } } @@ -259,9 +259,14 @@ void Domains::ProductDomain::writeJSON(RooFit::Detail::JSONNode &node) const for (auto const &item : _map) { auto const &elem = item.second; RooFit::Detail::JSONNode &varnode = RooJSONFactoryWSTool::appendNamedChild(variablesNode, item.first); - writeBound(varnode["min"], elem.hasMin ? elem.min : -RooNumber::infinity()); - writeBound(varnode["max"], elem.hasMax ? elem.max : RooNumber::infinity()); - writeBinning(varnode, elem); + const double min = elem.hasMin ? elem.min : -RooNumber::infinity(); + const double max = elem.hasMax ? elem.max : RooNumber::infinity(); + writeBound(varnode, "min", min); + writeBound(varnode, "max", max); + // A binning requires a bounded range. + if (!RooNumber::isInfinite(min) && !RooNumber::isInfinite(max)) { + writeBinning(varnode, elem); + } } } void Domains::ProductDomain::populate(RooWorkspace &ws) const diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index 7240c0c18a6c0..07b81635b6409 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -514,12 +514,11 @@ TEST(RooFitHS3, ProductDomainEntriesExportExplicitBounds) EXPECT_DOUBLE_EQ((*xAxis)["min"].val_double(), -10.0); EXPECT_DOUBLE_EQ((*xAxis)["max"].val_double(), 10.0); + // Infinite bounds are expressed by omitting them, since JSON has no literal for infinity. auto const *meanAxis = RooJSONFactoryWSTool::findNamedChild((*defaultDomain)["axes"], "mean"); ASSERT_NE(meanAxis, nullptr); - ASSERT_TRUE(meanAxis->has_child("min")); - ASSERT_TRUE(meanAxis->has_child("max")); - EXPECT_TRUE((*meanAxis)["min"].is_null()); - EXPECT_TRUE((*meanAxis)["max"].is_null()); + EXPECT_FALSE(meanAxis->has_child("min")); + EXPECT_FALSE(meanAxis->has_child("max")); RooWorkspace imported; ASSERT_TRUE(RooJSONFactoryWSTool{imported}.importJSONfromString(json)); @@ -529,6 +528,21 @@ TEST(RooFitHS3, ProductDomainEntriesExportExplicitBounds) EXPECT_LT(importedMean->getMin(), 0.0); EXPECT_TRUE(std::isinf(importedMean->getMax())); EXPECT_GT(importedMean->getMax(), 0.0); + + // Files written by older RooFit versions express infinite bounds as null, which is still understood. + const std::string legacyJson = R"({ + "metadata": {"hs3_version": "0.2"}, + "domains": [{"name": "default_domain", "type": "product_domain", + "axes": [{"name": "a", "min": null, "max": 3.0}, {"name": "b", "min": 1.0}]}] + })"; + RooWorkspace legacy; + ASSERT_TRUE(RooJSONFactoryWSTool{legacy}.importJSONfromString(legacyJson)); + ASSERT_NE(legacy.var("a"), nullptr); + EXPECT_TRUE(std::isinf(legacy.var("a")->getMin())); + EXPECT_DOUBLE_EQ(legacy.var("a")->getMax(), 3.0); + ASSERT_NE(legacy.var("b"), nullptr); + EXPECT_DOUBLE_EQ(legacy.var("b")->getMin(), 1.0); + EXPECT_TRUE(std::isinf(legacy.var("b")->getMax())); } TEST(RooFitHS3, ProductDomainEntriesExportBinning) @@ -865,14 +879,14 @@ TEST(RooFitHS3, RooGaussianConstVarSigmaExport) EXPECT_NE(domainAxes.find("\"name\":\"sigma_real\""), std::string::npos) << domainAxes; // The unbounded constant RooRealVar is still a RooRealVar, so it gets a - // domain axis with explicit null bounds that distinguishes it from a RooConstVar. + // domain axis without bounds that distinguishes it from a RooConstVar. auto tree = RooFit::Detail::JSONTree::create(json); auto const *defaultDomain = RooJSONFactoryWSTool::findNamedChild(tree->rootnode()["domains"], "default_domain"); ASSERT_NE(defaultDomain, nullptr); auto const *meanAxis = RooJSONFactoryWSTool::findNamedChild((*defaultDomain)["axes"], "mean"); ASSERT_NE(meanAxis, nullptr); - EXPECT_TRUE((*meanAxis)["min"].is_null()); - EXPECT_TRUE((*meanAxis)["max"].is_null()); + EXPECT_FALSE(meanAxis->has_child("min")); + EXPECT_FALSE(meanAxis->has_child("max")); RooWorkspace imported; ASSERT_TRUE(RooJSONFactoryWSTool{imported}.importJSONfromString(json)); From 734c08665cc288a448cb255d6806fa72cb7b1150 Mon Sep 17 00:00:00 2001 From: Carsten Burgard Date: Thu, 5 Nov 2026 11:30:58 +0100 Subject: [PATCH 4/9] [RF][HS3][after HS3 json-fixes] Split the Crystal Ball into crystalball_dist and crystalball_doublesided_dist HS3 PR #135 defines `crystalball_dist` as the single-sided Crystal Ball (`sigma`, `alpha`, `n`, as RooCBShape) and the new `crystalball_doublesided_dist` as the double-sided one (`alpha_L`, `n_L`, `alpha_R`, `n_R`, and `sigma` or `sigma_L`/`sigma_R`, without a symmetric shorthand). * RooCBShape is exported as `crystalball_dist`, as before. * A single-sided RooCrystalBall is exported as `crystalball_dist`, and read back as a RooCBShape (same function, same sign convention for alpha). Before, it was written as an incomplete `crystalball_doublesided_dist` without `alpha_R`/`n_R`, which could not be read back. * A double-sided RooCrystalBall is exported as `crystalball_doublesided_dist`, with `sigma` if both sides share it, and the tails always as `alpha_L`/`n_L`/`alpha_R`/`n_R`. * `crystalball_doublesided_dist` is imported as RooCrystalBall, also with a single `sigma`, and invalid field combinations are rejected. * `crystalball_dist` with any `_L`/`_R` field is the double-sided form of HS3 versions before 0.3. It is still read as RooCrystalBall (including `alpha`/`n` for both tails with `sigma_L`/`sigma_R`), with a one-time deprecation warning. The `crystalball_doublesided_dist` factory expression and the RooCrystalBall export keys are removed, since the new importer and exporter handle all cases. Assisted-by: claude-code:claude-opus-5-5 --- roofit/hs3/src/JSONFactories_RooFitCore.cxx | 150 ++++++++++++++++++ roofit/hs3/src/RooFitHS3_wsexportkeys.cxx | 13 -- .../src/RooFitHS3_wsfactoryexpressions.cxx | 6 - roofit/hs3/test/testRooFitHS3.cxx | 143 +++++++++++++++++ 4 files changed, 293 insertions(+), 19 deletions(-) diff --git a/roofit/hs3/src/JSONFactories_RooFitCore.cxx b/roofit/hs3/src/JSONFactories_RooFitCore.cxx index ce7102885bc66..16ae38d8569d5 100644 --- a/roofit/hs3/src/JSONFactories_RooFitCore.cxx +++ b/roofit/hs3/src/JSONFactories_RooFitCore.cxx @@ -17,6 +17,8 @@ #include #include #include +#include +#include #include #include #include @@ -66,7 +68,9 @@ #include #include #include +#include #include +#include #include #include @@ -869,9 +873,152 @@ class DeprecatedKeyImporter : public RooFit::JSONIO::Importer { mutable bool _warned = false; }; +namespace CrystalBall { + +constexpr const char *tailKeys[] = {"alpha_L", "n_L", "alpha_R", "n_R"}; + +bool hasAny(const JSONNode &p, std::initializer_list keys) +{ + for (const char *key : keys) { + if (p.has_child(key)) { + return true; + } + } + return false; +} + +bool hasDoubleSidedKeys(const JSONNode &p) +{ + return hasAny(p, {"alpha_L", "n_L", "alpha_R", "n_R", "sigma_L", "sigma_R"}); +} + +// Create a RooCrystalBall with two tails. With `symmetricShorthand`, the tails may also be given as "alpha" and "n" +// for both sides, as in the "crystalball_dist" of HS3 versions before 0.3. +void importDoubleSided(RooJSONFactoryWSTool *tool, const JSONNode &p, std::string const &type, bool symmetricShorthand) +{ + const std::string name(RooJSONFactoryWSTool::name(p)); + const std::string context = type + " '" + name + "'"; + + const bool asymmetricTails = hasAny(p, {"alpha_L", "n_L", "alpha_R", "n_R"}); + const bool symmetricTails = hasAny(p, {"alpha", "n"}); + if (symmetricTails && !symmetricShorthand) { + RooJSONFactoryWSTool::error(context + " must not have \"alpha\" or \"n\", the tails are given by \"alpha_L\", " + + "\"n_L\", \"alpha_R\" and \"n_R\""); + } + if (symmetricTails && asymmetricTails) { + RooJSONFactoryWSTool::error(context + + " must not combine \"alpha\"/\"n\" with \"alpha_L\"/\"n_L\"/\"alpha_R\"/\"n_R\""); + } + if (asymmetricTails || !symmetricTails) { + for (const char *key : tailKeys) { + if (!p.has_child(key)) { + RooJSONFactoryWSTool::error(context + " is missing \"" + key + + "\": \"alpha_L\", \"n_L\", \"alpha_R\" and \"n_R\" are all required"); + } + } + } else if (!p.has_child("alpha") || !p.has_child("n")) { + RooJSONFactoryWSTool::error(context + " needs both \"alpha\" and \"n\""); + } + + const bool asymmetricWidths = hasAny(p, {"sigma_L", "sigma_R"}); + if (asymmetricWidths && p.has_child("sigma")) { + RooJSONFactoryWSTool::error(context + " must not combine \"sigma\" with \"sigma_L\"/\"sigma_R\""); + } + if (asymmetricWidths && !(p.has_child("sigma_L") && p.has_child("sigma_R"))) { + RooJSONFactoryWSTool::error(context + " needs both \"sigma_L\" and \"sigma_R\""); + } + if (!asymmetricWidths && !p.has_child("sigma")) { + RooJSONFactoryWSTool::error(context + " needs either \"sigma\" or both \"sigma_L\" and \"sigma_R\""); + } + + RooAbsReal *m = tool->requestArg(p, "m"); + RooAbsReal *m0 = tool->requestArg(p, "m0"); + RooAbsReal *sigmaL = tool->requestArg(p, asymmetricWidths ? "sigma_L" : "sigma"); + RooAbsReal *sigmaR = tool->requestArg(p, asymmetricWidths ? "sigma_R" : "sigma"); + RooAbsReal *alphaL = tool->requestArg(p, symmetricTails ? "alpha" : "alpha_L"); + RooAbsReal *nL = tool->requestArg(p, symmetricTails ? "n" : "n_L"); + RooAbsReal *alphaR = tool->requestArg(p, symmetricTails ? "alpha" : "alpha_R"); + RooAbsReal *nR = tool->requestArg(p, symmetricTails ? "n" : "n_R"); + tool->wsEmplace(name, *m, *m0, *sigmaL, *sigmaR, *alphaL, *nL, *alphaR, *nR); +} + +} // namespace CrystalBall + +// "crystalball_dist" is the single-sided Crystal Ball, which is left to the factory expression (RooCBShape). Before +// HS3 0.3, it was also the double-sided Crystal Ball, recognizable by the "_L"/"_R" fields. Such documents are still +// read, with a deprecation warning. +class CrystalBallImporter : public RooFit::JSONIO::Importer { +public: + bool importArg(RooJSONFactoryWSTool *tool, const JSONNode &p) const override + { + if (!CrystalBall::hasDoubleSidedKeys(p)) { + return false; + } + if (!_warned) { + RooJSONFactoryWSTool::warning("RooFitHS3: \"crystalball_dist\" with the fields \"alpha_L\", \"n_L\", " + "\"alpha_R\", \"n_R\", \"sigma_L\" or \"sigma_R\" is deprecated, please use " + "\"crystalball_doublesided_dist\" for the double-sided Crystal Ball."); + _warned = true; + } + CrystalBall::importDoubleSided(tool, p, "crystalball_dist", /*symmetricShorthand=*/true); + return true; + } + +private: + mutable bool _warned = false; +}; + +bool importCrystalBallDoubleSided(RooJSONFactoryWSTool *tool, const JSONNode &p) +{ + CrystalBall::importDoubleSided(tool, p, "crystalball_doublesided_dist", /*symmetricShorthand=*/false); + return true; +} + /////////////////////////////////////////////////////////////////////////////////////////////////////// // specialized exporter implementations /////////////////////////////////////////////////////////////////////////////////////////////////////// +// Write a reference to `arg`: the value if it is a literal constant, and the name otherwise. +void writeReference(JSONNode &node, RooAbsArg const &arg) +{ + auto const *constVar = dynamic_cast(&arg); + std::istringstream stream(arg.GetName()); + double value = 0.0; + if (constVar && (stream >> value) && stream.eof()) { + node << constVar->getVal(); + } else { + node << arg.GetName(); + } +} + +// A RooCrystalBall with one tail is the single-sided "crystalball_dist" (read back as RooCBShape, which follows the +// same convention for the sign of alpha). With two tails, it is a "crystalball_doublesided_dist", whose tails are +// always written as "alpha_L"/"n_L"/"alpha_R"/"n_R": there is no shorthand for symmetric tails. +bool exportCrystalBall(RooJSONFactoryWSTool *, const RooAbsArg *func, JSONNode &elem, std::string const &key) +{ + auto const *pdf = static_cast(func); + writeReference(elem["m"], pdf->x()); + writeReference(elem["m0"], pdf->x0()); + if (!pdf->alphaR() || !pdf->nR()) { + elem["type"] << "crystalball_dist"; + writeReference(elem["sigma"], pdf->sigmaL()); + writeReference(elem["alpha"], pdf->alphaL()); + writeReference(elem["n"], pdf->nL()); + return true; + } + elem["type"] << key; + if (&pdf->sigmaL() == &pdf->sigmaR()) { + writeReference(elem["sigma"], pdf->sigmaL()); + } else { + writeReference(elem["sigma_L"], pdf->sigmaL()); + writeReference(elem["sigma_R"], pdf->sigmaR()); + } + writeReference(elem["alpha_L"], pdf->alphaL()); + writeReference(elem["n_L"], pdf->nL()); + writeReference(elem["alpha_R"], *pdf->alphaR()); + writeReference(elem["n_R"], *pdf->nR()); + return true; +} + template bool exportAddPdf(RooJSONFactoryWSTool *, const RooAbsArg *func, JSONNode &elem, std::string const &key) { @@ -1459,6 +1606,8 @@ void registerExporter(TClass const *cl, std::string key, bool topPriority = true STATIC_EXECUTE([]() { registerImporter("density_function_dist"); registerImporter("rate_extended_dist"); + RooFit::JSONIO::registerImporter("crystalball_dist", std::make_unique()); + registerImporter("crystalball_doublesided_dist"); RooFit::JSONIO::registerImporter("ARGUS_dist", std::make_unique("ARGUS_dist", "argus_dist")); RooFit::JSONIO::registerImporter("binsampling", std::make_unique("binsampling", "binsampling_dist")); @@ -1500,6 +1649,7 @@ STATIC_EXECUTE([]() { registerExporter>(RooAddPdf::Class(), "mixture_dist", false); registerExporter>(RooAddModel::Class(), "mixture_resolution_model", false); registerExporter(RooBinSamplingPdf::Class(), "binsampling_dist", false); + registerExporter(RooCrystalBall::Class(), "crystalball_doublesided_dist", false); registerExporter(RooBinWidthFunction::Class(), "binvolume", false); registerExporter>(RooLegacyExpPoly::Class(), "legacy_exp_poly_dist", false); registerExporter(RooExponential::Class(), "exponential_dist", false); diff --git a/roofit/hs3/src/RooFitHS3_wsexportkeys.cxx b/roofit/hs3/src/RooFitHS3_wsexportkeys.cxx index fec22e4350490..6e42794def03e 100644 --- a/roofit/hs3/src/RooFitHS3_wsexportkeys.cxx +++ b/roofit/hs3/src/RooFitHS3_wsexportkeys.cxx @@ -49,19 +49,6 @@ auto RooFitHS3_wsexportkeys = R"({ "coefList": "coefficients" } }, - "RooCrystalBall": { - "type": "crystalball_doublesided_dist", - "proxies": { - "alphaL": "alpha_L", - "alphaR": "alpha_R", - "nL": "n_L", - "nR": "n_R", - "x": "m", - "x0": "m0", - "sigmaL": "sigma_L", - "sigmaR": "sigma_R" - } - }, "RooEffProd": { "type": "efficiency_product_pdf_dist", "proxies": { diff --git a/roofit/hs3/src/RooFitHS3_wsfactoryexpressions.cxx b/roofit/hs3/src/RooFitHS3_wsfactoryexpressions.cxx index a938956e564c2..82fbf349da660 100644 --- a/roofit/hs3/src/RooFitHS3_wsfactoryexpressions.cxx +++ b/roofit/hs3/src/RooFitHS3_wsfactoryexpressions.cxx @@ -139,12 +139,6 @@ auto RooFitHS3_wsfactoryexpressions = R"({ "arguments": [ "x" ] - }, - "crystalball_doublesided_dist": { - "class": "RooCrystalBall", - "arguments": [ - "m", "m0", "sigma_L", "sigma_R", "alpha_L", "n_L", "alpha_R", "n_R" - ] } })"; diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index 07b81635b6409..4b9cd46355328 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -9,9 +9,11 @@ #include #include #include +#include #include #include #include +#include #include #include #include @@ -3587,6 +3589,147 @@ TEST(RooFitHS3, AnalysisLegacySingularKeys) EXPECT_NE(mc2->GetNuisanceParameters()->find("sigma"), nullptr); } +// "crystalball_dist" is the single-sided Crystal Ball, "crystalball_doublesided_dist" the double-sided one. Before +// HS3 0.3, "crystalball_dist" with the "_L"/"_R" fields was the double-sided one, which is still read. +TEST(RooFitHS3, CrystalBallTypes) +{ + RooHelpers::LocalChangeMsgLevel changeMsgLvl(RooFit::WARNING); + + RooRealVar x{"x", "x", 0.0, 10.0}; + x.setBins(20); + RooRealVar mean{"mean", "mean", 5.0, 0.0, 10.0}; + RooRealVar sigma{"sigma", "sigma", 1.0, 0.1, 5.0}; + RooRealVar sigmaR{"sigmaR", "sigmaR", 1.5, 0.1, 5.0}; + RooRealVar alphaL{"alphaL", "alphaL", 1.2, 0.1, 5.0}; + RooRealVar nL{"nL", "nL", 3.0, 0.5, 10.0}; + RooRealVar alphaR{"alphaR", "alphaR", 2.0, 0.1, 5.0}; + RooRealVar nR{"nR", "nR", 5.0, 0.5, 10.0}; + RooRealVar alphaNeg{"alphaNeg", "alphaNeg", -1.2, -5.0, -0.1}; + + auto exportToString = [](RooAbsArg const &arg) { + RooWorkspace ws; + ws.import(arg, RooFit::Silence()); + return RooJSONFactoryWSTool{ws}.exportJSONtoString(); + }; + + // RooCBShape is the single-sided crystalball_dist. + RooCBShape cbShape{"cbShape", "", x, mean, sigma, alphaL, nL}; + EXPECT_EQ(validate(cbShape), 0); + EXPECT_NE(exportToString(cbShape).find(R"("type":"crystalball_dist")"), std::string::npos); + + // A single-sided RooCrystalBall is a crystalball_dist too, for both signs of alpha. It is read back as a + // RooCBShape, which is the same function. + for (RooRealVar *alpha : {&alphaL, &alphaNeg}) { + RooCrystalBall cbSingle{"cbSingle", "", x, mean, sigma, *alpha, nL, /*doubleSided=*/false}; + const std::string json = exportToString(cbSingle); + EXPECT_NE(json.find(R"("type":"crystalball_dist")"), std::string::npos) << json; + EXPECT_NE(json.find(std::string(R"("alpha":")") + alpha->GetName() + "\""), std::string::npos) << json; + EXPECT_EQ(json.find("alpha_L"), std::string::npos) << json; + + // The class changes on the way back, so validate() can't be used: the parameter points are written in a + // different order. Compare the values instead, and check that the JSON is stable from the second export on. + RooWorkspace ws2; + RooJSONFactoryWSTool{ws2}.importJSONfromString(json); + auto *cbShapeBack = dynamic_cast(ws2.pdf("cbSingle")); + ASSERT_NE(cbShapeBack, nullptr); + RooRealVar &x2 = *ws2.var("x"); + RooArgSet nset1{x}; + RooArgSet nset2{x2}; + for (int i = 0; i < x.numBins(); ++i) { + x.setBin(i); + x2.setBin(i); + EXPECT_NEAR(cbShapeBack->getVal(nset2), cbSingle.getVal(nset1), 1e-10) << alpha->GetName() << " bin " << i; + } + const std::string json2 = RooJSONFactoryWSTool{ws2}.exportJSONtoString(); + RooWorkspace ws3; + RooJSONFactoryWSTool{ws3}.importJSONfromString(json2); + EXPECT_EQ(RooJSONFactoryWSTool{ws3}.exportJSONtoString(), json2); + } + + // Double-sided RooCrystalBall shapes are crystalball_doublesided_dist, with the tails always written for both + // sides, and the width written as "sigma" if both sides share it. + RooCrystalBall cbSymmetric{"cbSymmetric", "", x, mean, sigma, alphaL, nL, /*doubleSided=*/true}; + RooCrystalBall cbAsymmetric{"cbAsymmetric", "", x, mean, sigma, sigmaR, alphaL, nL, alphaR, nR}; + EXPECT_EQ(validate(cbSymmetric), 0); + EXPECT_EQ(validate(cbAsymmetric), 0); + { + const std::string json = exportToString(cbSymmetric); + EXPECT_NE(json.find(R"("type":"crystalball_doublesided_dist")"), std::string::npos) << json; + EXPECT_NE(json.find(R"("sigma":"sigma")"), std::string::npos) << json; + EXPECT_NE(json.find(R"("alpha_R":"alphaL")"), std::string::npos) << json; + EXPECT_EQ(json.find(R"("alpha":)"), std::string::npos) << json; + } + { + const std::string json = exportToString(cbAsymmetric); + EXPECT_NE(json.find(R"("type":"crystalball_doublesided_dist")"), std::string::npos) << json; + EXPECT_NE(json.find(R"("sigma_R":"sigmaR")"), std::string::npos) << json; + EXPECT_NE(json.find(R"("alpha_R":"alphaR")"), std::string::npos) << json; + } + + auto document = [](std::string const &distribution) { + return R"({ + "metadata": {"hs3_version": "0.3"}, + "domains": [{"name": "default_domain", "type": "product_domain", "axes": [{"name": "m", "min": 0, "max": 10}]}], + "distributions": [)" + + distribution + R"(] + })"; + }; + auto importPdf = [&](std::string const &distribution, std::string *warnings = nullptr) { + auto ws = std::make_unique(); + const std::string messages = captureMessages(RooFit::WARNING, RooFit::ObjectHandling, [&] { + RooJSONFactoryWSTool{*ws}.importJSONfromString(document(distribution)); + }); + if (warnings) { + *warnings = messages; + } + return ws; + }; + + // Import of the two standard types. + auto single = importPdf(R"({"name": "cb", "type": "crystalball_dist", "m": "m", "m0": 5.0, "sigma": 1.0, + "alpha": -1.2, "n": 3.0})"); + EXPECT_NE(dynamic_cast(single->pdf("cb")), nullptr); + auto doubleSided = importPdf(R"({"name": "cb", "type": "crystalball_doublesided_dist", "m": "m", "m0": 5.0, + "sigma": 1.0, "alpha_L": 1.2, "n_L": 3.0, "alpha_R": 2.0, "n_R": 5.0})"); + EXPECT_NE(dynamic_cast(doubleSided->pdf("cb")), nullptr); + + // The double-sided form of crystalball_dist from before HS3 0.3 is still read, with a deprecation warning. This + // includes the symmetric tails given as "alpha"/"n" with asymmetric widths. + std::string warnings; + auto legacy = importPdf(R"({"name": "cb", "type": "crystalball_dist", "m": "m", "m0": 5.0, "sigma": 1.0, + "alpha_L": 1.2, "n_L": 3.0, "alpha_R": 2.0, "n_R": 5.0})", + &warnings); + auto *legacyCb = dynamic_cast(legacy->pdf("cb")); + ASSERT_NE(legacyCb, nullptr); + ASSERT_NE(legacyCb->alphaR(), nullptr); + EXPECT_DOUBLE_EQ(legacyCb->alphaR()->getVal(), 2.0); + EXPECT_NE(warnings.find("please use \"crystalball_doublesided_dist\""), std::string::npos) << warnings; + auto legacySymmetric = importPdf(R"({"name": "cb", "type": "crystalball_dist", "m": "m", "m0": 5.0, + "sigma_L": 1.0, "sigma_R": 1.5, "alpha": 1.2, "n": 3.0})"); + auto *legacySymmetricCb = dynamic_cast(legacySymmetric->pdf("cb")); + ASSERT_NE(legacySymmetricCb, nullptr); + ASSERT_NE(legacySymmetricCb->alphaR(), nullptr); + EXPECT_DOUBLE_EQ(legacySymmetricCb->alphaR()->getVal(), 1.2); + + // Invalid field combinations. + expectImportThrowsWithError(document(R"({"name": "cb", "type": "crystalball_doublesided_dist", "m": "m", + "m0": 5.0, "sigma": 1.0, "alpha": 1.2, "n": 3.0})"), + "must not have \"alpha\" or \"n\""); + expectImportThrowsWithError(document(R"({"name": "cb", "type": "crystalball_doublesided_dist", "m": "m", + "m0": 5.0, "sigma": 1.0, "alpha_L": 1.2, "n_L": 3.0, "alpha_R": 2.0})"), + "is missing \"n_R\""); + expectImportThrowsWithError(document(R"({"name": "cb", "type": "crystalball_doublesided_dist", "m": "m", + "m0": 5.0, "sigma": 1.0, "sigma_L": 1.0, "sigma_R": 1.0, "alpha_L": 1.2, "n_L": 3.0, "alpha_R": 2.0, + "n_R": 5.0})"), + "must not combine \"sigma\""); + expectImportThrowsWithError(document(R"({"name": "cb", "type": "crystalball_doublesided_dist", "m": "m", + "m0": 5.0, "sigma_L": 1.0, "alpha_L": 1.2, "n_L": 3.0, "alpha_R": 2.0, "n_R": 5.0})"), + "needs both \"sigma_L\" and \"sigma_R\""); + expectImportThrowsWithError(document(R"({"name": "cb", "type": "crystalball_dist", "m": "m", "m0": 5.0, + "sigma": 1.0, "alpha": 1.2, "n": 3.0, "alpha_R": 2.0, "n_R": 5.0})"), + "must not combine"); +} + // A RooRealVar that is constant in all parameter points must stay a RooRealVar, and a RooConstVar must stay a // RooConstVar. In the parameter points both are written the same way (a value and "const": true). The only // difference is that the RooRealVar is listed in a domain and the RooConstVar is not: a parameter without domain is From e8bbd7bacf4d4c98570af1272dc76fd0571a7c4a Mon Sep 17 00:00:00 2001 From: Carsten Burgard Date: Thu, 5 Nov 2026 11:52:49 +0100 Subject: [PATCH 5/9] [RF][HS3][after HS3 json-fixes] HistFactory: samples without errors don't contribute to the staterror HS3 PRs #135 and #124 make the "errors" of a HistFactory sample optional: a sample without "errors" contributes no statistical uncertainty to a staterror modifier. RooFit used the content as squared error instead, i.e. assumed Poisson uncertainties. That is kept for documents before HS3 0.3. In HS3 >= 0.3 documents, this only matters if the staterror is constrained through the legacy "constraint" key, since otherwise the gammas are either unconstrained or use the named constraint distributions. Assisted-by: claude-code:claude-opus-5-5 --- roofit/hs3/src/JSONFactories_HistFactory.cxx | 6 ++- roofit/hs3/test/testRooFitHS3.cxx | 47 ++++++++++++++++++++ 2 files changed, 52 insertions(+), 1 deletion(-) diff --git a/roofit/hs3/src/JSONFactories_HistFactory.cxx b/roofit/hs3/src/JSONFactories_HistFactory.cxx index c5f34761b65f7..fef6882906355 100644 --- a/roofit/hs3/src/JSONFactories_HistFactory.cxx +++ b/roofit/hs3/src/JSONFactories_HistFactory.cxx @@ -916,9 +916,13 @@ class HistFactoryImporter : public RooFit::JSONIO::Importer { sumW.resize(nbins); sumW2.resize(nbins); } + // A sample without "errors" contributes no statistical uncertainty in HS3. Before HS3 0.3, RooFit + // assumed Poisson uncertainties for it. + const bool useWeightSquared = + comp["data"].has_child("errors") || isHS3VersionBefore(tool->hs3VersionInput(), 0, 3); for (size_t i = 0; i < nbins; ++i) { sumW[i] += dh->weight(i); - sumW2[i] += dh->weightSquared(i); + sumW2[i] += useWeightSquared ? dh->weightSquared(i) : 0.; } if (gammaParnames.empty()) { if (auto staterrorParams = staterror->find("parameters")) { diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index 4b9cd46355328..f6758650baf69 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -3729,6 +3729,53 @@ TEST(RooFitHS3, CrystalBallTypes) "sigma": 1.0, "alpha": 1.2, "n": 3.0, "alpha_R": 2.0, "n_R": 5.0})"), "must not combine"); } +// A HistFactory sample without "errors" contributes no statistical uncertainty to the staterror in HS3 >= 0.3. +// Before, RooFit assumed Poisson uncertainties for it. +TEST(RooFitHS3, HistFactorySampleWithoutErrors) +{ + RooHelpers::LocalChangeMsgLevel changeMsgLvl(RooFit::WARNING); + + auto makeJSON = [](std::string const &version) { + const std::string staterror = R"({"type": "staterror", "parameters": ["g0", "g1"], "constraint": "Gauss"})"; + return R"({ + "metadata": {"hs3_version": ")" + + version + R"("}, + "distributions": [ + { + "name": "model_channel0", + "type": "histfactory_dist", + "axes": [{"name": "x", "min": 0.0, "max": 2.0, "nbins": 2}], + "samples": [ + {"name": "a", "data": {"contents": [10.0, 20.0], "errors": [4.0, 6.0]}, "modifiers": [)" + + staterror + R"(]}, + {"name": "b", "data": {"contents": [10.0, 20.0]}, "modifiers": [)" + + staterror + R"(]} + ] + } + ] + })"; + }; + auto relativeErrors = [](RooWorkspace &ws) { + std::vector result; + for (const char *name : {"g0_sigma", "g1_sigma"}) { + auto *sigma = dynamic_cast(ws.arg(name)); + result.push_back(sigma ? sigma->getVal() : -1.0); + } + return result; + }; + + RooWorkspace ws; + ASSERT_TRUE(RooJSONFactoryWSTool{ws}.importJSONfromString(makeJSON("0.3"))); + const auto errors = relativeErrors(ws); + EXPECT_DOUBLE_EQ(errors[0], 4.0 / 20.0); + EXPECT_DOUBLE_EQ(errors[1], 6.0 / 40.0); + + RooWorkspace wsLegacy; + ASSERT_TRUE(RooJSONFactoryWSTool{wsLegacy}.importJSONfromString(makeJSON("0.2"))); + const auto legacyErrors = relativeErrors(wsLegacy); + EXPECT_DOUBLE_EQ(legacyErrors[0], std::sqrt(16.0 + 10.0) / 20.0); + EXPECT_DOUBLE_EQ(legacyErrors[1], std::sqrt(36.0 + 20.0) / 40.0); +} // A RooRealVar that is constant in all parameter points must stay a RooRealVar, and a RooConstVar must stay a // RooConstVar. In the parameter points both are written the same way (a value and "const": true). The only From 5140e3b4af081be714db8ad47eedfeb68f71d199 Mon Sep 17 00:00:00 2001 From: Carsten Burgard Date: Mon, 26 Oct 2026 00:46:24 +0100 Subject: [PATCH 6/9] [RF][HS3][after HS3 #124/#129] HistFactory: reference constraints by name HS3 PR #124 replaces the `parameter` and `constraint` (a constraint type like `Gauss`) of HistFactory modifiers with the lists `parameters` and `constraints`. Each entry of `constraints` names a distribution in the top-level `distributions`, and `null` leaves a parameter unconstrained. `shapesys` loses its `data.vals`, since the uncertainties are encoded in the constraint distributions. HS3 PR #129 states that the constraint terms belong to the prediction, which is how RooFit already builds the model. Import: * `parameters` and `constraints` are read for all modifiers, and take precedence over the legacy `parameter`, `constraint`, `constraint_name` and `constraint_type`, which are still read for older files. * For `staterror`, the named constraint distributions are used instead of creating Poisson terms. Without any constraint information, the gammas are unconstrained in documents with `hs3_version` >= 0.3; older documents still get the Poisson constraints that RooFit used to create implicitly. * The `nom_*` global observable is now only created when RooFit builds a constraint itself (legacy `Gauss`). Export: * All modifiers write `parameters` and, if any parameter is constrained, `constraints` with the names of the constraint distributions, which are exported alongside. `parameter`, `constraint` and `shapesys` `data.vals` are no longer written. * The RooPoisson (or RooGaussian) constraint terms of the staterror gammas are exported as named `poisson_dist` (or `gaussian_dist`) and referenced from the staterror modifiers. #124 recommends a `barlow_beeston_lite_poisson_constraint_dist`, which RooFit doesn't support yet; any distribution type is allowed. * The channel-level `staterror.constraint_type` and the `constraint` type on staterror modifiers (written since the previous HistFactory commits) are no longer written, but still read. Tests that checked the exported `constraint` key are updated to the new form, and a new test covers import and export by name. This commit depends on HS3 PRs #124 and #129 and should only be merged together with or after them. Assisted-by: claude-code:claude-opus-5-5 --- roofit/hs3/src/JSONFactories_HistFactory.cxx | 194 ++++++++++++++----- roofit/hs3/test/testRooFitHS3.cxx | 145 +++++++++++--- 2 files changed, 260 insertions(+), 79 deletions(-) diff --git a/roofit/hs3/src/JSONFactories_HistFactory.cxx b/roofit/hs3/src/JSONFactories_HistFactory.cxx index fef6882906355..028bc95b5dfb7 100644 --- a/roofit/hs3/src/JSONFactories_HistFactory.cxx +++ b/roofit/hs3/src/JSONFactories_HistFactory.cxx @@ -444,6 +444,23 @@ void exportGammaParameters(JSONNode &mod, std::vector const ¶m mod["parameters"].fill_seq(paramNames); } +/// Export the names of the constraint distributions of a modifier, with null for unconstrained parameters. Nothing is +/// written if all parameters are unconstrained. +void exportConstraintNames(JSONNode &mod, std::vector const &constraints) +{ + if (std::none_of(constraints.begin(), constraints.end(), [](auto *pdf) { return pdf != nullptr; })) { + return; + } + auto &names = mod["constraints"].set_seq(); + for (auto *constraint : constraints) { + if (constraint) { + names.append_child() << constraint->GetName(); + } else { + names.append_child().set_null(); + } + } +} + RooRealVar &createNominal(RooWorkspace &ws, std::string const &parname, double val, double min, double max) { RooRealVar &nom = getOrCreate(ws, "nom_" + parname, val, min, max); @@ -486,8 +503,8 @@ RooAbsPdf &createLegacyConstraint(RooJSONFactoryWSTool &tool, const JSONNode &mo { if (constraintType == "Gauss") { param.setError(1.0); - return getOrCreate(*tool.workspace(), constraintName(param.GetName()), param, - *tool.workspace()->var(std::string("nom_") + param.GetName()), 1.); + RooRealVar &nominal = createNominal(*tool.workspace(), param.GetName(), 0.0, -10, 10); + return getOrCreate(*tool.workspace(), constraintName(param.GetName()), param, nominal, 1.); } RooJSONFactoryWSTool::error("legacy constraint value '" + constraintType + "' for modifier '" + @@ -553,9 +570,72 @@ const JSONNode *findStaterror(const JSONNode &comp) return nullptr; } +// The entries of the HS3 "constraints" of a modifier: the names of the constraint distributions, or std::nullopt for +// unconstrained parameters (null entries). +std::vector> constraintNames(const JSONNode &mod) +{ + std::vector> out; + auto *constraints = mod.find("constraints"); + if (!constraints) { + return out; + } + if (!constraints->is_seq()) { + RooJSONFactoryWSTool::error("\"constraints\" of modifier '" + RooJSONFactoryWSTool::name(mod) + + "' must be a list"); + } + for (const auto &entry : constraints->children()) { + out.push_back(entry.is_null() ? std::nullopt : std::make_optional(entry.val())); + } + return out; +} + +// The parameter of a modifier with a single parameter, given in the HS3 "parameters" or in the legacy "parameter". +std::optional modifierParameter(const JSONNode &mod) +{ + if (auto *parameters = mod.find("parameters")) { + if (!parameters->is_seq() || parameters->num_children() != 1) { + RooJSONFactoryWSTool::error("\"parameters\" of the " + mod["type"].val() + + " modifier must be a list with one entry"); + } + return parameters->child(0).val(); + } + if (auto *parameter = mod.find("parameter")) { + return parameter->val(); + } + return std::nullopt; +} + +// The constraint distribution named in the HS3 "constraints" of a modifier with a single parameter. Returns an empty +// string if the parameter is explicitly unconstrained, and std::nullopt if the modifier has no "constraints". +std::optional singleConstraintName(const JSONNode &mod) +{ + if (!mod.has_child("constraints")) { + return std::nullopt; + } + const auto names = constraintNames(mod); + if (names.size() != 1) { + RooJSONFactoryWSTool::error("\"constraints\" of the " + mod["type"].val() + + " modifier must have one entry per parameter"); + } + return names.front().value_or(""); +} + RooAbsPdf & getOrCreateConstraint(RooJSONFactoryWSTool &tool, const JSONNode &mod, RooRealVar ¶m, const std::string &sample) { + // HS3 references the constraint distributions by name. + if (auto name = singleConstraintName(mod); name && !name->empty()) { + auto *constraint = findNamedConstraint(tool, *name, sample); + if (!constraint) { + RooJSONFactoryWSTool::error("unable to find the constraint distribution '" + *name + "' of the modifier of '" + + param.GetName() + "'"); + } + if (auto gauss = dynamic_cast(constraint)) { + param.setError(gauss->getSigma().getVal()); + } + return *constraint; + } + JSONNode const *constrName = mod.find("constraint_name"); if (constrName) { auto constraint_name = constrName->val(); @@ -600,6 +680,9 @@ getOrCreateConstraint(RooJSONFactoryWSTool &tool, const JSONNode &mod, RooRealVa } bool hasConstraint(const JSONNode &mod) { + if (auto name = singleConstraintName(mod)) { + return !name->empty(); + } return mod.has_child("constraint") || mod.has_child("constraint_name") || mod.has_child("constraint_type"); } @@ -618,7 +701,6 @@ void addSysConstraint(RooJSONFactoryWSTool &tool, const JSONNode &mod, RooRealVa } return; } - createNominal(*tool.workspace(), param.GetName(), 0.0, -10, 10); constraints.add(getOrCreateConstraint(tool, mod, param, sample)); } @@ -711,17 +793,17 @@ bool importHistSample(RooJSONFactoryWSTool &tool, RooDataHist &dh, RooArgSet con int idx = 0; for (const auto &mod : p["modifiers"].children()) { std::string const &modtype = mod["type"].val(); + const std::optional parameter = + modtype == "normfactor" || modtype == "normsys" || modtype == "histosys" ? modifierParameter(mod) + : std::nullopt; std::string const &sysname = - mod.has_child("name") - ? mod["name"].val() - : (mod.has_child("parameter") ? mod["parameter"].val() : "syst_" + std::to_string(idx)); + mod.has_child("name") ? mod["name"].val() : parameter.value_or("syst_" + std::to_string(idx)); ++idx; if (modtype == "staterror") { // this is dealt with at a different place, ignore it for now } else if (modtype == "normfactor") { - // The "name" is only a label of the modifier, the parameter is given by "parameter" if present. - auto *parameter = mod.find("parameter"); - RooRealVar &constrParam = getOrCreate(ws, parameter ? parameter->val() : sysname, 1., -3, 5); + // The "name" is only a label of the modifier, the parameter is given by "parameters" if present. + RooRealVar &constrParam = getOrCreate(ws, parameter.value_or(sysname), 1., -3, 5); constrParam.setError(0.0); normElems.add(constrParam); if (hasConstraint(mod)) { @@ -729,8 +811,7 @@ bool importHistSample(RooJSONFactoryWSTool &tool, RooDataHist &dh, RooArgSet con constraints.add(getOrCreateConstraint(tool, mod, constrParam, sampleName)); } } else if (modtype == "normsys") { - auto *parameter = mod.find("parameter"); - std::string parname(parameter ? parameter->val() : "alpha_" + sysname); + std::string parname(parameter.value_or("alpha_" + sysname)); auto &par = getOrCreate(ws, parname, 0., -5, 5); overall_nps.add(par); auto &data = mod["data"]; @@ -756,8 +837,7 @@ bool importHistSample(RooJSONFactoryWSTool &tool, RooDataHist &dh, RooArgSet con addSysConstraint(tool, mod, par, sysname, sampleName, warnedUnconstrained, constraints); } else if (modtype == "histosys") { - auto *parameter = mod.find("parameter"); - std::string parname(parameter ? parameter->val() : "alpha_" + sysname); + std::string parname(parameter.value_or("alpha_" + sysname)); auto &par = getOrCreate(ws, parname, 0., -5, 5); histNps.add(par); auto &data = mod["data"]; @@ -901,6 +981,7 @@ class HistFactoryImporter : public RooFit::JSONIO::Importer { std::vector sumW2; std::vector gammaParnames; std::string modifierStatErrType; + std::optional>> statErrConstraintNames; RooArgSet observables = RooJSONFactoryWSTool::readAxes(p); std::string fprefix = name; @@ -931,7 +1012,12 @@ class HistFactoryImporter : public RooFit::JSONIO::Importer { } } } - // The constraint type given on the staterror modifier takes precedence over the channel-level setting. + // HS3 names the constraint distributions of the gammas in "constraints". + if (!statErrConstraintNames && staterror->has_child("constraints")) { + statErrConstraintNames = constraintNames(*staterror); + } + // The legacy constraint type given on the staterror modifier takes precedence over the channel-level + // setting. if (modifierStatErrType.empty()) { // ROOT 6.38 and 6.40.00 wrote "constraint_type". auto constraint = staterror->find("constraint"); @@ -954,6 +1040,11 @@ class HistFactoryImporter : public RooFit::JSONIO::Importer { if (!modifierStatErrType.empty()) { statErrType = modifierStatErrType; } + // Without any constraint information, the gammas are unconstrained in HS3. Older documents relied on RooFit + // creating Poisson constraints. + const bool legacyStatErrConstraints = statErrConstraintNames || !modifierStatErrType.empty() || + p.has_child(::Literals::staterror) || + isHS3VersionBefore(tool->hs3VersionInput(), 0, 3); RooAbsArg *mcStatObject = nullptr; RooArgSet constraints; @@ -972,9 +1063,28 @@ class HistFactoryImporter : public RooFit::JSONIO::Importer { errs[i] = std::max(errs[i], 0.); } - mcStatObject = - &createPHF("mc_stat_" + channelName, "stat_" + channelName, gammaParnames, errs, *tool, constraints, - observables, statErrType, defaultGammaMin, defaultStatErrorGammaMax, statErrThresh); + const bool createConstraints = !statErrConstraintNames && legacyStatErrConstraints; + mcStatObject = &createPHF("mc_stat_" + channelName, "stat_" + channelName, gammaParnames, errs, *tool, + constraints, observables, statErrType, defaultGammaMin, defaultStatErrorGammaMax, + statErrThresh, createConstraints); + if (statErrConstraintNames) { + if (statErrConstraintNames->size() != errs.size()) { + RooJSONFactoryWSTool::error("the staterror modifier of channel '" + name + "' has " + + std::to_string(statErrConstraintNames->size()) + " constraints, but " + + std::to_string(errs.size()) + " parameters"); + } + for (auto const &constraintName : *statErrConstraintNames) { + if (!constraintName) { + continue; + } + auto *constraint = findNamedConstraint(*tool, *constraintName, name); + if (!constraint) { + RooJSONFactoryWSTool::error("unable to find the constraint distribution '" + *constraintName + + "' of the staterror modifier of channel '" + name + "'"); + } + constraints.add(*constraint); + } + } } LegacyInterpolationDefault legacyInterpolationDefault; @@ -1327,6 +1437,7 @@ struct Sample { std::vector otherElements; bool useBarlowBeestonLight = false; std::vector staterrorParameters; + std::vector staterrorConstraints; Sample(const std::string &n) : name{n} {} }; @@ -1352,7 +1463,6 @@ struct Channel { std::map tot_yield; std::map tot_yield2; std::map rel_errors; - std::string statErrorConstraintType; RooArgSet const *varSet = nullptr; long unsigned int nBins = 0; }; @@ -1516,11 +1626,11 @@ Channel readChannel(RooJSONFactoryWSTool *tool, const std::string &pdfname, cons sample.staterrorParameters.push_back(static_cast(g)); ++idx; RooAbsPdf *constraint = findConstraint(g); + sample.staterrorConstraints.push_back(constraint); channel.tot_yield[idx] += sample.hist[idx - 1]; channel.tot_yield2[idx] += (sample.hist[idx - 1] * sample.hist[idx - 1]); if (constraint) { channel.rel_errors[idx] = constraintRelError(*constraint, *g); - channel.statErrorConstraintType = dynamic_cast(constraint) ? "Poisson" : "Gauss"; } } sample.useBarlowBeestonLight = true; @@ -1757,12 +1867,10 @@ std::optional defaultInterpolation(const Channel &channel) bool exportChannel(RooJSONFactoryWSTool *tool, const Channel &channel, JSONNode &elem) { - // 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"] << sys.constraint->GetName(); - } + // Write the parameter and the name of the constraint distribution of a modifier with a single parameter. + auto writeParameter = [](JSONNode &mod, RooAbsReal const ¶m, RooAbsPdf const *constraint) { + mod["parameters"].set_seq().append_child() << param.GetName(); + exportConstraintNames(mod, {constraint}); }; auto addModifier = [](JSONNode &modifiers, std::string const &name, const char *type) -> JSONNode & { auto &mod = modifiers.append_child(); @@ -1773,10 +1881,6 @@ bool exportChannel(RooJSONFactoryWSTool *tool, const Channel &channel, JSONNode }; elem["type"] << "histfactory_dist"; - // The constraint type of the staterror is also written on the channel level, where older RooFit versions read it. - if (!channel.statErrorConstraintType.empty()) { - elem[::Literals::staterror].set_map()["constraint_type"] << channel.statErrorConstraintType; - } const auto channelDefaultInterpolation = defaultInterpolation(channel); if (channelDefaultInterpolation) { writeInterpolation(elem["default_interpolation"], *channelDefaultInterpolation); @@ -1794,21 +1898,19 @@ bool exportChannel(RooJSONFactoryWSTool *tool, const Channel &channel, JSONNode auto &mod = modifiers.append_child(); mod.set_map(); mod["name"] << nf.name; - mod["parameter"] << nf.param->GetName(); + writeParameter(mod, *nf.param, nf.constraint); mod["type"] << "normfactor"; if (nf.constraint) { - mod["constraint"] << nf.constraint->GetName(); tool->queueExport(*nf.constraint); } } for (const auto &sys : sample.normsys) { auto &mod = addModifier(modifiers, sys.name, "normsys"); - mod["parameter"] << sys.param->GetName(); + writeParameter(mod, *sys.param, sys.constraint); if (!channelDefaultInterpolation || sys.interpolation != *channelDefaultInterpolation) { writeInterpolation(mod["interpolation"], sys.interpolation); } - writeConstraint(mod, sys); auto &data = mod["data"].set_map(); data["lo"] << sys.low; data["hi"] << sys.high; @@ -1816,11 +1918,10 @@ bool exportChannel(RooJSONFactoryWSTool *tool, const Channel &channel, JSONNode for (const auto &sys : sample.histosys) { auto &mod = addModifier(modifiers, sys.name, "histosys"); - mod["parameter"] << sys.param->GetName(); + writeParameter(mod, *sys.param, sys.constraint); if (!channelDefaultInterpolation || sys.interpolation != *channelDefaultInterpolation) { writeInterpolation(mod["interpolation"], sys.interpolation); } - writeConstraint(mod, sys); auto &data = mod["data"].set_map(); if (channel.nBins != sys.low.size() || channel.nBins != sys.high.size()) { RooJSONFactoryWSTool::error("inconsistent binning: " + std::to_string(channel.nBins) + @@ -1834,18 +1935,8 @@ bool exportChannel(RooJSONFactoryWSTool *tool, const Channel &channel, JSONNode for (const auto &sys : sample.shapesys) { 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(); - for (auto *constraint : sys.constraintPdfs) { - if (constraint) { - constraintNames.append_child() << constraint->GetName(); - } else { - constraintNames.append_child().set_null(); - } - } - } - mod["data"].set_map()["vals"].fill_seq(sys.constraints); + // The relative uncertainties are encoded in the constraint distributions. + exportConstraintNames(mod, sys.constraintPdfs); } for (const auto &other : sample.otherElements) { @@ -1858,9 +1949,7 @@ bool exportChannel(RooJSONFactoryWSTool *tool, const Channel &channel, JSONNode if (sample.useBarlowBeestonLight) { auto &mod = addModifier(modifiers, ::Literals::staterror, ::Literals::staterror); exportGammaParameters(mod, sample.staterrorParameters); - if (!channel.statErrorConstraintType.empty()) { - mod["constraint"] << channel.statErrorConstraintType; - } + exportConstraintNames(mod, sample.staterrorConstraints); } if (!observablesWritten) { @@ -2020,6 +2109,11 @@ bool tryExportHistFactory(RooJSONFactoryWSTool *tool, const std::string &pdfname } } } + for (auto *constraint : sample.staterrorConstraints) { + if (constraint) { + tool->queueExport(*constraint); + } + } } // Export all the custom modifiers diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index f6758650baf69..17ae9d12769a0 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -2638,7 +2638,8 @@ TEST(RooFitHS3, HistFactoryConstraintKeyMigration) ASSERT_TRUE(RooJSONFactoryWSTool{ws}.importJSONfromString(jsonStr)); const std::string exported = RooJSONFactoryWSTool{ws}.exportJSONtoString(); - EXPECT_NE(exported.find("\"constraint\":\"muConstraint\""), std::string::npos) << exported; + EXPECT_NE(exported.find(R"("constraints":["muConstraint"])"), std::string::npos) << exported; + EXPECT_EQ(exported.find("\"constraint\":"), std::string::npos) << exported; EXPECT_EQ(exported.find("constraint_name"), std::string::npos) << exported; EXPECT_EQ(exported.find("constraint_type"), std::string::npos) << exported; } @@ -2703,8 +2704,8 @@ TEST(RooFitHS3, HistFactoryLegacyConstraintTypeInConstraintKey) 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_NE(exported.find(R"("constraints":["alpha_lumiConstraint"])"), std::string::npos) << exported; + EXPECT_EQ(exported.find("\"constraint\":"), std::string::npos) << exported; EXPECT_EQ(exported.find("constraint_name"), std::string::npos) << exported; EXPECT_EQ(exported.find("constraint_type"), std::string::npos) << exported; } @@ -3346,9 +3347,9 @@ TEST(RooFitHS3, HistFactoryModifierWithoutConstraintIsUnconstrained) auto tree = RooFit::Detail::JSONTree::create(json1); const auto *channel = RooJSONFactoryWSTool::findNamedChild(tree->rootnode()["distributions"], "model_channel0"); ASSERT_NE(channel, nullptr) << json1; - EXPECT_FALSE(findModifier(*channel, "norm")->has_child("constraint")) << json1; - EXPECT_FALSE(findModifier(*channel, "shape")->has_child("constraint")) << json1; - EXPECT_TRUE(findModifier(*channel, "constrained")->has_child("constraint")) << json1; + EXPECT_FALSE(findModifier(*channel, "norm")->has_child("constraints")) << json1; + EXPECT_FALSE(findModifier(*channel, "shape")->has_child("constraints")) << json1; + EXPECT_TRUE(findModifier(*channel, "constrained")->has_child("constraints")) << json1; RooWorkspace ws2; ASSERT_TRUE(RooJSONFactoryWSTool{ws2}.importJSONfromString(json1)); @@ -3356,8 +3357,9 @@ TEST(RooFitHS3, HistFactoryModifierWithoutConstraintIsUnconstrained) EXPECT_EQ(RooJSONFactoryWSTool{ws2}.exportJSONtoString(), json1); } -// The constraint type of the staterror is read from the modifier, with the -// channel-level setting as fallback, and written to both places. +// The legacy constraint type of the staterror is read from the modifier, with +// the channel-level setting as fallback. On export, the constraint +// distributions are referenced by name. TEST(RooFitHS3, HistFactoryStatErrorConstraint) { RooHelpers::LocalChangeMsgLevel changeMsgLvl(RooFit::WARNING); @@ -3402,14 +3404,22 @@ TEST(RooFitHS3, HistFactoryStatErrorConstraint) ASSERT_TRUE(RooJSONFactoryWSTool{ws}.importJSONfromString(makeJSON("Gauss", "Poisson"))); EXPECT_TRUE(hasGaussianStatConstraints(ws)); + // The constraints are exported as distributions that the staterror modifier references by name. const std::string json = RooJSONFactoryWSTool{ws}.exportJSONtoString(); auto tree = RooFit::Detail::JSONTree::create(json); const auto *channel = RooJSONFactoryWSTool::findNamedChild(tree->rootnode()["distributions"], "model_channel0"); ASSERT_NE(channel, nullptr) << json; - EXPECT_EQ((*channel)["staterror"]["constraint_type"].val(), "Gauss") << json; + EXPECT_FALSE(channel->has_child("staterror")) << json; const auto *modifier = findModifier(*channel, "staterror"); ASSERT_NE(modifier, nullptr) << json; - EXPECT_EQ((*modifier)["constraint"].val(), "Gauss") << json; + EXPECT_FALSE(modifier->has_child("constraint")) << json; + ASSERT_EQ((*modifier)["constraints"].num_children(), 2u) << json; + for (auto const &constraintName : (*modifier)["constraints"].children()) { + const auto *constraint = + RooJSONFactoryWSTool::findNamedChild(tree->rootnode()["distributions"], constraintName.val()); + ASSERT_NE(constraint, nullptr) << json; + EXPECT_EQ((*constraint)["type"].val(), "gaussian_dist") << json; + } RooWorkspace ws2; ASSERT_TRUE(RooJSONFactoryWSTool{ws2}.importJSONfromString(json)); @@ -3529,24 +3539,6 @@ TEST(RooFitHS3, UnbinnedAxisRangesOnImport) } } -// Errors of zero in binned data stay zero, also before the first positive error. -TEST(RooFitHS3, BinnedDataZeroErrors) -{ - RooHelpers::LocalChangeMsgLevel changeMsgLvl(RooFit::WARNING); - - RooWorkspace ws; - ASSERT_TRUE(RooJSONFactoryWSTool{ws}.importJSONfromString(R"({ - "metadata": {"hs3_version": "0.3"}, - "data": [{"name": "d", "type": "binned", "axes": [{"name": "x", "min": 0.0, "max": 3.0, "nbins": 3}], - "contents": [10.0, 20.0, 30.0], "errors": [0.0, 2.0, 0.0]}] - })")); - auto *data = dynamic_cast(ws.data("d")); - ASSERT_NE(data, nullptr); - EXPECT_DOUBLE_EQ(data->weightSquared(0), 0.0); - EXPECT_DOUBLE_EQ(data->weightSquared(1), 4.0); - EXPECT_DOUBLE_EQ(data->weightSquared(2), 0.0); -} - // The analysis keys "parameters_of_interest" and "domains" are written. The // singular keys of HS3 v0.2.9 are still read, with a deprecation warning. TEST(RooFitHS3, AnalysisLegacySingularKeys) @@ -3729,6 +3721,101 @@ TEST(RooFitHS3, CrystalBallTypes) "sigma": 1.0, "alpha": 1.2, "n": 3.0, "alpha_R": 2.0, "n_R": 5.0})"), "must not combine"); } + +// HistFactory modifiers reference their parameters with "parameters" and their constraint distributions by name with +// "constraints", where null leaves a parameter unconstrained. +TEST(RooFitHS3, HistFactoryConstraintsByName) +{ + RooHelpers::LocalChangeMsgLevel changeMsgLvl(RooFit::WARNING); + + const std::string json = R"({ + "metadata": {"hs3_version": "0.3"}, + "distributions": [ + { + "name": "model_channel0", + "type": "histfactory_dist", + "axes": [{"name": "x", "min": 0.0, "max": 2.0, "nbins": 2}], + "default_interpolation": {"type": "mult", "in": "poly6", "out": "exp"}, + "samples": [ + { + "name": "sig", + "data": {"contents": [10.0, 20.0], "errors": [1.0, 2.0]}, + "modifiers": [ + {"type": "normfactor", "parameters": ["mu"]}, + {"type": "normsys", "parameters": ["alpha_a"], "constraints": ["alpha_a_constraint"], + "data": {"lo": 0.9, "hi": 1.1}}, + {"type": "normsys", "parameters": ["alpha_b"], "constraints": [null], + "data": {"lo": 0.8, "hi": 1.2}}, + {"type": "staterror", "parameters": ["gamma_0", "gamma_1"], + "constraints": ["gamma_0_constraint", null]} + ] + } + ] + }, + {"name": "alpha_a_constraint", "type": "gaussian_dist", "x": "alpha_a", "mean": 0.0, "sigma": 1.0}, + {"name": "gamma_0_constraint", "type": "gaussian_dist", "x": "gamma_0", "mean": 1.0, "sigma": 0.1} + ], + "domains": [{"name": "default_domain", "type": "product_domain", "axes": [ + {"name": "alpha_a", "min": -5, "max": 5}, {"name": "alpha_b", "min": -5, "max": 5}, + {"name": "gamma_0", "min": 0, "max": 5}, {"name": "gamma_1", "min": 0, "max": 5}]}] + })"; + + RooWorkspace ws; + ASSERT_TRUE(RooJSONFactoryWSTool{ws}.importJSONfromString(json)); + auto *model = dynamic_cast(ws.pdf("model_channel0")); + ASSERT_NE(model, nullptr); + RooArgSet constraintNames; + for (RooAbsArg *pdf : model->pdfList()) { + if (!dynamic_cast(pdf)) { + constraintNames.add(*pdf); + } + } + // Exactly the two named constraints, and no constraints created by RooFit. + EXPECT_EQ(constraintNames.size(), 2u); + EXPECT_NE(constraintNames.find("alpha_a_constraint"), nullptr); + EXPECT_NE(constraintNames.find("gamma_0_constraint"), nullptr); + EXPECT_NE(ws.var("mu"), nullptr); + + // The export references the same constraints by name, and the result is stable under a round trip. + const std::string exported = RooJSONFactoryWSTool{ws}.exportJSONtoString(); + EXPECT_NE(exported.find(R"("constraints":["alpha_a_constraint"])"), std::string::npos) << exported; + EXPECT_NE(exported.find(R"("constraints":["gamma_0_constraint",null])"), std::string::npos) << exported; + EXPECT_EQ(exported.find(R"("parameter":)"), std::string::npos) << exported; + RooWorkspace ws2; + ASSERT_TRUE(RooJSONFactoryWSTool{ws2}.importJSONfromString(exported)); + EXPECT_EQ(RooJSONFactoryWSTool{ws2}.exportJSONtoString(), exported); + + // In HS3 >= 0.3, a staterror without any constraint information is unconstrained. + std::string unconstrained = json; + const std::string statConstraints = R"(, + "constraints": ["gamma_0_constraint", null])"; + ASSERT_NE(unconstrained.find(statConstraints), std::string::npos); + unconstrained.replace(unconstrained.find(statConstraints), statConstraints.size(), ""); + RooWorkspace ws3; + ASSERT_TRUE(RooJSONFactoryWSTool{ws3}.importJSONfromString(unconstrained)); + auto *model3 = dynamic_cast(ws3.pdf("model_channel0")); + ASSERT_NE(model3, nullptr); + EXPECT_EQ(model3->pdfList().size(), 2u); // the sum and alpha_a_constraint +} + +// Errors of zero in binned data stay zero, also before the first positive error. +TEST(RooFitHS3, BinnedDataZeroErrors) +{ + RooHelpers::LocalChangeMsgLevel changeMsgLvl(RooFit::WARNING); + + RooWorkspace ws; + ASSERT_TRUE(RooJSONFactoryWSTool{ws}.importJSONfromString(R"({ + "metadata": {"hs3_version": "0.3"}, + "data": [{"name": "d", "type": "binned", "axes": [{"name": "x", "min": 0.0, "max": 3.0, "nbins": 3}], + "contents": [10.0, 20.0, 30.0], "errors": [0.0, 2.0, 0.0]}] + })")); + auto *data = dynamic_cast(ws.data("d")); + ASSERT_NE(data, nullptr); + EXPECT_DOUBLE_EQ(data->weightSquared(0), 0.0); + EXPECT_DOUBLE_EQ(data->weightSquared(1), 4.0); + EXPECT_DOUBLE_EQ(data->weightSquared(2), 0.0); +} + // A HistFactory sample without "errors" contributes no statistical uncertainty to the staterror in HS3 >= 0.3. // Before, RooFit assumed Poisson uncertainties for it. TEST(RooFitHS3, HistFactorySampleWithoutErrors) @@ -4026,7 +4113,7 @@ TEST(RooFitHS3, FormulaNormSysConstraint) RooWorkspace ws1; importFormulaNormSysChannel(ws1); const std::string json = RooJSONFactoryWSTool{ws1}.exportJSONtoString(); - EXPECT_NE(json.find(R"("constraint":"alpha_sysConstraint")"), std::string::npos) << json; + EXPECT_NE(json.find(R"("constraints":["alpha_sysConstraint"])"), std::string::npos) << json; RooWorkspace ws2; ASSERT_TRUE(RooJSONFactoryWSTool{ws2}.importJSONfromString(json)); From 7d79897b97a8bc87f5d570c9d08996379691b076 Mon Sep 17 00:00:00 2001 From: Carsten Burgard Date: Mon, 26 Oct 2026 00:46:39 +0100 Subject: [PATCH 7/9] [RF][HS3][after HS3 #124/#129] Write min and max on irregular binned axes RooFit wrote only `edges` for binned axes with an irregular binning. HS3 PR #124 requires `name`, `min`, `max` and exactly one of `nbins` or `edges` on the axes of a `histfactory_dist`, while HS3 PR #135 makes `min`/`max` optional next to `edges`, but allows them if they equal the first and last edge. Writing them satisfies both, so binned axes written via `exportAxis()` (binned data, histograms, ParamHistFunc and HistFactory axes) now always carry `min` and `max`. The `axes` of generic formulas are unchanged, because their reader requires either `edges` or `min`/`max`/`nbins`. Assisted-by: claude-code:claude-opus-5-5 --- roofit/hs3/src/RooJSONFactoryWSTool.cxx | 5 +++++ roofit/hs3/test/testRooFitHS3.cxx | 24 ++++++++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/roofit/hs3/src/RooJSONFactoryWSTool.cxx b/roofit/hs3/src/RooJSONFactoryWSTool.cxx index 0b80d475be453..72db5e934cedd 100644 --- a/roofit/hs3/src/RooJSONFactoryWSTool.cxx +++ b/roofit/hs3/src/RooJSONFactoryWSTool.cxx @@ -1380,6 +1380,11 @@ void RooJSONFactoryWSTool::exportAxis(JSONNode &obsNode, RooRealVar const &var) obsNode["name"] << name; writeAxisBinning(obsNode, var.getBinning()); + // Binned axes always carry their range, also if it is redundant with the "edges" of an irregular binning. + if (!obsNode.has_child("min")) { + obsNode["min"] << var.getBinning().lowBound(); + obsNode["max"] << var.getBinning().highBound(); + } } /** diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index 17ae9d12769a0..7d7185d3667bb 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -3798,6 +3798,30 @@ TEST(RooFitHS3, HistFactoryConstraintsByName) EXPECT_EQ(model3->pdfList().size(), 2u); // the sum and alpha_a_constraint } +// Binned axes with an irregular binning carry "min" and "max" in addition to the "edges". +TEST(RooFitHS3, IrregularBinnedAxesHaveRange) +{ + RooRealVar x{"x", "x", 0.0, 3.0}; + std::vector edges{0.0, 1.0, 3.0}; + x.setBinning(RooBinning{2, edges.data()}); + RooDataHist dh{"dh", "dh", RooArgSet{x}}; + RooWorkspace ws; + ws.import(dh, RooFit::Silence()); + + const std::string json = RooJSONFactoryWSTool{ws}.exportJSONtoString(); + auto tree = RooFit::Detail::JSONTree::create(json); + const auto *dataNode = RooJSONFactoryWSTool::findNamedChild(tree->rootnode()["data"], "dh"); + ASSERT_NE(dataNode, nullptr) << json; + const auto &axis = (*dataNode)["axes"].child(0); + EXPECT_TRUE(axis.has_child("edges")) << json; + EXPECT_DOUBLE_EQ(axis["min"].val_double(), 0.0) << json; + EXPECT_DOUBLE_EQ(axis["max"].val_double(), 3.0) << json; + + RooWorkspace ws2; + ASSERT_TRUE(RooJSONFactoryWSTool{ws2}.importJSONfromString(json)); + EXPECT_EQ(RooJSONFactoryWSTool{ws2}.exportJSONtoString(), json); +} + // Errors of zero in binned data stay zero, also before the first positive error. TEST(RooFitHS3, BinnedDataZeroErrors) { From 6115e08538167924a31ef357a5e3cfcbf213f5e1 Mon Sep 17 00:00:00 2001 From: Carsten Burgard Date: Mon, 26 Oct 2026 00:49:48 +0100 Subject: [PATCH 8/9] [tutorials][RF][HS3][after HS3 #124/#129] Use named constraints in rf515_hfJSON Write the HistFactory modifiers of the rf515_hfJSON tutorial in the form of HS3 PR #124: every modifier names its parameters in `parameters`, and its constraint distributions by name in `constraints`. The constraint terms that RooFit used to create implicitly are now explicit: * unit Gaussians (mean 0, sigma 1) for the normsys parameters, as recommended by #124; * Poisson terms for the two staterror gammas, with the tau values (400 and 100) that follow from the sample errors. #124 recommends a `barlow_beeston_lite_poisson_constraint_dist`, which RooFit doesn't support yet, so `poisson_dist` is used. The gammas are declared in a domain and in the default parameter point, so that the functions referring to them can be imported. The document now declares `hs3_version` 0.3. The likelihood is the same as before up to an additive constant of 1.3e-5 (checked at several parameter points), and the fit result agrees within the minimizer tolerance. Assisted-by: claude-code:claude-opus-5-5 --- tutorials/roofit/roofit/rf515_hfJSON.json | 128 +++++++++++++++++++++- 1 file changed, 124 insertions(+), 4 deletions(-) diff --git a/tutorials/roofit/roofit/rf515_hfJSON.json b/tutorials/roofit/roofit/rf515_hfJSON.json index 382b8f9c39c06..2a65966ff8944 100644 --- a/tutorials/roofit/roofit/rf515_hfJSON.json +++ b/tutorials/roofit/roofit/rf515_hfJSON.json @@ -55,6 +55,39 @@ ] } ], + "domains": [ + { + "name": "staterror_parameters", + "type": "product_domain", + "axes": [ + { + "name": "gamma_bin_0", + "min": 0.0, + "max": 5.0 + }, + { + "name": "gamma_bin_1", + "min": 0.0, + "max": 5.0 + } + ] + } + ], + "parameter_points": [ + { + "name": "default_values", + "parameters": [ + { + "name": "gamma_bin_0", + "value": 1.0 + }, + { + "name": "gamma_bin_1", + "value": 1.0 + } + ] + } + ], "distributions": [ { "name": "model_channel1", @@ -85,12 +118,20 @@ "hi": 1.05, "lo": 0.95 }, - "constraint": "Gauss", + "constraints": [ + "alpha_syst1_constraint" + ], "name": "syst1", + "parameters": [ + "alpha_syst1" + ], "type": "normsys" }, { "name": "mu", + "parameters": [ + "mu" + ], "type": "normfactor" } ], @@ -113,12 +154,25 @@ "hi": 1.05, "lo": 0.95 }, - "constraint": "Gauss", + "constraints": [ + "alpha_syst2_constraint" + ], "name": "syst2", + "parameters": [ + "alpha_syst2" + ], "type": "normsys" }, { + "constraints": [ + "gamma_bin_0_constraint", + "gamma_bin_1_constraint" + ], "name": "mcstat", + "parameters": [ + "gamma_bin_0", + "gamma_bin_1" + ], "type": "staterror" } ], @@ -141,12 +195,25 @@ "hi": 1.05, "lo": 0.95 }, - "constraint": "Gauss", + "constraints": [ + "alpha_syst3_constraint" + ], "name": "syst3", + "parameters": [ + "alpha_syst3" + ], "type": "normsys" }, { + "constraints": [ + "gamma_bin_0_constraint", + "gamma_bin_1_constraint" + ], "name": "mcstat", + "parameters": [ + "gamma_bin_0", + "gamma_bin_1" + ], "type": "staterror" } ], @@ -154,6 +221,59 @@ } ], "type": "histfactory_dist" + }, + { + "name": "alpha_syst1_constraint", + "type": "gaussian_dist", + "x": "alpha_syst1", + "mean": 0.0, + "sigma": 1.0 + }, + { + "name": "alpha_syst2_constraint", + "type": "gaussian_dist", + "x": "alpha_syst2", + "mean": 0.0, + "sigma": 1.0 + }, + { + "name": "alpha_syst3_constraint", + "type": "gaussian_dist", + "x": "alpha_syst3", + "mean": 0.0, + "sigma": 1.0 + }, + { + "name": "gamma_bin_0_constraint", + "type": "poisson_dist", + "x": 400, + "mean": "gamma_bin_0_expected", + "integer": false + }, + { + "name": "gamma_bin_1_constraint", + "type": "poisson_dist", + "x": 100, + "mean": "gamma_bin_1_expected", + "integer": false + } + ], + "functions": [ + { + "name": "gamma_bin_0_expected", + "type": "product", + "factors": [ + "gamma_bin_0", + 400 + ] + }, + { + "name": "gamma_bin_1_expected", + "type": "product", + "factors": [ + "gamma_bin_1", + 100 + ] } ], "data": [ @@ -175,6 +295,6 @@ } ], "metadata": { - "hs3_version": "0.1.90" + "hs3_version": "0.3" } } From dd27321217798f8fbe25eb152a72535a6b142de0 Mon Sep 17 00:00:00 2001 From: Carsten Burgard Date: Thu, 5 Nov 2026 11:50:53 +0100 Subject: [PATCH 9/9] [RF][HS3][after HS3 #124] HistFactory: write shapesys and shapefactor as "shapefactor" Following the proposal for HS3 PR #124, HS3 has a single per-bin modifier type, `shapefactor`, whose parameters are constrained by the distributions named in `constraints`, if given, and free otherwise. `shapesys` is a deprecated alias. * Export: HistFactory's ShapeSys and ShapeFactor are both written as `shapefactor`, with the constraint distributions found for the gammas in `constraints` (null for free gammas, omitted if all are free). The type is no longer derived from the name of the ParamHistFunc, so free gammas no longer make the export fail with "cannot find constraint", and constraints of any type are allowed (the unused relative errors, which only supported Gaussian and Poisson constraints, are no longer computed). * Import: `shapesys` is read as `shapefactor`, with a deprecation warning (once per channel) for HS3 >= 0.3. The ParamHistFunc is named `__ShapeSys` if constrained and `__shapeFactor` if free, as in HistFactory. * Without `constraints`, RooFit only creates constraints from `data.vals` for documents before HS3 0.3 or with a legacy constraint key; otherwise the parameters are free and `data.vals` is ignored with a warning. The HistFactory test of the HS3 import of ShapeFactors now round-trips a free ShapeFactor, which needed constant gammas before. Assisted-by: claude-code:claude-opus-5-5 --- roofit/histfactory/test/testHistFactory.cxx | 48 +++++----- roofit/hs3/src/JSONFactories_HistFactory.cxx | 73 +++++++++------ roofit/hs3/test/testRooFitHS3.cxx | 96 ++++++++++++++++++++ 3 files changed, 161 insertions(+), 56 deletions(-) diff --git a/roofit/histfactory/test/testHistFactory.cxx b/roofit/histfactory/test/testHistFactory.cxx index 29acd2c4ded64..a155ab858ad1e 100644 --- a/roofit/histfactory/test/testHistFactory.cxx +++ b/roofit/histfactory/test/testHistFactory.cxx @@ -700,15 +700,10 @@ INSTANTIATE_TEST_SUITE_P(HistFactory, HFFixtureFit, testing::Values(ROOFIT_EVAL_BACKENDS_WITH_CODEGEN)), getNameFromInfo); -// Regression test for the HS3 importer's handling of "shapefactor" modifiers. -// HistFactory's ShapeFactor is exported with type "shapesys" (the only -// modifier type the writer ever emits for a ParamHistFunc), but valid HS3 JSON -// may also use the dedicated "shapefactor" type for an unconstrained -// ParamHistFunc - and the importer must accept it. This test starts from a -// HistFactory model built with MakeModelAndMeasurementFast, rewrites the -// modifier type to "shapefactor" in the JSON, and checks that the importer -// recognises it. Without the fix in JSONFactories_HistFactory.cxx the import -// throws "modifier ... of unknown type 'shapefactor'". +// HS3 round trip of a free HistFactory ShapeFactor. It is written as a +// "shapefactor" modifier without "constraints", and must survive the +// JSON -> workspace -> JSON round trip with free gammas. The deprecated +// "shapesys" alias is read as well. TEST(HistFactory, HS3ImportShapeFactorModifier) { using namespace RooStats::HistFactory; @@ -745,12 +740,9 @@ TEST(HistFactory, HS3ImportShapeFactorModifier) chan.AddSample(sig); // ShapeFactor on the background: an unconstrained, bin-by-bin scaling. - // Make the gammas constant so that the workspace is well-defined for - // re-export (free shapefactor gammas have no constraints attached). Sample bkg("background", "background", inputFile); ShapeFactor sf; sf.SetName("bkgShape"); - sf.SetConstant(true); bkg.AddShapeFactor(sf); chan.AddSample(bkg); @@ -762,26 +754,30 @@ TEST(HistFactory, HS3ImportShapeFactorModifier) const std::string js = RooJSONFactoryWSTool{*ws}.exportJSONtoString(); - // Rewrite the modifier type for "bkgShape" from "shapesys" to "shapefactor". - // The HistFactory exporter always writes "shapesys", but the importer should - // accept the more accurate "shapefactor" type as well. - const std::regex pattern{"\"name\":\"bkgShape\",\"parameters\":\\[([^\\]]*)\\],\"type\":\"shapesys\""}; - const std::string jsShapeFactor = - std::regex_replace(js, pattern, "\"name\":\"bkgShape\",\"parameters\":[$1],\"type\":\"shapefactor\""); - ASSERT_NE(js, jsShapeFactor) << "Failed to substitute shapesys -> shapefactor in JSON"; + // The free ShapeFactor is written as "shapefactor", without constraints. + const std::regex pattern{"\"name\":\"bkgShape\",\"parameters\":\\[[^\\]]*\\],\"type\":\"shapefactor\""}; + EXPECT_TRUE(std::regex_search(js, pattern)) << js; + EXPECT_EQ(js.find("shapesys"), std::string::npos) << js; RooWorkspace wsFromJson{"new"}; - ASSERT_NO_THROW(RooJSONFactoryWSTool{wsFromJson}.importJSONfromString(jsShapeFactor)) - << "Importer rejected the 'shapefactor' modifier type"; + ASSERT_NO_THROW(RooJSONFactoryWSTool{wsFromJson}.importJSONfromString(js)); - // The imported workspace should expose the same ParamHistFunc gammas. - EXPECT_NE(wsFromJson.var("gamma_bkgShape_bin_0"), nullptr); - EXPECT_NE(wsFromJson.var("gamma_bkgShape_bin_1"), nullptr); + // The imported workspace should expose the same free ParamHistFunc gammas. + ASSERT_NE(wsFromJson.var("gamma_bkgShape_bin_0"), nullptr); + ASSERT_NE(wsFromJson.var("gamma_bkgShape_bin_1"), nullptr); + EXPECT_FALSE(wsFromJson.var("gamma_bkgShape_bin_0")->isConstant()); - // Re-exporting should give back the original JSON, since the writer emits - // type "shapesys" in both cases. + // Re-exporting should give back the original JSON. const std::string js2 = RooJSONFactoryWSTool{wsFromJson}.exportJSONtoString(); EXPECT_EQ(js, js2) << "JSON -> WS -> JSON roundtrip changed the JSON"; + + // The deprecated "shapesys" alias gives the same model. + const std::string jsShapeSys = + std::regex_replace(js, std::regex{"\"type\":\"shapefactor\""}, "\"type\":\"shapesys\""); + ASSERT_NE(js, jsShapeSys); + RooWorkspace wsFromAlias{"alias"}; + ASSERT_NO_THROW(RooJSONFactoryWSTool{wsFromAlias}.importJSONfromString(jsShapeSys)); + EXPECT_EQ(RooJSONFactoryWSTool{wsFromAlias}.exportJSONtoString(), js); } // Issue #20697: Sample::AddShapeFactor() now allows to set the initial value diff --git a/roofit/hs3/src/JSONFactories_HistFactory.cxx b/roofit/hs3/src/JSONFactories_HistFactory.cxx index 028bc95b5dfb7..fdf007987dcd8 100644 --- a/roofit/hs3/src/JSONFactories_HistFactory.cxx +++ b/roofit/hs3/src/JSONFactories_HistFactory.cxx @@ -753,7 +753,7 @@ bool importHistSample(RooJSONFactoryWSTool &tool, RooDataHist &dh, RooArgSet con RooAbsArg const *mcStatObject, const std::string &fprefix, const JSONNode &p, const std::optional &defaultInterpolation, LegacyInterpolationDefault &legacyInterpolationDefault, bool &warnedUnconstrained, - RooArgSet &constraints) + bool &warnedShapeSys, RooArgSet &constraints) { RooWorkspace &ws = *tool.workspace(); @@ -853,8 +853,13 @@ bool importHistSample(RooJSONFactoryWSTool &tool, RooDataHist &dh, RooArgSet con legacyInterpolationDefault)); addSysConstraint(tool, mod, par, sysname, sampleName, warnedUnconstrained, constraints); } else if (modtype == "shapesys" || modtype == "shapefactor") { - std::string funcName = channelName + "_" + sysname + "_ShapeSys"; - // funcName should be "__ShapeSys" + // HS3 has a single per-bin modifier type, "shapefactor", whose parameters are constrained by the + // distributions in "constraints", if given. "shapesys" is a deprecated alias. + if (modtype == "shapesys" && !isHS3VersionBefore(tool.hs3VersionInput(), 0, 3) && !warnedShapeSys) { + RooJSONFactoryWSTool::warning("RooFitHS3: the modifier type \"shapesys\" is deprecated, please use " + "\"shapefactor\" instead."); + warnedShapeSys = true; + } std::vector vals; if (mod["data"].has_child("vals")) { for (const auto &v : mod["data"]["vals"].children()) { @@ -906,9 +911,26 @@ bool importHistSample(RooJSONFactoryWSTool &tool, RooDataHist &dh, RooArgSet con "not a supported legacy constraint type"); } } + // Without "constraints", only the legacy constraint keys, or documents before HS3 0.3, make RooFit + // create constraints from "data.vals". Otherwise, the parameters are free. + const bool legacyConstraint = mod.has_child("constraint_type") || mod.has_child("constraint"); + const bool createConstraints = + !hasConstraintList && (legacyConstraint || isHS3VersionBefore(tool.hs3VersionInput(), 0, 3)); + if (!hasConstraintList && !createConstraints && !vals.empty()) { + oocoutW(nullptr, ObjectHandling) + << "RooFitHS3: the \"data.vals\" of modifier '" << sysname << "' of sample '" << sampleName + << "' are ignored, because it names no constraints, so its " + << "parameters are free" << std::endl; + vals.clear(); + } + const bool constrained = createConstraints ? !vals.empty() && constraint != "Const" + : std::any_of(constraintPdfs.begin(), constraintPdfs.end(), + [](RooAbsPdf *pdf) { return pdf != nullptr; }); + // Follow the naming of HistFactory: "__ShapeSys" for constrained and + // "__shapeFactor" for free parameters. + const std::string funcName = channelName + "_" + sysname + (constrained ? "_ShapeSys" : "_shapeFactor"); shapeElems.add(createPHF(funcName, sysname, parnames, vals, tool, constraints, varlist, constraint, - defaultGammaMin, defaultShapeSysGammaMax, minShapeUncertainty, - /*createConstraints=*/!hasConstraintList)); + defaultGammaMin, defaultShapeSysGammaMax, minShapeUncertainty, createConstraints)); for (auto *constraintPdf : constraintPdfs) { if (constraintPdf) { constraints.add(*constraintPdf); @@ -1090,13 +1112,14 @@ class HistFactoryImporter : public RooFit::JSONIO::Importer { LegacyInterpolationDefault legacyInterpolationDefault; legacyInterpolationDefault.allowed = isHS3VersionBefore(tool->hs3VersionInput(), 0, 3); bool warnedUnconstrained = false; + bool warnedShapeSys = false; int idx = 0; RooArgList funcs; RooArgList coefs; for (const auto &comp : p["samples"].children()) { importHistSample(*tool, *data[idx], observables, mcStatObject, fprefix, comp, defaultInterpolation, - legacyInterpolationDefault, warnedUnconstrained, constraints); + legacyInterpolationDefault, warnedUnconstrained, warnedShapeSys, constraints); ++idx; std::string const &compName = RooJSONFactoryWSTool::name(comp); @@ -1274,9 +1297,9 @@ struct HistoSys { high.assign(h->dataHist().weightArray(), h->dataHist().weightArray() + h->dataHist().numEntries()); } }; +// A per-bin modifier ("shapefactor"): HistFactory's ShapeSys (constrained) or ShapeFactor (free). struct ShapeSys { std::string name; - std::vector constraints; std::vector constraintPdfs; std::vector parameters; ShapeSys(const std::string &n) : name{n} {} @@ -1634,32 +1657,22 @@ Channel readChannel(RooJSONFactoryWSTool *tool, const std::string &pdfname, cons } } sample.useBarlowBeestonLight = true; - } else { // other ShapeSys + } else { // ShapeSys or ShapeFactor ShapeSys sys(phf->GetName()); erasePrefix(sys.name, channel.name + "_"); - bool isshapesys = eraseSuffix(sys.name, "_ShapeSys") || eraseSuffix(sys.name, "_shapeSys"); - bool isshapefactor = eraseSuffix(sys.name, "_ShapeFactor") || eraseSuffix(sys.name, "_shapeFactor"); + for (std::string_view suffix : {"_ShapeSys", "_shapeSys", "_ShapeFactor", "_shapeFactor"}) { + if (eraseSuffix(sys.name, suffix)) + break; + } + // Both are written as "shapefactor": the constrained parameters name their constraint distribution in + // "constraints", the others are free. for (const auto &g : phf->paramList()) { sys.parameters.push_back(static_cast(g)); - RooAbsPdf *constraint = nullptr; - if (isshapesys) { - constraint = findConstraint(g); - if (!constraint) - constraint = ws->pdf(constraintName(g->GetName())); - if (!constraint && !g->isConstant()) { - RooJSONFactoryWSTool::error("cannot find constraint for " + std::string(g->GetName())); - } - } else if (!isshapefactor) { - RooJSONFactoryWSTool::error("unknown type of shapesys " + std::string(phf->GetName())); - } - if (!constraint) { - sys.constraints.push_back(0.0); - sys.constraintPdfs.push_back(nullptr); - } else { - sys.constraints.push_back(constraintRelError(*constraint, *g)); - sys.constraintPdfs.push_back(constraint); - } + RooAbsPdf *constraint = findConstraint(g); + if (!constraint) + constraint = ws->pdf(constraintName(g->GetName())); + sys.constraintPdfs.push_back(constraint); } sample.shapesys.emplace_back(std::move(sys)); } @@ -1801,7 +1814,7 @@ void ensureUniqueModifiers(const Channel &channel, const Sample &sample) for (const auto &modifier : sample.histosys) add("histosys", modifier.name); for (const auto &modifier : sample.shapesys) - add("shapesys", modifier.name); + add("shapefactor", modifier.name); for (const auto &modifier : sample.otherElements) add("custom", modifier.name); for (const auto &modifier : sample.tmpElements) @@ -1933,7 +1946,7 @@ bool exportChannel(RooJSONFactoryWSTool *tool, const Channel &channel, JSONNode } for (const auto &sys : sample.shapesys) { - auto &mod = addModifier(modifiers, sys.name, "shapesys"); + auto &mod = addModifier(modifiers, sys.name, "shapefactor"); exportGammaParameters(mod, sys.parameters); // The relative uncertainties are encoded in the constraint distributions. exportConstraintNames(mod, sys.constraintPdfs); diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index 7d7185d3667bb..8b280064847ae 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -3888,6 +3888,102 @@ TEST(RooFitHS3, HistFactorySampleWithoutErrors) EXPECT_DOUBLE_EQ(legacyErrors[1], std::sqrt(36.0 + 20.0) / 40.0); } +// HS3 has one per-bin modifier type, "shapefactor", whose parameters are constrained by the distributions named in +// "constraints", if given, and free otherwise. "shapesys" is a deprecated alias. HistFactory's ShapeSys (constrained) +// and ShapeFactor (free) are both written as "shapefactor". +TEST(RooFitHS3, HistFactoryShapeFactor) +{ + RooHelpers::LocalChangeMsgLevel changeMsgLvl(RooFit::WARNING); + + auto makeJSON = [](std::string const &version, std::string const &modifiers, + std::string const &constraintDist = "") { + return R"({ + "metadata": {"hs3_version": ")" + + version + R"("}, + "distributions": [ + { + "name": "model_channel0", + "type": "histfactory_dist", + "axes": [{"name": "x", "min": 0.0, "max": 2.0, "nbins": 2}], + "samples": [{"name": "sig", "data": {"contents": [10.0, 20.0]}, "modifiers": [)" + + modifiers + R"(]}] + })" + + constraintDist + R"( + ], + "domains": [{"name": "default_domain", "type": "product_domain", "axes": [ + {"name": "g_free_0", "min": 0, "max": 5}, {"name": "g_free_1", "min": 0, "max": 5}, + {"name": "g_c_0", "min": 0, "max": 5}, {"name": "g_c_1", "min": 0, "max": 5}]}] + })"; + }; + auto countGammaConstraints = [](RooWorkspace &ws) { + std::size_t n = 0; + for (RooAbsArg *pdf : ws.allPdfs()) { + if (dynamic_cast(pdf) || dynamic_cast(pdf)) { + ++n; + } + } + return n; + }; + const std::string freeModifier = + R"({"type": "shapefactor", "name": "free", "parameters": ["g_free_0", "g_free_1"]})"; + const std::string constrainedModifier = R"({"type": "shapefactor", "name": "constr", + "parameters": ["g_c_0", "g_c_1"], "constraints": ["g_c_0_constraint", null]})"; + const std::string json = + makeJSON("0.3", freeModifier + "," + constrainedModifier, + R"(, {"name": "g_c_0_constraint", "type": "gaussian_dist", "x": "g_c_0", "mean": 1.0, "sigma": 0.1})"); + + RooWorkspace ws; + ASSERT_TRUE(RooJSONFactoryWSTool{ws}.importJSONfromString(json)); + // The functions are named as in HistFactory: ShapeFactor for free, ShapeSys for constrained parameters. + EXPECT_NE(ws.function("channel0_free_shapeFactor"), nullptr); + EXPECT_NE(ws.function("channel0_constr_ShapeSys"), nullptr); + EXPECT_EQ(countGammaConstraints(ws), 1u); + ASSERT_NE(ws.var("g_free_0"), nullptr); + EXPECT_FALSE(ws.var("g_free_0")->isConstant()); + + // Both are written as "shapefactor", and free parameters don't prevent the round trip. + const std::string exported = RooJSONFactoryWSTool{ws}.exportJSONtoString(); + EXPECT_EQ(countOccurrences(exported, R"("type":"shapefactor")"), 2u) << exported; + EXPECT_EQ(exported.find("shapesys"), std::string::npos) << exported; + EXPECT_NE(exported.find(R"("constraints":["g_c_0_constraint",null])"), std::string::npos) << exported; + RooWorkspace ws2; + ASSERT_TRUE(RooJSONFactoryWSTool{ws2}.importJSONfromString(exported)); + EXPECT_EQ(RooJSONFactoryWSTool{ws2}.exportJSONtoString(), exported); + + // "shapesys" is a deprecated alias. + std::string withAlias = json; + withAlias.replace(withAlias.find(R"("type": "shapefactor")"), std::string(R"("type": "shapefactor")").size(), + R"("type": "shapesys")"); + RooWorkspace ws3; + const std::string aliasWarnings = captureMessages(RooFit::WARNING, RooFit::ObjectHandling, [&] { + EXPECT_TRUE(RooJSONFactoryWSTool{ws3}.importJSONfromString(withAlias)); + }); + EXPECT_NE(aliasWarnings.find(R"(the modifier type "shapesys" is deprecated)"), std::string::npos) << aliasWarnings; + EXPECT_EQ(RooJSONFactoryWSTool{ws3}.exportJSONtoString(), exported); + + // In HS3 >= 0.3, "data.vals" without "constraints" don't create constraints. + RooWorkspace ws4; + const std::string valsWarnings = captureMessages(RooFit::WARNING, RooFit::ObjectHandling, [&] { + EXPECT_TRUE(RooJSONFactoryWSTool{ws4}.importJSONfromString( + makeJSON("0.3", R"({"type": "shapefactor", "name": "free", "parameters": ["g_free_0", "g_free_1"], + "data": {"vals": [0.1, 0.2]}})"))); + }); + EXPECT_NE(valsWarnings.find(R"(the "data.vals" of modifier 'free' of sample 'sig' are ignored)"), std::string::npos) + << valsWarnings; + EXPECT_NE(ws4.function("channel0_free_shapeFactor"), nullptr); + EXPECT_EQ(countGammaConstraints(ws4), 0u); + + // Before HS3 0.3, a "shapesys" with "data.vals" is constrained as before, without deprecation warning. + RooWorkspace ws5; + const std::string legacyWarnings = captureMessages(RooFit::WARNING, RooFit::ObjectHandling, [&] { + EXPECT_TRUE(RooJSONFactoryWSTool{ws5}.importJSONfromString( + makeJSON("0.2", R"({"type": "shapesys", "name": "constr", "data": {"vals": [0.1, 0.2]}})"))); + }); + EXPECT_EQ(legacyWarnings.find("deprecated"), std::string::npos) << legacyWarnings; + EXPECT_NE(ws5.function("channel0_constr_ShapeSys"), nullptr); + EXPECT_EQ(countGammaConstraints(ws5), 2u); +} + // A RooRealVar that is constant in all parameter points must stay a RooRealVar, and a RooConstVar must stay a // RooConstVar. In the parameter points both are written the same way (a value and "const": true). The only // difference is that the RooRealVar is listed in a domain and the RooConstVar is not: a parameter without domain is