diff --git a/.github/workflows/wheels.yml b/.github/workflows/wheels.yml index 2bb47ee..97b0ba6 100644 --- a/.github/workflows/wheels.yml +++ b/.github/workflows/wheels.yml @@ -59,6 +59,17 @@ jobs: env: CIBW_ARCHS: ${{ matrix.arch }} MACOSX_DEPLOYMENT_TARGET: 11.0 + CORRECTIONLIB_ULP_REPORT: ${{ github.workspace }}/ulp-report.md + + # cibuildwheel overwrites GITHUB_STEP_SUMMARY with its own build table once it + # finishes, so the tests collect this separately and it gets appended after + - name: Report float assertion ULPs + if: always() + shell: bash + run: | + if [ -f ulp-report.md ]; then + cat ulp-report.md >> "$GITHUB_STEP_SUMMARY" + fi - name: Upload wheels uses: actions/upload-artifact@v7 diff --git a/.gitmodules b/.gitmodules index 5083c4e..e38c4b4 100644 --- a/.gitmodules +++ b/.gitmodules @@ -16,3 +16,7 @@ [submodule "lwtnn"] path = lwtnn url = https://github.com/lwtnn/lwtnn.git +[submodule "eigen"] + path = eigen + url = https://gitlab.com/libeigen/eigen.git + shallow = true diff --git a/CMakeLists.txt b/CMakeLists.txt index 87fbb53..4c3f9f4 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -40,14 +40,32 @@ set(THREADS_PREFER_PTHREAD_FLAG ON) find_package(Threads) find_package(ZLIB) -# Configure lwtnn to build static library only and download dependencies +# Configure lwtnn to build static library only and download dependencies. +# Eigen comes from the vendored copy rather than from lwtnn's BUILTIN_EIGEN, which +# configures Eigen's own CMake project and so fails on systems that have MPFR but no +# GMP development headers. Eigen is header only, so pointing lwtnn's FindEigen3 at +# the headers is all it takes; override to build against another copy, e.g. +# -DEIGEN3_INCLUDE_DIR=/usr/include/eigen3 +set(EIGEN3_INCLUDE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/eigen" + CACHE PATH "Directory holding the Eigen headers to build the bundled lwtnn against") set(BUILD_STATIC_LIBRARY ON) set(BUILD_TESTING OFF) set(BUILTIN_BOOST ON) -set(BUILTIN_EIGEN ON) +set(BUILTIN_EIGEN OFF) set(CMAKE_POSITION_INDEPENDENT_CODE ON) add_subdirectory(lwtnn EXCLUDE_FROM_ALL) +# lwtnn drops EIGEN3_INCLUDE_DIR verbatim into its targets' usage requirements, and +# its install(EXPORT) rules reject an absolute in-tree path there. lwtnn does its own +# BUILD_INTERFACE wrapping in the BUILTIN_EIGEN branch; do the same for our copy. +foreach(lwtnn_target IN ITEMS lwtnn lwtnn-stat) + get_target_property(lwtnn_include_dirs ${lwtnn_target} INTERFACE_INCLUDE_DIRECTORIES) + string(REPLACE "${EIGEN3_INCLUDE_DIR}" "$" + lwtnn_include_dirs "${lwtnn_include_dirs}") + set_target_properties(${lwtnn_target} PROPERTIES + INTERFACE_INCLUDE_DIRECTORIES "${lwtnn_include_dirs}") +endforeach() + configure_file(include/version.h.in ${CMAKE_CURRENT_BINARY_DIR}/include/correctionlib_version.h) install(FILES ${CMAKE_CURRENT_BINARY_DIR}/include/correctionlib_version.h DESTINATION ${PKG_INSTALL}/include) diff --git a/eigen b/eigen new file mode 160000 index 0000000..3147391 --- /dev/null +++ b/eigen @@ -0,0 +1 @@ +Subproject commit 3147391d946bb4b6c68edd901f2add6ac1f31f8c diff --git a/pyproject.toml b/pyproject.toml index 4abb92b..1e1057a 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -147,7 +147,10 @@ Homepage = "https://github.com/cms-nanoAOD/correctionlib" [tool.scikit-build] metadata.version.provider = "scikit_build_core.metadata.setuptools_scm" cmake.version = ">=3.11.0" -sdist.include = ["src/correctionlib/version.py"] +# only Eigen/ is needed to build lwtnn; the rest of the Eigen checkout (tests, +# docs, demos, unsupported/) would roughly triple the sdist for nothing +sdist.exclude = ["eigen/**"] +sdist.include = ["src/correctionlib/version.py", "eigen/Eigen/**"] [tool.scikit-build.cmake.define] BUILD_DEMO = "OFF" @@ -159,11 +162,16 @@ write_to = "src/correctionlib/version.py" skip = ["cp3{9,10}*-win_arm64"] test-groups = ["test"] test-command = "python -m pytest {package}/tests" -test-skip = ["*-musllinux_*", "cp3{10,11,12}-win32", "cp*-win_arm64", "cp*-manylinux_aarch64"] +test-skip = ["*-musllinux_*", "cp3{10,11,12}-win32", "cp*-win_arm64"] [tool.cibuildwheel.environment] # this makes sure that we build only on platforms that have a corresponding numpy wheel PIP_ONLY_BINARY = ":all:" +[tool.cibuildwheel.linux] +# the linux tests run in a container, so the ULP report path has to be handed in +# explicitly; the other platforms see the runner environment already +environment-pass = ["CORRECTIONLIB_ULP_REPORT"] + [tool.codespell] ignore-regex = "[A-Za-z0-9+/]{100,}" # ignore long base64 strings diff --git a/tests/conftest.py b/tests/conftest.py new file mode 100644 index 0000000..7736119 --- /dev/null +++ b/tests/conftest.py @@ -0,0 +1,138 @@ +"""Reports how far our approximate float assertions actually land from their reference. + +The wheel builds run this suite on every architecture we ship, so the summary printed +at the end of a run is a per-architecture record of the floating point spread, readable +straight out of the Actions log. See #348, where an aarch64 build differed from the +reference by a single ULP and the whole suite got skipped on that platform in response. + +Set CORRECTIONLIB_ULP_REPORT to also collect the report as markdown, which is how +wheels.yml lifts it into the GitHub Actions job summary. It cannot be written to +GITHUB_STEP_SUMMARY directly from here: cibuildwheel overwrites that file once its +own build table is ready, which is after every test run has finished. +""" + +import os +import pathlib +import platform +import struct +import sys +import sysconfig +from collections import defaultdict + +import pytest + + +def _ordinal(value: float) -> int: + """Map a double onto an integer such that adjacent doubles are adjacent integers.""" + (bits,) = struct.unpack("= 0 else -0x8000000000000000 - bits + + +def ulp_distance(actual: float, expected: float) -> float: + """Count the representable doubles between two floats.""" + if actual != actual or expected != expected: # NaN + return float("inf") + if actual in (float("inf"), float("-inf")) or expected in ( + float("inf"), + float("-inf"), + ): + return float("inf") + if actual == expected: + return 0 + try: + return abs(_ordinal(actual) - _ordinal(expected)) + except (OverflowError, struct.error): + return float("inf") + + +_measurements: "defaultdict[str, list[tuple[float, float, float]]]" = defaultdict(list) + + +@pytest.fixture +def ulp_report(request): + """Assert approximate equality, recording the ULP distance for the run summary. + + Takes the same keyword arguments as pytest.approx; pass ``label`` to name the + measurement when a test makes more than one. + """ + + def check(actual, expected, label=None, **approx_kwargs): + name = request.node.name + if label is not None: + name = f"{name}[{label}]" + _measurements[name].append((actual, expected, ulp_distance(actual, expected))) + assert actual == pytest.approx(expected, **approx_kwargs) + + return check + + +def _build_tag() -> str: + """Name this interpreter and platform the way the wheel it came from is named.""" + return f"{sys.implementation.cache_tag}-{sysconfig.get_platform()}" + + +def _markdown_report_path(host_mount="/host"): + """Where to collect the markdown report, if anywhere. + + cibuildwheel runs the Linux tests inside a container, where the runner's + filesystem is mounted at /host, so a path handed to us by the workflow needs + translating before we can write to it. + """ + target = os.environ.get("CORRECTIONLIB_ULP_REPORT") + if not target and not os.environ.get("CIBUILDWHEEL"): + # outside cibuildwheel nothing overwrites the job summary, so use it directly + target = os.environ.get("GITHUB_STEP_SUMMARY") + if not target: + return None + path = pathlib.Path(target) + if path.parent.is_dir(): + return path + in_container = pathlib.Path(host_mount) / path.relative_to(path.anchor) + return in_container if in_container.parent.is_dir() else None + + +def _write_markdown_report(path, rows): + header = "### Float assertion ULP report" + existing = path.read_text(encoding="utf-8") if path.exists() else "" + lines = [] + if header not in existing: + lines += [ + header, + "", + "| build | assertion | ulp | actual | expected |", + "| --- | --- | ---: | --- | --- |", + ] + build = _build_tag() + for name, ulps, actual, expected in rows: + lines.append(f"| `{build}` | {name} | {ulps} | `{actual!r}` | `{expected!r}` |") + with path.open("a", encoding="utf-8") as fp: + fp.write("\n".join(lines) + "\n") + + +def pytest_terminal_summary(terminalreporter): + if not _measurements: + return + terminalreporter.write_sep("=", "float assertion ULP report") + terminalreporter.write_line( + f"{platform.machine()} {sys.platform} " + f"python {'.'.join(str(v) for v in sys.version_info[:3])}" + f"{'t' if sysconfig.get_config_var('Py_GIL_DISABLED') else ''}" + ) + width = max(len(name) for name in _measurements) + rows = [] + for name, measurements in sorted(_measurements.items()): + actual, expected, ulps = max(measurements, key=lambda m: m[2]) + detail = "exact" if ulps == 0 else f"{actual!r} vs {expected!r}" + count = f" (worst of {len(measurements)})" if len(measurements) > 1 else "" + terminalreporter.write_line(f"{name:<{width}} {ulps:>4} ulp {detail}{count}") + rows.append((name, ulps, actual, expected)) + + path = _markdown_report_path() + if path is None: + return + try: + _write_markdown_report(path, rows) + except OSError as exc: # never fail a test run over the report + terminalreporter.write_line(f"could not write ULP report to {path}: {exc}") diff --git a/tests/test_core.py b/tests/test_core.py index b62a4c5..985e36a 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -141,7 +141,7 @@ def test_evaluator(): platform.architecture() in {("32bit", "ELF"), ("32bit", "")}, reason="cibuildwheel tests fail while building i686 wheels due to floating point rounding differences of order 1e-16", ) -def test_tformula(): +def test_tformula(ulp_report): def evaluate(expr, variables, parameters): cset = { "schema_version": 2, @@ -431,11 +431,12 @@ def evaluate(expr, variables, parameters): * (math.log(x) / math.log(10) - v[5]) ) # the following shows a small numerical error: 1.2512381067949132 - 1.251238106794914 == -8e-16 - assert evaluate( - "max(0.0001,[0]+[1]/(pow(log10(x),2)+[2])+[3]*exp(-1*([4]*((log10(x)-[5])*(log10(x)-[5])))))", - [x], - v, - ) == pytest.approx( + ulp_report( + evaluate( + "max(0.0001,[0]+[1]/(pow(log10(x),2)+[2])+[3]*exp(-1*([4]*((log10(x)-[5])*(log10(x)-[5])))))", + [x], + v, + ), max( 0.0001, v[0] diff --git a/tests/test_lwtnn.py b/tests/test_lwtnn.py index d5197f3..1487b4e 100644 --- a/tests/test_lwtnn.py +++ b/tests/test_lwtnn.py @@ -24,7 +24,7 @@ def test_lwtnn_bad_opaque(): CorrectionSet.from_string(json.dumps(data)) -def test_lwtnn_example(): +def test_lwtnn_example(ulp_report): cset = CorrectionSet.from_file(str(LWTNN_TEST_FIXTURE)) corr = cset["electron_fastsim_sf"] @@ -38,4 +38,7 @@ def test_lwtnn_example(): gen_phi, gen_iso, ) - assert sf == 0.95186825355646787 + # lwtnn's Eigen kernels sum in an order that depends on the vector ISA and the + # Eigen version, so the last couple of ULPs are not reproducible across + # platforms -- an aarch64 wheel build differed by one ULP here, see #348. + ulp_report(sf, 0.95186825355646787, rel=1e-12) diff --git a/tests/test_ndpolyfit.py b/tests/test_ndpolyfit.py index 46ecc0a..4169936 100644 --- a/tests/test_ndpolyfit.py +++ b/tests/test_ndpolyfit.py @@ -1,10 +1,9 @@ import numpy as np -import pytest from correctionlib import convert -def test_ndpoly(): +def test_ndpoly(ulp_report): corr, _ = convert.ndpolyfit( points=[np.array([0.0, 1.0, 0.0, 1.0]), np.array([10.0, 20.0, 10.0, 20.0])], values=np.array([0.9, 0.95, 0.94, 0.98]), @@ -13,4 +12,4 @@ def test_ndpoly(): degree=(1, 1), ) ceval = corr.to_evaluator() - assert ceval.evaluate(0.2, 13.0) == pytest.approx(1.0801881480751705) + ulp_report(ceval.evaluate(0.2, 13.0), 1.0801881480751705)