From 76b2dda8401d433303d877f9939de04344443f00 Mon Sep 17 00:00:00 2001 From: Carsten Burgard Date: Sun, 25 Oct 2026 23:21:38 +0100 Subject: [PATCH] [RF][HS3] Pin HS3TestSuite fixtures and runner, add expected-failure list The HS3 conformance suite was split upstream: the fixtures stay in HS3TestSuite, while the runner moved to HS3TestSuiteCode with a new backend plugin interface. Fetch and pin both repositories, at their current master HEADs: HS3TestSuite 9f799b27f63b40d1ba96ff66d14d61787892606d HS3TestSuiteCode 36aeb1c67b14dbbd9d9e9b79a4bad561c5e6dac7 Adapt the RooFit backend to the new interface (multi-parameter scans, likelihood targets, pdf_scan and function_scan checks). If the repositories can't be reached at configuration time, fail with a message explaining the alternatives instead of a FetchContent error. Add roofit/hs3/test/hs3testsuite_expected_failures.txt. Checks listed there, and checks marked xfail for RooFit in the upstream manifest, run as expected failures. Unexpected successes and stale entries fail the test, so the list can only shrink. It starts out empty: all checks pass except rf209_anaconv, which is already xfail upstream. Document the option, the pins and the list in roofit_hs3.md. Assisted-by: claude-code:claude-opus-5-5 --- roofit/doc/developers/roofit_hs3.md | 43 +++++++++ roofit/hs3/test/CMakeLists.txt | 42 +++++++-- .../test/hs3testsuite_expected_failures.txt | 8 ++ .../hs3/test/hs3testsuite_roofit_backend.py | 87 ++++++++++++++----- roofit/hs3/test/test_hs3_suite.py | 51 ++++++++--- 5 files changed, 193 insertions(+), 38 deletions(-) create mode 100644 roofit/hs3/test/hs3testsuite_expected_failures.txt diff --git a/roofit/doc/developers/roofit_hs3.md b/roofit/doc/developers/roofit_hs3.md index 5f718110a40fd..b3bd5c68da938 100644 --- a/roofit/doc/developers/roofit_hs3.md +++ b/roofit/doc/developers/roofit_hs3.md @@ -196,6 +196,49 @@ 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. +### Conformance testing + +The HS3 conformance test suite checks that RooFit imports and evaluates a +set of frozen HS3 files like the reference implementation. It consists of +two repositories: the fixtures in +[HS3TestSuite](https://github.com/hep-statistics-serialization-standard/HS3TestSuite) +and the Python runner in +[HS3TestSuiteCode](https://github.com/hep-statistics-serialization-standard/HS3TestSuiteCode). + +The suite is enabled with the CMake option `-Dtest_roofit_hs3testsuite=ON`, +which requires `-Dtesting=ON`, `-Dpyroot=ON` and the `jsonschema` Python +package. Both repositories are fetched at configuration time, so the option +requires network access; alternatively, point +`FETCHCONTENT_SOURCE_DIR_HS3TESTSUITE` and +`FETCHCONTENT_SOURCE_DIR_HS3TESTSUITECODE` to local checkouts. The option is +enabled in the ROOT CI. Run the suite with + +``` {.sh} + ctest -R roofit-hs3-hs3-suite --output-on-failure +``` + +Each check of each fixture is reported as a separate test case by +`roofit/hs3/test/test_hs3_suite.py`, which uses +`roofit/hs3/test/hs3testsuite_roofit_backend.py` to run the checks with +RooFit. + +Both repositories are pinned to explicit commits in +`roofit/hs3/test/CMakeLists.txt`. Checks that are known to fail with the +current RooFit are listed in +`roofit/hs3/test/hs3testsuite_expected_failures.txt`, one +`::` per line, together with a one-line reason. They are +run as expected failures: if one of them starts to pass, or if an entry +matches no check, the test fails, so that the list can only shrink. When you +fix a conformance issue in RooFit, remove the corresponding entries in the +same pull request. + +To bump the pinned commits, update `hs3testsuite_tag` and +`hs3testsuitecode_tag` in +`roofit/hs3/test/CMakeLists.txt`, re-run the suite and update the list of +expected failures accordingly. Make sure the runner and the fixtures are +compatible, and adapt the RooFit backend if the backend interface of the +runner has changed. + ### Contributing If you encounter a missing importer or exporter, please consider diff --git a/roofit/hs3/test/CMakeLists.txt b/roofit/hs3/test/CMakeLists.txt index 4a7b26b32d6c1..ad974c4bdb254 100644 --- a/roofit/hs3/test/CMakeLists.txt +++ b/roofit/hs3/test/CMakeLists.txt @@ -3,18 +3,44 @@ ROOT_ADD_GTEST(testRooFitHS3 testRooFitHS3.cxx LIBRARIES RooFitCore RooFit RooFi ROOT_ADD_GTEST(testJSONInterface testJSONInterface.cxx LIBRARIES RooFitHS3 Matrix) if(test_roofit_hs3testsuite) + # The HS3 conformance test suite is split into two repositories: the frozen fixtures + # (HS3TestSuite) and the Python runner (HS3TestSuiteCode). Both are pinned to explicit + # commits. See roofit/doc/developers/roofit_hs3.md ("Conformance testing") for how to + # bump them together with hs3testsuite_expected_failures.txt. + set(hs3testsuite_url https://github.com/hep-statistics-serialization-standard/HS3TestSuite.git) + set(hs3testsuite_tag 9f799b27f63b40d1ba96ff66d14d61787892606d) + set(hs3testsuitecode_url https://github.com/hep-statistics-serialization-standard/HS3TestSuiteCode.git) + set(hs3testsuitecode_tag 36aeb1c67b14dbbd9d9e9b79a4bad561c5e6dac7) + include(FetchContent) - FetchContent_Declare( - hs3testsuite - GIT_REPOSITORY https://github.com/hep-statistics-serialization-standard/HS3TestSuite.git - GIT_TAG b2e5f19023025d03b7d2239154a02e69f43a1374 - ) # suppress git interactive prompt so fetch fails fast if the repo is gone or unreachable set(ENV{GIT_TERMINAL_PROMPT} "0") - FetchContent_MakeAvailable(hs3testsuite) - ROOT_ADD_PYUNITTEST(hs3-suite test_hs3_suite.py - PYTHON_DEPS jsonschema + foreach(repo hs3testsuite hs3testsuitecode) + string(TOUPPER ${repo} repo_upper) + # Probe the network first if the sources still have to be downloaded, so that an + # offline configuration fails with an actionable message instead of a git error + # from the FetchContent sub-build. + if(NOT FETCHCONTENT_FULLY_DISCONNECTED AND NOT FETCHCONTENT_SOURCE_DIR_${repo_upper} + AND NOT EXISTS ${FETCHCONTENT_BASE_DIR}/${repo}-src/.git) + execute_process(COMMAND ${GIT_EXECUTABLE} ls-remote --exit-code ${${repo}_url} HEAD + RESULT_VARIABLE ls_remote_result OUTPUT_QUIET ERROR_QUIET TIMEOUT 60) + if(NOT ls_remote_result EQUAL 0) + message(FATAL_ERROR "-Dtest_roofit_hs3testsuite=ON requires network access at configuration " + "time, but ${${repo}_url} could not be reached. Configure with network " + "access, provide a local checkout of commit ${${repo}_tag} with " + "-DFETCHCONTENT_SOURCE_DIR_${repo_upper}=, or disable the HS3 test " + "suite with -Dtest_roofit_hs3testsuite=OFF.") + endif() + endif() + FetchContent_Declare(${repo} GIT_REPOSITORY ${${repo}_url} GIT_TAG ${${repo}_tag}) + endforeach() + FetchContent_MakeAvailable(hs3testsuite hs3testsuitecode) + + ROOT_ADD_PYUNITTEST(hs3-suite test_hs3_suite.py + PYTHON_DEPS jsonschema ENVIRONMENT HS3TESTSUITE_ROOT=${hs3testsuite_SOURCE_DIR} + HS3TESTSUITECODE_ROOT=${hs3testsuitecode_SOURCE_DIR} + HS3TESTSUITE_EXPECTED_FAILURES=${CMAKE_CURRENT_SOURCE_DIR}/hs3testsuite_expected_failures.txt COPY_TO_BUILDDIR hs3testsuite_roofit_backend.py ) endif() diff --git a/roofit/hs3/test/hs3testsuite_expected_failures.txt b/roofit/hs3/test/hs3testsuite_expected_failures.txt new file mode 100644 index 0000000000000..806b3217fcf56 --- /dev/null +++ b/roofit/hs3/test/hs3testsuite_expected_failures.txt @@ -0,0 +1,8 @@ +# Checks of the HS3 conformance test suite that are known to fail with the +# current RooFit. See roofit/doc/developers/roofit_hs3.md, "Conformance testing". +# +# Format: one check per line, :: # +# +# These checks are run as expected failures: if one of them starts to pass, or +# if an entry matches no check, the test fails. Remove entries in the same pull +# request that fixes them. diff --git a/roofit/hs3/test/hs3testsuite_roofit_backend.py b/roofit/hs3/test/hs3testsuite_roofit_backend.py index 47fa56e77ded5..a3102ba9c7a8a 100644 --- a/roofit/hs3/test/hs3testsuite_roofit_backend.py +++ b/roofit/hs3/test/hs3testsuite_roofit_backend.py @@ -1,5 +1,6 @@ from __future__ import annotations +import json import os import sys from contextlib import contextmanager @@ -41,40 +42,86 @@ def run_structure_check(self, workspace, check: dict[str, Any]) -> None: if missing: raise AssertionError(f"missing {key}: {sorted(missing)}") - def run_twice_delta_nll_scan(self, workspace, check: dict[str, Any]) -> list[float]: - target = check["target"] - pdf = workspace.pdf(target["pdf"]) - if pdf is None: - raise AssertionError(f"PDF {target['pdf']!r} not found") - data = workspace.data(target["data"]) - if data is None: - raise AssertionError(f"data {target['data']!r} not found") + def run_twice_delta_nll_scan(self, workspace, check: dict[str, Any], hs3_path: Path) -> list[float]: + pdf_data_objs = [] + for pdf_name, data_name in self._resolve_target_pairs(check["target"], hs3_path): + pdf = workspace.pdf(pdf_name) + if not pdf: + raise AssertionError(f"PDF {pdf_name!r} not found") + data = workspace.data(data_name) + if not data: + raise AssertionError(f"data {data_name!r} not found") + pdf_data_objs.append((pdf, data)) self._apply_parameter_point(workspace, check["reference_point"]) with suppress_root_output(): - nll = pdf.createNLL( - data, - self.ROOT.RooFit.NumCPU(1), - self.ROOT.RooFit.EvalBackend("legacy"), - ) + nlls = [ + pdf.createNLL(data, self.ROOT.RooFit.NumCPU(1), self.ROOT.RooFit.EvalBackend("legacy")) + for pdf, data in pdf_data_objs + ] + if len(nlls) == 1: + nll = nlls[0] + else: + nll = self.ROOT.RooAddition("hs3suite_nll", "hs3suite_nll", self.ROOT.RooArgList(*nlls)) reference = float(nll.getVal()) + return [2.0 * (value - reference) for value in self._scan(workspace, check, lambda: float(nll.getVal()))] + + def run_pdf_scan(self, workspace, check: dict[str, Any], hs3_path: Path) -> list[float]: + target = check["target"] + pdf = workspace.pdf(target["pdf"]) + if not pdf: + raise AssertionError(f"PDF {target['pdf']!r} not found") + norm_set = self.ROOT.RooArgSet() + for name in target["observables"]: + observable = workspace.var(name) + if not observable: + raise AssertionError(f"observable {name!r} not found") + norm_set.add(observable) + return self._scan(workspace, check, lambda: float(pdf.getVal(norm_set))) + + def run_function_scan(self, workspace, check: dict[str, Any], hs3_path: Path) -> list[float]: + name = check["target"]["function"] + func = workspace.function(name) or workspace.arg(name) + if not func: + raise AssertionError(f"function {name!r} not found") + return self._scan(workspace, check, lambda: float(func.getVal())) + + def _scan(self, workspace, check: dict[str, Any], evaluate) -> list[float]: values = [] - scan_parameter = check["scan_parameter"] + scan_parameters = check["scan_parameters"] for point in check["scan_points"]: self._apply_parameter_point(workspace, check["reference_point"]) - var = workspace.var(scan_parameter) - if var is None: - raise AssertionError(f"scan parameter {scan_parameter!r} not found") - var.setVal(float(point)) + self._apply_parameter_point(workspace, dict(zip(scan_parameters, point, strict=True))) with suppress_root_output(): - values.append(2.0 * (float(nll.getVal()) - reference)) + values.append(evaluate()) return values + def _resolve_target_pairs(self, target: dict[str, Any], hs3_path: Path) -> list[tuple[str, str]]: + if "likelihood" not in target: + return [(target["pdf"], target["data"])] + with open(hs3_path, encoding="utf-8") as f: + likelihoods = json.load(f).get("likelihoods", []) + name = target["likelihood"] + entry = next((e for e in likelihoods if e.get("name") == name), None) + if entry is None: + raise AssertionError(f"likelihood {name!r} not found in {hs3_path}") + if len(entry["distributions"]) != len(entry["data"]): + raise AssertionError(f"likelihood {name!r}: distributions/data length mismatch") + return list(zip(entry["distributions"], entry["data"])) + def _apply_parameter_point(self, workspace, values: dict[str, float]) -> None: for name, value in values.items(): var = workspace.var(name) - if var is not None: + if var: var.setVal(float(value)) + continue + arg = workspace.arg(name) + if not arg: + raise AssertionError(f"parameter {name!r} not found") + # Parameters outside of all domains are imported as constants and cannot be + # changed. Skip them, like the HS3TestSuite reference backend does. + if not isinstance(arg, self.ROOT.RooConstVar): + raise AssertionError(f"parameter {name!r} is a {arg.ClassName()}, which cannot be set") @contextmanager diff --git a/roofit/hs3/test/test_hs3_suite.py b/roofit/hs3/test/test_hs3_suite.py index 5c7804afd028d..b0ecca89b164e 100644 --- a/roofit/hs3/test/test_hs3_suite.py +++ b/roofit/hs3/test/test_hs3_suite.py @@ -4,15 +4,30 @@ from pathlib import Path hs3_root_dir = os.environ.get("HS3TESTSUITE_ROOT") -if hs3_root_dir: - sys.path.insert(0, hs3_root_dir) +hs3_code_dir = os.environ.get("HS3TESTSUITECODE_ROOT") +if hs3_code_dir: + sys.path.insert(0, hs3_code_dir) -try: - from hs3suite.backends import _BACKENDS - from hs3suite.runner import run_suite - _BACKENDS["roofit"] = ("hs3testsuite_roofit_backend", "RooFitBackend") +def load_expected_failures(path): + """Read the list of expected failures. + + Every non-empty line that is not a comment has the form + ``:: # ``. + """ + expected = {} + if not path: + return expected + for line in Path(path).read_text().splitlines(): + entry, _, reason = line.partition("#") + entry = entry.strip() + if entry: + expected[entry] = reason.strip() + return expected + +try: + from hs3suite.runner import run_suite except ImportError as e: TestHS3Suite = type( "TestHS3Suite", @@ -20,9 +35,9 @@ dict(test_dependencies=lambda self, e=e: self.fail(f"Missing dependency: {e}")), ) else: - hs3_root_path = Path(hs3_root_dir) + expected_failures = load_expected_failures(os.environ.get("HS3TESTSUITE_EXPECTED_FAILURES")) try: - results = run_suite(hs3_root_path, "roofit") + results = run_suite(Path(hs3_root_dir), "hs3testsuite_roofit_backend:RooFitBackend") except Exception as e: def test(self, e=e): @@ -36,16 +51,32 @@ def test(self, e=e): def _make(result): def test(self): - if result.status == "failed": + if result.status in ("failed", "xfail"): self.fail(result.message) return + key = f"{result.test_id}::{result.check_id}" test.__name__ = f"test_{result.test_id}__{result.check_id}" - test.__doc__ = f"{result.test_id}::{result.check_id}" + test.__doc__ = key + # Known failures are listed in hs3testsuite_expected_failures.txt, or + # marked as "xfail" for RooFit in the manifest of the HS3TestSuite. An + # unexpected success fails the test run, so the list has to be shrunk as + # soon as a fixture starts to pass. + if result.status == "xfail" or key in expected_failures: + test = unittest.expectedFailure(test) return test func = _make(r) namespace[func.__name__] = func + + # Entries that don't correspond to any check (e.g. after a fixture was renamed + # upstream) would silently hide nothing, so they are reported as failures. + stale = sorted(set(expected_failures).difference(f"{r.test_id}::{r.check_id}" for r in results)) + + def test_expected_failures_list_is_current(self): + self.assertEqual(stale, [], "entries in the list of expected failures that match no check") + + namespace[test_expected_failures_list_is_current.__name__] = test_expected_failures_list_is_current TestHS3Suite = type("TestHS3Suite", (unittest.TestCase,), namespace) if __name__ == "__main__":