From 870ece1c5398faad938da8d6e4ad05ce3ab7777e Mon Sep 17 00:00:00 2001 From: Carsten Burgard Date: Sun, 25 Oct 2026 19:27:20 +0100 Subject: [PATCH 01/35] [RF][HS3] Validate the hs3_version of imported documents The importer only checked that `metadata.hs3_version` exists. It now also requires it to be a string, parses the leading `MAJOR.MINOR`, and warns if the document is newer than the version RooFit writes (or if the string cannot be parsed). The exported version remains a single constant. The parsed version is kept available to importers during the import via `RooJSONFactoryWSTool::hs3VersionInput()`. Assisted-by: claude-code:claude-opus-5-5 --- .../hs3/inc/RooFitHS3/RooJSONFactoryWSTool.h | 4 +++ roofit/hs3/src/JSONIOUtils.cxx | 24 +++++++++++++ roofit/hs3/src/JSONIOUtils.h | 1 + roofit/hs3/src/RooJSONFactoryWSTool.cxx | 35 ++++++++++++++++--- roofit/hs3/test/testRooFitHS3.cxx | 23 ++++++++++++ 5 files changed, 82 insertions(+), 5 deletions(-) diff --git a/roofit/hs3/inc/RooFitHS3/RooJSONFactoryWSTool.h b/roofit/hs3/inc/RooFitHS3/RooJSONFactoryWSTool.h index 23f84b127e4cf..c711a16b355cf 100644 --- a/roofit/hs3/inc/RooFitHS3/RooJSONFactoryWSTool.h +++ b/roofit/hs3/inc/RooFitHS3/RooJSONFactoryWSTool.h @@ -206,6 +206,9 @@ class RooJSONFactoryWSTool { std::string getStringAttribute(const std::string &obj, const std::string &attrib); void setStringAttribute(const std::string &obj, const std::string &attrib, const std::string &value); + /// The "hs3_version" of the document that is currently being imported, or an empty string outside of an import. + std::string const &hs3VersionInput() const { return _hs3VersionInput; } + private: template T *requestImpl(const std::string &objname); @@ -251,6 +254,7 @@ class RooJSONFactoryWSTool { const RooFit::Detail::JSONNode *_attributesNode = nullptr; RooFit::Detail::JSONNode *_rootnodeOutput = nullptr; RooFit::Detail::JSONNode *_varsNode = nullptr; + std::string _hs3VersionInput; RooWorkspace &_workspace; // objects to represent intermediate information diff --git a/roofit/hs3/src/JSONIOUtils.cxx b/roofit/hs3/src/JSONIOUtils.cxx index 04e4494712130..5a921a470775d 100644 --- a/roofit/hs3/src/JSONIOUtils.cxx +++ b/roofit/hs3/src/JSONIOUtils.cxx @@ -2,6 +2,7 @@ #include +#include #include using RooFit::Detail::JSONNode; @@ -100,3 +101,26 @@ void writeAxisBinning(JSONNode &node, const RooAbsBinning &binning) edges.append_child() << binning.binHigh(i); } } + +// Parse the leading "MAJOR.MINOR" of an HS3 version string like "0.2", "0.2.9" or "0.3". +bool parseHS3Version(std::string const &version, int &major, int &minor) +{ + std::size_t pos = 0; + auto readNumber = [&](int &out) { + const std::size_t begin = pos; + while (pos < version.size() && std::isdigit(static_cast(version[pos]))) { + ++pos; + } + // Reject empty and absurdly long numbers, which std::stoi could not represent. + if (pos == begin || pos - begin > 9) { + return false; + } + out = std::stoi(version.substr(begin, pos - begin)); + return true; + }; + if (!readNumber(major) || pos >= version.size() || version[pos] != '.') { + return false; + } + ++pos; + return readNumber(minor); +} diff --git a/roofit/hs3/src/JSONIOUtils.h b/roofit/hs3/src/JSONIOUtils.h index e38c821a92556..d7672b8ebb4e8 100644 --- a/roofit/hs3/src/JSONIOUtils.h +++ b/roofit/hs3/src/JSONIOUtils.h @@ -11,5 +11,6 @@ bool endsWith(std::string_view str, std::string_view suffix); std::string removeSuffix(std::string_view str, std::string_view suffix); std::unique_ptr varJSONString(const RooFit::Detail::JSONNode &treeRoot); void writeAxisBinning(RooFit::Detail::JSONNode &node, const RooAbsBinning &binning); +bool parseHS3Version(std::string const &version, int &major, int &minor); #endif diff --git a/roofit/hs3/src/RooJSONFactoryWSTool.cxx b/roofit/hs3/src/RooJSONFactoryWSTool.cxx index 1e5a245fcbb82..c675bde178721 100644 --- a/roofit/hs3/src/RooJSONFactoryWSTool.cxx +++ b/roofit/hs3/src/RooJSONFactoryWSTool.cxx @@ -755,6 +755,26 @@ RooAbsCategory *findCategoryObservable(RooAbsData const &data) return cat; } +/// Warn if the HS3 version of an imported document is not understood or newer than what RooFit writes. +void checkHS3Version(std::string const &version) +{ + int major = 0; + int minor = 0; + int supportedMajor = 0; + int supportedMinor = 0; + parseHS3Version(hs3VersionTag, supportedMajor, supportedMinor); + if (!parseHS3Version(version, major, minor)) { + RooJSONFactoryWSTool::warning("RooFitHS3: cannot parse hs3_version '" + version + + "', expected a string of the form \"MAJOR.MINOR\"."); + return; + } + if (std::make_pair(major, minor) > std::make_pair(supportedMajor, supportedMinor)) { + RooJSONFactoryWSTool::warning("RooFitHS3: the document has hs3_version '" + version + + "', which is newer than the version supported by this RooFit ('" + hs3VersionTag + + "'). Some content might not be imported correctly."); + } +} + } // namespace RooJSONFactoryWSTool::RooJSONFactoryWSTool(RooWorkspace &ws) : _workspace{ws} {} @@ -1939,12 +1959,11 @@ void RooJSONFactoryWSTool::setStringAttribute(const std::string &obj, const std: void RooJSONFactoryWSTool::importAllNodes(const JSONNode &n) { // Per HS3 standard, the hs3_version in the metadata is required. So we - // error out if it is missing. TODO: now we are only checking if the - // hs3_version tag exists, but in the future when the HS3 specification - // versions are actually frozen, we should also check if the hs3_version is - // one that RooFit can actually read. + // error out if it is missing or not a string, and warn if the document is + // newer than what this version of RooFit was written for. auto metadata = n.find("metadata"); - if (!metadata || !metadata->find("hs3_version")) { + auto versionNode = metadata ? metadata->find("hs3_version") : nullptr; + if (!versionNode) { std::stringstream ss; ss << "The HS3 version is missing in the JSON!\n" << "Please include the HS3 version in the metadata field, e.g.:\n" @@ -1954,6 +1973,11 @@ void RooJSONFactoryWSTool::importAllNodes(const JSONNode &n) << " }"; error(ss.str()); } + if (versionNode->is_container() || versionNode->is_null() || versionNode->is_number()) { + error(std::string("The HS3 version in the metadata must be a string, e.g. \"") + hs3VersionTag + "\""); + } + _hs3VersionInput = versionNode->val(); + checkHS3Version(_hs3VersionInput); _rootnodeInput = &n; @@ -2047,6 +2071,7 @@ void RooJSONFactoryWSTool::importAllNodes(const JSONNode &n) } _rootnodeInput = nullptr; + _hs3VersionInput.clear(); _domains.reset(); _functionsByName.clear(); _distributionsByName.clear(); diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index a972fa14cae3c..e743c388c3abc 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -2892,3 +2892,26 @@ TEST(RooFitHS3, GenericTypeNames) EXPECT_NE(json.find("\"name\":\"c_exponential_inverted\",\"type\":\"generic\""), std::string::npos) << json; EXPECT_EQ(json.find("\"type\":\"generic_function\""), std::string::npos) << json; } + +// The hs3_version must be a string. Documents that are newer than what RooFit +// writes are imported with a warning. +TEST(RooFitHS3, VersionStringHandling) +{ + auto importWithVersion = [](std::string const &version) { + RooWorkspace ws; + return RooJSONFactoryWSTool{ws}.importJSONfromString(R"({"metadata": {"hs3_version": )" + version + "}}"); + }; + + expectImportThrowsWithError(R"({"metadata": {}})", "The HS3 version is missing"); + expectImportThrowsWithError(R"({"metadata": {"hs3_version": 0.2}})", "must be a string"); + + std::string warnings = + captureMessages(RooFit::WARNING, RooFit::IO, [&] { EXPECT_TRUE(importWithVersion("\"0.2\"")); }); + EXPECT_EQ(warnings.find("hs3_version"), std::string::npos) << warnings; + + warnings = captureMessages(RooFit::WARNING, RooFit::IO, [&] { EXPECT_TRUE(importWithVersion("\"99.1.4\"")); }); + EXPECT_NE(warnings.find("'99.1.4', which is newer"), std::string::npos) << warnings; + + warnings = captureMessages(RooFit::WARNING, RooFit::IO, [&] { EXPECT_TRUE(importWithVersion("\"latest\"")); }); + EXPECT_NE(warnings.find("cannot parse hs3_version 'latest'"), std::string::npos) << warnings; +} From dbf865421a16c15994dc2b33778cc83159cc8985 Mon Sep 17 00:00:00 2001 From: Carsten Burgard Date: Sun, 25 Oct 2026 19:28:03 +0100 Subject: [PATCH 02/35] [RF][HS3] Reject duplicate names of top-level HS3 objects HS3 requires the names of all objects in a document to be unique. The importer built its function/distribution indices with `emplace`, so a duplicate silently resolved to the first definition. Duplicated names within or across `distributions`, `functions`, `data`, `domains` and `parameter_points` are now an error. RooFit itself named likelihoods after their analysis, and analyses without matching dataset after the pdf, so its own files violate this rule. On export, likelihoods are now called `_likelihood`, and an analysis without dataset is named after its ModelConfig. On import, a name clash that involves `likelihoods` or `analyses` only triggers a warning, so that files written by older ROOT versions can still be read. This is unambiguous: analyses are not referenced by name, and likelihoods are only looked up from analyses. Assisted-by: claude-code:claude-opus-5-5 --- roofit/hs3/src/RooJSONFactoryWSTool.cxx | 45 +++++++++++++++++++++-- roofit/hs3/test/testRooFitHS3.cxx | 49 +++++++++++++++++++++++++ 2 files changed, 91 insertions(+), 3 deletions(-) diff --git a/roofit/hs3/src/RooJSONFactoryWSTool.cxx b/roofit/hs3/src/RooJSONFactoryWSTool.cxx index c675bde178721..9bf6efac0b417 100644 --- a/roofit/hs3/src/RooJSONFactoryWSTool.cxx +++ b/roofit/hs3/src/RooJSONFactoryWSTool.cxx @@ -775,6 +775,41 @@ 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. +void checkUniqueNames(JSONNode const &rootnode) +{ + std::map sectionOfName; + for (const char *section : + {"distributions", "functions", "data", "domains", "parameter_points", "likelihoods", "analyses"}) { + auto seq = rootnode.find(section); + if (!seq || !seq->is_seq()) { + continue; + } + for (JSONNode const &child : seq->children()) { + if (!child.is_map() || !child.has_child("name")) { + continue; // malformed entries are reported when they are imported + } + std::string const name = child["name"].val(); + auto inserted = sectionOfName.emplace(name, section); + if (inserted.second) { + continue; + } + std::string const &firstSection = inserted.first->second; + // RooFit used to give analyses and likelihoods the name of the pdf or the analysis. Accept this for backwards + // 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 (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; + } + RooJSONFactoryWSTool::error("duplicate name '" + name + "': it is defined in \"" + firstSection + + "\" and in \"" + section + "\", but names must be unique within the document"); + } + } +} + } // namespace RooJSONFactoryWSTool::RooJSONFactoryWSTool(RooWorkspace &ws) : _workspace{ws} {} @@ -1649,7 +1684,8 @@ void RooJSONFactoryWSTool::exportModelConfig(JSONNode &rootnode, RooFit::ModelCo } } if (founddata == 0) { - exportSingleModelConfig(rootnode, mc, pdf->GetName(), nullptr); + // Named after the ModelConfig, because the name of the pdf is already taken by the distribution. + exportSingleModelConfig(rootnode, mc, mc.GetName(), nullptr); } } } @@ -1664,9 +1700,11 @@ void RooJSONFactoryWSTool::exportSingleModelConfig(JSONNode &rootnode, RooFit::M auto &domains = analysisNode["domains"].set_seq(); - analysisNode["likelihood"] << analysisName; + // HS3 names must be unique across all top-level components, so the likelihood can't have the analysis name. + const std::string likelihoodName = analysisName + "_likelihood"; + analysisNode["likelihood"] << likelihoodName; - auto &nllNode = appendNamedChild(rootnode["likelihoods"], analysisName); + auto &nllNode = appendNamedChild(rootnode["likelihoods"], likelihoodName); nllNode["distributions"].set_seq(); nllNode["data"].set_seq(); @@ -1978,6 +2016,7 @@ void RooJSONFactoryWSTool::importAllNodes(const JSONNode &n) } _hs3VersionInput = versionNode->val(); checkHS3Version(_hs3VersionInput); + checkUniqueNames(n); _rootnodeInput = &n; diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index e743c388c3abc..fbd42cb3b028d 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -2915,3 +2915,52 @@ TEST(RooFitHS3, VersionStringHandling) warnings = captureMessages(RooFit::WARNING, RooFit::IO, [&] { EXPECT_TRUE(importWithVersion("\"latest\"")); }); EXPECT_NE(warnings.find("cannot parse hs3_version 'latest'"), std::string::npos) << warnings; } + +// Names of top-level objects must be unique across the whole HS3 document. +TEST(RooFitHS3, DuplicateNamesAreRejected) +{ + // The same name used for a distribution and a function. + expectImportThrowsWithError(R"({ + "metadata": {"hs3_version": "0.2"}, + "distributions": [{"name": "dup", "type": "gaussian_dist", "x": "x", "mean": 0.0, "sigma": 1.0}], + "functions": [{"name": "dup", "type": "sum", "summands": ["x"]}] + })", + "duplicate name 'dup'"); + + // The same name used twice within one section. + expectImportThrowsWithError(R"({ + "metadata": {"hs3_version": "0.2"}, + "parameter_points": [{"name": "pp", "parameters": []}, {"name": "pp", "parameters": []}] + })", + "duplicate name 'pp'"); + + // Exported likelihoods don't share their name with the analysis anymore. + RooWorkspace ws; + ws.factory("Gaussian::pdf(x[0, -5, 5], mu[0, -5, 5], 1.0)"); + RooFit::ModelConfig mc{"ModelConfig", &ws}; + mc.SetPdf("pdf"); + mc.SetParametersOfInterest("mu"); + mc.SetObservables("x"); + ws.import(mc); + const std::string json = RooJSONFactoryWSTool{ws}.exportJSONtoString(); + EXPECT_NE(json.find(R"("likelihood":"ModelConfig_likelihood")"), std::string::npos) << json; + RooWorkspace ws2; + const std::string warnings = captureMessages( + RooFit::WARNING, RooFit::IO, [&] { EXPECT_TRUE(RooJSONFactoryWSTool{ws2}.importJSONfromString(json)); }); + EXPECT_EQ(warnings.find("not allowed by the HS3 standard"), std::string::npos) << warnings; + + // Files written by older ROOT versions named the likelihood (and the analysis) after the pdf. They can still be + // read. + std::string legacyJson = json; + const std::string likelihoodName = "ModelConfig_likelihood"; + for (std::size_t pos = 0; (pos = legacyJson.find(likelihoodName, pos)) != std::string::npos;) { + legacyJson.replace(pos, likelihoodName.size(), "pdf"); + } + RooWorkspace ws3; + const std::string legacyWarnings = captureMessages( + RooFit::WARNING, RooFit::IO, [&] { EXPECT_TRUE(RooJSONFactoryWSTool{ws3}.importJSONfromString(legacyJson)); }); + EXPECT_NE(legacyWarnings.find(R"(the name 'pdf' is used in "distributions" and in "likelihoods")"), + std::string::npos) + << legacyWarnings << legacyJson; + EXPECT_NE(ws3.obj("ModelConfig"), nullptr); +} From e9558fa098fa94051bf6a371a24ecd88444060e4 Mon Sep 17 00:00:00 2001 From: Carsten Burgard Date: Sun, 25 Oct 2026 19:28:44 +0100 Subject: [PATCH 03/35] [RF][HS3] Make the "default_domain" optional on import The importer required a domain named exactly `default_domain`, which is not part of the HS3 standard. If it is absent, variables are now created with the range of the first domain that mentions them (or RooFit's default if none does). Export still writes `default_domain`, and documents that contain it are imported exactly as before. Assisted-by: claude-code:claude-opus-5-5 --- roofit/hs3/src/Domains.cxx | 19 +++++++++++++++---- roofit/hs3/test/testRooFitHS3.cxx | 27 +++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 4 deletions(-) diff --git a/roofit/hs3/src/Domains.cxx b/roofit/hs3/src/Domains.cxx index 218cf62cdf674..2910fd85573d9 100644 --- a/roofit/hs3/src/Domains.cxx +++ b/roofit/hs3/src/Domains.cxx @@ -54,6 +54,12 @@ void Domains::populate(RooWorkspace &ws) const auto default_domain = _map.find(defaultDomainName); if (default_domain != _map.end()) { default_domain->second.populate(ws); + } else { + // Without the RooFit-specific "default_domain", the variables are created with the range of the domain that + // mentions them. + for (const auto &domain : _map) { + domain.second.populate(ws); + } } for (const auto &domain : _map) { if (domain.first == defaultDomainName) @@ -78,15 +84,20 @@ void Domains::writeVariable(RooRealVar &var) const auto default_domain = _map.find(defaultDomainName); if (default_domain != _map.end()) { default_domain->second.writeVariable(var); + return; + } + // Without the RooFit-specific "default_domain", use the first domain that mentions the variable. + for (const auto &domain : _map) { + if (domain.second.hasVariable(var.GetName())) { + domain.second.writeVariable(var); + return; + } } } void Domains::readJSON(RooFit::Detail::JSONNode const &node) { - auto default_domain_element = RooJSONFactoryWSTool::findNamedChild(node, defaultDomainName); - if (!default_domain_element) { - RooJSONFactoryWSTool::error("\"domains\" do not contain \"" + std::string{defaultDomainName} + "\""); - } + // The "default_domain" is written by RooFit, but it is not required by the HS3 standard. for (auto &domain : node.children()) { if (!domain.has_child("name")) { RooJSONFactoryWSTool::error("encountered domain without \"name\""); diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index fbd42cb3b028d..4be7217da153a 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -2964,3 +2964,30 @@ TEST(RooFitHS3, DuplicateNamesAreRejected) << legacyWarnings << legacyJson; EXPECT_NE(ws3.obj("ModelConfig"), nullptr); } + +// The RooFit-specific "default_domain" is optional on import. Variables then +// take the range of the domain that mentions them. +TEST(RooFitHS3, DefaultDomainIsOptional) +{ + const std::string json = R"({ + "metadata": {"hs3_version": "0.2"}, + "domains": [ + {"name": "my_domain", "type": "product_domain", "axes": [{"name": "mu", "min": -3.0, "max": 4.0}]} + ], + "parameter_points": [{"name": "default_values", "parameters": [{"name": "mu", "value": 0.5}]}], + "distributions": [{"name": "pdf", "type": "gaussian_dist", "x": "mu", "mean": 0.0, "sigma": 1.0}] + })"; + + RooWorkspace ws; + ASSERT_TRUE(RooJSONFactoryWSTool{ws}.importJSONfromString(json)); + RooRealVar *mu = ws.var("mu"); + ASSERT_NE(mu, nullptr); + EXPECT_DOUBLE_EQ(mu->getMin(), -3.0); + EXPECT_DOUBLE_EQ(mu->getMax(), 4.0); + EXPECT_DOUBLE_EQ(mu->getVal(), 0.5); + EXPECT_FALSE(mu->isConstant()); + + // On export, the "default_domain" is still written. + const std::string exported = RooJSONFactoryWSTool{ws}.exportJSONtoString(); + EXPECT_NE(exported.find(R"("name":"default_domain")"), std::string::npos) << exported; +} From 564e474959a5d868d4290abf9c2e282c2b3e653a Mon Sep 17 00:00:00 2001 From: Carsten Burgard Date: Sun, 25 Oct 2026 19:29:12 +0100 Subject: [PATCH 04/35] [RF][HS3] Import parameters that only appear in a parameter point A parameter listed in a parameter point that was not created by any distribution, function or domain used to be ignored silently. It is now created as a `RooRealVar`. It is constant if `"const": true` or if no domain lists it, since there is no range to vary it in; `"const": false` is the same as omitting `const`. A string `value` is accepted for categorical parameters if a `RooCategory` of that name exists; otherwise it is ignored with a warning instead of being read as a number. Assisted-by: claude-code:claude-opus-5-5 --- roofit/hs3/src/RooJSONFactoryWSTool.cxx | 30 ++++++++++++- roofit/hs3/test/testRooFitHS3.cxx | 56 +++++++++++++++++++++++++ 2 files changed, 85 insertions(+), 1 deletion(-) diff --git a/roofit/hs3/src/RooJSONFactoryWSTool.cxx b/roofit/hs3/src/RooJSONFactoryWSTool.cxx index 9bf6efac0b417..873af24b732bd 100644 --- a/roofit/hs3/src/RooJSONFactoryWSTool.cxx +++ b/roofit/hs3/src/RooJSONFactoryWSTool.cxx @@ -2056,8 +2056,36 @@ void RooJSONFactoryWSTool::importAllNodes(const JSONNode &n) RooArgSet vars; for (const auto &var : snsh["parameters"].children()) { - if (RooRealVar *rrv = _workspace.var(RooJSONFactoryWSTool::name(var))) { + const std::string varName = RooJSONFactoryWSTool::name(var); + JSONNode const *valueNode = var.find("value"); + const bool hasStringValue = valueNode && !valueNode->is_number(); + if (RooCategory *cat = _workspace.cat(varName)) { + if (hasStringValue) { + cat->setLabel(valueNode->val().c_str()); + } else if (valueNode) { + cat->setIndex(valueNode->val_int()); + } + vars.add(*cat); + continue; + } + if (hasStringValue) { + warning("RooFitHS3: ignoring the string value of parameter '" + varName + "' in parameter point '" + + name + "', because there is no category with that name."); + continue; + } + RooRealVar *rrv = _workspace.var(varName); + if (!rrv && !_workspace.arg(varName)) { + // The parameter is not used by any other component: create it. It is constant if "const" is true or + // if no domain lists it, since there is no range to vary it in. "const": false is the same as no + // "const" at all. + testValidName(varName, true); + rrv = &wsEmplace(varName, 1.); configureVariable(*_domains, var, *rrv); + rrv->setConstant(rrv->isConstant() || !_domains->hasVariable(varName.c_str())); + } else if (rrv) { + configureVariable(*_domains, var, *rrv); + } + if (rrv) { vars.add(*rrv); } } diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index 4be7217da153a..a1f288b618187 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -2991,3 +2991,59 @@ TEST(RooFitHS3, DefaultDomainIsOptional) const std::string exported = RooJSONFactoryWSTool{ws}.exportJSONtoString(); EXPECT_NE(exported.find(R"("name":"default_domain")"), std::string::npos) << exported; } + +// Parameters that only appear in a parameter point are created on import, and +// categorical parameters can be set with a string value. +TEST(RooFitHS3, ParametersOnlyInParameterPoints) +{ + const std::string json = R"({ + "metadata": {"hs3_version": "0.2"}, + "domains": [ + {"name": "default_domain", "type": "product_domain", "axes": [ + {"name": "free", "min": -1.0, "max": 1.0}, {"name": "freeButConst", "min": -1.0, "max": 1.0}]} + ], + "parameter_points": [ + { + "name": "point", + "parameters": [ + {"name": "fixed", "value": 2.5}, + {"name": "fixedExplicitFalse", "value": 1.5, "const": false}, + {"name": "free", "value": 0.25}, + {"name": "freeButConst", "value": 0.5, "const": true}, + {"name": "channel", "value": "CR"} + ] + } + ] + })"; + + RooWorkspace ws; + RooCategory channel{"channel", "channel", {{"SR", 0}, {"CR", 1}}}; + ws.import(channel); + ASSERT_TRUE(RooJSONFactoryWSTool{ws}.importJSONfromString(json)); + + RooRealVar *fixed = ws.var("fixed"); + ASSERT_NE(fixed, nullptr); + EXPECT_DOUBLE_EQ(fixed->getVal(), 2.5); + EXPECT_TRUE(fixed->isConstant()); + + RooRealVar *free = ws.var("free"); + ASSERT_NE(free, nullptr); + EXPECT_DOUBLE_EQ(free->getVal(), 0.25); + EXPECT_FALSE(free->isConstant()); + + // "const": false is the same as no "const": without a domain, the parameter stays constant. + RooRealVar *fixedExplicitFalse = ws.var("fixedExplicitFalse"); + ASSERT_NE(fixedExplicitFalse, nullptr); + EXPECT_TRUE(fixedExplicitFalse->isConstant()); + + // "const": true makes a parameter constant also if a domain lists it. + RooRealVar *freeButConst = ws.var("freeButConst"); + ASSERT_NE(freeButConst, nullptr); + EXPECT_TRUE(freeButConst->isConstant()); + + const RooArgSet *snapshot = ws.getSnapshot("point"); + ASSERT_NE(snapshot, nullptr); + auto *channelInSnapshot = dynamic_cast(snapshot->find("channel")); + ASSERT_NE(channelInSnapshot, nullptr); + EXPECT_EQ(channelInSnapshot->getCurrentLabel(), std::string("CR")); +} From 01f95314f71402d375c37eb4d848df8cd8d0d0f5 Mon Sep 17 00:00:00 2001 From: Carsten Burgard Date: Sun, 25 Oct 2026 19:29:38 +0100 Subject: [PATCH 05/35] [RF][HS3] Don't write binning on the axes of unbinned data HS3 forbids `nbins` and `edges` on the axes of unbinned data, but the unbinned-data exporter used the shared binning writer. It now writes only `name`, `min` and `max` for each axis. The binning of the observables is still preserved in the domains. Assisted-by: claude-code:claude-opus-5-5 --- roofit/hs3/src/RooJSONFactoryWSTool.cxx | 7 +++++- roofit/hs3/test/testRooFitHS3.cxx | 33 +++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/roofit/hs3/src/RooJSONFactoryWSTool.cxx b/roofit/hs3/src/RooJSONFactoryWSTool.cxx index 873af24b732bd..5db5036bcc242 100644 --- a/roofit/hs3/src/RooJSONFactoryWSTool.cxx +++ b/roofit/hs3/src/RooJSONFactoryWSTool.cxx @@ -1498,7 +1498,12 @@ void RooJSONFactoryWSTool::exportData(RooAbsData const &data) auto &observablesNode = output["axes"].set_seq(); for (auto *var : static_range_cast(variables)) { _domains->readVariable(*var); - exportAxis(observablesNode.append_child().set_map(), *var); + // HS3 forbids binning information on the axes of unbinned data. + auto &axis = observablesNode.append_child().set_map(); + testValidName(var->GetName(), false); + axis["name"] << var->GetName(); + axis["min"] << var->getMin(); + axis["max"] << var->getMax(); } auto &coords = output["entries"].set_seq(); std::vector weightVals; diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index a1f288b618187..786f0975074f5 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -3047,3 +3047,36 @@ TEST(RooFitHS3, ParametersOnlyInParameterPoints) ASSERT_NE(channelInSnapshot, nullptr); EXPECT_EQ(channelInSnapshot->getCurrentLabel(), std::string("CR")); } + +// HS3 forbids binning information on the axes of unbinned data, regardless of +// the binning of the observable in RooFit. +TEST(RooFitHS3, UnbinnedDatasetAxesHaveNoBinning) +{ + RooWorkspace ws; + RooRealVar x{"x", "x", 0.0, 10.0}; + RooRealVar y{"y", "y", 0.0, 3.0}; + x.setBins(5); + std::vector edges{0.0, 1.0, 3.0}; + y.setBinning(RooBinning{2, edges.data()}); + RooDataSet ds{"ds", "ds", RooArgSet{x, y}}; + for (double val : {1.0, 2.0, 2.5}) { + x.setVal(val); + y.setVal(val); + ds.add(RooArgSet{x, y}); + } + ws.import(ds, RooFit::Silence()); + + const std::string json = RooJSONFactoryWSTool{ws}.exportJSONtoString(); + auto tree = RooFit::Detail::JSONTree::create(json); + const auto *dataNode = RooJSONFactoryWSTool::findNamedChild(tree->rootnode()["data"], "ds"); + ASSERT_NE(dataNode, nullptr) << json; + EXPECT_EQ((*dataNode)["type"].val(), "unbinned"); + ASSERT_EQ((*dataNode)["axes"].num_children(), 2u); + for (const auto &axis : (*dataNode)["axes"].children()) { + EXPECT_TRUE(axis.has_child("name")); + EXPECT_TRUE(axis.has_child("min")); + EXPECT_TRUE(axis.has_child("max")); + EXPECT_FALSE(axis.has_child("nbins")) << json; + EXPECT_FALSE(axis.has_child("edges")) << json; + } +} From e2d46d06d83ecedfa91c49c55c69b7ff8d8562a0 Mon Sep 17 00:00:00 2001 From: Carsten Burgard Date: Sun, 25 Oct 2026 19:30:22 +0100 Subject: [PATCH 06/35] [RF][HS3] Write and read the "init" parameter point of analyses On export, if a ModelConfig has a snapshot, the analysis now references the parameter point of that snapshot as `init`. On import, the snapshot is re-created under the same name so that repeated round trips are stable, and an `init` that refers to a parameter point that doesn't exist is reported with a warning instead of being skipped silently. (All parameter points are imported before the analyses, so their order in the file does not matter.) Assisted-by: claude-code:claude-opus-5-5 --- roofit/hs3/src/RooJSONFactoryWSTool.cxx | 40 ++++++++++++++++- roofit/hs3/test/testRooFitHS3.cxx | 58 +++++++++++++++++++++++++ 2 files changed, 96 insertions(+), 2 deletions(-) diff --git a/roofit/hs3/src/RooJSONFactoryWSTool.cxx b/roofit/hs3/src/RooJSONFactoryWSTool.cxx index 5db5036bcc242..17cb94c65a042 100644 --- a/roofit/hs3/src/RooJSONFactoryWSTool.cxx +++ b/roofit/hs3/src/RooJSONFactoryWSTool.cxx @@ -631,8 +631,28 @@ void importAnalysis(const JSONNode &rootnode, const JSONNode &analysisNode, cons } } - if (analysisNode.has_child("init") && workspace.getSnapshot(analysisNode["init"].val().c_str())) { - mc->SetSnapshot(*workspace.getSnapshot(analysisNode["init"].val().c_str())); + if (analysisNode.has_child("init")) { + const std::string initName = analysisNode["init"].val(); + RooArgSet const *init = workspace.getSnapshot(initName.c_str()); + if (!init) { + RooJSONFactoryWSTool::warning("RooFitHS3: the parameter point '" + initName + + "' used as \"init\" of analysis '" + analysisName + "' does not exist."); + return; + } + // ModelConfig::SetSnapshot() saves the snapshot as "__snapshot". If "init" already follows + // this pattern, which is the case for files written by RooFit, pick the set name such that it is preserved. + std::string setName = initName; + const std::string prefix = mcname + "_"; + const std::string suffix = "_snapshot"; + if (initName.size() >= prefix.size() + suffix.size() && startsWith(initName, prefix) && + endsWith(initName, suffix)) { + setName = initName.substr(prefix.size(), initName.size() - prefix.size() - suffix.size()); + } + // Take an owning copy: SetSnapshot() might replace the workspace snapshot that `init` points to. + RooArgSet initSet; + init->snapshot(initSet); + initSet.setName(setName.c_str()); + mc->SetSnapshot(initSet); } } @@ -1772,6 +1792,22 @@ void RooJSONFactoryWSTool::exportSingleModelConfig(JSONNode &rootnode, RooFit::M writeProductDomain("_global_observables", mc.GetGlobalObservables()); writeProductDomain("_parameters_of_interest", mc.GetParametersOfInterest()); + // ModelConfig::SetSnapshot() saves the snapshot as "__snapshot", and the snapshots are exported + // as parameter points. Reference it as the initial values of the analysis. + if (std::unique_ptr snapshot{mc.GetSnapshot()}) { + const std::string prefix = std::string(mc.GetName()) + "_"; + std::vector candidates; + for (TObject *snsh : _workspace.getSnapshots()) { + const std::string snshName = snsh->GetName(); + if (startsWith(snshName, prefix) && endsWith(snshName, "_snapshot") && _workspace.set(snshName)) { + candidates.push_back(snshName); + } + } + if (candidates.size() == 1) { + analysisNode["init"] << candidates.front(); + } + } + auto &modelConfigAux = getRooFitInternal(rootnode, "ModelConfigs", analysisName); modelConfigAux.set_map(); modelConfigAux["pdfName"] << pdf->GetName(); diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index 786f0975074f5..6cdcd32b0a0c1 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -3080,3 +3080,61 @@ TEST(RooFitHS3, UnbinnedDatasetAxesHaveNoBinning) EXPECT_FALSE(axis.has_child("edges")) << json; } } + +// The snapshot of a ModelConfig is exported as the "init" parameter point of +// the analysis, and restored on import. +TEST(RooFitHS3, AnalysisInitRoundTrip) +{ + RooWorkspace ws1; + ws1.factory("Gaussian::pdf(x[0, -5, 5], mu[0, -5, 5], 1.0)"); + RooFit::ModelConfig mc1{"ModelConfig", &ws1}; + mc1.SetPdf("pdf"); + mc1.SetParametersOfInterest("mu"); + mc1.SetObservables("x"); + ws1.var("mu")->setVal(1.5); + RooArgSet initSet{*ws1.var("mu")}; + initSet.setName("POI"); + mc1.SetSnapshot(initSet); + ws1.var("mu")->setVal(0.0); + ws1.import(mc1); + + const std::string json1 = RooJSONFactoryWSTool{ws1}.exportJSONtoString(); + EXPECT_NE(json1.find(R"("init":"ModelConfig_POI_snapshot")"), std::string::npos) << json1; + + RooWorkspace ws2; + ASSERT_TRUE(RooJSONFactoryWSTool{ws2}.importJSONfromString(json1)); + auto *mc2 = dynamic_cast(ws2.obj("ModelConfig")); + ASSERT_NE(mc2, nullptr); + std::unique_ptr snapshot{mc2->GetSnapshot()}; + ASSERT_NE(snapshot, nullptr); + auto *mu = dynamic_cast(snapshot->find("mu")); + ASSERT_NE(mu, nullptr); + EXPECT_DOUBLE_EQ(mu->getVal(), 1.5); + + // The snapshot name must be stable under repeated round trips. + EXPECT_EQ(RooJSONFactoryWSTool{ws2}.exportJSONtoString(), json1); + + // An "init" that refers to a missing parameter point is reported. + std::string missingInit = json1; + missingInit.replace(missingInit.find(R"("init":"ModelConfig_POI_snapshot")"), + std::string(R"("init":"ModelConfig_POI_snapshot")").size(), R"("init":"does_not_exist")"); + RooWorkspace ws3; + const std::string warnings = captureMessages( + RooFit::WARNING, RooFit::IO, [&] { EXPECT_TRUE(RooJSONFactoryWSTool{ws3}.importJSONfromString(missingInit)); }); + EXPECT_NE(warnings.find("'does_not_exist' used as \"init\""), std::string::npos) << warnings; + + // The snapshot of an unnamed set is called "__snapshot", which must be stable as well. + RooWorkspace ws4; + ws4.factory("Gaussian::pdf(x[0, -5, 5], mu[0, -5, 5], 1.0)"); + RooFit::ModelConfig mc4{"ModelConfig", &ws4}; + mc4.SetPdf("pdf"); + mc4.SetParametersOfInterest("mu"); + mc4.SetObservables("x"); + mc4.SetSnapshot(RooArgSet{*ws4.var("mu")}); + ws4.import(mc4); + const std::string json4 = RooJSONFactoryWSTool{ws4}.exportJSONtoString(); + EXPECT_NE(json4.find(R"("init":"ModelConfig__snapshot")"), std::string::npos) << json4; + RooWorkspace ws5; + ASSERT_TRUE(RooJSONFactoryWSTool{ws5}.importJSONfromString(json4)); + EXPECT_EQ(RooJSONFactoryWSTool{ws5}.exportJSONtoString(), json4); +} From 620afdbfa1574bcb690ec71f0e0f2322b1796bab Mon Sep 17 00:00:00 2001 From: Carsten Burgard Date: Sun, 25 Oct 2026 19:32:27 +0100 Subject: [PATCH 07/35] [RF][HS3] Align type keys and fields with the HS3 catalog * RooArgusBG is exported as `argus_dist`. `ARGUS_dist` is still imported, with a one-time deprecation warning. * RooBinSamplingPdf is exported as `binsampling_dist`, the key its importer was registered for, so it can be read back. `binsampling` is still imported, with a one-time deprecation warning. * `rate_extended_dist` uses the HS3 fields `rate` and `dist`. `norm` and `pdf` are still accepted on import. RooFormulaVar and RooGenericPdf were already exported as `generic` and `generic_dist`, with `generic_function` still importable, so they needed no change. The Crystal Ball keys are left unchanged here; they follow a pending change of the HS3 standard. Assisted-by: claude-code:claude-opus-5-5 --- roofit/hs3/src/JSONFactories_RooFitCore.cxx | 39 ++++++++++-- roofit/hs3/src/RooFitHS3_wsexportkeys.cxx | 2 +- .../src/RooFitHS3_wsfactoryexpressions.cxx | 9 +++ roofit/hs3/test/testRooFitHS3.cxx | 61 +++++++++++++++++++ 4 files changed, 105 insertions(+), 6 deletions(-) diff --git a/roofit/hs3/src/JSONFactories_RooFitCore.cxx b/roofit/hs3/src/JSONFactories_RooFitCore.cxx index f809bbe07fcc1..2891d97bdb603 100644 --- a/roofit/hs3/src/JSONFactories_RooFitCore.cxx +++ b/roofit/hs3/src/JSONFactories_RooFitCore.cxx @@ -515,8 +515,9 @@ bool importFFTConvPdf(RooJSONFactoryWSTool *tool, const JSONNode &p) bool importExtendPdf(RooJSONFactoryWSTool *tool, const JSONNode &p) { std::string name(RooJSONFactoryWSTool::name(p)); - RooAbsPdf *pdf = tool->requestArg(p, "pdf"); - RooAbsReal *norm = tool->requestArg(p, "norm"); + // HS3 calls the fields "dist" and "rate". RooFit used to write "pdf" and "norm", which are still accepted. + RooAbsPdf *pdf = tool->requestArg(p, p.has_child("pdf") && !p.has_child("dist") ? "pdf" : "dist"); + RooAbsReal *norm = tool->requestArg(p, p.has_child("norm") && !p.has_child("rate") ? "norm" : "rate"); if (p.has_child("range")) { std::string rangeName = p["range"].val(); tool->wsEmplace(name, *pdf, *norm, rangeName.c_str()); @@ -715,6 +716,30 @@ bool importSpline(RooJSONFactoryWSTool *tool, const JSONNode &p) return true; } +// Importer that is registered with top priority for a deprecated type key. It warns once, and then lets the importers +// or the factory expression that are registered for the deprecated key do the actual work. +class DeprecatedKeyImporter : public RooFit::JSONIO::Importer { +public: + DeprecatedKeyImporter(std::string key, std::string replacement) + : _key{std::move(key)}, _replacement{std::move(replacement)} + { + } + bool importArg(RooJSONFactoryWSTool *, const JSONNode &) const override + { + if (!_warned) { + RooJSONFactoryWSTool::warning("RooFitHS3: the type \"" + _key + "\" is deprecated, please use \"" + + _replacement + "\" instead."); + _warned = true; + } + return false; + } + +private: + const std::string _key; + const std::string _replacement; + mutable bool _warned = false; +}; + /////////////////////////////////////////////////////////////////////////////////////////////////////// // specialized exporter implementations /////////////////////////////////////////////////////////////////////////////////////////////////////// @@ -1038,8 +1063,8 @@ bool exportExtendPdf(RooJSONFactoryWSTool *, const RooAbsArg *func, JSONNode &el if (auto rangeName = pdf->getRangeName()) { elem["range"] << rangeName->GetName(); } - elem["pdf"] << pdf->pdf().GetName(); - elem["norm"] << pdf->getN().GetName(); + elem["dist"] << pdf->pdf().GetName(); + elem["rate"] << pdf->getN().GetName(); return true; } @@ -1178,12 +1203,16 @@ void registerExporter(TClass const *cl, std::string key, bool topPriority = true STATIC_EXECUTE([]() { registerImporter("density_function_dist"); registerImporter("rate_extended_dist"); + RooFit::JSONIO::registerImporter("ARGUS_dist", std::make_unique("ARGUS_dist", "argus_dist")); + RooFit::JSONIO::registerImporter("binsampling", + std::make_unique("binsampling", "binsampling_dist")); registerImporter("product", false); registerImporter("product_dist", false); registerImporter("sum", false); registerImporter("mixture_dist", false); registerImporter("mixture_resolution_model", false); registerImporter("binsampling_dist", false); + registerImporter("binsampling", false); registerImporter>("binvolume", false); registerImporter>("inverse_binvolume", false); registerImporter>("legacy_exp_poly_dist", false); @@ -1214,7 +1243,7 @@ STATIC_EXECUTE([]() { registerExporter(RooWrapperPdf::Class(), "density_function_dist"); registerExporter>(RooAddPdf::Class(), "mixture_dist", false); registerExporter>(RooAddModel::Class(), "mixture_resolution_model", false); - registerExporter(RooBinSamplingPdf::Class(), "binsampling", false); + registerExporter(RooBinSamplingPdf::Class(), "binsampling_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 20423610811ac..fec22e4350490 100644 --- a/roofit/hs3/src/RooFitHS3_wsexportkeys.cxx +++ b/roofit/hs3/src/RooFitHS3_wsexportkeys.cxx @@ -8,7 +8,7 @@ auto RooFitHS3_wsexportkeys = R"({ } }, "RooArgusBG": { - "type": "ARGUS_dist", + "type": "argus_dist", "proxies": { "m": "mass", "m0": "resonance", diff --git a/roofit/hs3/src/RooFitHS3_wsfactoryexpressions.cxx b/roofit/hs3/src/RooFitHS3_wsfactoryexpressions.cxx index 6076465bcb8ea..a938956e564c2 100644 --- a/roofit/hs3/src/RooFitHS3_wsfactoryexpressions.cxx +++ b/roofit/hs3/src/RooFitHS3_wsfactoryexpressions.cxx @@ -1,6 +1,15 @@ namespace { auto RooFitHS3_wsfactoryexpressions = R"({ + "argus_dist": { + "class": "RooArgusBG", + "arguments": [ + "mass", + "resonance", + "slope", + "power" + ] + }, "ARGUS_dist": { "class": "RooArgusBG", "arguments": [ diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index 6cdcd32b0a0c1..5303d6e21af3f 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -10,8 +10,10 @@ #include #include #include +#include #include #include +#include #include #include #include @@ -3138,3 +3140,62 @@ TEST(RooFitHS3, AnalysisInitRoundTrip) ASSERT_TRUE(RooJSONFactoryWSTool{ws5}.importJSONfromString(json4)); EXPECT_EQ(RooJSONFactoryWSTool{ws5}.exportJSONtoString(), json4); } + +// Type keys follow the HS3 catalog. The old RooFit-specific keys are still accepted on import. +TEST(RooFitHS3, TypeKeysMatchTheCatalog) +{ + RooHelpers::LocalChangeMsgLevel changeMsgLvl(RooFit::WARNING); + + RooRealVar x{"x", "x", 0.0, 10.0}; + x.setBins(10); + RooRealVar mean{"mean", "mean", 5.0, 0.0, 10.0}; + RooRealVar sigma{"sigma", "sigma", 1.0, 0.1, 5.0}; + + // Import a single distribution from an HS3 document. + auto importPdf = [](std::string const &distribution) { + auto ws = std::make_unique(); + RooJSONFactoryWSTool{*ws}.importJSONfromString(R"({ + "metadata": {"hs3_version": "0.2"}, + "domains": [{"name": "default_domain", "type": "product_domain", "axes": [{"name": "m", "min": 0, "max": 10}]}], + "distributions": [)" + distribution + + R"(] + })"); + return ws; + }; + + // argus_dist is the HS3 key, ARGUS_dist is deprecated. + { + RooWorkspace ws; + ws.factory("ArgusBG::argus(x[0, 10], 9.0, -1.0, 0.5)"); + const std::string json = RooJSONFactoryWSTool{ws}.exportJSONtoString(); + EXPECT_NE(json.find(R"("type":"argus_dist")"), std::string::npos) << json; + } + std::unique_ptr argus; + const std::string argusWarnings = captureMessages(RooFit::WARNING, RooFit::IO, [&] { + argus = importPdf( + R"({"name": "argus", "type": "ARGUS_dist", "mass": "m", "resonance": 9.0, "slope": -1.0, "power": 0.5})"); + }); + EXPECT_NE(argus->pdf("argus"), nullptr); + EXPECT_NE(argusWarnings.find(R"(the type "ARGUS_dist" is deprecated)"), std::string::npos) << argusWarnings; + + // RooBinSamplingPdf is exported and imported with the same key. + RooGaussian gauss{"gauss", "", x, mean, sigma}; + RooBinSamplingPdf binSampling{"binSampling", "", x, gauss, 1e-3}; + EXPECT_EQ(validate(binSampling), 0); + + // rate_extended_dist uses the HS3 field names "rate" and "dist". + RooRealVar nEvents{"nEvents", "nEvents", 100.0, 0.0, 1000.0}; + RooExtendPdf extended{"extended", "", gauss, nEvents}; + EXPECT_EQ(validate(extended), 0); + { + RooWorkspace ws; + ws.import(extended, RooFit::Silence()); + const std::string json = RooJSONFactoryWSTool{ws}.exportJSONtoString(); + EXPECT_NE(json.find(R"("dist":"gauss")"), std::string::npos) << json; + EXPECT_NE(json.find(R"("rate":"nEvents")"), std::string::npos) << json; + EXPECT_EQ(json.find(R"("range")"), std::string::npos) << json; + } + auto legacyExtended = importPdf(R"({"name": "ext", "type": "rate_extended_dist", "pdf": "g", "norm": 10.0}, + {"name": "g", "type": "gaussian_dist", "x": "m", "mean": 5.0, "sigma": 1.0})"); + EXPECT_NE(dynamic_cast(legacyExtended->pdf("ext")), nullptr); +} From 0f3312989a6f5b2d1007e7ea5f8bbabdc8d97211 Mon Sep 17 00:00:00 2001 From: Carsten Burgard Date: Sun, 25 Oct 2026 19:34:10 +0100 Subject: [PATCH 08/35] [RF][HS3] Export TFormula ternaries as the HS3 ifelse() conditional HS3 expresses conditionals as `ifelse(condition, a, b)`, but RooFit wrote the C-style ternary `c ? a : b` that TFormula uses. The generic formula exporter now rewrites ternaries at any nesting level into `ifelse()`. If an expression can't be rewritten safely, a warning is printed and the expression is exported unchanged. TFormula doesn't understand `ifelse()`, so on import it is translated back into a parenthesized ternary. Without this, RooFit could not read its own output. Expressions without ternaries are exported unchanged. Assisted-by: claude-code:claude-opus-5-5 --- roofit/hs3/src/JSONFactories_RooFitCore.cxx | 219 ++++++++++++++++++++ roofit/hs3/test/testRooFitHS3.cxx | 37 ++++ 2 files changed, 256 insertions(+) diff --git a/roofit/hs3/src/JSONFactories_RooFitCore.cxx b/roofit/hs3/src/JSONFactories_RooFitCore.cxx index 2891d97bdb603..74b7e2253ad5e 100644 --- a/roofit/hs3/src/JSONFactories_RooFitCore.cxx +++ b/roofit/hs3/src/JSONFactories_RooFitCore.cxx @@ -161,10 +161,102 @@ void replaceIdentifier(TString &expr, std::string_view identifier, std::string_v expr = out.c_str(); } +bool isIdentifierChar(char c) +{ + return std::isalnum(static_cast(c)) || c == '_'; +} + +std::string trimmed(std::string const &str) +{ + const auto begin = str.find_first_not_of(" \t\n"); + if (begin == std::string::npos) { + return ""; + } + return str.substr(begin, str.find_last_not_of(" \t\n") - begin + 1); +} + +// Index of the bracket that closes the one at `open`, or std::string::npos if it is not closed. +std::size_t findClosingBracket(std::string const &str, std::size_t open) +{ + int depth = 0; + for (std::size_t i = open; i < str.size(); ++i) { + if (str[i] == '(' || str[i] == '[') { + ++depth; + } else if ((str[i] == ')' || str[i] == ']') && --depth == 0) { + return i; + } + } + return std::string::npos; +} + +// Split `str` at the commas that are not enclosed in brackets. +std::vector splitTopLevelCommas(std::string const &str) +{ + std::vector parts; + int depth = 0; + std::size_t start = 0; + for (std::size_t i = 0; i < str.size(); ++i) { + if (str[i] == '(' || str[i] == '[') { + ++depth; + } else if (str[i] == ')' || str[i] == ']') { + --depth; + } else if (str[i] == ',' && depth == 0) { + parts.push_back(str.substr(start, i - start)); + start = i + 1; + } + } + parts.push_back(str.substr(start)); + return parts; +} + +// Remove parentheses that enclose the whole string, e.g. "((a+b))" becomes "a+b". +std::string stripEnclosingParentheses(std::string str) +{ + str = trimmed(str); + while (str.size() >= 2 && str.front() == '(' && findClosingBracket(str, 0) == str.size() - 1) { + str = trimmed(str.substr(1, str.size() - 2)); + } + return str; +} + +// Rewrite the HS3 conditional "ifelse(c,a,b)" into the C-style ternary "((c)?(a):(b))" that TFormula understands. +// Malformed calls are left unchanged, so that TFormula reports them. +std::string ifElseToTernary(std::string const &expr) +{ + static const std::string keyword = "ifelse"; + std::string out; + std::size_t pos = 0; + while (pos < expr.size()) { + const std::size_t found = expr.find(keyword, pos); + if (found == std::string::npos) { + break; + } + const std::size_t open = expr.find_first_not_of(" \t", found + keyword.size()); + const bool isCall = + (found == 0 || !isIdentifierChar(expr[found - 1])) && open != std::string::npos && expr[open] == '('; + const std::size_t close = isCall ? findClosingBracket(expr, open) : std::string::npos; + const auto args = close != std::string::npos ? splitTopLevelCommas(expr.substr(open + 1, close - open - 1)) + : std::vector{}; + if (args.size() != 3) { + out += expr.substr(pos, found + keyword.size() - pos); + pos = found + keyword.size(); + continue; + } + out += expr.substr(pos, found - pos); + out += "((" + ifElseToTernary(trimmed(args[0])) + ")?(" + ifElseToTernary(trimmed(args[1])) + "):(" + + ifElseToTernary(trimmed(args[2])) + "))"; + pos = close + 1; + } + return out + expr.substr(pos); +} + void translateImportedExpression(TString &expr) { replaceIdentifier(expr, "PI", "TMath::Pi()"); replaceIdentifier(expr, "EULER", "TMath::E()"); + if (std::string(expr.Data()).find("ifelse") != std::string::npos) { + expr = ifElseToTernary(expr.Data()).c_str(); + } } int readPositiveInteger(const JSONNode &node, const std::string &context) @@ -840,6 +932,123 @@ void cleanExpression(TString &expr) replaceIdentifier(expr, "TMath::E()", "EULER"); } +bool ternariesToIfElse(std::string const &in, std::string &out); + +// Rewrite one comma-free part of an expression, see ternariesToIfElse(). +bool ternaryPartToIfElse(std::string const &in, std::string &out) +{ + // Find the first '?' that is not enclosed in brackets. A ':' that is not part of "::" can only follow a '?'. + std::size_t question = std::string::npos; + int depth = 0; + for (std::size_t i = 0; i < in.size() && question == std::string::npos; ++i) { + const char c = in[i]; + if (c == '(' || c == '[') { + ++depth; + } else if (c == ')' || c == ']') { + --depth; + } else if (c == ':' && i + 1 < in.size() && in[i + 1] == ':') { + ++i; + } else if (depth == 0 && c == ':') { + return false; + } else if (depth == 0 && c == '?') { + question = i; + } else if (depth == 0 && c == ',') { + return false; + } + } + + if (question == std::string::npos) { + // No ternary on this level: copy the expression and rewrite the ternaries inside of brackets. + out.clear(); + for (std::size_t i = 0; i < in.size(); ++i) { + if (in[i] == ')' || in[i] == ']') { + return false; + } + if (in[i] != '(' && in[i] != '[') { + out += in[i]; + continue; + } + const std::size_t close = findClosingBracket(in, i); + if (close == std::string::npos) { + return false; + } + const std::string inner = in.substr(i + 1, close - i - 1); + std::string innerOut; + if (!ternariesToIfElse(inner, innerOut)) { + return false; + } + // Parentheses that only group a ternary are redundant around the ifelse() function call. + const std::string before = trimmed(in.substr(0, i)); + const bool isGrouping = in[i] == '(' && (before.empty() || !isIdentifierChar(before.back())); + if (isGrouping && innerOut != trimmed(inner) && startsWith(innerOut, "ifelse(") && + findClosingBracket(innerOut, std::string("ifelse").size()) == innerOut.size() - 1) { + out += innerOut; + } else { + out += in[i] + innerOut + in[close]; + } + i = close; + } + return true; + } + + // Find the ':' that belongs to this '?', taking nested ternaries into account. + std::size_t colon = std::string::npos; + int nested = 0; + depth = 0; + for (std::size_t i = question + 1; i < in.size() && colon == std::string::npos; ++i) { + const char c = in[i]; + if (c == '(' || c == '[') { + ++depth; + } else if (c == ')' || c == ']') { + --depth; + } else if (c == ':' && i + 1 < in.size() && in[i + 1] == ':') { + ++i; + } else if (depth == 0 && c == '?') { + ++nested; + } else if (depth == 0 && c == ':') { + if (nested == 0) { + colon = i; + } else { + --nested; + } + } + } + if (colon == std::string::npos) { + return false; + } + + std::string condition; + std::string ifTrue; + std::string ifFalse; + if (!ternaryPartToIfElse(stripEnclosingParentheses(in.substr(0, question)), condition) || + !ternaryPartToIfElse(stripEnclosingParentheses(in.substr(question + 1, colon - question - 1)), ifTrue) || + !ternaryPartToIfElse(stripEnclosingParentheses(in.substr(colon + 1)), ifFalse)) { + return false; + } + if (condition.empty() || ifTrue.empty() || ifFalse.empty()) { + return false; + } + out = "ifelse(" + condition + "," + ifTrue + "," + ifFalse + ")"; + return true; +} + +// Rewrite the C-style ternaries "c ? a : b" in a TFormula expression into the HS3 conditional "ifelse(c,a,b)". Returns +// false if this is not safely possible, e.g. because of unbalanced brackets. +bool ternariesToIfElse(std::string const &in, std::string &out) +{ + out.clear(); + bool first = true; + for (std::string const &part : splitTopLevelCommas(in)) { + std::string partOut; + if (!ternaryPartToIfElse(part, partOut)) { + return false; + } + out += (first ? "" : ",") + partOut; + first = false; + } + return true; +} + template bool exportFormulaArg(RooJSONFactoryWSTool *, const RooAbsArg *func, JSONNode &elem, std::string const &key) { @@ -858,6 +1067,16 @@ bool exportFormulaArg(RooJSONFactoryWSTool *, const RooAbsArg *func, JSONNode &e expression.ReplaceAll(("x[" + std::to_string(idx) + "]").c_str(), par->GetName()); expression.ReplaceAll(("@" + std::to_string(idx)).c_str(), par->GetName()); } + if (expression.Contains("?")) { + std::string rewritten; + if (ternariesToIfElse(expression.Data(), rewritten)) { + expression = rewritten.c_str(); + } else { + RooJSONFactoryWSTool::warning(std::string("RooFitHS3: could not translate the ternary operators in the " + "expression of '") + + func->GetName() + "' to ifelse(), exporting the expression unchanged."); + } + } elem["expression"] << expression.Data(); for (const RooAbsArg *dependent : pdf->dependents()) { diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index 5303d6e21af3f..b283add7dca18 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -3199,3 +3199,40 @@ TEST(RooFitHS3, TypeKeysMatchTheCatalog) {"name": "g", "type": "gaussian_dist", "x": "m", "mean": 5.0, "sigma": 1.0})"); EXPECT_NE(dynamic_cast(legacyExtended->pdf("ext")), nullptr); } + +// TFormula ternaries are exported as the HS3 conditional ifelse(), and ifelse() +// is translated back on import. +TEST(RooFitHS3, TernaryIsExportedAsIfElse) +{ + RooRealVar x{"x", "x", 0.5, -2.0, 2.0}; + RooRealVar y{"y", "y", -0.5, -2.0, 2.0}; + RooFormulaVar formula{"formula", "2 * (x > 0 ? (y > 0 ? x * y : -x) : 3 * x) + 1", RooArgList{x, y}}; + + RooWorkspace ws1; + ws1.import(formula, RooFit::Silence()); + const std::string json1 = RooJSONFactoryWSTool{ws1}.exportJSONtoString(); + EXPECT_NE(json1.find(R"("expression":"2 * ifelse(x > 0,ifelse(y > 0,x * y,-x),3 * x) + 1")"), std::string::npos) + << json1; + EXPECT_EQ(json1.find('?'), std::string::npos) << json1; + + RooWorkspace ws2; + ASSERT_TRUE(RooJSONFactoryWSTool{ws2}.importJSONfromString(json1)); + EXPECT_EQ(RooJSONFactoryWSTool{ws2}.exportJSONtoString(), json1); + for (double xVal : {-1.0, 0.5}) { + for (double yVal : {-1.0, 0.5}) { + ws1.var("x")->setVal(xVal); + ws1.var("y")->setVal(yVal); + ws2.var("x")->setVal(xVal); + ws2.var("y")->setVal(yVal); + EXPECT_DOUBLE_EQ(ws2.function("formula")->getVal(), ws1.function("formula")->getVal()) + << "x=" << xVal << ", y=" << yVal; + } + } + + // Expressions without ternaries are not changed. + RooFormulaVar plain{"plain", "(x + y) * (x - y)", RooArgList{x, y}}; + RooWorkspace ws3; + ws3.import(plain, RooFit::Silence()); + const std::string json3 = RooJSONFactoryWSTool{ws3}.exportJSONtoString(); + EXPECT_NE(json3.find(R"json("expression":"(x + y) * (x - y)")json"), std::string::npos) << json3; +} From 896224f62b1328d2b265ebe71385856547e77ea3 Mon Sep 17 00:00:00 2001 From: Carsten Burgard Date: Sun, 25 Oct 2026 19:34:50 +0100 Subject: [PATCH 09/35] [RF][HS3] Check the "extended" flag of imported mixture distributions The `extended` field of `mixture_dist` was ignored on import; the RooAddPdf derived its extended mode from the number of coefficients only. If the flag is present, it is now checked against the coefficient count: * `extended: true` with one coefficient fewer than summands is an error (HS3 requires such a mixture to be non-extended); * `extended: false` without coefficients is an error (RooFit would build an extended sum of extended distributions); * `extended: false` with one coefficient per summand is valid HS3, but a RooAddPdf cannot be forced to be non-extendable, so a warning advises not to use the extended term when fitting. Assisted-by: claude-code:claude-opus-5-5 --- roofit/hs3/src/JSONFactories_RooFitCore.cxx | 22 ++++++++++++ roofit/hs3/test/testRooFitHS3.cxx | 40 +++++++++++++++++++++ 2 files changed, 62 insertions(+) diff --git a/roofit/hs3/src/JSONFactories_RooFitCore.cxx b/roofit/hs3/src/JSONFactories_RooFitCore.cxx index 74b7e2253ad5e..805b60fd807b6 100644 --- a/roofit/hs3/src/JSONFactories_RooFitCore.cxx +++ b/roofit/hs3/src/JSONFactories_RooFitCore.cxx @@ -412,6 +412,28 @@ bool importAddition(RooJSONFactoryWSTool *tool, const JSONNode &p) bool importAddPdf(RooJSONFactoryWSTool *tool, const JSONNode &p) { std::string name(RooJSONFactoryWSTool::name(p)); + if (p.has_child("extended")) { + // Check the explicit "extended" flag against the number of coefficients, which determines the extended mode of + // the RooAddPdf. + const bool extended = p["extended"].val_bool(); + const std::size_t nSummands = p.has_child("summands") ? p["summands"].num_children() : 0; + const std::size_t nCoefficients = p.has_child("coefficients") ? p["coefficients"].num_children() : 0; + const std::string counts = + std::to_string(nCoefficients) + " coefficients for " + std::to_string(nSummands) + " summands"; + if (extended && nCoefficients + 1 == nSummands) { + RooJSONFactoryWSTool::error("mixture_dist '" + name + "' is declared as extended, but it has " + counts + + ", which defines a non-extended mixture"); + } + if (!extended && nCoefficients == 0 && nSummands > 0) { + RooJSONFactoryWSTool::error("mixture_dist '" + name + "' is declared as not extended, but it has " + counts + + ", which defines an extended mixture of extended distributions"); + } + if (!extended && nCoefficients == nSummands) { + RooJSONFactoryWSTool::warning("RooFitHS3: mixture_dist '" + name + "' is declared as not extended, but " + + "RooFit can use it as extended because it has one coefficient per summand. " + + "Don't use the extended likelihood term when fitting it."); + } + } if (!tool->requestArgList(p, "coefficients").empty()) { tool->wsEmplace(name, tool->requestArgList(p, "summands"), tool->requestArgList(p, "coefficients")); diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index b283add7dca18..c35bab782d5e1 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -3236,3 +3236,43 @@ TEST(RooFitHS3, TernaryIsExportedAsIfElse) const std::string json3 = RooJSONFactoryWSTool{ws3}.exportJSONtoString(); EXPECT_NE(json3.find(R"json("expression":"(x + y) * (x - y)")json"), std::string::npos) << json3; } + +// The explicit "extended" flag of a mixture_dist has to be consistent with the number of coefficients. +TEST(RooFitHS3, MixtureDistExtendedFlag) +{ + auto makeJSON = [](std::string const &coefficients, std::string const &extended) { + return R"({ + "metadata": {"hs3_version": "0.2"}, + "domains": [{"name": "default_domain", "type": "product_domain", "axes": [{"name": "x", "min": -5, "max": 5}]}], + "parameter_points": [{"name": "default_values", "parameters": [{"name": "f", "value": 0.3}, + {"name": "n1", "value": 30}, + {"name": "n2", "value": 70}]}], + "distributions": [ + {"name": "g1", "type": "gaussian_dist", "x": "x", "mean": -1.0, "sigma": 1.0}, + {"name": "g2", "type": "gaussian_dist", "x": "x", "mean": 1.0, "sigma": 1.0}, + {"name": "mix", "type": "mixture_dist", "summands": ["g1", "g2"], "coefficients": )" + + coefficients + R"(, "extended": )" + extended + R"(} + ] + })"; + }; + + RooHelpers::LocalChangeMsgLevel changeMsgLvl(RooFit::WARNING); + + { + RooWorkspace ws; + ASSERT_TRUE(RooJSONFactoryWSTool{ws}.importJSONfromString(makeJSON(R"(["f"])", "false"))); + EXPECT_EQ(ws.pdf("mix")->extendMode(), RooAbsPdf::CanNotBeExtended); + } + { + RooWorkspace ws; + ASSERT_TRUE(RooJSONFactoryWSTool{ws}.importJSONfromString(makeJSON(R"(["n1", "n2"])", "true"))); + EXPECT_NE(ws.pdf("mix")->extendMode(), RooAbsPdf::CanNotBeExtended); + } + expectImportThrowsWithError(makeJSON(R"(["f"])", "true"), "is declared as extended, but it has 1 coefficients"); + + RooWorkspace ws; + const std::string warnings = captureMessages(RooFit::WARNING, RooFit::IO, [&] { + EXPECT_TRUE(RooJSONFactoryWSTool{ws}.importJSONfromString(makeJSON(R"(["n1", "n2"])", "false"))); + }); + EXPECT_NE(warnings.find("is declared as not extended"), std::string::npos) << warnings; +} From b36df0c9f909f8095b6cb21002604341df3829ba Mon Sep 17 00:00:00 2001 From: Carsten Burgard Date: Sun, 25 Oct 2026 19:35:12 +0100 Subject: [PATCH 10/35] [RF][HS3] HistFactory: recognize "LogNormal", use "parameter" of normfactors * HS3 spells the log-normal constraint type `LogNormal`. It is now recognized as a constraint type like the older RooFit spelling `Lognormal`, which is kept as an alias. * For a `normfactor` that has both `name` and `parameter`, the RooRealVar was named after `name`, breaking the correlation of normfactors that share a `parameter` but have different labels. The parameter is now named after `parameter`; `name` is only the label of the modifier. Assisted-by: claude-code:claude-opus-5-5 --- roofit/hs3/src/JSONFactories_HistFactory.cxx | 7 +++++-- roofit/hs3/test/testRooFitHS3.cxx | 18 ++++++++++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/roofit/hs3/src/JSONFactories_HistFactory.cxx b/roofit/hs3/src/JSONFactories_HistFactory.cxx index bb62724c95cb4..c49da5417d49b 100644 --- a/roofit/hs3/src/JSONFactories_HistFactory.cxx +++ b/roofit/hs3/src/JSONFactories_HistFactory.cxx @@ -441,7 +441,8 @@ std::string constraintName(std::string const ¶mName) bool isLegacyConstraintType(std::string const &value) { - return value == "Gauss" || value == "Poisson" || value == "Const" || value == "Lognormal"; + // HS3 spells "LogNormal", older RooFit versions used "Lognormal". + return value == "Gauss" || value == "Poisson" || value == "Const" || value == "LogNormal" || value == "Lognormal"; } RooAbsPdf *findNamedConstraint(RooJSONFactoryWSTool &tool, std::string const &constraintName, std::string const &sample) @@ -668,7 +669,9 @@ bool importHistSample(RooJSONFactoryWSTool &tool, RooDataHist &dh, RooArgSet con if (modtype == "staterror") { // this is dealt with at a different place, ignore it for now } else if (modtype == "normfactor") { - RooRealVar &constrParam = getOrCreate(ws, sysname, 1., -3, 5); + // 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); constrParam.setError(0.0); normElems.add(constrParam); if (mod.has_child("constraint") || mod.has_child("constraint_name") || mod.has_child("constraint_type")) { diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index c35bab782d5e1..59141a8194be8 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -3276,3 +3276,21 @@ TEST(RooFitHS3, MixtureDistExtendedFlag) }); EXPECT_NE(warnings.find("is declared as not extended"), std::string::npos) << warnings; } + +// The parameter of a normfactor is given by "parameter", the "name" is only a label. +// The HS3 spelling of the log-normal constraint type is recognized. +TEST(RooFitHS3, HistFactoryNormFactorParameterAndLogNormal) +{ + RooHelpers::LocalChangeMsgLevel changeMsgLvl(RooFit::WARNING); + + RooWorkspace ws; + ASSERT_TRUE(RooJSONFactoryWSTool{ws}.importJSONfromString( + makeHistFactoryJSON(R"({"name": "lumi_modifier", "parameter": "lumi", "type": "normfactor"})"))); + EXPECT_NE(ws.var("lumi"), nullptr); + EXPECT_EQ(ws.var("lumi_modifier"), nullptr); + + expectImportThrowsWithError( + makeHistFactoryJSON( + R"({"name": "sys", "parameter": "alpha_sys", "type": "normsys", "constraint": "LogNormal", "data": {"lo": 0.9, "hi": 1.1}})"), + "legacy constraint value 'LogNormal' for modifier 'sys' is a known constraint type"); +} From 449637d76c31c19d697ee182153ea8937e5f17de Mon Sep 17 00:00:00 2001 From: Carsten Burgard Date: Sun, 25 Oct 2026 19:36:39 +0100 Subject: [PATCH 11/35] [RF][HS3] HistFactory: normsys/histosys without constraint are unconstrained In HS3, a modifier without `constraint` is unconstrained. RooFit instead created a unit Gaussian constraint for `normsys` and `histosys` modifiers that had none of `constraint`, `constraint_name` or `constraint_type`. Now no constraint is created and an info message is printed. The `nom_*` global observable is only created for constrained modifiers. On export, a non-constant normsys/histosys parameter without a recognized constraint used to be an error. Such a modifier is now exported without `constraint`, as long as no other constraint term of the channel depends on its parameter (that case is still an error, because the constraint is not supported). This keeps RooFit able to write the models it reads. The rf515_hfJSON tutorial relied on the implicit Gaussian constraints; its normsys modifiers now request them with `"constraint": "Gauss"`, so the tutorial model is unchanged. For the same reason, the "Constraint" case of the HistFactoryIncompatibleDuplicateModifiers test now requests its two different constraint terms explicitly. Assisted-by: claude-code:claude-opus-5-5 --- roofit/hs3/src/JSONFactories_HistFactory.cxx | 86 ++++++++++++++------ roofit/hs3/test/testRooFitHS3.cxx | 39 ++++++++- tutorials/roofit/roofit/rf515_hfJSON.json | 3 + 3 files changed, 101 insertions(+), 27 deletions(-) diff --git a/roofit/hs3/src/JSONFactories_HistFactory.cxx b/roofit/hs3/src/JSONFactories_HistFactory.cxx index c49da5417d49b..6f83f08624b24 100644 --- a/roofit/hs3/src/JSONFactories_HistFactory.cxx +++ b/roofit/hs3/src/JSONFactories_HistFactory.cxx @@ -32,6 +32,7 @@ #include #include #include +#include #include #include @@ -574,6 +575,25 @@ getOrCreateConstraint(RooJSONFactoryWSTool &tool, const JSONNode &mod, RooRealVa } RooJSONFactoryWSTool::error("unknown or invalid constraint for modifier '" + RooJSONFactoryWSTool::name(mod) + "'"); } +bool hasConstraint(const JSONNode &mod) +{ + return mod.has_child("constraint") || mod.has_child("constraint_name") || mod.has_child("constraint_type"); +} + +// Create the constraint of a normsys or histosys modifier. Without a constraint, the parameter is unconstrained. +void addSysConstraint(RooJSONFactoryWSTool &tool, const JSONNode &mod, RooRealVar ¶m, std::string const &sysname, + std::string const &sample, RooArgSet &constraints) +{ + if (!hasConstraint(mod)) { + oocoutI(nullptr, IO) << "RooFitHS3: modifier '" << sysname << "' of sample '" << sample + << "' has no constraint, so its parameter '" << param.GetName() << "' is unconstrained" + << std::endl; + return; + } + createNominal(*tool.workspace(), param.GetName(), 0.0, -10, 10); + constraints.add(getOrCreateConstraint(tool, mod, param, sample)); +} + double poissonTau(RooPoisson const &constraint, RooAbsArg const &gamma) { auto const *mean = dynamic_cast(&constraint.getMean()); @@ -674,14 +694,13 @@ bool importHistSample(RooJSONFactoryWSTool &tool, RooDataHist &dh, RooArgSet con RooRealVar &constrParam = getOrCreate(ws, parameter ? parameter->val() : sysname, 1., -3, 5); constrParam.setError(0.0); normElems.add(constrParam); - if (mod.has_child("constraint") || mod.has_child("constraint_name") || mod.has_child("constraint_type")) { + if (hasConstraint(mod)) { // for norm factors, constraints are optional constraints.add(getOrCreateConstraint(tool, mod, constrParam, sampleName)); } } else if (modtype == "normsys") { auto *parameter = mod.find("parameter"); std::string parname(parameter ? parameter->val() : "alpha_" + sysname); - createNominal(ws, parname, 0.0, -10, 10); auto &par = getOrCreate(ws, parname, 0., -5, 5); overall_nps.add(par); auto &data = mod["data"]; @@ -704,11 +723,10 @@ bool importHistSample(RooJSONFactoryWSTool &tool, RooDataHist &dh, RooArgSet con overall_high.push_back(high); overall_interp.push_back(interp); - constraints.add(getOrCreateConstraint(tool, mod, par, sampleName)); + addSysConstraint(tool, mod, par, sysname, sampleName, constraints); } else if (modtype == "histosys") { auto *parameter = mod.find("parameter"); std::string parname(parameter ? parameter->val() : "alpha_" + sysname); - createNominal(ws, parname, 0.0, -10, 10); auto &par = getOrCreate(ws, parname, 0., -5, 5); histNps.add(par); auto &data = mod["data"]; @@ -721,7 +739,7 @@ bool importHistSample(RooJSONFactoryWSTool &tool, RooDataHist &dh, RooArgSet con const std::string context = "interpolation for histosys modifier '" + sysname + "' in sample '" + sampleName + "' of channel '" + channelName + "'"; histoInterp.push_back(interpolationCode(mod, defaultInterpolation, InterpolationClass::Piecewise, context)); - constraints.add(getOrCreateConstraint(tool, mod, par, sampleName)); + addSysConstraint(tool, mod, par, sysname, sampleName, constraints); } else if (modtype == "shapesys" || modtype == "shapefactor") { std::string funcName = channelName + "_" + sysname + "_ShapeSys"; // funcName should be "__ShapeSys" @@ -1332,17 +1350,14 @@ Channel readChannel(RooJSONFactoryWSTool *tool, const std::string &pdfname, cons RooAbsReal *var = static_cast(fip->variables().at(i)); std::string sysname(var->GetName()); erasePrefix(sysname, "alpha_"); + // A parameter without constraint is unconstrained, see checkUnconstrainedModifiers(). const auto *constraint = findConstraint(var); - if (!constraint && !var->isConstant()) { - RooJSONFactoryWSTool::error("cannot find constraint for " + std::string(var->GetName())); - } else { - const std::string context = "normsys modifier '" + sysname + "' in sample '" + sample.name + - "' of channel '" + channel.name + "'"; - sample.normsys.emplace_back( - sysname, var, fip->high()[i], fip->low()[i], - interpolationFromCode(fip->interpolationCodes()[i], InterpolationClass::Flexible, context), - constraint); - } + const std::string context = "normsys modifier '" + sysname + "' in sample '" + sample.name + + "' of channel '" + channel.name + "'"; + sample.normsys.emplace_back( + sysname, var, fip->high()[i], fip->low()[i], + interpolationFromCode(fip->interpolationCodes()[i], InterpolationClass::Flexible, context), + constraint); } } else if (!pip && (pip = dynamic_cast(e))) { // nothing to do here, already assigned @@ -1415,17 +1430,14 @@ Channel readChannel(RooJSONFactoryWSTool *tool, const std::string &pdfname, cons erasePrefix(sysname, "alpha_"); if (auto lo = dynamic_cast(pip->lowList().at(i))) { if (auto hi = dynamic_cast(pip->highList().at(i))) { + // A parameter without constraint is unconstrained, see checkUnconstrainedModifiers(). const auto *constraint = findConstraint(var); - if (!constraint && !var->isConstant()) { - RooJSONFactoryWSTool::error("cannot find constraint for " + std::string(var->GetName())); - } else { - const std::string context = "histosys modifier '" + sysname + "' in sample '" + sample.name + - "' of channel '" + channel.name + "'"; - sample.histosys.emplace_back( - sysname, var, lo, hi, - interpolationFromCode(pip->interpolationCodes()[i], InterpolationClass::Piecewise, context), - constraint); - } + const std::string context = "histosys modifier '" + sysname + "' in sample '" + sample.name + + "' of channel '" + channel.name + "'"; + sample.histosys.emplace_back( + sysname, var, lo, hi, + interpolationFromCode(pip->interpolationCodes()[i], InterpolationClass::Piecewise, context), + constraint); } } } @@ -1848,6 +1860,29 @@ std::vector findLostConstraints(const Channel &channel, const std:: return lostConstraints; } +// A normsys or histosys modifier without a recognized constraint is exported as unconstrained. That is only correct if +// none of the other constraint terms of the channel depends on its parameter. +void checkUnconstrainedModifiers(const Channel &channel, const std::vector &constraints) +{ + auto check = [&](auto const &modifiers) { + for (auto const &sys : modifiers) { + if (sys.constraint || sys.param->isConstant()) { + continue; + } + for (RooAbsPdf const *pdf : constraints) { + if (pdf->dependsOn(*sys.param)) { + RooJSONFactoryWSTool::error("cannot find constraint for " + std::string(sys.param->GetName()) + + ": the constraint term '" + pdf->GetName() + "' is not supported"); + } + } + } + }; + for (const auto &sample : channel.samples) { + check(sample.normsys); + check(sample.histosys); + } +} + bool tryExportHistFactory(RooJSONFactoryWSTool *tool, const std::string &pdfname, const RooRealSumPdf *sumpdf, std::vector constraints, JSONNode &elem) { @@ -1874,6 +1909,7 @@ bool tryExportHistFactory(RooJSONFactoryWSTool *tool, const std::string &pdfname } canonicalizeModifiers(channel); + checkUnconstrainedModifiers(channel, constraints); // stat error handling configureStatError(channel); diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index 59141a8194be8..04a612bce26b1 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -2500,11 +2500,11 @@ INSTANTIATE_TEST_SUITE_P( IncompatibleModifierCase{"Constraint", R"( { - "name": "input_1", "parameter": "alpha_dup", "type": "normsys", + "name": "input_1", "parameter": "alpha_dup", "type": "normsys", "constraint": "Gauss", "interpolation": 1, "data": {"lo": 0.8, "hi": 1.2} }, { - "name": "input_2", "parameter": "dup", "type": "normsys", + "name": "input_2", "parameter": "dup", "type": "normsys", "constraint": "Gauss", "interpolation": 1, "data": {"lo": 0.9, "hi": 1.1} })", "", "constraint metadata differs"}, @@ -3294,3 +3294,38 @@ TEST(RooFitHS3, HistFactoryNormFactorParameterAndLogNormal) R"({"name": "sys", "parameter": "alpha_sys", "type": "normsys", "constraint": "LogNormal", "data": {"lo": 0.9, "hi": 1.1}})"), "legacy constraint value 'LogNormal' for modifier 'sys' is a known constraint type"); } + +// A normsys or histosys modifier without "constraint" is unconstrained, and +// it is exported without constraint. +TEST(RooFitHS3, HistFactoryModifierWithoutConstraintIsUnconstrained) +{ + RooHelpers::LocalChangeMsgLevel changeMsgLvl(RooFit::WARNING); + + const std::string modifiers = R"( + {"name": "norm", "parameter": "alpha_norm", "type": "normsys", "data": {"lo": 0.8, "hi": 1.2}}, + {"name": "shape", "parameter": "alpha_shape", "type": "histosys", + "data": {"lo": {"contents": [8.0, 18.0]}, "hi": {"contents": [12.0, 22.0]}}}, + {"name": "constrained", "parameter": "alpha_constrained", "type": "normsys", "constraint": "Gauss", + "data": {"lo": 0.9, "hi": 1.1}})"; + const std::string defaultInterpolation = R"({"type": "mult", "in": "poly6", "out": "exp"})"; + + RooWorkspace ws1; + ASSERT_TRUE(RooJSONFactoryWSTool{ws1}.importJSONfromString(makeHistFactoryJSON(modifiers, defaultInterpolation))); + EXPECT_EQ(ws1.pdf("alpha_normConstraint"), nullptr); + EXPECT_EQ(ws1.pdf("alpha_shapeConstraint"), nullptr); + EXPECT_NE(ws1.pdf("alpha_constrainedConstraint"), nullptr); + EXPECT_EQ(ws1.var("nom_alpha_norm"), nullptr); + + const std::string json1 = RooJSONFactoryWSTool{ws1}.exportJSONtoString(); + 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; + + RooWorkspace ws2; + ASSERT_TRUE(RooJSONFactoryWSTool{ws2}.importJSONfromString(json1)); + EXPECT_EQ(ws2.pdf("alpha_normConstraint"), nullptr); + EXPECT_EQ(RooJSONFactoryWSTool{ws2}.exportJSONtoString(), json1); +} diff --git a/tutorials/roofit/roofit/rf515_hfJSON.json b/tutorials/roofit/roofit/rf515_hfJSON.json index 852280232fd73..8bd0389ac5df5 100644 --- a/tutorials/roofit/roofit/rf515_hfJSON.json +++ b/tutorials/roofit/roofit/rf515_hfJSON.json @@ -63,6 +63,7 @@ "hi": 1.05, "lo": 0.95 }, + "constraint": "Gauss", "name": "syst1", "type": "normsys" }, @@ -84,6 +85,7 @@ "hi": 1.05, "lo": 0.95 }, + "constraint": "Gauss", "name": "syst2", "type": "normsys" }, @@ -105,6 +107,7 @@ "hi": 1.05, "lo": 0.95 }, + "constraint": "Gauss", "name": "syst3", "type": "normsys" }, From f9fc39f942bc67415b133eb7639f860dbd25c868 Mon Sep 17 00:00:00 2001 From: Carsten Burgard Date: Sun, 25 Oct 2026 19:37:16 +0100 Subject: [PATCH 12/35] [RF][HS3] HistFactory: read and write the staterror constraint on the modifier HS3 gives the constraint type of a `staterror` on the modifier (`Poisson` or `Gauss`), but RooFit ignored it and only read the private channel-level field `histfactory_dist.staterror.constraint_type`, defaulting to Poisson. The export wrote neither, so the choice between Gaussian and Poisson stat constraints was lost. On import, the `constraint` of the staterror modifier is now used, with the channel-level field as fallback. On export, the constraint type is written both on the staterror modifiers and on the channel level, so that the files can still be read by older ROOT versions. Assisted-by: claude-code:claude-opus-5-5 --- roofit/hs3/src/JSONFactories_HistFactory.cxx | 25 +++++++ roofit/hs3/test/testRooFitHS3.cxx | 73 ++++++++++++++++++++ 2 files changed, 98 insertions(+) diff --git a/roofit/hs3/src/JSONFactories_HistFactory.cxx b/roofit/hs3/src/JSONFactories_HistFactory.cxx index 6f83f08624b24..f8922377d35e3 100644 --- a/roofit/hs3/src/JSONFactories_HistFactory.cxx +++ b/roofit/hs3/src/JSONFactories_HistFactory.cxx @@ -868,6 +868,7 @@ class HistFactoryImporter : public RooFit::JSONIO::Importer { std::vector sumW; std::vector sumW2; std::vector gammaParnames; + std::string modifierStatErrType; RooArgSet observables = RooJSONFactoryWSTool::readAxes(p); std::string fprefix = name; @@ -894,10 +895,25 @@ class HistFactoryImporter : public RooFit::JSONIO::Importer { } } } + // The constraint type given on the staterror modifier takes precedence over the channel-level setting. + if (modifierStatErrType.empty()) { + if (auto constraint = staterror->find("constraint")) { + modifierStatErrType = constraint->val(); + if (modifierStatErrType != "Poisson" && modifierStatErrType != "Gauss") { + RooJSONFactoryWSTool::error("unsupported constraint '" + modifierStatErrType + + "' of the staterror modifier in channel '" + name + + "', supported are \"Poisson\" and \"Gauss\""); + } + } + } } data.emplace_back(std::move(dh)); } + if (!modifierStatErrType.empty()) { + statErrType = modifierStatErrType; + } + RooAbsArg *mcStatObject = nullptr; RooArgSet constraints; if (!sumW.empty()) { @@ -1288,6 +1304,7 @@ 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; }; @@ -1455,6 +1472,7 @@ Channel readChannel(RooJSONFactoryWSTool *tool, const std::string &pdfname, cons 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; @@ -1707,6 +1725,10 @@ 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); @@ -1788,6 +1810,9 @@ 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; + } } if (!observablesWritten) { diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index 04a612bce26b1..6009b102dec41 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -3329,3 +3329,76 @@ TEST(RooFitHS3, HistFactoryModifierWithoutConstraintIsUnconstrained) EXPECT_EQ(ws2.pdf("alpha_normConstraint"), nullptr); 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. +TEST(RooFitHS3, HistFactoryStatErrorConstraint) +{ + RooHelpers::LocalChangeMsgLevel changeMsgLvl(RooFit::WARNING); + + auto makeJSON = [](std::string const &modifierConstraint, std::string const &channelConstraint) { + return R"({ + "metadata": {"hs3_version": "0.2"}, + "distributions": [ + { + "name": "model_channel0", + "type": "histfactory_dist", + "axes": [{"name": "x", "min": 0.0, "max": 2.0, "nbins": 2}],)" + + (channelConstraint.empty() ? "" : R"("staterror": {"constraint_type": ")" + channelConstraint + R"("},)") + + R"( + "samples": [ + { + "name": "sig", + "data": {"contents": [10.0, 20.0], "errors": [1.0, 2.0]}, + "modifiers": [{"name": "mcstat", "type": "staterror")" + + (modifierConstraint.empty() ? "" : R"(, "constraint": ")" + modifierConstraint + R"(")") + R"(}] + } + ] + } + ] + })"; + }; + auto hasGaussianStatConstraints = [](RooWorkspace &ws) { + bool foundGauss = false; + bool foundPoisson = false; + for (RooAbsArg *pdf : ws.allPdfs()) { + if (std::string(pdf->GetName()).find("gamma_stat_channel0") == std::string::npos) + continue; + foundGauss |= dynamic_cast(pdf) != nullptr; + foundPoisson |= dynamic_cast(pdf) != nullptr; + } + EXPECT_NE(foundGauss, foundPoisson); + return foundGauss; + }; + + { + RooWorkspace ws; + ASSERT_TRUE(RooJSONFactoryWSTool{ws}.importJSONfromString(makeJSON("Gauss", "Poisson"))); + EXPECT_TRUE(hasGaussianStatConstraints(ws)); + + 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; + const auto *modifier = findModifier(*channel, "staterror"); + ASSERT_NE(modifier, nullptr) << json; + EXPECT_EQ((*modifier)["constraint"].val(), "Gauss") << json; + + RooWorkspace ws2; + ASSERT_TRUE(RooJSONFactoryWSTool{ws2}.importJSONfromString(json)); + EXPECT_TRUE(hasGaussianStatConstraints(ws2)); + EXPECT_EQ(RooJSONFactoryWSTool{ws2}.exportJSONtoString(), json); + } + { + RooWorkspace ws; + ASSERT_TRUE(RooJSONFactoryWSTool{ws}.importJSONfromString(makeJSON("", "Gauss"))); + EXPECT_TRUE(hasGaussianStatConstraints(ws)); + } + { + RooWorkspace ws; + ASSERT_TRUE(RooJSONFactoryWSTool{ws}.importJSONfromString(makeJSON("", ""))); + EXPECT_FALSE(hasGaussianStatConstraints(ws)); + } + expectImportThrowsWithError(makeJSON("LogNormal", ""), "unsupported constraint 'LogNormal' of the staterror"); +} From c6e57b413310aff32d7fae86103db2ccc2db66fe Mon Sep 17 00:00:00 2001 From: Carsten Burgard Date: Sun, 25 Oct 2026 19:38:00 +0100 Subject: [PATCH 13/35] [RF][HS3] HistFactory: reject modifiers without interpolation in HS3 >= 0.3 HS3 requires `default_interpolation` if a modifier has no `interpolation`, and forbids implementation-specific defaults. RooFit silently fell back to code 4. Documents with `hs3_version` 0.3 or newer that rely on this are now rejected. ROOT 6.38 and earlier never wrote the interpolation of histosys modifiers (or of normsys modifiers with the default code 4), nor a `default_interpolation`. To keep these files readable, documents with an older `hs3_version` (and objects imported without a document, e.g. with `importJSONElement()`) still get code 4, with one deprecation warning per channel. The export already writes `default_interpolation` whenever a modifier omits `interpolation`, so RooFit doesn't produce such documents. The rf515_hfJSON tutorial now states its interpolation explicitly with a `default_interpolation`, which avoids the deprecation warning. Assisted-by: claude-code:claude-opus-5-5 --- roofit/hs3/src/JSONFactories_HistFactory.cxx | 48 +++++++++++++++++--- roofit/hs3/test/testRooFitHS3.cxx | 37 +++++++++++++++ tutorials/roofit/roofit/rf515_hfJSON.json | 1 + 3 files changed, 80 insertions(+), 6 deletions(-) diff --git a/roofit/hs3/src/JSONFactories_HistFactory.cxx b/roofit/hs3/src/JSONFactories_HistFactory.cxx index f8922377d35e3..dfeb346861173 100644 --- a/roofit/hs3/src/JSONFactories_HistFactory.cxx +++ b/roofit/hs3/src/JSONFactories_HistFactory.cxx @@ -337,8 +337,27 @@ std::vector readInterpolations(const JSONNode &object, std::size_t nParamet return codes; } +// Whether an imported document predates HS3 v0.3. Before that, RooFit did not write the interpolation of modifiers +// that used its implicit default. +bool isLegacyHS3Version(std::string const &version) +{ + int major = 0; + int minor = 0; + if (!parseHS3Version(version, major, minor)) { + return true; + } + return std::make_pair(major, minor) <= std::make_pair(0, 2); +} + +// The fallback for modifiers without interpolation in documents that predate HS3 v0.3, see interpolationCode(). +struct LegacyInterpolationDefault { + bool allowed = false; + bool warned = false; +}; + int interpolationCode(const JSONNode &modifier, const std::optional &defaultInterpolation, - InterpolationClass interpolationClass, const std::string &context) + InterpolationClass interpolationClass, const std::string &context, + LegacyInterpolationDefault &legacyDefault) { const auto toCode = [&](const Interpolation &interpolation) { return codeFromInterpolation(interpolation, interpolationClass, context); @@ -356,9 +375,20 @@ int interpolationCode(const JSONNode &modifier, const std::optional &defaultInterpolation, RooArgSet &constraints) + const std::optional &defaultInterpolation, + LegacyInterpolationDefault &legacyInterpolationDefault, RooArgSet &constraints) { RooWorkspace &ws = *tool.workspace(); @@ -706,7 +737,8 @@ bool importHistSample(RooJSONFactoryWSTool &tool, RooDataHist &dh, RooArgSet con auto &data = mod["data"]; const std::string context = "interpolation for normsys modifier '" + sysname + "' in sample '" + sampleName + "' of channel '" + channelName + "'"; - const int interp = interpolationCode(mod, defaultInterpolation, InterpolationClass::Flexible, context); + const int interp = interpolationCode(mod, defaultInterpolation, InterpolationClass::Flexible, context, + legacyInterpolationDefault); double low = data["lo"].val_double(); double high = data["hi"].val_double(); @@ -738,7 +770,8 @@ bool importHistSample(RooJSONFactoryWSTool &tool, RooDataHist &dh, RooArgSet con RooJSONFactoryWSTool::readBinnedData(data["hi"], sysname + "High_" + prefixedName, varlist))); const std::string context = "interpolation for histosys modifier '" + sysname + "' in sample '" + sampleName + "' of channel '" + channelName + "'"; - histoInterp.push_back(interpolationCode(mod, defaultInterpolation, InterpolationClass::Piecewise, context)); + histoInterp.push_back(interpolationCode(mod, defaultInterpolation, InterpolationClass::Piecewise, context, + legacyInterpolationDefault)); addSysConstraint(tool, mod, par, sysname, sampleName, constraints); } else if (modtype == "shapesys" || modtype == "shapefactor") { std::string funcName = channelName + "_" + sysname + "_ShapeSys"; @@ -936,12 +969,15 @@ class HistFactoryImporter : public RooFit::JSONIO::Importer { observables, statErrType, defaultGammaMin, defaultStatErrorGammaMax, statErrThresh); } + LegacyInterpolationDefault legacyInterpolationDefault; + legacyInterpolationDefault.allowed = isLegacyHS3Version(tool->hs3VersionInput()); + int idx = 0; RooArgList funcs; RooArgList coefs; for (const auto &comp : p["samples"].children()) { importHistSample(*tool, *data[idx], observables, mcStatObject, fprefix, comp, defaultInterpolation, - constraints); + legacyInterpolationDefault, constraints); ++idx; std::string const &compName = RooJSONFactoryWSTool::name(comp); diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index 6009b102dec41..8efefa7ab907a 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -3402,3 +3402,40 @@ TEST(RooFitHS3, HistFactoryStatErrorConstraint) } expectImportThrowsWithError(makeJSON("LogNormal", ""), "unsupported constraint 'LogNormal' of the staterror"); } + +// HS3 forbids implementation-specific default interpolations. Documents from +// before HS3 v0.3 still get the implicit default of older RooFit versions. +TEST(RooFitHS3, HistFactoryMissingInterpolation) +{ + RooHelpers::LocalChangeMsgLevel changeMsgLvl(RooFit::WARNING); + + const std::string modifiers = R"( + {"name": "norm", "parameter": "alpha_norm", "type": "normsys", "data": {"lo": 0.8, "hi": 1.2}}, + {"name": "norm2", "parameter": "alpha_norm2", "type": "normsys", "data": {"lo": 0.9, "hi": 1.1}})"; + + std::string json = makeHistFactoryJSON(modifiers); + const std::string legacyVersion = R"("hs3_version": "0.2")"; + std::string currentJson = json; + currentJson.replace(currentJson.find(legacyVersion), legacyVersion.size(), R"("hs3_version": "0.3")"); + expectImportThrowsWithError(currentJson, + "interpolation for normsys modifier 'norm' in sample 'sig' of channel " + "'channel0' is not given, and the channel has no \"default_interpolation\""); + + RooWorkspace ws; + const std::string warnings = captureMessages( + RooFit::WARNING, RooFit::IO, [&] { ASSERT_TRUE(RooJSONFactoryWSTool{ws}.importJSONfromString(json)); }); + EXPECT_EQ(countOccurrences(warnings, "implicit default of older RooFit versions"), 1u) << warnings; + auto *norm = dynamic_cast(ws.function("sig_channel0_epsilon")); + ASSERT_NE(norm, nullptr); + EXPECT_EQ(norm->interpolationCodes()[0], 4); + + // 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; + 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)); +} diff --git a/tutorials/roofit/roofit/rf515_hfJSON.json b/tutorials/roofit/roofit/rf515_hfJSON.json index 8bd0389ac5df5..55994c6fc8656 100644 --- a/tutorials/roofit/roofit/rf515_hfJSON.json +++ b/tutorials/roofit/roofit/rf515_hfJSON.json @@ -52,6 +52,7 @@ "nbins": 2 } ], + "default_interpolation": {"type": "mult", "in": "poly6", "out": "exp"}, "samples": [ { "data": { From 215ed6a5d2bb3a61de99174f74f62146d00587bf Mon Sep 17 00:00:00 2001 From: Carsten Burgard Date: Sun, 25 Oct 2026 19:38:37 +0100 Subject: [PATCH 14/35] [RF][HS3] Validate the size of matrices, error arrays and bin edges Malformed input could be read out of bounds: * the rows of `multivariate_normal_dist` covariance and correlation matrices are now checked to form a square matrix of the right size; * the `errors` of binned data must have as many entries as `contents`; * the `edges` of binned axes must contain at least two strictly ascending values. Assisted-by: claude-code:claude-opus-5-5 --- roofit/hs3/src/JSONFactories_RooFitCore.cxx | 15 ++++++++++++ roofit/hs3/src/JSONIOUtils.cxx | 20 +++++++++++++++ roofit/hs3/src/JSONIOUtils.h | 2 ++ roofit/hs3/src/RooJSONFactoryWSTool.cxx | 10 +++++--- roofit/hs3/test/testRooFitHS3.cxx | 27 +++++++++++++++++++++ 5 files changed, 70 insertions(+), 4 deletions(-) diff --git a/roofit/hs3/src/JSONFactories_RooFitCore.cxx b/roofit/hs3/src/JSONFactories_RooFitCore.cxx index 805b60fd807b6..5bb36235425e1 100644 --- a/roofit/hs3/src/JSONFactories_RooFitCore.cxx +++ b/roofit/hs3/src/JSONFactories_RooFitCore.cxx @@ -717,8 +717,22 @@ bool importMultiVarGaussian(RooJSONFactoryWSTool *tool, const JSONNode &p) TMatrixDSym covmat; + // The matrix must be square, otherwise the entries would be written out of bounds. + auto checkSquare = [&](const char *key, std::size_t n) { + if (p[key].num_children() != n) { + RooJSONFactoryWSTool::error("\"" + std::string(key) + "\" in '" + name + "' must have " + std::to_string(n) + + " rows"); + } + for (const auto &row : p[key].children()) { + if (!row.is_seq() || row.num_children() != n) { + RooJSONFactoryWSTool::error("\"" + std::string(key) + "\" in '" + name + "' must be a square matrix"); + } + } + }; + if (has_cov) { int n = p["covariances"].num_children(); + checkSquare("covariances", n); int i = 0; covmat.ResizeTo(n, n); for (const auto &row : p["covariances"].children()) { @@ -732,6 +746,7 @@ bool importMultiVarGaussian(RooJSONFactoryWSTool *tool, const JSONNode &p) } else { std::vector variances; variances << p["standard_deviations"]; + checkSquare("correlations", variances.size()); covmat.ResizeTo(variances.size(), variances.size()); int i = 0; for (const auto &row : p["correlations"].children()) { diff --git a/roofit/hs3/src/JSONIOUtils.cxx b/roofit/hs3/src/JSONIOUtils.cxx index 5a921a470775d..fa1f8140020f7 100644 --- a/roofit/hs3/src/JSONIOUtils.cxx +++ b/roofit/hs3/src/JSONIOUtils.cxx @@ -1,6 +1,7 @@ #include "JSONIOUtils.h" #include +#include #include #include @@ -124,3 +125,22 @@ bool parseHS3Version(std::string const &version, int &major, int &minor) ++pos; return readNumber(minor); } + +// Read the bin edges of an axis, which must be at least two strictly ascending numbers. +std::vector readEdges(const JSONNode &node, std::string const &context) +{ + if (!node.is_seq()) { + RooJSONFactoryWSTool::error("\"edges\" of " + context + " must be a list"); + } + std::vector edges; + for (auto const &edge : node.children()) { + edges.push_back(edge.val_double()); + if (edges.size() > 1 && !(edges.back() > edges[edges.size() - 2])) { + RooJSONFactoryWSTool::error("\"edges\" of " + context + " must be strictly ascending"); + } + } + if (edges.size() < 2) { + RooJSONFactoryWSTool::error("\"edges\" of " + context + " must contain at least two values"); + } + return edges; +} diff --git a/roofit/hs3/src/JSONIOUtils.h b/roofit/hs3/src/JSONIOUtils.h index d7672b8ebb4e8..e09485c0d0dcc 100644 --- a/roofit/hs3/src/JSONIOUtils.h +++ b/roofit/hs3/src/JSONIOUtils.h @@ -2,6 +2,7 @@ #define JSONIOUtils_h #include +#include #include class RooAbsBinning; @@ -12,5 +13,6 @@ std::string removeSuffix(std::string_view str, std::string_view suffix); std::unique_ptr varJSONString(const RooFit::Detail::JSONNode &treeRoot); void writeAxisBinning(RooFit::Detail::JSONNode &node, const RooAbsBinning &binning); bool parseHS3Version(std::string const &version, int &major, int &minor); +std::vector readEdges(const RooFit::Detail::JSONNode &node, std::string const &context); #endif diff --git a/roofit/hs3/src/RooJSONFactoryWSTool.cxx b/roofit/hs3/src/RooJSONFactoryWSTool.cxx index 17cb94c65a042..fd111a9fa485a 100644 --- a/roofit/hs3/src/RooJSONFactoryWSTool.cxx +++ b/roofit/hs3/src/RooJSONFactoryWSTool.cxx @@ -1557,10 +1557,7 @@ RooArgSet RooJSONFactoryWSTool::readAxes(const JSONNode &topNode) for (JSONNode const &node : topNode["axes"].children()) { if (node.has_child("edges")) { - std::vector edges; - for (auto const &bound : node["edges"].children()) { - edges.push_back(bound.val_double()); - } + std::vector edges = readEdges(node["edges"], "axis '" + node["name"].val() + "'"); auto obs = std::make_unique(node["name"].val().c_str(), node["name"].val().c_str(), edges[0], edges[edges.size() - 1]); RooBinning bins(obs->getMin(), obs->getMax()); @@ -1607,6 +1604,11 @@ RooJSONFactoryWSTool::readBinnedData(const JSONNode &n, const std::string &name, errors = &n["errors"]; if (!errors->is_seq()) RooJSONFactoryWSTool::error("errors are not in list form"); + if (errors->num_children() != contents.num_children()) { + RooJSONFactoryWSTool::error("inconsistent number of errors and contents in '" + name + + "': errors=" + std::to_string(errors->num_children()) + + ", contents=" + std::to_string(contents.num_children())); + } } auto bins = generateBinIndices(vars); diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index 8efefa7ab907a..c577f545b4276 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -3439,3 +3439,30 @@ TEST(RooFitHS3, HistFactoryMissingInterpolation) exportedCurrent.replace(exportedCurrent.find(exportedVersion), exportedVersion.size(), R"("hs3_version":"0.3")"); EXPECT_TRUE(RooJSONFactoryWSTool{ws2}.importJSONfromString(exportedCurrent)); } + +// Malformed matrices, error arrays and bin edges are rejected instead of being read out of bounds. +TEST(RooFitHS3, MalformedArraysAreRejected) +{ + auto binnedData = [](std::string const &axis, std::string const &extra) { + return R"({ + "metadata": {"hs3_version": "0.2"}, + "data": [{"name": "data", "type": "binned", "axes": [)" + + axis + R"(], "contents": [1.0, 2.0])" + extra + R"(}] + })"; + }; + const std::string goodAxis = R"({"name": "x", "min": 0.0, "max": 2.0, "nbins": 2})"; + + expectImportThrowsWithError(binnedData(goodAxis, R"(, "errors": [1.0])"), + "inconsistent number of errors and contents in 'data'"); + expectImportThrowsWithError(binnedData(R"({"name": "x", "edges": [0.0, 2.0, 1.0]})", ""), + "\"edges\" of axis 'x' must be strictly ascending"); + expectImportThrowsWithError(binnedData(R"({"name": "x", "edges": []})", ""), + "\"edges\" of axis 'x' must contain at least two values"); + + expectImportThrowsWithError(R"({ + "metadata": {"hs3_version": "0.2"}, + "distributions": [{"name": "mvn", "type": "multivariate_normal_dist", "x": ["x1", "x2"], "mean": [0.0, 0.0], + "covariances": [[1.0, 0.0, 0.0], [0.0, 1.0]]}] + })", + "\"covariances\" in 'mvn' must be a square matrix"); +} From 97c0e689a8de9db0efdc5a244eb5c48e8148ac79 Mon Sep 17 00:00:00 2001 From: Carsten Burgard Date: Sun, 25 Oct 2026 19:39:07 +0100 Subject: [PATCH 15/35] [RF][HS3] Take the range of unbinned observables from the data axes The `min`/`max` of the axes of unbinned data were ignored on import; the observables kept whatever range they had, and entries outside of it were clipped. If an observable got no range from a domain, its range is now set from the axis. If a domain range disagrees with the axis, it is kept and a warning is printed. Assisted-by: claude-code:claude-opus-5-5 --- roofit/hs3/src/RooJSONFactoryWSTool.cxx | 35 +++++++++++++++++++++-- roofit/hs3/test/testRooFitHS3.cxx | 38 +++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 3 deletions(-) diff --git a/roofit/hs3/src/RooJSONFactoryWSTool.cxx b/roofit/hs3/src/RooJSONFactoryWSTool.cxx index fd111a9fa485a..87d42ba78c995 100644 --- a/roofit/hs3/src/RooJSONFactoryWSTool.cxx +++ b/roofit/hs3/src/RooJSONFactoryWSTool.cxx @@ -442,8 +442,36 @@ void getObservables(RooWorkspace const &ws, const JSONNode &node, RooAbsCollecti } } +// Make the ranges of the observables of unbinned data consistent with the "min" and "max" of the axes. Variables that +// didn't get their range from a domain take it from the axis. If `domains` is null, all ranges are considered explicit. +void applyUnbinnedAxisRanges(const JSONNode &p, RooWorkspace &workspace, RooFit::JSONIO::Detail::Domains const *domains) +{ + for (const auto &axis : p["axes"].children()) { + if (!axis.has_child("min") || !axis.has_child("max")) { + continue; + } + const std::string name = RooJSONFactoryWSTool::name(axis); + RooRealVar *var = workspace.var(name); + if (!var) { + continue; // reported by getObservables() + } + const double min = axis["min"].val_double(); + const double max = axis["max"].val_double(); + if (domains && !domains->hasVariable(name.c_str())) { + var->setRange(min, max); + } else if (var->getMin() != min || var->getMax() != max) { + std::stringstream ss; + ss << "RooFitHS3: the range [" << min << ", " << max << "] of the axis '" << name << "' of the dataset '" + << RooJSONFactoryWSTool::name(p) << "' differs from the range [" << var->getMin() << ", " << var->getMax() + << "] of the variable."; + RooJSONFactoryWSTool::warning(ss.str()); + } + } +} + // Create a RooAbsData (binned or unbinned) from the JSON node `p`. -std::unique_ptr loadData(const JSONNode &p, RooWorkspace &workspace) +std::unique_ptr +loadData(const JSONNode &p, RooWorkspace &workspace, RooFit::JSONIO::Detail::Domains const *domains) { std::string name(RooJSONFactoryWSTool::name(p)); @@ -457,6 +485,7 @@ std::unique_ptr loadData(const JSONNode &p, RooWorkspace &workspace) // unbinned RooArgList varlist; getObservables(workspace, p, varlist); + applyUnbinnedAxisRanges(p, workspace, domains); RooArgSet vars(varlist); auto data = std::make_unique(name, name, vars, RooFit::WeightVar()); auto &coords = p["entries"]; @@ -2155,7 +2184,7 @@ void RooJSONFactoryWSTool::importAllNodes(const JSONNode &n) std::vector> datasets; if (auto dataNode = n.find("data")) { for (const auto &p : dataNode->children()) { - datasets.push_back(loadData(p, _workspace)); + datasets.push_back(loadData(p, _workspace, _domains.get())); } } @@ -2258,7 +2287,7 @@ void RooJSONFactoryWSTool::importJSONElement(const std::string &name, const std: if (isVariable) { this->importVariableElement(n); } else if (isData) { - auto absData = loadData(n, _workspace); + auto absData = loadData(n, _workspace, nullptr); _workspace.import(*absData); } else { this->importFunction(n, false); diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index c577f545b4276..d2616fe6f9d20 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -3466,3 +3466,41 @@ TEST(RooFitHS3, MalformedArraysAreRejected) })", "\"covariances\" in 'mvn' must be a square matrix"); } + +// The observables of unbinned data take their range from the axes, unless a domain defines it. +TEST(RooFitHS3, UnbinnedAxisRangesOnImport) +{ + auto makeJSON = [](std::string const &domainAxes) { + return R"({ + "metadata": {"hs3_version": "0.2"}, + "domains": [{"name": "default_domain", "type": "product_domain", "axes": [)" + + domainAxes + R"(]}], + "parameter_points": [{"name": "default_values", "parameters": [{"name": "x", "value": 0.0}]}], + "distributions": [{"name": "pdf", "type": "gaussian_dist", "x": "x", "mean": 0.0, "sigma": 1.0}], + "data": [{"name": "data", "type": "unbinned", "axes": [{"name": "x", "min": -3.0, "max": 8.0}], + "entries": [[-2.5], [7.5]]}] + })"; + }; + + RooHelpers::LocalChangeMsgLevel changeMsgLvl(RooFit::WARNING); + { + // "x" is not in any domain, so it gets the range of the axis and no entry is clipped. + RooWorkspace ws; + ScopedNoDomainConstVarImportFlag noConstVars{false}; + ASSERT_TRUE(RooJSONFactoryWSTool{ws}.importJSONfromString(makeJSON(R"({"name": "other", "min": 0, "max": 1})"))); + EXPECT_DOUBLE_EQ(ws.var("x")->getMin(), -3.0); + EXPECT_DOUBLE_EQ(ws.var("x")->getMax(), 8.0); + const RooArgSet *row = ws.data("data")->get(1); + EXPECT_DOUBLE_EQ(static_cast(row->find("x"))->getVal(), 7.5); + } + { + // A disagreeing domain range is kept, with a warning. + RooWorkspace ws; + const std::string warnings = captureMessages(RooFit::WARNING, RooFit::IO, [&] { + EXPECT_TRUE( + RooJSONFactoryWSTool{ws}.importJSONfromString(makeJSON(R"({"name": "x", "min": -10, "max": 10})"))); + }); + EXPECT_DOUBLE_EQ(ws.var("x")->getMin(), -10.0); + EXPECT_NE(warnings.find("of the axis 'x' of the dataset 'data' differs"), std::string::npos) << warnings; + } +} From ef7393a361e4ba6063aa495a4e92942086188618 Mon Sep 17 00:00:00 2001 From: Carsten Burgard Date: Sun, 25 Oct 2026 19:40:03 +0100 Subject: [PATCH 16/35] [RF][HS3][docs] Update the RooFitHS3 developer documentation * The export keys and factory expressions are no longer central JSON files in `etc/`, but catalogs embedded in `roofit/hs3/src/RooFitHS3_wsexportkeys.cxx` and `roofit/hs3/src/RooFitHS3_wsfactoryexpressions.cxx`. Point to them, and explain how to add entries there or load additional catalogs at runtime. * Describe `importArg()` as the function to override, with `importFunction()`/`importPdf()` as compatibility shims. * Register importers and exporters with the template helpers or with `std::unique_ptr`, as the current API requires, instead of raw `new`. Assisted-by: claude-code:claude-opus-5-5 --- roofit/doc/developers/roofit_hs3.md | 69 +++++++++++++++++++++-------- 1 file changed, 51 insertions(+), 18 deletions(-) diff --git a/roofit/doc/developers/roofit_hs3.md b/roofit/doc/developers/roofit_hs3.md index 5f718110a40fd..696ea79ceb335 100644 --- a/roofit/doc/developers/roofit_hs3.md +++ b/roofit/doc/developers/roofit_hs3.md @@ -47,12 +47,16 @@ implementation(s) take precedence. ### Writing your own importers and exporters: Proxy-based Proxy-based implementations can be added very easily and without -actually writing any `C++` code -- you only need to add a short item -to a list in a `JSON` file, namely the -[export keys](https://github.com/root-project/root/blob/master/etc/RooFitHS3_wsexportkeys.json) +actually writing any `C++` logic -- you only need to add a short item +to a `JSON` catalog, namely the +[export keys](https://github.com/root-project/root/blob/master/roofit/hs3/src/RooFitHS3_wsexportkeys.cxx) for an exporter, or the -[factory expressions](https://github.com/root-project/root/blob/master/etc/RooFitHS3_wsfactoryexpressions.json) -for an importer. +[factory expressions](https://github.com/root-project/root/blob/master/roofit/hs3/src/RooFitHS3_wsfactoryexpressions.cxx) +for an importer. The built-in catalogs are `JSON` documents embedded as +raw string literals in these two source files. They are compiled into +the `RooFitHS3` library and loaded automatically when the library is +loaded, so there are no separate `JSON` files to install or find at +runtime. This works in the following way: Every `RooFit` class performs dependency tracking via proxies, which have names. This can be @@ -67,7 +71,7 @@ interface. Hence: expression`. For the importer, an entry in the -[factory expressions](https://github.com/root-project/root/blob/master/etc/RooFitHS3_wsfactoryexpressions.json) +[factory expressions](https://github.com/root-project/root/blob/master/roofit/hs3/src/RooFitHS3_wsfactoryexpressions.cxx) needs to be added as follows: ``` {.json} @@ -82,7 +86,7 @@ needs to be added as follows: ``` Similarly, for the exporter, an entry in the -[export keys](https://github.com/root-project/root/blob/master/etc/RooFitHS3_wsexportkeys.json) +[export keys](https://github.com/root-project/root/blob/master/roofit/hs3/src/RooFitHS3_wsexportkeys.cxx) needs to be added as follows: ``` {.json} @@ -97,12 +101,18 @@ needs to be added as follows: ``` -If you don't want to edit the central `json` files containing the -factory expressions or export keys, you can also put your custom -export keys or factory expressions into a different json file and load -that using `RooFit::JSONIO::loadExportKeys(const std::string +To contribute a catalog entry to `ROOT`, edit the embedded catalog in +`roofit/hs3/src/RooFitHS3_wsexportkeys.cxx` or +`roofit/hs3/src/RooFitHS3_wsfactoryexpressions.cxx` and rebuild `ROOT`. +If you don't want to change `ROOT` itself, you can put your custom +export keys or factory expressions into your own `JSON` file (with the +same format as the embedded catalogs) and add them to the built-in ones +at runtime using `RooFit::JSONIO::loadExportKeys(const std::string &fname)` and `RooFit::JSONIO::loadFactoryExpressions(const -std::string &fname)`. +std::string &fname)`. Both functions also have an overload that reads +from a `std::istream`. You can inspect the currently known entries with +`RooFit::JSONIO::printExportKeys()` and +`RooFit::JSONIO::printFactoryExpressions()`. If either the importer or the exporter cannot be created with factory expressions and export keys, you can instead write a custom `C++` @@ -121,9 +131,11 @@ in `ROOT`. Any importer should take the following form: ``` {.cpp} + using RooFit::Detail::JSONNode; + class MyClassFactory : public RooFit::JSONIO::Importer { public: - bool importFunction(RooJSONFactoryWSTool *tool, const JSONNode &p) const override + bool importArg(RooJSONFactoryWSTool *tool, const JSONNode &p) const override { std::string name(RooJSONFactoryWSTool::name(p)); @@ -145,14 +157,24 @@ Any importer should take the following form: }; ``` -If the class you are trying to import inherits from `RooAbsPdf` rather -than from `RooAbsReal`, you should define `importPdf` instead of -`importFunction`, with the same signature. +The same `importArg` function is used for functions (inheriting from +`RooAbsReal`) and pdfs (inheriting from `RooAbsPdf`). If the importer +can't handle a given `JSON` node, it should return `false` so that the +next importer is tried. The functions `importFunction` and `importPdf` +of older `ROOT` versions are still called for backwards compatibility, +so existing importers that override them keep working, but new code +should override `importArg`. Once your importer implementation exists, you need to register it with the tool using a line like the following: ``` {.cpp} - RooFit::JSONIO::registerImporter("", new MyClassFactory(), true); + RooFit::JSONIO::registerImporter("", true); +``` + +This is a shorthand for passing an instance owned by a `std::unique_ptr`: + +``` {.cpp} + RooFit::JSONIO::registerImporter("", std::make_unique(), true); ``` As there can be several importers for the same `json` key, the last @@ -189,9 +211,20 @@ The implementation of an exporter works in a very similar fashion: Also this needs to be registered with the tool ``` {.cpp} - RooFit::JSONIO::registerExporter(MyClass::Class(), new MyClassStreamer(), true); + RooFit::JSONIO::registerExporter(MyClass::Class(), true); ``` +or equivalently + +``` {.cpp} + RooFit::JSONIO::registerExporter(MyClass::Class(), std::make_unique(), true); +``` + +If the `TClass` of your class is not available where you register the +exporter, you can also pass the class name as a string instead of +`MyClass::Class()`. If `exportObject` returns `false`, the next +exporter is tried, and finally the export keys. + For more complicated cases where members are lists of elements, the methods `is_seq()`, `set_seq()`, `is_map()`, `set_map()` and ranged-based for-loops via `children()` might come in handy. From 5092e2b515bd164c5a4bb3fa1fdcebb05e075cb4 Mon Sep 17 00:00:00 2001 From: Carsten Burgard Date: Thu, 5 Nov 2026 11:52:48 +0100 Subject: [PATCH 17/35] [RF][HS3] Keep errors of zero in binned data before the first positive error RooDataHist only starts to store squared weights when a bin is set with a positive error, and then initializes them from the weights of the bins set so far. So for "errors": [0, 2], the first bin got a squared weight equal to its content instead of 0. The bin with the first positive error is now set first. Assisted-by: claude-code:claude-opus-5-5 --- roofit/hs3/src/RooJSONFactoryWSTool.cxx | 8 ++++++++ roofit/hs3/test/testRooFitHS3.cxx | 20 ++++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/roofit/hs3/src/RooJSONFactoryWSTool.cxx b/roofit/hs3/src/RooJSONFactoryWSTool.cxx index 87d42ba78c995..9189286fd8f8a 100644 --- a/roofit/hs3/src/RooJSONFactoryWSTool.cxx +++ b/roofit/hs3/src/RooJSONFactoryWSTool.cxx @@ -33,6 +33,7 @@ #include #include +#include #include #include #include @@ -1658,6 +1659,13 @@ RooJSONFactoryWSTool::readBinnedData(const JSONNode &n, const std::string &name, errorVals.push_back(err.val_double()); } } + // RooDataHist only starts to store squared weights when it sees the first positive error, and then initializes + // them from the weights of the bins set so far. Set that bin first, so that errors of zero before it stay zero. + auto firstPositive = std::find_if(errorVals.begin(), errorVals.end(), [](double err) { return err > 0.; }); + if (firstPositive != errorVals.end()) { + const std::size_t ibin = std::distance(errorVals.begin(), firstPositive); + dh->set(ibin, contentVals[ibin], errorVals[ibin]); + } for (size_t ibin = 0; ibin < bins.size(); ++ibin) { const double err = errors ? errorVals[ibin] : -1; dh->set(ibin, contentVals[ibin], err); diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index d2616fe6f9d20..81e054bb29262 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -3504,3 +3505,22 @@ TEST(RooFitHS3, UnbinnedAxisRangesOnImport) EXPECT_NE(warnings.find("of the axis 'x' of the dataset 'data' differs"), std::string::npos) << warnings; } } + +// 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); +} + From 88a0c43530459cffd084263649d7fd77804b15c9 Mon Sep 17 00:00:00 2001 From: Carsten Burgard Date: Thu, 5 Nov 2026 13:48:17 +0100 Subject: [PATCH 18/35] [RF][HS3] Test that a constant RooRealVar stays distinct from a RooConstVar A RooRealVar that is constant in all parameter points and a RooConstVar are written the same way in the parameter points (a value and "const": true). Only the domains tell them apart: the RooRealVar is listed in a domain, the RooConstVar is not, and a parameter without domain is constant and read back as a RooConstVar. The test checks this, the restored types and constness (also from snapshots), and that the round trip is stable. Assisted-by: claude-code:claude-opus-5-5 --- roofit/hs3/test/testRooFitHS3.cxx | 72 +++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index 81e054bb29262..99299f37aaf9f 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -3524,3 +3524,75 @@ TEST(RooFitHS3, BinnedDataZeroErrors) EXPECT_DOUBLE_EQ(data->weightSquared(2), 0.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 +// difference is that the RooRealVar is listed in a domain and the RooConstVar is not: a parameter without domain is +// constant (it has no range to vary in), and RooFit reads it back as a RooConstVar. +TEST(RooFitHS3, ConstRooRealVarStaysDistinctFromRooConstVar) +{ + ScopedNoDomainConstVarImportFlag flagGuard{true}; + RooHelpers::LocalChangeMsgLevel changeMsgLvl(RooFit::WARNING); + + RooRealVar x{"x", "x", 0.0, -10.0, 10.0}; + RooConstVar meanConst{"mean_const", "mean_const", 1.0}; + RooRealVar sigmaFixed{"sigma_fixed", "sigma_fixed", 1.0, 0.1, 10.0}; + sigmaFixed.setConstant(true); + RooGaussian gauss{"gauss", "gauss", x, meanConst, sigmaFixed}; + + RooWorkspace ws; + ws.import(gauss, RooFit::Silence()); + // The RooRealVar is constant in every snapshot, with different values. + RooRealVar &sigmaInWs = *ws.var("sigma_fixed"); + ws.saveSnapshot("snapshot_a", RooArgSet{sigmaInWs}); + sigmaInWs.setVal(2.0); + ws.saveSnapshot("snapshot_b", RooArgSet{sigmaInWs}); + sigmaInWs.setVal(1.0); + + const std::string json = RooJSONFactoryWSTool{ws}.exportJSONtoString(); + auto tree = RooFit::Detail::JSONTree::create(json); + auto &root = tree->rootnode(); + + // In every parameter point, the two are written the same way. + std::size_t nSigmaEntries = 0; + for (auto const &point : root["parameter_points"].children()) { + for (auto const ¶m : point["parameters"].children()) { + const std::string name = param["name"].val(); + if (name != "sigma_fixed" && name != "mean_const") { + continue; + } + EXPECT_TRUE(param.has_child("const") && param["const"].val_bool()) + << name << " in " << point["name"].val() << ": " << json; + EXPECT_FALSE(param.has_child("min") || param.has_child("max")) << name << ": " << json; + nSigmaEntries += name == "sigma_fixed"; + } + } + EXPECT_EQ(nSigmaEntries, 3u) << json; // default_values, snapshot_a, snapshot_b + + // Only the domains tell them apart. + auto inAnyDomain = [&](std::string const &name) { + for (auto const &domain : root["domains"].children()) { + if (RooJSONFactoryWSTool::findNamedChild(domain["axes"], name)) { + return true; + } + } + return false; + }; + EXPECT_TRUE(inAnyDomain("sigma_fixed")) << json; + EXPECT_FALSE(inAnyDomain("mean_const")) << json; + + // On import, the types and the constness are restored, also from the snapshots. + RooWorkspace imported; + ASSERT_TRUE(RooJSONFactoryWSTool{imported}.importJSONfromString(json)); + EXPECT_NE(dynamic_cast(imported.obj("mean_const")), nullptr); + EXPECT_EQ(imported.var("mean_const"), nullptr); + RooRealVar *sigmaImported = imported.var("sigma_fixed"); + ASSERT_NE(sigmaImported, nullptr); + EXPECT_TRUE(sigmaImported->isConstant()); + ASSERT_TRUE(imported.loadSnapshot("snapshot_b")); + EXPECT_DOUBLE_EQ(sigmaImported->getVal(), 2.0); + EXPECT_TRUE(sigmaImported->isConstant()); + ASSERT_TRUE(imported.loadSnapshot("snapshot_a")); // back to the value of the default parameter point + + // The round trip is stable. + EXPECT_EQ(RooJSONFactoryWSTool{imported}.exportJSONtoString(), json); +} From 3eb77b65cc5a069727f090f87f093f1073251216 Mon Sep 17 00:00:00 2001 From: Carsten Burgard Date: Thu, 5 Nov 2026 13:54:14 +0100 Subject: [PATCH 19/35] [tutorials][RF][HS3] Format rf515_hfJSON.json with clang-format The code-formatting check also covers JSON files, and the tutorial JSON changed in this PR didn't follow the clang-format style. Whitespace only: the parsed JSON content is unchanged. Assisted-by: claude-code:claude-opus-5-5 --- tutorials/roofit/roofit/rf515_hfJSON.json | 314 ++++++++++++---------- 1 file changed, 175 insertions(+), 139 deletions(-) diff --git a/tutorials/roofit/roofit/rf515_hfJSON.json b/tutorials/roofit/roofit/rf515_hfJSON.json index 55994c6fc8656..382b8f9c39c06 100644 --- a/tutorials/roofit/roofit/rf515_hfJSON.json +++ b/tutorials/roofit/roofit/rf515_hfJSON.json @@ -1,144 +1,180 @@ { - "misc": { - "ROOT_internal": { - "combined_datasets": { - "observed" : { - "index_cat": "channelCat", - "indices": [0], - "labels": ["channel1"] - } + "misc": { + "ROOT_internal": { + "combined_datasets": { + "observed": { + "index_cat": "channelCat", + "indices": [ + 0 + ], + "labels": [ + "channel1" + ] + } + }, + "combined_distributions": { + "main": { + "distributions": [ + "model_channel1" + ], + "index_cat": "channelCat", + "indices": [ + 0 + ], + "labels": [ + "channel1" + ] + } + }, + "ModelConfigs": { + "main": { + "combined_data_name": "observed", + "mcName": "ModelConfig", + "pdfName": "main" + } + } + } + }, + "analyses": [ + { + "name": "main", + "likelihood": "main_likelihood", + "parameters_of_interest": [ + "mu" + ] + } + ], + "likelihoods": [ + { + "name": "main_likelihood", + "distributions": [ + "model_channel1" + ], + "data": [ + "observed_channel1" + ] + } + ], + "distributions": [ + { + "name": "model_channel1", + "axes": [ + { + "name": "obs_x_channel1", + "max": 2.0, + "min": 1.0, + "nbins": 2 + } + ], + "default_interpolation": { + "type": "mult", + "in": "poly6", + "out": "exp" + }, + "samples": [ + { + "data": { + "contents": [ + 20, + 10 + ] + }, + "modifiers": [ + { + "data": { + "hi": 1.05, + "lo": 0.95 + }, + "constraint": "Gauss", + "name": "syst1", + "type": "normsys" + }, + { + "name": "mu", + "type": "normfactor" + } + ], + "name": "signal" }, - "combined_distributions": { - "main" : { - "distributions": [ - "model_channel1" - ], - "index_cat": "channelCat", - "indices": [0], - "labels": ["channel1"] - } + { + "data": { + "contents": [ + 100, + 0 + ], + "errors": [ + 5, + 0 + ] + }, + "modifiers": [ + { + "data": { + "hi": 1.05, + "lo": 0.95 + }, + "constraint": "Gauss", + "name": "syst2", + "type": "normsys" + }, + { + "name": "mcstat", + "type": "staterror" + } + ], + "name": "background1" }, - "ModelConfigs": { - "main": { - "combined_data_name": "observed", - "mcName": "ModelConfig", - "pdfName": "main" - } + { + "data": { + "contents": [ + 0, + 100 + ], + "errors": [ + 0, + 10 + ] + }, + "modifiers": [ + { + "data": { + "hi": 1.05, + "lo": 0.95 + }, + "constraint": "Gauss", + "name": "syst3", + "type": "normsys" + }, + { + "name": "mcstat", + "type": "staterror" + } + ], + "name": "background2" + } + ], + "type": "histfactory_dist" + } + ], + "data": [ + { + "name": "observed_channel1", + "axes": [ + { + "name": "obs_x_channel1", + "nbins": 2, + "min": 1, + "max": 2 } - } - }, - "analyses": [ - { - "name": "main", - "likelihood": "main_likelihood", - "parameters_of_interest": ["mu"] - } - ], - "likelihoods": [ - { - "name": "main_likelihood", - "distributions": ["model_channel1"], - "data": ["observed_channel1"] - } - ], - "distributions": [ - { - "name": "model_channel1", - "axes": [ - { - "name": "obs_x_channel1", - "max": 2.0, - "min": 1.0, - "nbins": 2 - } - ], - "default_interpolation": {"type": "mult", "in": "poly6", "out": "exp"}, - "samples": [ - { - "data": { - "contents": [20, 10] - }, - "modifiers": [ - { - "data": { - "hi": 1.05, - "lo": 0.95 - }, - "constraint": "Gauss", - "name": "syst1", - "type": "normsys" - }, - { - "name": "mu", - "type": "normfactor" - } - ], - "name": "signal" - }, - { - "data": { - "contents": [100, 0], - "errors": [5, 0] - }, - "modifiers": [ - { - "data": { - "hi": 1.05, - "lo": 0.95 - }, - "constraint": "Gauss", - "name": "syst2", - "type": "normsys" - }, - { - "name": "mcstat", - "type": "staterror" - } - ], - "name": "background1" - }, - { - "data": { - "contents": [0, 100], - "errors": [0, 10] - }, - "modifiers": [ - { - "data": { - "hi": 1.05, - "lo": 0.95 - }, - "constraint": "Gauss", - "name": "syst3", - "type": "normsys" - }, - { - "name": "mcstat", - "type": "staterror" - } - ], - "name": "background2" - } - ], - "type": "histfactory_dist" - } - ], - "data": [ - { - "name": "observed_channel1", - "axes": [ - { - "name": "obs_x_channel1", - "nbins": 2, - "min": 1, - "max": 2 - } - ], - "contents": [122, 112], - "type": "binned" - } - ], - "metadata": { - "hs3_version" : "0.1.90" - } + ], + "contents": [ + 122, + 112 + ], + "type": "binned" + } + ], + "metadata": { + "hs3_version": "0.1.90" + } } From 0d0d4c4ce00dfbc1350b6a27d0ef1030a39b4456 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Thu, 5 Nov 2026 15:08:45 +0000 Subject: [PATCH 20/35] [RF][HS3] Write transformed helper functions once Pdfs that share a parameter, like two RooExponentials with one slope, each wrote the same helper function. The duplicate-name check then rejected the file on import. Assisted-by: claude-code:claude-opus-5-5 --- roofit/hs3/src/RooJSONFactoryWSTool.cxx | 4 ++++ roofit/hs3/test/testRooFitHS3.cxx | 21 +++++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/roofit/hs3/src/RooJSONFactoryWSTool.cxx b/roofit/hs3/src/RooJSONFactoryWSTool.cxx index 9189286fd8f8a..8a482964ba191 100644 --- a/roofit/hs3/src/RooJSONFactoryWSTool.cxx +++ b/roofit/hs3/src/RooJSONFactoryWSTool.cxx @@ -1065,6 +1065,10 @@ std::string RooJSONFactoryWSTool::exportTransformed(const RooAbsReal *original, const std::string &formula) { std::string newname = std::string(original->GetName()) + suffix; + // Another object that shares the parameter might have exported the helper already. + if (findNamedChild((*_rootnodeOutput)["functions"], newname)) { + return newname; + } RooFit::Detail::JSONNode &trafo_node = appendNamedChild((*_rootnodeOutput)["functions"], newname); trafo_node["type"] << "generic"; trafo_node["expression"] << TString::Format(formula.c_str(), original->GetName()).Data(); diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index 99299f37aaf9f..a8003f7cbb1ba 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -3596,3 +3596,24 @@ TEST(RooFitHS3, ConstRooRealVarStaysDistinctFromRooConstVar) // The round trip is stable. EXPECT_EQ(RooJSONFactoryWSTool{imported}.exportJSONtoString(), json); } + +// Pdfs that share a parameter also share the helper functions that are exported for it. +TEST(RooFitHS3, SharedTransformedParameters) +{ + RooHelpers::LocalChangeMsgLevel changeMsgLvl(RooFit::WARNING); + + RooWorkspace wsExponential; + wsExponential.factory("Exponential::exponential_1(x[0, 10], c[-0.1, -5, 0])"); + wsExponential.factory("Exponential::exponential_2(y[0, 10], c)"); + const std::string jsonExponential = RooJSONFactoryWSTool{wsExponential}.exportJSONtoString(); + EXPECT_EQ(countOccurrences(jsonExponential, R"("name":"c_exponential_inverted")"), 1u) << jsonExponential; + EXPECT_EQ(validate(wsExponential, "exponential_1"), 0); + + RooWorkspace wsLognormal; + wsLognormal.factory("Lognormal::lognormal_1(x[2, 1.1, 10], m0[2, 1.1, 10], k[2, 1.1, 5])"); + wsLognormal.factory("Lognormal::lognormal_2(y[2, 1.1, 10], m0, k)"); + const std::string jsonLognormal = RooJSONFactoryWSTool{wsLognormal}.exportJSONtoString(); + EXPECT_EQ(countOccurrences(jsonLognormal, R"("name":"m0_lognormal_log")"), 1u) << jsonLognormal; + EXPECT_EQ(countOccurrences(jsonLognormal, R"("name":"k_lognormal_log")"), 1u) << jsonLognormal; + EXPECT_EQ(validate(wsLognormal, "lognormal_1"), 0); +} From 812ab99ab4e2af5036f73b847c2c7a227b642061 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Thu, 5 Nov 2026 15:09:35 +0000 Subject: [PATCH 21/35] [RF][HS3] Name all analyses after their ModelConfig ModelConfigs sharing a pdf and a dataset, like the usual S+B and B-only pair, got the same analysis name. Only the last one survived, and its domains were written twice, which the duplicate-name check rejects. Assisted-by: claude-code:claude-opus-5-5 --- roofit/hs3/src/RooJSONFactoryWSTool.cxx | 5 ++- roofit/hs3/test/testRooFitHS3.cxx | 44 +++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 3 deletions(-) diff --git a/roofit/hs3/src/RooJSONFactoryWSTool.cxx b/roofit/hs3/src/RooJSONFactoryWSTool.cxx index 8a482964ba191..f1d4d374e702e 100644 --- a/roofit/hs3/src/RooJSONFactoryWSTool.cxx +++ b/roofit/hs3/src/RooJSONFactoryWSTool.cxx @@ -1734,7 +1734,7 @@ void RooJSONFactoryWSTool::exportModelConfig(JSONNode &rootnode, RooFit::ModelCo if (hasdata && !matches(combDataSets.at(i), simpdf)) continue; - std::string analysisName(simpdf->GetName()); + std::string analysisName(mc.GetName()); if (hasdata) analysisName += "_" + combDataSets[i].name; @@ -1747,12 +1747,11 @@ void RooJSONFactoryWSTool::exportModelConfig(JSONNode &rootnode, RooFit::ModelCo if (observables.equals(*(data->get()))) { std::map mapping; mapping[pdf->GetName()] = data->GetName(); - exportSingleModelConfig(rootnode, mc, std::string(pdf->GetName()) + "_" + data->GetName(), &mapping); + exportSingleModelConfig(rootnode, mc, std::string(mc.GetName()) + "_" + data->GetName(), &mapping); ++founddata; } } if (founddata == 0) { - // Named after the ModelConfig, because the name of the pdf is already taken by the distribution. exportSingleModelConfig(rootnode, mc, mc.GetName(), nullptr); } } diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index a8003f7cbb1ba..8631b842fb777 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -3617,3 +3617,47 @@ TEST(RooFitHS3, SharedTransformedParameters) EXPECT_EQ(countOccurrences(jsonLognormal, R"("name":"k_lognormal_log")"), 1u) << jsonLognormal; EXPECT_EQ(validate(wsLognormal, "lognormal_1"), 0); } + +// ModelConfigs that share the pdf and the dataset, like an S+B and B-only pair, are exported as separate analyses. +TEST(RooFitHS3, ModelConfigsSharingPdfAndData) +{ + RooHelpers::LocalChangeMsgLevel changeMsgLvl(RooFit::WARNING); + + auto roundTrip = [](RooWorkspace &ws, const char *pdf, const char *observables) { + for (const char *name : {"ModelConfig", "ModelConfig_with_poi_0"}) { + RooFit::ModelConfig mc{name, &ws}; + mc.SetPdf(pdf); + mc.SetParametersOfInterest("mu"); + mc.SetNuisanceParameters("sigma"); + mc.SetObservables(observables); + ws.import(mc); + } + RooWorkspace imported; + ASSERT_TRUE(RooJSONFactoryWSTool{imported}.importJSONfromString(RooJSONFactoryWSTool{ws}.exportJSONtoString())); + EXPECT_NE(imported.obj("ModelConfig"), nullptr); + EXPECT_NE(imported.obj("ModelConfig_with_poi_0"), nullptr); + }; + + { + RooWorkspace ws; + ws.factory("Gaussian::pdf(x[0, -5, 5], mu[0, -5, 5], sigma[1, 0.1, 5])"); + RooDataSet data{"data", "data", *ws.var("x")}; + data.add(*ws.var("x")); + ws.import(data, RooFit::Silence()); + roundTrip(ws, "pdf", "x"); + } + { + RooWorkspace ws; + ws.factory("Gaussian::pdf_a(x[0, -5, 5], mu[0, -5, 5], sigma[1, 0.1, 5])"); + ws.factory("Gaussian::pdf_b(x, mu, sigma)"); + ws.factory("SIMUL::sim_pdf(channel[a=0, b=1], a=pdf_a, b=pdf_b)"); + RooArgSet observables{*ws.var("x"), *ws.cat("channel")}; + RooDataSet data{"data", "data", observables}; + for (int index : {0, 1}) { + ws.cat("channel")->setIndex(index); + data.add(observables); + } + ws.import(data, RooFit::Silence()); + roundTrip(ws, "sim_pdf", "x,channel"); + } +} From 76dcca68cabb83b4310b711025757fcdd7738f85 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Thu, 5 Nov 2026 15:09:59 +0000 Subject: [PATCH 22/35] [RF][HS3] Reject duplicate names only from HS3 0.3 on RooFit still writes clashing names, for example for a named range and a pdf of the same name, and it labels its files as older than 0.3. Throwing on these made it unable to read its own output, so they only get a warning now, like a missing interpolation. Assisted-by: claude-code:claude-opus-5-5 --- roofit/hs3/src/JSONFactories_HistFactory.cxx | 17 ++----- roofit/hs3/src/JSONIOUtils.cxx | 13 ++++++ roofit/hs3/src/JSONIOUtils.h | 1 + roofit/hs3/src/RooJSONFactoryWSTool.cxx | 9 ++-- roofit/hs3/test/testRooFitHS3.cxx | 47 +++++++++++++++++++- 5 files changed, 67 insertions(+), 20 deletions(-) diff --git a/roofit/hs3/src/JSONFactories_HistFactory.cxx b/roofit/hs3/src/JSONFactories_HistFactory.cxx index dfeb346861173..865128bc101ee 100644 --- a/roofit/hs3/src/JSONFactories_HistFactory.cxx +++ b/roofit/hs3/src/JSONFactories_HistFactory.cxx @@ -337,19 +337,8 @@ std::vector readInterpolations(const JSONNode &object, std::size_t nParamet return codes; } -// Whether an imported document predates HS3 v0.3. Before that, RooFit did not write the interpolation of modifiers -// that used its implicit default. -bool isLegacyHS3Version(std::string const &version) -{ - int major = 0; - int minor = 0; - if (!parseHS3Version(version, major, minor)) { - return true; - } - return std::make_pair(major, minor) <= std::make_pair(0, 2); -} - -// The fallback for modifiers without interpolation in documents that predate HS3 v0.3, see interpolationCode(). +// The fallback for modifiers without interpolation in documents that predate HS3 v0.3, see interpolationCode(). Before +// that, RooFit did not write the interpolation of modifiers that used its implicit default. struct LegacyInterpolationDefault { bool allowed = false; bool warned = false; @@ -970,7 +959,7 @@ class HistFactoryImporter : public RooFit::JSONIO::Importer { } LegacyInterpolationDefault legacyInterpolationDefault; - legacyInterpolationDefault.allowed = isLegacyHS3Version(tool->hs3VersionInput()); + legacyInterpolationDefault.allowed = isHS3VersionBefore(tool->hs3VersionInput(), 0, 3); int idx = 0; RooArgList funcs; diff --git a/roofit/hs3/src/JSONIOUtils.cxx b/roofit/hs3/src/JSONIOUtils.cxx index fa1f8140020f7..d0329ff98ef98 100644 --- a/roofit/hs3/src/JSONIOUtils.cxx +++ b/roofit/hs3/src/JSONIOUtils.cxx @@ -5,6 +5,7 @@ #include #include +#include using RooFit::Detail::JSONNode; using RooFit::Detail::JSONTree; @@ -126,6 +127,18 @@ bool parseHS3Version(std::string const &version, int &major, int &minor) return readNumber(minor); } +// Whether an HS3 version predates `major.minor`. Versions that can't be parsed count as old. That includes the empty +// version of objects that are imported without a document. +bool isHS3VersionBefore(std::string const &version, int major, int minor) +{ + int versionMajor = 0; + int versionMinor = 0; + if (!parseHS3Version(version, versionMajor, versionMinor)) { + return true; + } + return std::make_pair(versionMajor, versionMinor) < std::make_pair(major, minor); +} + // Read the bin edges of an axis, which must be at least two strictly ascending numbers. std::vector readEdges(const JSONNode &node, std::string const &context) { diff --git a/roofit/hs3/src/JSONIOUtils.h b/roofit/hs3/src/JSONIOUtils.h index e09485c0d0dcc..139aa40fd8cfc 100644 --- a/roofit/hs3/src/JSONIOUtils.h +++ b/roofit/hs3/src/JSONIOUtils.h @@ -13,6 +13,7 @@ std::string removeSuffix(std::string_view str, std::string_view suffix); std::unique_ptr varJSONString(const RooFit::Detail::JSONNode &treeRoot); void writeAxisBinning(RooFit::Detail::JSONNode &node, const RooAbsBinning &binning); bool parseHS3Version(std::string const &version, int &major, int &minor); +bool isHS3VersionBefore(std::string const &version, int major, int minor); std::vector readEdges(const RooFit::Detail::JSONNode &node, std::string const &context); #endif diff --git a/roofit/hs3/src/RooJSONFactoryWSTool.cxx b/roofit/hs3/src/RooJSONFactoryWSTool.cxx index f1d4d374e702e..a7e5f263d3573 100644 --- a/roofit/hs3/src/RooJSONFactoryWSTool.cxx +++ b/roofit/hs3/src/RooJSONFactoryWSTool.cxx @@ -825,8 +825,9 @@ 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. -void checkUniqueNames(JSONNode const &rootnode) +/// 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. +void checkUniqueNames(JSONNode const &rootnode, bool strict) { std::map sectionOfName; for (const char *section : @@ -849,7 +850,7 @@ void checkUniqueNames(JSONNode const &rootnode) // 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 (isLegacyNamed(firstSection) || isLegacyNamed(section)) { + if (!strict || 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; @@ -2099,7 +2100,7 @@ void RooJSONFactoryWSTool::importAllNodes(const JSONNode &n) } _hs3VersionInput = versionNode->val(); checkHS3Version(_hs3VersionInput); - checkUniqueNames(n); + checkUniqueNames(n, !isHS3VersionBefore(_hs3VersionInput, 0, 3)); _rootnodeInput = &n; diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index 8631b842fb777..8de78b199c84f 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -2924,7 +2924,7 @@ TEST(RooFitHS3, DuplicateNamesAreRejected) { // The same name used for a distribution and a function. expectImportThrowsWithError(R"({ - "metadata": {"hs3_version": "0.2"}, + "metadata": {"hs3_version": "0.3"}, "distributions": [{"name": "dup", "type": "gaussian_dist", "x": "x", "mean": 0.0, "sigma": 1.0}], "functions": [{"name": "dup", "type": "sum", "summands": ["x"]}] })", @@ -2932,7 +2932,7 @@ TEST(RooFitHS3, DuplicateNamesAreRejected) // The same name used twice within one section. expectImportThrowsWithError(R"({ - "metadata": {"hs3_version": "0.2"}, + "metadata": {"hs3_version": "0.3"}, "parameter_points": [{"name": "pp", "parameters": []}, {"name": "pp", "parameters": []}] })", "duplicate name 'pp'"); @@ -3661,3 +3661,46 @@ TEST(RooFitHS3, ModelConfigsSharingPdfAndData) roundTrip(ws, "sim_pdf", "x,channel"); } } + +// Before HS3 v0.3, name clashes only give a warning. RooFit still writes such documents. +TEST(RooFitHS3, NameClashesInOlderDocuments) +{ + RooHelpers::LocalChangeMsgLevel changeMsgLvl(RooFit::WARNING); + + auto expectImportWarns = [](std::string const &json, std::string const &name) { + RooWorkspace ws; + const std::string warnings = captureMessages( + RooFit::WARNING, RooFit::IO, [&] { EXPECT_TRUE(RooJSONFactoryWSTool{ws}.importJSONfromString(json)); }); + EXPECT_NE(warnings.find("the name '" + name + "' is used"), std::string::npos) << warnings << json; + }; + + expectImportWarns(R"({ + "metadata": {"hs3_version": "0.2"}, + "parameter_points": [{"name": "pp", "parameters": []}, {"name": "pp", "parameters": []}] + })", + "pp"); + + // A named range and a pdf. + { + RooWorkspace ws; + ws.factory("Gaussian::signal(x[0, -5, 5], mu[0, -5, 5], 1.0)"); + ws.var("x")->setRange("signal", -1, 1); + expectImportWarns(RooJSONFactoryWSTool{ws}.exportJSONtoString(), "signal"); + } + // A snapshot and a pdf. + { + RooWorkspace ws; + ws.factory("Gaussian::model(x[0, -5, 5], mu[0, -5, 5], 1.0)"); + ws.saveSnapshot("model", *ws.var("mu")); + expectImportWarns(RooJSONFactoryWSTool{ws}.exportJSONtoString(), "model"); + } + // A dataset and a pdf. + { + RooWorkspace ws; + ws.factory("Gaussian::model(x[0, -5, 5], mu[0, -5, 5], 1.0)"); + RooDataSet data{"model", "model", *ws.var("x")}; + data.add(*ws.var("x")); + ws.import(data, RooFit::Silence()); + expectImportWarns(RooJSONFactoryWSTool{ws}.exportJSONtoString(), "model"); + } +} From 18917bda2e3603e074213a515298586c86b493c0 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Thu, 5 Nov 2026 15:10:14 +0000 Subject: [PATCH 23/35] [RF][HS3] Read infinite bounds of unbinned axes The exporter writes infinite bounds as null, but the new range handling for unbinned axes called val_double() on them. Files with an unbounded observable could not be read anymore. Assisted-by: claude-code:claude-opus-5-5 --- roofit/hs3/src/Domains.cxx | 10 +--------- roofit/hs3/src/JSONIOUtils.cxx | 10 ++++++++++ roofit/hs3/src/JSONIOUtils.h | 1 + roofit/hs3/src/RooJSONFactoryWSTool.cxx | 5 +++-- roofit/hs3/test/testRooFitHS3.cxx | 22 ++++++++++++++++++++++ 5 files changed, 37 insertions(+), 11 deletions(-) diff --git a/roofit/hs3/src/Domains.cxx b/roofit/hs3/src/Domains.cxx index 2910fd85573d9..a68513335d094 100644 --- a/roofit/hs3/src/Domains.cxx +++ b/roofit/hs3/src/Domains.cxx @@ -11,6 +11,7 @@ */ #include "Domains.h" +#include "JSONIOUtils.h" #include #include @@ -29,15 +30,6 @@ constexpr static auto defaultDomainName = "default_domain"; namespace { -double readBound(RooFit::Detail::JSONNode const &node, const char *key, double defaultValue) -{ - if (!node.has_child(key)) { - return defaultValue; - } - auto const &bound = node[key]; - return bound.is_null() ? defaultValue : bound.val_double(); -} - void writeBound(RooFit::Detail::JSONNode &node, double value) { if (RooNumber::isInfinite(value)) { diff --git a/roofit/hs3/src/JSONIOUtils.cxx b/roofit/hs3/src/JSONIOUtils.cxx index d0329ff98ef98..dd634f469444e 100644 --- a/roofit/hs3/src/JSONIOUtils.cxx +++ b/roofit/hs3/src/JSONIOUtils.cxx @@ -104,6 +104,16 @@ void writeAxisBinning(JSONNode &node, const RooAbsBinning &binning) } } +// Read a bound of an axis. Infinite bounds are written as null. +double readBound(JSONNode const &node, const char *key, double defaultValue) +{ + if (!node.has_child(key)) { + return defaultValue; + } + auto const &bound = node[key]; + return bound.is_null() ? defaultValue : bound.val_double(); +} + // Parse the leading "MAJOR.MINOR" of an HS3 version string like "0.2", "0.2.9" or "0.3". bool parseHS3Version(std::string const &version, int &major, int &minor) { diff --git a/roofit/hs3/src/JSONIOUtils.h b/roofit/hs3/src/JSONIOUtils.h index 139aa40fd8cfc..2ac14662be7dc 100644 --- a/roofit/hs3/src/JSONIOUtils.h +++ b/roofit/hs3/src/JSONIOUtils.h @@ -12,6 +12,7 @@ bool endsWith(std::string_view str, std::string_view suffix); std::string removeSuffix(std::string_view str, std::string_view suffix); std::unique_ptr varJSONString(const RooFit::Detail::JSONNode &treeRoot); void writeAxisBinning(RooFit::Detail::JSONNode &node, const RooAbsBinning &binning); +double readBound(RooFit::Detail::JSONNode const &node, const char *key, double defaultValue); bool parseHS3Version(std::string const &version, int &major, int &minor); bool isHS3VersionBefore(std::string const &version, int major, int minor); std::vector readEdges(const RooFit::Detail::JSONNode &node, std::string const &context); diff --git a/roofit/hs3/src/RooJSONFactoryWSTool.cxx b/roofit/hs3/src/RooJSONFactoryWSTool.cxx index a7e5f263d3573..81f3c9248800d 100644 --- a/roofit/hs3/src/RooJSONFactoryWSTool.cxx +++ b/roofit/hs3/src/RooJSONFactoryWSTool.cxx @@ -14,6 +14,7 @@ #include #include +#include #include #include #include @@ -456,8 +457,8 @@ void applyUnbinnedAxisRanges(const JSONNode &p, RooWorkspace &workspace, RooFit: if (!var) { continue; // reported by getObservables() } - const double min = axis["min"].val_double(); - const double max = axis["max"].val_double(); + const double min = readBound(axis, "min", -RooNumber::infinity()); + const double max = readBound(axis, "max", RooNumber::infinity()); if (domains && !domains->hasVariable(name.c_str())) { var->setRange(min, max); } else if (var->getMin() != min || var->getMax() != max) { diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index 8de78b199c84f..6accd75ce7717 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -27,6 +27,7 @@ #include #include #include +#include #include #include #include @@ -3704,3 +3705,24 @@ TEST(RooFitHS3, NameClashesInOlderDocuments) expectImportWarns(RooJSONFactoryWSTool{ws}.exportJSONtoString(), "model"); } } + +// Infinite bounds of unbinned observables are written as null and have to be read back. +TEST(RooFitHS3, UnbinnedDataWithInfiniteBounds) +{ + RooHelpers::LocalChangeMsgLevel changeMsgLvl(RooFit::WARNING); + + RooRealVar x{"x", "x", 1.0, 0.0, RooNumber::infinity()}; + RooRealVar y{"y", "y", 1.0, -RooNumber::infinity(), RooNumber::infinity()}; + RooDataSet ds{"ds", "ds", RooArgSet{x, y}}; + ds.add(RooArgSet{x, y}); + + RooWorkspace ws1; + ws1.import(ds, RooFit::Silence()); + RooWorkspace ws2; + ASSERT_TRUE(RooJSONFactoryWSTool{ws2}.importJSONfromString(RooJSONFactoryWSTool{ws1}.exportJSONtoString())); + EXPECT_EQ(ws2.data("ds")->numEntries(), 1); + EXPECT_DOUBLE_EQ(ws2.var("x")->getMin(), 0.0); + EXPECT_TRUE(RooNumber::isInfinite(ws2.var("x")->getMax())); + EXPECT_TRUE(RooNumber::isInfinite(ws2.var("y")->getMin())); + EXPECT_TRUE(RooNumber::isInfinite(ws2.var("y")->getMax())); +} From 6d5ec9087b556473c178b599ab067b72e349a413 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Thu, 5 Nov 2026 15:11:22 +0000 Subject: [PATCH 24/35] [RF][HS3] Accept a mixture of one extended summand Zero coefficients for one summand also satisfied the check for "one coefficient fewer than summands", so RooFit rejected its own export of a RooAddPdf with a single extended pdf. Assisted-by: claude-code:claude-opus-5-5 --- roofit/hs3/src/JSONFactories_RooFitCore.cxx | 2 +- roofit/hs3/test/testRooFitHS3.cxx | 16 ++++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/roofit/hs3/src/JSONFactories_RooFitCore.cxx b/roofit/hs3/src/JSONFactories_RooFitCore.cxx index 5bb36235425e1..ce7102885bc66 100644 --- a/roofit/hs3/src/JSONFactories_RooFitCore.cxx +++ b/roofit/hs3/src/JSONFactories_RooFitCore.cxx @@ -420,7 +420,7 @@ bool importAddPdf(RooJSONFactoryWSTool *tool, const JSONNode &p) const std::size_t nCoefficients = p.has_child("coefficients") ? p["coefficients"].num_children() : 0; const std::string counts = std::to_string(nCoefficients) + " coefficients for " + std::to_string(nSummands) + " summands"; - if (extended && nCoefficients + 1 == nSummands) { + if (extended && nCoefficients > 0 && nCoefficients + 1 == nSummands) { RooJSONFactoryWSTool::error("mixture_dist '" + name + "' is declared as extended, but it has " + counts + ", which defines a non-extended mixture"); } diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index 6accd75ce7717..9867dcfdcc3a0 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -3726,3 +3726,19 @@ TEST(RooFitHS3, UnbinnedDataWithInfiniteBounds) EXPECT_TRUE(RooNumber::isInfinite(ws2.var("y")->getMin())); EXPECT_TRUE(RooNumber::isInfinite(ws2.var("y")->getMax())); } + +// A RooAddPdf of a single extended pdf has no coefficients, and it is still extended. +TEST(RooFitHS3, MixtureOfOneExtendedSummand) +{ + RooHelpers::LocalChangeMsgLevel changeMsgLvl(RooFit::WARNING); + + RooRealVar x{"x", "x", 0.0, 10.0}; + RooRealVar mean{"mean", "mean", 5.0, 0.0, 10.0}; + RooRealVar sigma{"sigma", "sigma", 1.0, 0.1, 5.0}; + RooRealVar nEvents{"nEvents", "nEvents", 100.0, 0.0, 1000.0}; + RooGaussian gauss{"gauss", "", x, mean, sigma}; + RooExtendPdf extended{"extended", "", gauss, nEvents}; + RooAddPdf mixture{"mixture", "", RooArgList{extended}}; + ASSERT_NE(mixture.extendMode(), RooAbsPdf::CanNotBeExtended); + EXPECT_EQ(validate(mixture), 0); +} From 9e3203c7a00c6017417ff91ec0c0716c4b26e19d Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Thu, 5 Nov 2026 15:11:41 +0000 Subject: [PATCH 25/35] [RF][HS3] Keep the constraint of formula normsys parseOverallModifierFormula() requires a constraint but didn't store it, which only worked because the importer implied a Gaussian. Now that a missing constraint means unconstrained, the export threw instead, claiming that the RooGaussian is not supported. Assisted-by: claude-code:claude-opus-5-5 --- roofit/hs3/src/JSONFactories_HistFactory.cxx | 3 ++ roofit/hs3/test/testRooFitHS3.cxx | 37 ++++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/roofit/hs3/src/JSONFactories_HistFactory.cxx b/roofit/hs3/src/JSONFactories_HistFactory.cxx index 865128bc101ee..67b8a866ab330 100644 --- a/roofit/hs3/src/JSONFactories_HistFactory.cxx +++ b/roofit/hs3/src/JSONFactories_HistFactory.cxx @@ -1263,6 +1263,9 @@ NormSys parseOverallModifierFormula(const std::string &s, RooFormulaVar *formula sys.low = -sign * p2->getVal(); } + // A recognized formula has exactly one constrained parameter. + sys.constraint = constr2 ? constr2 : constr3; + // Preserve the legacy export behaviour for recognized explicit formulae. sys.interpolation = multiplicativePiecewiseExponential; diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index 9867dcfdcc3a0..0ae32a66590f4 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -3742,3 +3742,40 @@ TEST(RooFitHS3, MixtureOfOneExtendedSummand) ASSERT_NE(mixture.extendMode(), RooAbsPdf::CanNotBeExtended); EXPECT_EQ(validate(mixture), 0); } + +namespace { + +// Imports a channel with a normsys that is encoded in an explicit formula and has a RooGaussian constraint. +void importFormulaNormSysChannel(RooWorkspace &ws) +{ + RooRealVar x{"x", "x", 0.0, 2.0}; + x.setBins(2); + RooRealVar alpha{"alpha_sys", "alpha_sys", 0.0, -5.0, 5.0}; + RooRealVar nominal{"nom_alpha_sys", "nom_alpha_sys", 0.0}; + RooRealVar sigma{"sigma_alpha_sys", "sigma_alpha_sys", 1.0}; + RooRealVar mu{"mu", "mu", 1.0, 0.0, 5.0}; + RooHistFunc hist{"hist_sig", "hist_sig", RooArgSet{x}, makeDataHist("hist_sig_data", x, {100.0, 200.0})}; + RooBinWidthFunction binWidth{"sig_binWidth", "sig_binWidth", hist, true}; + RooProduct shapes{"model_ch_sig_shapes", "", RooArgList{hist, binWidth}}; + RooFormulaVar norm{"sig_norm", "(1+alpha_sys*0.1)*mu", RooArgList{alpha, mu}}; + RooRealSumPdf sum{"ch_model", "ch_model", RooArgList{shapes}, RooArgList{norm}, true}; + RooGaussian constraint{"alpha_sysConstraint", "", alpha, nominal, sigma}; + RooProdPdf prod{"model_ch", "model_ch", RooArgSet{constraint}, RooFit::Conditional(sum, x)}; + ws.import(prod, RooFit::Silence()); +} + +} // namespace + +TEST(RooFitHS3, FormulaNormSysConstraint) +{ + RooHelpers::LocalChangeMsgLevel changeMsgLvl(RooFit::WARNING); + + RooWorkspace ws1; + importFormulaNormSysChannel(ws1); + const std::string json = RooJSONFactoryWSTool{ws1}.exportJSONtoString(); + EXPECT_NE(json.find(R"("constraint":"alpha_sysConstraint")"), std::string::npos) << json; + + RooWorkspace ws2; + ASSERT_TRUE(RooJSONFactoryWSTool{ws2}.importJSONfromString(json)); + EXPECT_NE(ws2.pdf("alpha_sysConstraint"), nullptr); +} From d02e8fff283f839e7c9067c8fedb697a656e85b9 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Thu, 5 Nov 2026 15:12:06 +0000 Subject: [PATCH 26/35] [RF][HS3] Fix the factors of formula normsys A formula like 1+0.1*alpha was exported with hi=0.1 and lo=-0.1, but the factors of a normsys are absolute: hi=1.1 and lo=0.9. The re-imported model scaled the sample by 0.1 at alpha=1, and evaluated to NaN for alpha<0. Assisted-by: claude-code:claude-opus-5-5 --- roofit/hs3/src/JSONFactories_HistFactory.cxx | 16 ++++++++-------- roofit/hs3/test/testRooFitHS3.cxx | 20 ++++++++++++++++++++ 2 files changed, 28 insertions(+), 8 deletions(-) diff --git a/roofit/hs3/src/JSONFactories_HistFactory.cxx b/roofit/hs3/src/JSONFactories_HistFactory.cxx index 67b8a866ab330..9b8847cc75a8c 100644 --- a/roofit/hs3/src/JSONFactories_HistFactory.cxx +++ b/roofit/hs3/src/JSONFactories_HistFactory.cxx @@ -1244,23 +1244,23 @@ NormSys parseOverallModifierFormula(const std::string &s, RooFormulaVar *formula if (constr2 && !p3) { sys.name = p2->GetName(); sys.param = p2; - sys.high = sign * toDouble(token3); - sys.low = -sign * toDouble(token3); + sys.high = 1.0 + sign * toDouble(token3); + sys.low = 1.0 - sign * toDouble(token3); } else if (!p2 && constr3) { sys.name = p3->GetName(); sys.param = p3; - sys.high = sign * toDouble(token2); - sys.low = -sign * toDouble(token2); + sys.high = 1.0 + sign * toDouble(token2); + sys.low = 1.0 - sign * toDouble(token2); } else if (constr2 && p3 && !constr3) { sys.name = v2->GetName(); sys.param = v2; - sys.high = sign * p3->getVal(); - sys.low = -sign * p3->getVal(); + sys.high = 1.0 + sign * p3->getVal(); + sys.low = 1.0 - sign * p3->getVal(); } else if (p2 && !constr2 && constr3) { sys.name = v3->GetName(); sys.param = v3; - sys.high = sign * p2->getVal(); - sys.low = -sign * p2->getVal(); + sys.high = 1.0 + sign * p2->getVal(); + sys.low = 1.0 - sign * p2->getVal(); } // A recognized formula has exactly one constrained parameter. diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index 0ae32a66590f4..9c9c59457a16f 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -3779,3 +3779,23 @@ TEST(RooFitHS3, FormulaNormSysConstraint) ASSERT_TRUE(RooJSONFactoryWSTool{ws2}.importJSONfromString(json)); EXPECT_NE(ws2.pdf("alpha_sysConstraint"), nullptr); } + +// The +1 and -1 sigma variations of a normsys that is encoded in a formula survive the round trip. +TEST(RooFitHS3, FormulaNormSysVariations) +{ + RooHelpers::LocalChangeMsgLevel changeMsgLvl(RooFit::WARNING); + + RooWorkspace ws1; + importFormulaNormSysChannel(ws1); + RooWorkspace ws2; + ASSERT_TRUE(RooJSONFactoryWSTool{ws2}.importJSONfromString(RooJSONFactoryWSTool{ws1}.exportJSONtoString())); + + RooArgSet nset1{*ws1.var("x")}; + RooArgSet nset2{*ws2.var("x")}; + for (double alpha : {-1.0, 0.0, 1.0}) { + ws1.var("alpha_sys")->setVal(alpha); + ws2.var("alpha_sys")->setVal(alpha); + EXPECT_NEAR(ws2.pdf("ch_model")->expectedEvents(nset2), ws1.pdf("ch_model")->expectedEvents(nset1), 1e-8) + << "alpha=" << alpha; + } +} From 661aa75b7daf996a51eb9af93041832275e126b1 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Thu, 5 Nov 2026 15:13:26 +0000 Subject: [PATCH 27/35] [RF][HS3] Get the "init" name from the ModelConfig GetSnapshot() was only called as a null check, but it loads the snapshot and restores nothing but the values. After exportJSON(), a fixed parameter was floating and had lost its error and step width. Scanning the snapshot names also dropped or mixed up "init" for ModelConfigs with a common name prefix, like ModelConfig and ModelConfig_with_poi_0, and after several SetSnapshot() calls. Assisted-by: claude-code:claude-opus-5-5 --- roofit/hs3/src/RooJSONFactoryWSTool.cxx | 17 +----- roofit/hs3/test/testRooFitHS3.cxx | 71 ++++++++++++++++++++++ roofit/roofitcore/inc/RooFit/ModelConfig.h | 3 + 3 files changed, 77 insertions(+), 14 deletions(-) diff --git a/roofit/hs3/src/RooJSONFactoryWSTool.cxx b/roofit/hs3/src/RooJSONFactoryWSTool.cxx index 81f3c9248800d..73320ca35bc1a 100644 --- a/roofit/hs3/src/RooJSONFactoryWSTool.cxx +++ b/roofit/hs3/src/RooJSONFactoryWSTool.cxx @@ -1836,20 +1836,9 @@ void RooJSONFactoryWSTool::exportSingleModelConfig(JSONNode &rootnode, RooFit::M writeProductDomain("_global_observables", mc.GetGlobalObservables()); writeProductDomain("_parameters_of_interest", mc.GetParametersOfInterest()); - // ModelConfig::SetSnapshot() saves the snapshot as "__snapshot", and the snapshots are exported - // as parameter points. Reference it as the initial values of the analysis. - if (std::unique_ptr snapshot{mc.GetSnapshot()}) { - const std::string prefix = std::string(mc.GetName()) + "_"; - std::vector candidates; - for (TObject *snsh : _workspace.getSnapshots()) { - const std::string snshName = snsh->GetName(); - if (startsWith(snshName, prefix) && endsWith(snshName, "_snapshot") && _workspace.set(snshName)) { - candidates.push_back(snshName); - } - } - if (candidates.size() == 1) { - analysisNode["init"] << candidates.front(); - } + // The snapshots are exported as parameter points. Reference the one of the ModelConfig as the initial values. + if (_workspace.getSnapshot(mc.GetSnapshotName().c_str())) { + analysisNode["init"] << mc.GetSnapshotName(); } auto &modelConfigAux = getRooFitInternal(rootnode, "ModelConfigs", analysisName); diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index 9c9c59457a16f..73d7cfa82416b 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -3799,3 +3799,74 @@ TEST(RooFitHS3, FormulaNormSysVariations) << "alpha=" << alpha; } } + +// Exporting the snapshot of a ModelConfig doesn't change the variables in the workspace. +TEST(RooFitHS3, ExportKeepsVariableStateWithSnapshot) +{ + RooHelpers::LocalChangeMsgLevel changeMsgLvl(RooFit::WARNING); + + RooWorkspace ws; + ws.factory("Gaussian::pdf(x[0, -5, 5], mu[0, -5, 5], 1.0)"); + RooFit::ModelConfig mc{"ModelConfig", &ws}; + mc.SetPdf("pdf"); + mc.SetParametersOfInterest("mu"); + mc.SetObservables("x"); + RooRealVar &mu = *ws.var("mu"); + mu.setVal(1.5); + mc.SetSnapshot(RooArgSet{mu}); + ws.import(mc); + + mu.setVal(0.0); + mu.setError(0.25); + mu.setConstant(true); + + const std::string json = RooJSONFactoryWSTool{ws}.exportJSONtoString(); + EXPECT_DOUBLE_EQ(mu.getVal(), 0.0); + EXPECT_DOUBLE_EQ(mu.getError(), 0.25); + EXPECT_TRUE(mu.isConstant()); + EXPECT_NE(parameterStepWidthsNode(json).find(R"("step_width":0.25)"), std::string::npos) << json; + EXPECT_EQ(RooJSONFactoryWSTool{ws}.exportJSONtoString(), json); +} + +// The "init" of an analysis is the snapshot of its ModelConfig, whatever other snapshots exist. +TEST(RooFitHS3, AnalysisInitWithSeveralSnapshots) +{ + RooHelpers::LocalChangeMsgLevel changeMsgLvl(RooFit::WARNING); + + RooWorkspace ws; + ws.factory("Gaussian::pdf(x[0, -5, 5], mu[0, -5, 5], 1.0)"); + RooRealVar &mu = *ws.var("mu"); + RooFit::ModelConfig mc{"ModelConfig", &ws}; + // The name of the second ModelConfig starts with the name of the first one. + RooFit::ModelConfig mcBkgOnly{"ModelConfig_with_poi_0", &ws}; + for (RooFit::ModelConfig *config : {&mc, &mcBkgOnly}) { + config->SetPdf("pdf"); + config->SetParametersOfInterest("mu"); + config->SetObservables("x"); + } + + // Only the last call of SetSnapshot() counts. + mu.setVal(1.0); + RooArgSet firstSet{mu}; + firstSet.setName("first"); + mc.SetSnapshot(firstSet); + mu.setVal(1.5); + RooArgSet secondSet{mu}; + secondSet.setName("second"); + mc.SetSnapshot(secondSet); + mu.setVal(0.0); + mcBkgOnly.SetSnapshot(RooArgSet{mu}); + ws.import(mc); + ws.import(mcBkgOnly); + + RooWorkspace imported; + ASSERT_TRUE(RooJSONFactoryWSTool{imported}.importJSONfromString(RooJSONFactoryWSTool{ws}.exportJSONtoString())); + + auto expectSnapshotValue = [&](const char *name, double expected) { + std::unique_ptr snapshot{static_cast(imported.obj(name))->GetSnapshot()}; + ASSERT_NE(snapshot, nullptr) << name; + EXPECT_DOUBLE_EQ(static_cast(snapshot->find("mu"))->getVal(), expected) << name; + }; + expectSnapshotValue("ModelConfig", 1.5); + expectSnapshotValue("ModelConfig_with_poi_0", 0.0); +} diff --git a/roofit/roofitcore/inc/RooFit/ModelConfig.h b/roofit/roofitcore/inc/RooFit/ModelConfig.h index 3f3d9b302cba4..6a6d3018f3713 100644 --- a/roofit/roofitcore/inc/RooFit/ModelConfig.h +++ b/roofit/roofitcore/inc/RooFit/ModelConfig.h @@ -302,6 +302,9 @@ class ModelConfig final : public TNamed, public RooWorkspaceHandle { /// get RooArgSet for parameters for a particular hypothesis (return nullptr if not existing) const RooArgSet *GetSnapshot() const; + /// get the name of the workspace snapshot saved by SetSnapshot() (empty if there is none) + const std::string &GetSnapshotName() const { return fSnapshotName; } + void LoadSnapshot() const; RooWorkspace *GetWS() const override; From d663f1dbe4ed257cfd0bd25b7a3f272bd23bcdd8 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Thu, 5 Nov 2026 15:13:43 +0000 Subject: [PATCH 28/35] [RF][HS3] Refuse normsys with a function as parameter AddLogNormSyst() and AddGammaSyst() of HistFactory::Measurement use a function like alphaOfBeta_x as normsys parameter. The check for unsupported constraints missed this case, so it was exported as a free parameter and the re-imported model silently differed. Assisted-by: claude-code:claude-opus-5-5 --- roofit/hs3/src/JSONFactories_HistFactory.cxx | 6 ++++- roofit/hs3/test/testRooFitHS3.cxx | 25 ++++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/roofit/hs3/src/JSONFactories_HistFactory.cxx b/roofit/hs3/src/JSONFactories_HistFactory.cxx index 9b8847cc75a8c..8eeed2855a642 100644 --- a/roofit/hs3/src/JSONFactories_HistFactory.cxx +++ b/roofit/hs3/src/JSONFactories_HistFactory.cxx @@ -1914,7 +1914,7 @@ std::vector findLostConstraints(const Channel &channel, const std:: } // A normsys or histosys modifier without a recognized constraint is exported as unconstrained. That is only correct if -// none of the other constraint terms of the channel depends on its parameter. +// its parameter is a variable, and none of the other constraint terms of the channel depends on it. void checkUnconstrainedModifiers(const Channel &channel, const std::vector &constraints) { auto check = [&](auto const &modifiers) { @@ -1922,6 +1922,10 @@ void checkUnconstrainedModifiers(const Channel &channel, const std::vectorisConstant()) { continue; } + if (!dynamic_cast(sys.param)) { + RooJSONFactoryWSTool::error("cannot find constraint for " + std::string(sys.param->GetName()) + + ": functions are not supported as parameters of this modifier"); + } for (RooAbsPdf const *pdf : constraints) { if (pdf->dependsOn(*sys.param)) { RooJSONFactoryWSTool::error("cannot find constraint for " + std::string(sys.param->GetName()) + diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index 73d7cfa82416b..c314b6eab4323 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -3870,3 +3870,28 @@ TEST(RooFitHS3, AnalysisInitWithSeveralSnapshots) expectSnapshotValue("ModelConfig", 1.5); expectSnapshotValue("ModelConfig_with_poi_0", 0.0); } + +// A normsys parameter that is a function, as created by HistFactory::Measurement::AddLogNormSyst(), can't be exported. +// It would come back as a free parameter. +TEST(RooFitHS3, NormSysWithFunctionAsParameter) +{ + RooHelpers::LocalChangeMsgLevel changeMsgLvl(RooFit::WARNING); + + RooRealVar x{"x", "x", 0.0, 2.0}; + x.setBins(2); + RooRealVar beta{"beta_sys", "beta_sys", 1.0, 0.0, 5.0}; + RooRealVar nominal{"nom_beta_sys", "nom_beta_sys", 1.0}; + RooRealVar sigma{"sigma_beta_sys", "sigma_beta_sys", 0.05}; + RooFormulaVar alphaOfBeta{"alphaOfBeta_sys", "(beta_sys-1)/0.05", RooArgList{beta}}; + RooStats::HistFactory::FlexibleInterpVar norm{"sig_norm", "", RooArgList{alphaOfBeta}, 1.0, {0.95}, {1.05}}; + RooHistFunc hist{"hist_sig", "hist_sig", RooArgSet{x}, makeDataHist("hist_sig_data", x, {100.0, 200.0})}; + RooBinWidthFunction binWidth{"sig_binWidth", "sig_binWidth", hist, true}; + RooProduct shapes{"model_ch_sig_shapes", "", RooArgList{hist, binWidth}}; + RooRealSumPdf sum{"ch_model", "ch_model", RooArgList{shapes}, RooArgList{norm}, true}; + RooGaussian constraint{"beta_sysConstraint", "", beta, nominal, sigma}; + RooProdPdf prod{"model_ch", "model_ch", RooArgSet{constraint}, RooFit::Conditional(sum, x)}; + + RooWorkspace ws; + ws.import(prod, RooFit::Silence()); + expectExportThrowsWithError(ws, "cannot find constraint for alphaOfBeta_sys"); +} From b70f2e81ec100ab88182ebef4927e436f4e1e017 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Thu, 5 Nov 2026 15:14:12 +0000 Subject: [PATCH 29/35] [RF][HS3] Imply Gaussian constraints before HS3 0.2 The former HistFactory::JSONTool and the old rf515 tutorial file write normsys and histosys modifiers without constraint and with version 0.1.90. These models rely on the implicit unit Gaussian, so dropping it silently changed their fits. Objects imported without a document also keep it. Assisted-by: claude-code:claude-opus-5-5 --- roofit/hs3/src/JSONFactories_HistFactory.cxx | 5 ++-- roofit/hs3/test/testRooFitHS3.cxx | 25 ++++++++++++++++++-- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/roofit/hs3/src/JSONFactories_HistFactory.cxx b/roofit/hs3/src/JSONFactories_HistFactory.cxx index 8eeed2855a642..60d4f495467f4 100644 --- a/roofit/hs3/src/JSONFactories_HistFactory.cxx +++ b/roofit/hs3/src/JSONFactories_HistFactory.cxx @@ -599,11 +599,12 @@ bool hasConstraint(const JSONNode &mod) return mod.has_child("constraint") || mod.has_child("constraint_name") || mod.has_child("constraint_type"); } -// Create the constraint of a normsys or histosys modifier. Without a constraint, the parameter is unconstrained. +// Create the constraint of a normsys or histosys modifier. Without a constraint, the parameter is unconstrained. Only +// documents that predate HS3 v0.2 imply a unit Gaussian, like the ones written by the former HistFactory::JSONTool. void addSysConstraint(RooJSONFactoryWSTool &tool, const JSONNode &mod, RooRealVar ¶m, std::string const &sysname, std::string const &sample, RooArgSet &constraints) { - if (!hasConstraint(mod)) { + if (!hasConstraint(mod) && !isHS3VersionBefore(tool.hs3VersionInput(), 0, 2)) { oocoutI(nullptr, IO) << "RooFitHS3: modifier '" << sysname << "' of sample '" << sample << "' has no constraint, so its parameter '" << param.GetName() << "' is unconstrained" << std::endl; diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index c314b6eab4323..a17349db4b838 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -238,12 +238,14 @@ std::size_t countOccurrences(std::string_view haystack, std::string_view needle) return result; } -std::string makeHistFactoryJSON(std::string const &modifiers, std::string const &defaultInterpolation = "") +std::string makeHistFactoryJSON(std::string const &modifiers, std::string const &defaultInterpolation = "", + std::string const &version = "0.2") { const std::string defaultNode = defaultInterpolation.empty() ? "" : "\"default_interpolation\":" + defaultInterpolation + ","; return R"({ - "metadata": {"hs3_version": "0.2"}, + "metadata": {"hs3_version": ")" + + version + R"("}, "distributions": [ { "name": "model_channel0", @@ -3895,3 +3897,22 @@ TEST(RooFitHS3, NormSysWithFunctionAsParameter) ws.import(prod, RooFit::Silence()); expectExportThrowsWithError(ws, "cannot find constraint for alphaOfBeta_sys"); } + +// Documents that predate HS3 v0.2 imply a unit Gaussian constraint for normsys and histosys modifiers. +TEST(RooFitHS3, HistFactoryImplicitConstraintInOldDocuments) +{ + RooHelpers::LocalChangeMsgLevel changeMsgLvl(RooFit::WARNING); + + const std::string modifiers = R"( + {"name": "norm", "parameter": "alpha_norm", "type": "normsys", "data": {"lo": 0.8, "hi": 1.2}}, + {"name": "shape", "parameter": "alpha_shape", "type": "histosys", + "data": {"lo": {"contents": [8.0, 18.0]}, "hi": {"contents": [12.0, 22.0]}}})"; + const std::string defaultInterpolation = R"({"type": "mult", "in": "poly6", "out": "exp"})"; + + RooWorkspace ws; + ASSERT_TRUE( + RooJSONFactoryWSTool{ws}.importJSONfromString(makeHistFactoryJSON(modifiers, defaultInterpolation, "0.1.90"))); + EXPECT_NE(ws.pdf("alpha_normConstraint"), nullptr); + EXPECT_NE(ws.pdf("alpha_shapeConstraint"), nullptr); + EXPECT_NE(ws.var("nom_alpha_norm"), nullptr); +} From 45c86108b70c788c64675b43f936fb2b2de00676 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Thu, 5 Nov 2026 15:14:37 +0000 Subject: [PATCH 30/35] [RF][HS3] Warn about modifiers without constraint Older RooFit versions constrained these parameters, so a floating one might not be what the author of the document meant. Warn once per channel instead of writing an info message. Assisted-by: claude-code:claude-opus-5-5 --- roofit/hs3/src/JSONFactories_HistFactory.cxx | 23 ++++++++++++-------- roofit/hs3/test/testRooFitHS3.cxx | 15 +++++++++++++ 2 files changed, 29 insertions(+), 9 deletions(-) diff --git a/roofit/hs3/src/JSONFactories_HistFactory.cxx b/roofit/hs3/src/JSONFactories_HistFactory.cxx index 60d4f495467f4..7a6bcbf3717e9 100644 --- a/roofit/hs3/src/JSONFactories_HistFactory.cxx +++ b/roofit/hs3/src/JSONFactories_HistFactory.cxx @@ -32,7 +32,6 @@ #include #include #include -#include #include #include @@ -602,12 +601,16 @@ bool hasConstraint(const JSONNode &mod) // Create the constraint of a normsys or histosys modifier. Without a constraint, the parameter is unconstrained. Only // documents that predate HS3 v0.2 imply a unit Gaussian, like the ones written by the former HistFactory::JSONTool. void addSysConstraint(RooJSONFactoryWSTool &tool, const JSONNode &mod, RooRealVar ¶m, std::string const &sysname, - std::string const &sample, RooArgSet &constraints) + std::string const &sample, bool &warnedUnconstrained, RooArgSet &constraints) { if (!hasConstraint(mod) && !isHS3VersionBefore(tool.hs3VersionInput(), 0, 2)) { - oocoutI(nullptr, IO) << "RooFitHS3: modifier '" << sysname << "' of sample '" << sample - << "' has no constraint, so its parameter '" << param.GetName() << "' is unconstrained" - << std::endl; + if (!warnedUnconstrained) { + RooJSONFactoryWSTool::warning("RooFitHS3: modifier '" + sysname + "' of sample '" + sample + + "' has no constraint, so its parameter '" + param.GetName() + + "' is unconstrained. Older RooFit versions implied a unit Gaussian. Other " + "modifiers of this channel are not reported."); + warnedUnconstrained = true; + } return; } createNominal(*tool.workspace(), param.GetName(), 0.0, -10, 10); @@ -662,7 +665,8 @@ double constraintRelError(RooAbsPdf const &constraint, RooAbsArg const &gamma) bool importHistSample(RooJSONFactoryWSTool &tool, RooDataHist &dh, RooArgSet const &varlist, RooAbsArg const *mcStatObject, const std::string &fprefix, const JSONNode &p, const std::optional &defaultInterpolation, - LegacyInterpolationDefault &legacyInterpolationDefault, RooArgSet &constraints) + LegacyInterpolationDefault &legacyInterpolationDefault, bool &warnedUnconstrained, + RooArgSet &constraints) { RooWorkspace &ws = *tool.workspace(); @@ -745,7 +749,7 @@ bool importHistSample(RooJSONFactoryWSTool &tool, RooDataHist &dh, RooArgSet con overall_high.push_back(high); overall_interp.push_back(interp); - addSysConstraint(tool, mod, par, sysname, sampleName, constraints); + 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); @@ -762,7 +766,7 @@ bool importHistSample(RooJSONFactoryWSTool &tool, RooDataHist &dh, RooArgSet con sampleName + "' of channel '" + channelName + "'"; histoInterp.push_back(interpolationCode(mod, defaultInterpolation, InterpolationClass::Piecewise, context, legacyInterpolationDefault)); - addSysConstraint(tool, mod, par, sysname, sampleName, constraints); + addSysConstraint(tool, mod, par, sysname, sampleName, warnedUnconstrained, constraints); } else if (modtype == "shapesys" || modtype == "shapefactor") { std::string funcName = channelName + "_" + sysname + "_ShapeSys"; // funcName should be "__ShapeSys" @@ -961,13 +965,14 @@ class HistFactoryImporter : public RooFit::JSONIO::Importer { LegacyInterpolationDefault legacyInterpolationDefault; legacyInterpolationDefault.allowed = isHS3VersionBefore(tool->hs3VersionInput(), 0, 3); + bool warnedUnconstrained = 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, constraints); + legacyInterpolationDefault, warnedUnconstrained, constraints); ++idx; std::string const &compName = RooJSONFactoryWSTool::name(comp); diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index a17349db4b838..4d06a4d3c81d6 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -3916,3 +3916,18 @@ TEST(RooFitHS3, HistFactoryImplicitConstraintInOldDocuments) EXPECT_NE(ws.pdf("alpha_shapeConstraint"), nullptr); EXPECT_NE(ws.var("nom_alpha_norm"), nullptr); } + +// Parameters without constraint are reported once per channel, because older RooFit versions constrained them. +TEST(RooFitHS3, HistFactoryUnconstrainedModifierWarning) +{ + RooHelpers::LocalChangeMsgLevel changeMsgLvl(RooFit::WARNING); + + const std::string json = makeHistFactoryJSON(R"( + {"name": "norm", "parameter": "alpha_norm", "type": "normsys", "data": {"lo": 0.8, "hi": 1.2}}, + {"name": "norm2", "parameter": "alpha_norm2", "type": "normsys", "data": {"lo": 0.9, "hi": 1.1}})", + R"({"type": "mult", "in": "poly6", "out": "exp"})"); + RooWorkspace ws; + const std::string warnings = captureMessages( + RooFit::WARNING, RooFit::IO, [&] { EXPECT_TRUE(RooJSONFactoryWSTool{ws}.importJSONfromString(json)); }); + EXPECT_EQ(countOccurrences(warnings, "has no constraint"), 1u) << warnings; +} From b8ad07931e5944347069d6c1e91a238e52f6dd8d Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Thu, 5 Nov 2026 15:14:50 +0000 Subject: [PATCH 31/35] [RF][HS3] Reject log-normal constraints of gammas createPHF() turned everything except "Poisson" and "Const" into a Gaussian. A shapesys with "constraint": "LogNormal", the HS3 spelling, therefore silently got the wrong constraint instead of an error. The older spelling "Lognormal" did the same. Assisted-by: claude-code:claude-opus-5-5 --- roofit/hs3/src/JSONFactories_HistFactory.cxx | 5 ++++ roofit/hs3/test/testRooFitHS3.cxx | 24 ++++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/roofit/hs3/src/JSONFactories_HistFactory.cxx b/roofit/hs3/src/JSONFactories_HistFactory.cxx index 7a6bcbf3717e9..b6186f35d1342 100644 --- a/roofit/hs3/src/JSONFactories_HistFactory.cxx +++ b/roofit/hs3/src/JSONFactories_HistFactory.cxx @@ -520,6 +520,11 @@ ParamHistFunc &createPHF(const std::string &phfname, std::string const &sysname, if (!createConstraints) { configureConstrainedGammas(gammas, vals, minSigma); } else if (constraintType != "Const") { + if (constraintType == "LogNormal" || constraintType == "Lognormal") { + RooJSONFactoryWSTool::error("unsupported constraint '" + constraintType + + "' for the gamma parameters of '" + sysname + + "', supported are \"Poisson\" and \"Gauss\""); + } auto constraintsInfo = createGammaConstraints( gammas, vals, minSigma, constraintType == "Poisson" ? Constraint::Poisson : Constraint::Gaussian); for (auto const &term : constraintsInfo.constraints) { diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index 4d06a4d3c81d6..2e1feed6f4aab 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -3931,3 +3931,27 @@ TEST(RooFitHS3, HistFactoryUnconstrainedModifierWarning) RooFit::WARNING, RooFit::IO, [&] { EXPECT_TRUE(RooJSONFactoryWSTool{ws}.importJSONfromString(json)); }); EXPECT_EQ(countOccurrences(warnings, "has no constraint"), 1u) << warnings; } + +// Gamma parameters can't have log-normal constraints. Requesting one must not silently give a Gaussian. +TEST(RooFitHS3, HistFactoryLogNormalGammaConstraint) +{ + for (std::string type : {"LogNormal", "Lognormal"}) { + for (std::string key : {"constraint", "constraint_type"}) { + expectImportThrowsWithError(makeHistFactoryJSON(R"({"name": "shape", "type": "shapesys", ")" + key + + R"(": ")" + type + R"(", "data": {"vals": [0.1, 0.2]}})"), + "unsupported constraint '" + type + "'"); + } + } + + expectImportThrowsWithError(R"({ + "metadata": {"hs3_version": "0.2"}, + "distributions": [{ + "name": "model_channel0", "type": "histfactory_dist", + "axes": [{"name": "x", "min": 0.0, "max": 2.0, "nbins": 2}], + "staterror": {"constraint_type": "LogNormal"}, + "samples": [{"name": "sig", "data": {"contents": [10.0, 20.0]}, + "modifiers": [{"name": "mcstat", "type": "staterror"}]}] + }] + })", + "unsupported constraint 'LogNormal'"); +} From da1e3a982a94311abf3cd1a2ebb5163878a12e28 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Thu, 5 Nov 2026 15:15:05 +0000 Subject: [PATCH 32/35] [RF][HS3] Read "constraint_type" of staterror modifiers ROOT 6.38 and 6.40.00 wrote this key instead of "constraint", so their Gaussian stat errors still came back as Poisson. Assisted-by: claude-code:claude-opus-5-5 --- roofit/hs3/src/JSONFactories_HistFactory.cxx | 7 ++++++- roofit/hs3/test/testRooFitHS3.cxx | 11 +++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/roofit/hs3/src/JSONFactories_HistFactory.cxx b/roofit/hs3/src/JSONFactories_HistFactory.cxx index b6186f35d1342..c5f34761b65f7 100644 --- a/roofit/hs3/src/JSONFactories_HistFactory.cxx +++ b/roofit/hs3/src/JSONFactories_HistFactory.cxx @@ -929,7 +929,12 @@ class HistFactoryImporter : public RooFit::JSONIO::Importer { } // The constraint type given on the staterror modifier takes precedence over the channel-level setting. if (modifierStatErrType.empty()) { - if (auto constraint = staterror->find("constraint")) { + // ROOT 6.38 and 6.40.00 wrote "constraint_type". + auto constraint = staterror->find("constraint"); + if (!constraint) { + constraint = staterror->find("constraint_type"); + } + if (constraint) { modifierStatErrType = constraint->val(); if (modifierStatErrType != "Poisson" && modifierStatErrType != "Gauss") { RooJSONFactoryWSTool::error("unsupported constraint '" + modifierStatErrType + diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index 2e1feed6f4aab..ff0f98192ed49 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -3955,3 +3955,14 @@ TEST(RooFitHS3, HistFactoryLogNormalGammaConstraint) })", "unsupported constraint 'LogNormal'"); } + +// ROOT 6.38 and 6.40.00 wrote the constraint type of the staterror modifier as "constraint_type". +TEST(RooFitHS3, HistFactoryStatErrorConstraintType) +{ + RooHelpers::LocalChangeMsgLevel changeMsgLvl(RooFit::WARNING); + + RooWorkspace ws; + ASSERT_TRUE(RooJSONFactoryWSTool{ws}.importJSONfromString( + makeHistFactoryJSON(R"({"name": "staterror", "type": "staterror", "constraint_type": "Gauss"})"))); + EXPECT_NE(dynamic_cast(ws.pdf("gamma_stat_channel0_bin_0_constraint")), nullptr); +} From bcaec8b29be7c6d87a08042cbf9db229b3f6fc06 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Thu, 5 Nov 2026 15:15:27 +0000 Subject: [PATCH 33/35] [RF][HS3] Keep all-zero errors of binned data at zero Without any positive error, RooDataHist never started to store squared weights, so "errors": [0, 0] still read back as squared weight = content while [0, 2] was fixed. Switch on the squared weights up front instead of setting the first bin with a positive error first. Assisted-by: claude-code:claude-opus-5-5 --- roofit/hs3/src/RooJSONFactoryWSTool.cxx | 11 ++++------- roofit/hs3/test/testRooFitHS3.cxx | 18 ++++++++++++++++++ 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/roofit/hs3/src/RooJSONFactoryWSTool.cxx b/roofit/hs3/src/RooJSONFactoryWSTool.cxx index 73320ca35bc1a..088fd8dc78a96 100644 --- a/roofit/hs3/src/RooJSONFactoryWSTool.cxx +++ b/roofit/hs3/src/RooJSONFactoryWSTool.cxx @@ -34,7 +34,6 @@ #include #include -#include #include #include #include @@ -1665,12 +1664,10 @@ RooJSONFactoryWSTool::readBinnedData(const JSONNode &n, const std::string &name, errorVals.push_back(err.val_double()); } } - // RooDataHist only starts to store squared weights when it sees the first positive error, and then initializes - // them from the weights of the bins set so far. Set that bin first, so that errors of zero before it stay zero. - auto firstPositive = std::find_if(errorVals.begin(), errorVals.end(), [](double err) { return err > 0.; }); - if (firstPositive != errorVals.end()) { - const std::size_t ibin = std::distance(errorVals.begin(), firstPositive); - dh->set(ibin, contentVals[ibin], errorVals[ibin]); + // RooDataHist only starts to store squared weights when it sees a positive error, and then initializes them from + // the weights so far. Trigger that before setting any weight, so that errors of zero stay zero. + if (!errorVals.empty()) { + dh->set(0, 0., 1.); } for (size_t ibin = 0; ibin < bins.size(); ++ibin) { const double err = errors ? errorVals[ibin] : -1; diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index ff0f98192ed49..c9618af4af642 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -3966,3 +3966,21 @@ TEST(RooFitHS3, HistFactoryStatErrorConstraintType) makeHistFactoryJSON(R"({"name": "staterror", "type": "staterror", "constraint_type": "Gauss"})"))); EXPECT_NE(dynamic_cast(ws.pdf("gamma_stat_channel0_bin_0_constraint")), nullptr); } + +// Errors of zero also stay zero if there is no positive error at all. +TEST(RooFitHS3, BinnedDataAllErrorsZero) +{ + RooHelpers::LocalChangeMsgLevel changeMsgLvl(RooFit::WARNING); + + RooWorkspace ws; + ASSERT_TRUE(RooJSONFactoryWSTool{ws}.importJSONfromString(R"({ + "metadata": {"hs3_version": "0.2"}, + "data": [{"name": "d", "type": "binned", "axes": [{"name": "x", "min": 0.0, "max": 2.0, "nbins": 2}], + "contents": [10.0, 20.0], "errors": [0.0, 0.0]}] + })")); + auto *data = dynamic_cast(ws.data("d")); + ASSERT_NE(data, nullptr); + EXPECT_DOUBLE_EQ(data->weight(1), 20.0); + EXPECT_DOUBLE_EQ(data->weightSquared(0), 0.0); + EXPECT_DOUBLE_EQ(data->weightSquared(1), 0.0); +} From 14aecd7727bfa9ceb2642639bb2f1ff1cf897db5 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Thu, 5 Nov 2026 15:15:44 +0000 Subject: [PATCH 34/35] [RF][HS3] Create categories before parameter points The index categories of combined distributions were created after the parameter points were read. A string value for such a category was dropped, and a numeric value created a RooRealVar that took the place of the category. Assisted-by: claude-code:claude-opus-5-5 --- roofit/hs3/src/RooJSONFactoryWSTool.cxx | 5 +++-- roofit/hs3/test/testRooFitHS3.cxx | 26 +++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/roofit/hs3/src/RooJSONFactoryWSTool.cxx b/roofit/hs3/src/RooJSONFactoryWSTool.cxx index 088fd8dc78a96..1f1f771843443 100644 --- a/roofit/hs3/src/RooJSONFactoryWSTool.cxx +++ b/roofit/hs3/src/RooJSONFactoryWSTool.cxx @@ -2120,6 +2120,9 @@ void RooJSONFactoryWSTool::importAllNodes(const JSONNode &n) this->importDependants(n); + // This creates the index categories, which the parameter points can refer to. + combinePdfs(*_rootnodeInput, _workspace); + if (auto paramPointsNode = n.find("parameter_points")) { for (const auto &snsh : paramPointsNode->children()) { std::string name = RooJSONFactoryWSTool::name(snsh); @@ -2164,8 +2167,6 @@ void RooJSONFactoryWSTool::importAllNodes(const JSONNode &n) } } - combinePdfs(*_rootnodeInput, _workspace); - // Import attributes if (_attributesNode) { for (const auto &elem : _attributesNode->children()) { diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index c9618af4af642..f2eb003793d40 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -3984,3 +3984,29 @@ TEST(RooFitHS3, BinnedDataAllErrorsZero) EXPECT_DOUBLE_EQ(data->weightSquared(0), 0.0); EXPECT_DOUBLE_EQ(data->weightSquared(1), 0.0); } + +// Parameter points can set the index categories of combined distributions in the same document. +TEST(RooFitHS3, ParameterPointsWithDocumentCategories) +{ + RooHelpers::LocalChangeMsgLevel changeMsgLvl(RooFit::WARNING); + + for (std::string value : {R"("b")", "1"}) { + RooWorkspace ws; + ASSERT_TRUE(RooJSONFactoryWSTool{ws}.importJSONfromString(R"({ + "metadata": {"hs3_version": "0.2"}, + "domains": [{"name": "default_domain", "type": "product_domain", "axes": [{"name": "x", "min": -5, "max": 5}]}], + "distributions": [ + {"name": "pdf_a", "type": "gaussian_dist", "x": "x", "mean": 0.0, "sigma": 1.0}, + {"name": "pdf_b", "type": "gaussian_dist", "x": "x", "mean": 1.0, "sigma": 1.0} + ], + "parameter_points": [{"name": "point", "parameters": [{"name": "channel", "value": )" + + value + R"(}]}], + "misc": {"ROOT_internal": {"combined_distributions": {"sim_pdf": { + "distributions": ["pdf_a", "pdf_b"], "index_cat": "channel", "indices": [0, 1], "labels": ["a", "b"]}}}} + })")); + ASSERT_NE(ws.cat("channel"), nullptr) << value; + auto *channel = dynamic_cast(ws.getSnapshot("point")->find("channel")); + ASSERT_NE(channel, nullptr) << value; + EXPECT_EQ(channel->getCurrentLabel(), std::string("b")) << value; + } +} From 361e68aa70a971cc635019a43a8738327923224a Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Thu, 5 Nov 2026 15:16:01 +0000 Subject: [PATCH 35/35] [RF][HS3] Log warnings on the ObjectHandling topic RooJSONFactoryWSTool::warning() logged on RooFit::IO, which no default stream covers. Users therefore never saw these warnings, for example about newer HS3 versions, deprecated type keys, name clashes or lost constraints. The tests only did because they hijack the stream. Assisted-by: claude-code:claude-opus-5-5 --- roofit/hs3/src/RooJSONFactoryWSTool.cxx | 4 +- roofit/hs3/test/testRooFitHS3.cxx | 63 ++++++++++++++++--------- 2 files changed, 43 insertions(+), 24 deletions(-) diff --git a/roofit/hs3/src/RooJSONFactoryWSTool.cxx b/roofit/hs3/src/RooJSONFactoryWSTool.cxx index 1f1f771843443..9f372161b8e7c 100644 --- a/roofit/hs3/src/RooJSONFactoryWSTool.cxx +++ b/roofit/hs3/src/RooJSONFactoryWSTool.cxx @@ -1415,7 +1415,7 @@ std::string makeValidNameOrError(std::string const &in) } std::string out = RooFit::Detail::makeValidVarName(in); if (out != in) { - oocoutW(nullptr, IO) << "RooFitHS3: changed '" << in << "' to '" << out << "' to become a valid name"; + oocoutW(nullptr, ObjectHandling) << "RooFitHS3: changed '" << in << "' to '" << out << "' to become a valid name"; } return out; } @@ -2336,7 +2336,7 @@ void RooJSONFactoryWSTool::importVariableElement(const JSONNode &elementNode) */ std::ostream &RooJSONFactoryWSTool::warning(std::string const &str) { - return RooMsgService::instance().log(nullptr, RooFit::MsgLevel::WARNING, RooFit::IO) << str << std::endl; + return RooMsgService::instance().log(nullptr, RooFit::MsgLevel::WARNING, RooFit::ObjectHandling) << str << std::endl; } /** diff --git a/roofit/hs3/test/testRooFitHS3.cxx b/roofit/hs3/test/testRooFitHS3.cxx index f2eb003793d40..560aea148a51b 100644 --- a/roofit/hs3/test/testRooFitHS3.cxx +++ b/roofit/hs3/test/testRooFitHS3.cxx @@ -2326,8 +2326,8 @@ TEST(RooFitHS3, HistFactoryDuplicateModifiersAreCombined) ASSERT_TRUE(RooJSONFactoryWSTool{ws1}.importJSONfromString(jsonStr)); std::string exported; - const std::string warnings = - captureMessages(RooFit::WARNING, RooFit::IO, [&] { exported = RooJSONFactoryWSTool{ws1}.exportJSONtoString(); }); + const std::string warnings = captureMessages(RooFit::WARNING, RooFit::ObjectHandling, + [&] { exported = RooJSONFactoryWSTool{ws1}.exportJSONtoString(); }); EXPECT_NE(warnings.find("combined 2 duplicate modifiers named 'norm' of type 'normsys'"), std::string::npos) << warnings; @@ -2405,8 +2405,9 @@ TEST(RooFitHS3, HistFactoryDuplicateModifiersAreCombined) } // The operation must be logged as a warning, not as an error. - const std::string sentinelErrors = captureMessages( - RooFit::ERROR, RooFit::IO, [] { RooJSONFactoryWSTool::warning("duplicate-modifier warning-level sentinel"); }); + const std::string sentinelErrors = captureMessages(RooFit::ERROR, RooFit::ObjectHandling, [] { + RooJSONFactoryWSTool::warning("duplicate-modifier warning-level sentinel"); + }); EXPECT_TRUE(sentinelErrors.empty()) << sentinelErrors; } @@ -2912,13 +2913,15 @@ TEST(RooFitHS3, VersionStringHandling) expectImportThrowsWithError(R"({"metadata": {"hs3_version": 0.2}})", "must be a string"); std::string warnings = - captureMessages(RooFit::WARNING, RooFit::IO, [&] { EXPECT_TRUE(importWithVersion("\"0.2\"")); }); + captureMessages(RooFit::WARNING, RooFit::ObjectHandling, [&] { EXPECT_TRUE(importWithVersion("\"0.2\"")); }); EXPECT_EQ(warnings.find("hs3_version"), std::string::npos) << warnings; - warnings = captureMessages(RooFit::WARNING, RooFit::IO, [&] { EXPECT_TRUE(importWithVersion("\"99.1.4\"")); }); + warnings = + captureMessages(RooFit::WARNING, RooFit::ObjectHandling, [&] { EXPECT_TRUE(importWithVersion("\"99.1.4\"")); }); EXPECT_NE(warnings.find("'99.1.4', which is newer"), std::string::npos) << warnings; - warnings = captureMessages(RooFit::WARNING, RooFit::IO, [&] { EXPECT_TRUE(importWithVersion("\"latest\"")); }); + warnings = + captureMessages(RooFit::WARNING, RooFit::ObjectHandling, [&] { EXPECT_TRUE(importWithVersion("\"latest\"")); }); EXPECT_NE(warnings.find("cannot parse hs3_version 'latest'"), std::string::npos) << warnings; } @@ -2951,8 +2954,9 @@ TEST(RooFitHS3, DuplicateNamesAreRejected) const std::string json = RooJSONFactoryWSTool{ws}.exportJSONtoString(); EXPECT_NE(json.find(R"("likelihood":"ModelConfig_likelihood")"), std::string::npos) << json; RooWorkspace ws2; - const std::string warnings = captureMessages( - RooFit::WARNING, RooFit::IO, [&] { EXPECT_TRUE(RooJSONFactoryWSTool{ws2}.importJSONfromString(json)); }); + const std::string warnings = captureMessages(RooFit::WARNING, RooFit::ObjectHandling, [&] { + EXPECT_TRUE(RooJSONFactoryWSTool{ws2}.importJSONfromString(json)); + }); EXPECT_EQ(warnings.find("not allowed by the HS3 standard"), std::string::npos) << warnings; // Files written by older ROOT versions named the likelihood (and the analysis) after the pdf. They can still be @@ -2963,8 +2967,9 @@ TEST(RooFitHS3, DuplicateNamesAreRejected) legacyJson.replace(pos, likelihoodName.size(), "pdf"); } RooWorkspace ws3; - const std::string legacyWarnings = captureMessages( - RooFit::WARNING, RooFit::IO, [&] { EXPECT_TRUE(RooJSONFactoryWSTool{ws3}.importJSONfromString(legacyJson)); }); + const std::string legacyWarnings = captureMessages(RooFit::WARNING, RooFit::ObjectHandling, [&] { + EXPECT_TRUE(RooJSONFactoryWSTool{ws3}.importJSONfromString(legacyJson)); + }); EXPECT_NE(legacyWarnings.find(R"(the name 'pdf' is used in "distributions" and in "likelihoods")"), std::string::npos) << legacyWarnings << legacyJson; @@ -3125,8 +3130,9 @@ TEST(RooFitHS3, AnalysisInitRoundTrip) missingInit.replace(missingInit.find(R"("init":"ModelConfig_POI_snapshot")"), std::string(R"("init":"ModelConfig_POI_snapshot")").size(), R"("init":"does_not_exist")"); RooWorkspace ws3; - const std::string warnings = captureMessages( - RooFit::WARNING, RooFit::IO, [&] { EXPECT_TRUE(RooJSONFactoryWSTool{ws3}.importJSONfromString(missingInit)); }); + const std::string warnings = captureMessages(RooFit::WARNING, RooFit::ObjectHandling, [&] { + EXPECT_TRUE(RooJSONFactoryWSTool{ws3}.importJSONfromString(missingInit)); + }); EXPECT_NE(warnings.find("'does_not_exist' used as \"init\""), std::string::npos) << warnings; // The snapshot of an unnamed set is called "__snapshot", which must be stable as well. @@ -3175,7 +3181,7 @@ TEST(RooFitHS3, TypeKeysMatchTheCatalog) EXPECT_NE(json.find(R"("type":"argus_dist")"), std::string::npos) << json; } std::unique_ptr argus; - const std::string argusWarnings = captureMessages(RooFit::WARNING, RooFit::IO, [&] { + const std::string argusWarnings = captureMessages(RooFit::WARNING, RooFit::ObjectHandling, [&] { argus = importPdf( R"({"name": "argus", "type": "ARGUS_dist", "mass": "m", "resonance": 9.0, "slope": -1.0, "power": 0.5})"); }); @@ -3275,7 +3281,7 @@ TEST(RooFitHS3, MixtureDistExtendedFlag) expectImportThrowsWithError(makeJSON(R"(["f"])", "true"), "is declared as extended, but it has 1 coefficients"); RooWorkspace ws; - const std::string warnings = captureMessages(RooFit::WARNING, RooFit::IO, [&] { + const std::string warnings = captureMessages(RooFit::WARNING, RooFit::ObjectHandling, [&] { EXPECT_TRUE(RooJSONFactoryWSTool{ws}.importJSONfromString(makeJSON(R"(["n1", "n2"])", "false"))); }); EXPECT_NE(warnings.find("is declared as not extended"), std::string::npos) << warnings; @@ -3426,8 +3432,9 @@ TEST(RooFitHS3, HistFactoryMissingInterpolation) "'channel0' is not given, and the channel has no \"default_interpolation\""); RooWorkspace ws; - const std::string warnings = captureMessages( - RooFit::WARNING, RooFit::IO, [&] { ASSERT_TRUE(RooJSONFactoryWSTool{ws}.importJSONfromString(json)); }); + const std::string warnings = captureMessages(RooFit::WARNING, RooFit::ObjectHandling, [&] { + ASSERT_TRUE(RooJSONFactoryWSTool{ws}.importJSONfromString(json)); + }); EXPECT_EQ(countOccurrences(warnings, "implicit default of older RooFit versions"), 1u) << warnings; auto *norm = dynamic_cast(ws.function("sig_channel0_epsilon")); ASSERT_NE(norm, nullptr); @@ -3500,7 +3507,7 @@ TEST(RooFitHS3, UnbinnedAxisRangesOnImport) { // A disagreeing domain range is kept, with a warning. RooWorkspace ws; - const std::string warnings = captureMessages(RooFit::WARNING, RooFit::IO, [&] { + const std::string warnings = captureMessages(RooFit::WARNING, RooFit::ObjectHandling, [&] { EXPECT_TRUE( RooJSONFactoryWSTool{ws}.importJSONfromString(makeJSON(R"({"name": "x", "min": -10, "max": 10})"))); }); @@ -3672,8 +3679,9 @@ TEST(RooFitHS3, NameClashesInOlderDocuments) auto expectImportWarns = [](std::string const &json, std::string const &name) { RooWorkspace ws; - const std::string warnings = captureMessages( - RooFit::WARNING, RooFit::IO, [&] { EXPECT_TRUE(RooJSONFactoryWSTool{ws}.importJSONfromString(json)); }); + const std::string warnings = captureMessages(RooFit::WARNING, RooFit::ObjectHandling, [&] { + EXPECT_TRUE(RooJSONFactoryWSTool{ws}.importJSONfromString(json)); + }); EXPECT_NE(warnings.find("the name '" + name + "' is used"), std::string::npos) << warnings << json; }; @@ -3927,8 +3935,9 @@ TEST(RooFitHS3, HistFactoryUnconstrainedModifierWarning) {"name": "norm2", "parameter": "alpha_norm2", "type": "normsys", "data": {"lo": 0.9, "hi": 1.1}})", R"({"type": "mult", "in": "poly6", "out": "exp"})"); RooWorkspace ws; - const std::string warnings = captureMessages( - RooFit::WARNING, RooFit::IO, [&] { EXPECT_TRUE(RooJSONFactoryWSTool{ws}.importJSONfromString(json)); }); + const std::string warnings = captureMessages(RooFit::WARNING, RooFit::ObjectHandling, [&] { + EXPECT_TRUE(RooJSONFactoryWSTool{ws}.importJSONfromString(json)); + }); EXPECT_EQ(countOccurrences(warnings, "has no constraint"), 1u) << warnings; } @@ -4010,3 +4019,13 @@ TEST(RooFitHS3, ParameterPointsWithDocumentCategories) EXPECT_EQ(channel->getCurrentLabel(), std::string("b")) << value; } } + +// The warnings of the RooJSONFactoryWSTool are shown with the default message streams. +TEST(RooFitHS3, WarningsAreShownByDefault) +{ + EXPECT_TRUE( + RooMsgService::instance().isActive(static_cast(nullptr), RooFit::ObjectHandling, RooFit::WARNING)); + const std::string warnings = + captureMessages(RooFit::WARNING, RooFit::ObjectHandling, [] { RooJSONFactoryWSTool::warning("test message"); }); + EXPECT_NE(warnings.find("test message"), std::string::npos) << warnings; +}