From 018e0b56a0d3253065a51937ccceaf5a65228c31 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Thu, 15 Oct 2026 07:37:41 -0400 Subject: [PATCH 1/8] Compare each pull request's calibration fit with its base build's Pull request CI now also builds the PR's base, in the same way as the head (TESTING, one clone) and with the head's lockfile, so both run one model and environment. A new job compares the two final-epoch fits from their calibration logs. It fails when the share of national or local targets within 10% falls by more than 2pp, flags national targets whose error rises by more than 0.2, and posts the comparison on the PR. The thresholds (targets/fit_policy.yaml, adopted under d1344) come from five builds of the same data code: their shares moved by at most 0.93pp (national) and 0.69pp (local), and they flagged 0 to 6 targets per pair. Per-target moves only flag while #598 (two weight sets from one code) is open. Only calibration logs are uploaded, as before. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/pull_request.yaml | 123 ++++++++++ changelog.d/fit-comparison.added.md | 1 + policyengine_uk_data/targets/fit_policy.yaml | 29 +++ policyengine_uk_data/tests/test_fit_report.py | 179 ++++++++++++++ policyengine_uk_data/utils/fit_report.py | 231 ++++++++++++++++++ 5 files changed, 563 insertions(+) create mode 100644 changelog.d/fit-comparison.added.md create mode 100644 policyengine_uk_data/targets/fit_policy.yaml create mode 100644 policyengine_uk_data/tests/test_fit_report.py create mode 100644 policyengine_uk_data/utils/fit_report.py diff --git a/.github/workflows/pull_request.yaml b/.github/workflows/pull_request.yaml index 9443fa258..4b035dc3c 100644 --- a/.github/workflows/pull_request.yaml +++ b/.github/workflows/pull_request.yaml @@ -104,3 +104,126 @@ jobs: # Relaxes built-dataset outcome tolerances to match the reduced # fidelity of the TESTING build above (see test_cgt_band_donors). TESTING: "1" + # The no-worsening check (W3). The base is built the same way as the head: + # TESTING, one clone and the head's lockfile, so both builds run one model + # and environment and the comparison measures the data change. Only the + # base's calibration log leaves the job. Like the head's, it holds target + # estimates and values, aggregates; no dataset is uploaded or cached. + base-build: + name: Build the base for comparison + runs-on: ubuntu-latest + timeout-minutes: 60 + env: + HUGGING_FACE_TOKEN: ${{ secrets.HUGGING_FACE_TOKEN }} + PYTHONUNBUFFERED: "1" + PE_UK_DATA_OA_CLONES: "1" + steps: + - name: Check out the head, for its lockfile + uses: actions/checkout@v6 + with: + path: head + - name: Check out the base + uses: actions/checkout@v6 + with: + ref: ${{ github.event.pull_request.base.sha }} + path: base + - name: Install uv + uses: astral-sh/setup-uv@v8.1.0 + - name: Set up Python + uses: actions/setup-python@v6 + with: + python-version: 3.13 + - name: Sync the head's locked environment for the base + run: | + cp head/uv.lock base/uv.lock + cd base && uv sync --frozen --all-extras + - name: Download data inputs + working-directory: base + run: uv run --frozen make download + - name: Build the base datasets + working-directory: base + run: uv run --frozen make data + env: + TESTING: "1" + - name: Save the base calibration log + uses: actions/upload-artifact@v7 + with: + name: base_constituency_calibration_log.csv + path: base/constituency_calibration_log.csv + fit-comparison: + name: Calibration fit against the base + needs: [test, base-build] + # Report a failed side too: an earlier passing comment must not stand. + if: ${{ !cancelled() }} + runs-on: ubuntu-latest + timeout-minutes: 20 + permissions: + contents: read + issues: write + pull-requests: write + steps: + - uses: actions/checkout@v6 + - name: Install uv + uses: astral-sh/setup-uv@v8.1.0 + - name: Set up Python + uses: actions/setup-python@v6 + with: + python-version: 3.13 + - name: Sync locked environment + run: uv sync --frozen + - name: Download the head calibration log + uses: actions/download-artifact@v8 + continue-on-error: true + with: + name: constituency_calibration_log.csv + path: logs/head + - name: Download the base calibration log + uses: actions/download-artifact@v8 + continue-on-error: true + with: + name: base_constituency_calibration_log.csv + path: logs/base + - name: Compare the final fits + id: compare + env: + RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + run: | + marker='' + if [ ! -f logs/head/constituency_calibration_log.csv ] || [ ! -f logs/base/constituency_calibration_log.csv ]; then + { + echo "$marker" + echo '## Calibration fit against the base build: FAIL' + echo + echo "One of the builds produced no calibration log, so the fit could not be compared. See the [workflow run]($RUN_URL)." + } > fit.md + echo "status=1" >> "$GITHUB_OUTPUT" + else + set +e + uv run --frozen python -m policyengine_uk_data.utils.fit_report compare \ + logs/base/constituency_calibration_log.csv \ + logs/head/constituency_calibration_log.csv --markdown body.md + echo "status=$?" >> "$GITHUB_OUTPUT" + set -e + { echo "$marker"; cat body.md 2>/dev/null || echo '## Calibration fit against the base build: FAIL'; echo; echo "[Workflow run]($RUN_URL)"; } > fit.md + fi + cat fit.md >> "$GITHUB_STEP_SUMMARY" + - name: Post or update the pull request comment + if: ${{ !cancelled() && github.event.pull_request.head.repo.full_name == github.repository }} + env: + GH_TOKEN: ${{ github.token }} + PR: ${{ github.event.pull_request.number }} + run: | + marker='' + existing=$(gh api --paginate "repos/$GITHUB_REPOSITORY/issues/$PR/comments" \ + --jq ".[] | select(.user.login == \"github-actions[bot]\") | select(.body | startswith(\"$marker\")) | .id" \ + | tail -n 1) + if [ -n "$existing" ]; then + gh api -X PATCH "repos/$GITHUB_REPOSITORY/issues/comments/$existing" -F body=@fit.md > /dev/null + else + gh api -X POST "repos/$GITHUB_REPOSITORY/issues/$PR/comments" -F body=@fit.md > /dev/null + fi + - name: Fail on a fit regression + if: steps.compare.outputs.status != '0' + run: | + echo "::error::The head build's calibration fit is worse than the base's; see the job summary." + exit 1 diff --git a/changelog.d/fit-comparison.added.md b/changelog.d/fit-comparison.added.md new file mode 100644 index 000000000..6c7bebf99 --- /dev/null +++ b/changelog.d/fit-comparison.added.md @@ -0,0 +1 @@ +Build each pull request's base the same way as its head, with the head's lockfile, and fail when the share of national or local targets within 10% falls by more than 2 percentage points (`targets/fit_policy.yaml`). National targets whose error rises by more than 0.2 are flagged in a pull request comment. Adds `python -m policyengine_uk_data.utils.fit_report compare`. diff --git a/policyengine_uk_data/targets/fit_policy.yaml b/policyengine_uk_data/targets/fit_policy.yaml new file mode 100644 index 000000000..c0674db4c --- /dev/null +++ b/policyengine_uk_data/targets/fit_policy.yaml @@ -0,0 +1,29 @@ +# When a pull request's build fails or flags against its base build's fit +# (utils/fit_report.py). Both builds run the same way in pull request CI: +# TESTING, 32 epochs, one output-area clone, the head's lockfile. +# +# Adopted under cos d1344 (15 October 2026), from the final-epoch fits of +# five builds of the same data code: docs, changelog and CI pull requests +# while main's data code was unchanged from b34a4ea2 to 2b9c1c2c (runs +# 38006434333, 37959581668, 38009046475, 37960852139, 37864653693). +schema_version: 1 + +share_within_10pct: + # Fail when the share of national, or of local, targets within 10% of + # their values falls by more than this. Across the 20 ordered pairs of the + # five builds the national share moved by at most 0.93pp and the local by + # at most 0.69pp, so 2pp is about twice the same-code spread. + # Alternatives: 1pp leaves no margin over that spread while #598 (builds + # of the same code fall into two weight sets) is unexplained. 5pp would + # pass the 2.4pp local drop of #551's build against the build of the + # 1.58.1 release branch (runs 38661397028 and 38228500493). + allowed_drop: 0.02 + +national_target: + # Flag, for review, each national target whose error |estimate / target - + # 1| rises by more than this. The five builds give 0 to 6 such rises per + # pair (mean 1.8), so a list stays short. 0.1 gives up to 11 from noise + # alone, and 0.3 would hide the largest same-code move on the OBR, ONS + # population and DWP targets (0.21). Per-target moves don't fail until + # #598 is fixed. + flag_rise: 0.2 diff --git a/policyengine_uk_data/tests/test_fit_report.py b/policyengine_uk_data/tests/test_fit_report.py new file mode 100644 index 000000000..276540e26 --- /dev/null +++ b/policyengine_uk_data/tests/test_fit_report.py @@ -0,0 +1,179 @@ +"""The final calibration fit and the no-worsening comparison (utils/fit_report.py).""" + +import numpy as np +import pandas as pd +import pytest +from hypothesis import given, settings +from hypothesis import strategies as st + +from policyengine_uk_data.utils.fit_report import ( + FitPolicy, + compare_fits, + final_fit, + load_policy, + main, +) + +POLICY = FitPolicy(share_drop_allowance=0.02, target_flag_threshold=0.2) + + +def _log(national: dict, local=(), epochs=(0, 30)): + """A calibration log: national {metric: (estimate, target)}, local [(est, target)].""" + rows = [] + for epoch in epochs: + for metric, (estimate, target) in national.items(): + rows.append(("UK", metric, estimate, target, epoch)) + for i, (estimate, target) in enumerate(local): + rows.append((f"area {i}", f"local/{i}", estimate, target, epoch)) + return pd.DataFrame(rows, columns=["name", "metric", "estimate", "target", "epoch"]) + + +def test_final_fit_reads_the_last_epoch_and_drops_zero_targets(): + log = _log( + {"a": (105, 100), "b": (150, 100), "zero": (5, 0)}, local=[(9, 10), (20, 10)] + ) + log.loc[log.epoch == 0, "estimate"] = 1 # earlier epochs are ignored + fit = final_fit(log) + assert fit.epoch == 30 + assert fit.national.to_dict() == pytest.approx({"a": 0.05, "b": 0.5}) + assert fit.national_within == 0.5 + assert fit.local_within == 0.5 + + +def test_a_malformed_log_is_refused(): + with pytest.raises(ValueError, match="no rows"): + final_fit(_log({}, epochs=())) + log = _log({"a": (np.inf, 100)}) + with pytest.raises(ValueError, match="non-finite"): + final_fit(log) + log = pd.concat([_log({"a": (1, 1)}), _log({"a": (2, 1)})]) + with pytest.raises(ValueError, match="repeats"): + final_fit(log) + + +def test_a_drop_beyond_the_allowance_fails_and_within_it_passes(): + targets = {f"t{i}": (100, 100) for i in range(100)} + base = final_fit(_log(targets, local=[(100, 100)] * 10)) + worse = dict(targets, **{f"t{i}": (150, 100) for i in range(3)}) + head = final_fit(_log(worse, local=[(100, 100)] * 10)) + result = compare_fits(base, head, POLICY) + assert result["status"] == "fail" + assert ( + "national targets within 10% falls from 100.0% to 97.0%" + in result["failures"][0] + ) + assert [f["target"] for f in result["flags"]] == ["t0", "t1", "t2"] + slightly = dict(targets, t0=(150, 100)) + assert ( + compare_fits(base, final_fit(_log(slightly, local=[(100, 100)] * 10)), POLICY)[ + "status" + ] + == "pass" + ) + + +def test_builds_of_different_epochs_cannot_be_compared(): + base = final_fit(_log({"a": (1, 1)}, epochs=(0, 30))) + head = final_fit(_log({"a": (1, 1)}, epochs=(0, 510, 511))) + assert compare_fits(base, head, POLICY)["status"] == "fail" + # 32 epochs log last at 30, or at 31 once the final epoch is saved (#594). + final = final_fit(_log({"a": (1, 1)}, epochs=(0, 30, 31))) + result = compare_fits(base, final, POLICY) + assert result["status"] == "pass" + assert "saves the final epoch" in result["notes"][0] + + +def test_the_pull_request_workflow_uploads_only_calibration_logs(): + """The datasets are licensed microdata: no job may upload or cache one.""" + from pathlib import Path + + import yaml + + workflow = Path(__file__).parents[2] / ".github" / "workflows" / "pull_request.yaml" + jobs = yaml.safe_load(workflow.read_text())["jobs"] + uploads = [] + for job in jobs.values(): + for step in job.get("steps", []): + uses = step.get("uses", "") + assert not uses.startswith("actions/cache"), step + if uses.startswith("actions/upload-artifact"): + uploads.append(step["with"]["path"]) + assert uploads and all(p.endswith("calibration_log.csv") for p in uploads), uploads + assert jobs["fit-comparison"]["needs"] == ["test", "base-build"] + + +def test_new_and_removed_targets_are_noted_not_failed(): + base = final_fit(_log({"a": (1, 1), "old": (1, 1)})) + head = final_fit(_log({"a": (1, 1), "new": (1, 1)})) + result = compare_fits(base, head, POLICY) + assert result["status"] == "pass" + assert result["notes"] == [ + "1 national targets are new: new", + "1 national targets are gone: old", + ] + + +errors = st.lists(st.floats(0, 3), min_size=1, max_size=40) + + +@settings(max_examples=200, deadline=None) +@given(national=errors, local=errors) +def test_a_build_compared_with_itself_passes_with_nothing_flagged(national, local): + fit = final_fit( + _log( + {f"t{i}": (100 * (1 + e), 100) for i, e in enumerate(national)}, + local=[(100 * (1 + e), 100) for e in local], + ) + ) + result = compare_fits(fit, fit, POLICY) + assert result["status"] == "pass" and result["flags"] == [] + assert all(row["change"] == 0 for row in result["summary"].values()) + + +@settings(max_examples=200, deadline=None) +@given(base=errors, data=st.data()) +def test_improving_every_target_never_fails_and_changes_are_antisymmetric(base, data): + better = [data.draw(st.floats(0, e)) for e in base] + fit_base = final_fit( + _log({f"t{i}": (100 * (1 + e), 100) for i, e in enumerate(base)}) + ) + fit_head = final_fit( + _log({f"t{i}": (100 * (1 + e), 100) for i, e in enumerate(better)}) + ) + forward = compare_fits(fit_base, fit_head, POLICY) + assert forward["status"] == "pass" and forward["flags"] == [] + backward = compare_fits(fit_head, fit_base, POLICY) + for name in ("national", "local"): + a, b = forward["summary"][name]["change"], backward["summary"][name]["change"] + assert a == pytest.approx(-b) or (np.isnan(a) and np.isnan(b)) + + +def test_the_shipped_policy_loads(): + policy = load_policy() + assert policy.share_drop_allowance == 0.02 + assert policy.target_flag_threshold == 0.2 + + +def test_the_command_exits_one_on_a_failure(tmp_path): + targets = {f"t{i}": (100, 100) for i in range(50)} + _log(targets).to_csv(tmp_path / "base.csv", index=False) + _log(dict(targets, **{f"t{i}": (200, 100) for i in range(5)})).to_csv( + tmp_path / "head.csv", index=False + ) + out = tmp_path / "fit.md" + assert ( + main( + [ + "compare", + str(tmp_path / "base.csv"), + str(tmp_path / "head.csv"), + "--markdown", + str(out), + ] + ) + == 1 + ) + assert "## Calibration fit against the base build: fail" in out.read_text() + assert ( + main(["compare", str(tmp_path / "base.csv"), str(tmp_path / "base.csv")]) == 0 + ) diff --git a/policyengine_uk_data/utils/fit_report.py b/policyengine_uk_data/utils/fit_report.py new file mode 100644 index 000000000..ef2d2cd32 --- /dev/null +++ b/policyengine_uk_data/utils/fit_report.py @@ -0,0 +1,231 @@ +"""Final calibration fit from a build's log, and the comparison of two builds. + +A build writes ``constituency_calibration_log.csv``: each target's estimate +and value every tenth epoch, and at the last one. ``final_fit`` reads the +last epoch. A target's error is |estimate / target - 1|, and targets of zero +are left out. The national rows are those named ``UK``. + +``compare_fits`` checks that a pull request does not worsen the fit of the +build it would replace, both built the same way: same environment, epochs +and clone count. Two rules come from ``targets/fit_policy.yaml``: + +- **Fail** when the share of national (or local) targets within 10% falls + by more than the allowance. +- **Flag** each national target whose error rises by more than the + same-code spread, for review. + +Per-target moves are not yet gates: builds of the same code currently +differ by up to 0.62 on one target (#598). + + python -m policyengine_uk_data.utils.fit_report compare BASE.csv HEAD.csv \\ + [--markdown out.md] +""" + +from __future__ import annotations + +import argparse +import sys +from dataclasses import dataclass +from pathlib import Path +from typing import Optional + +import numpy as np +import pandas as pd +import yaml + +POLICY_PATH = Path(__file__).parents[1] / "targets" / "fit_policy.yaml" +WITHIN = 0.10 +_COLUMNS = ["name", "metric", "estimate", "target", "epoch"] + + +@dataclass(frozen=True) +class Fit: + epoch: int + national: pd.Series # error by target name + local_errors: np.ndarray + + @property + def national_within(self) -> float: + return ( + float(np.mean(self.national <= WITHIN)) + if len(self.national) + else float("nan") + ) + + @property + def local_within(self) -> float: + return ( + float(np.mean(self.local_errors <= WITHIN)) + if len(self.local_errors) + else float("nan") + ) + + +@dataclass(frozen=True) +class FitPolicy: + share_drop_allowance: float + target_flag_threshold: float + + +def load_policy(path=None) -> FitPolicy: + with open(path or POLICY_PATH) as f: + raw = yaml.safe_load(f) + if raw.get("schema_version") != 1: + raise ValueError("fit policy: schema_version must be 1") + return FitPolicy( + share_drop_allowance=float(raw["share_within_10pct"]["allowed_drop"]), + target_flag_threshold=float(raw["national_target"]["flag_rise"]), + ) + + +def errors(frame: pd.DataFrame) -> pd.Series: + """|estimate / target - 1| for rows with a non-zero target.""" + frame = frame[frame.target != 0] + return (frame.estimate / frame.target - 1).abs() + + +def final_fit(log) -> Fit: + """The fit at a calibration log's last epoch.""" + df = ( + pd.read_csv(log, usecols=_COLUMNS) if not isinstance(log, pd.DataFrame) else log + ) + if df.empty: + raise ValueError("the calibration log has no rows") + last = int(df.epoch.max()) + rows = df[df.epoch == last] + national = rows[rows.name == "UK"] + if national.metric.duplicated().any(): + raise ValueError( + "the calibration log repeats a national target at its last epoch" + ) + national_errors = errors(national.set_index("metric")) + local_errors = errors(rows[rows.name != "UK"]).to_numpy() + if not np.isfinite(national_errors).all() or not np.isfinite(local_errors).all(): + raise ValueError("the calibration log has non-finite estimates") + return Fit(last, national_errors, local_errors) + + +def compare_fits(base: Fit, head: Fit, policy: FitPolicy) -> dict: + """Failures, flags and the summary for a head build against its base.""" + failures, notes = [], [] + # A log's last epoch is a multiple of ten or, since #594, the final one; + # the same epoch count gives last epochs less than ten apart. + if abs(base.epoch - head.epoch) >= 10: + failures.append( + f"The builds ran different epochs (logs end at {base.epoch} and " + f"{head.epoch}), so their fits cannot be compared." + ) + elif base.epoch != head.epoch: + notes.append( + f"The base log ends at epoch {base.epoch} and the head's at {head.epoch}: " + "one of them saves the final epoch (#594)." + ) + summary = {} + for name, b, h in ( + ("national", base.national_within, head.national_within), + ("local", base.local_within, head.local_within), + ): + summary[name] = {"base": b, "head": h, "change": h - b} + if h < b - policy.share_drop_allowance: + failures.append( + f"The share of {name} targets within 10% falls from {b:.1%} to {h:.1%}, " + f"by more than the {policy.share_drop_allowance:.1%} allowed." + ) + common = base.national.index.intersection(head.national.index) + added = sorted(set(head.national.index) - set(common)) + removed = sorted(set(base.national.index) - set(common)) + if added: + notes.append(f"{len(added)} national targets are new: {', '.join(added[:10])}") + if removed: + notes.append( + f"{len(removed)} national targets are gone: {', '.join(removed[:10])}" + ) + # Largest rise first; ties keep the log's order, so a comparison is + # reproducible. + rise = (head.national[common] - base.national[common]).sort_values( + ascending=False, kind="mergesort" + ) + flags = [ + {"target": t, "base": float(base.national[t]), "head": float(head.national[t])} + for t, r in rise.items() + if r > policy.target_flag_threshold + ] + return { + "status": "fail" if failures else "pass", + "failures": failures, + "notes": notes, + "flags": flags, + "summary": summary, + "targets_compared": len(common), + } + + +def render(result: dict, policy: FitPolicy) -> str: + lines = [ + f"## Calibration fit against the base build: {result['status']}", + "", + "| Targets within 10% | Base | Head | Change |", + "|---|---:|---:|---:|", + ] + for name, row in result["summary"].items(): + lines.append( + f"| {name.capitalize()} | {row['base']:.1%} | {row['head']:.1%} | {100 * row['change']:+.1f}pp |" + ) + lines.append("") + lines.append( + f"A drop of more than {policy.share_drop_allowance:.1%} fails. " + f"{result['targets_compared']} national targets were compared." + ) + if result["failures"]: + lines += ["", "### Failures", ""] + [f"- {f}" for f in result["failures"]] + if result["notes"]: + lines += ["", "### Notes", ""] + [f"- {n}" for n in result["notes"]] + flags = result["flags"] + lines += [ + "", + f"### National targets whose error rises by more than {policy.target_flag_threshold:.2f} ({len(flags)})", + "", + ] + if flags: + lines += ["| Target | Base error | Head error |", "|---|---:|---:|"] + lines += [ + f"| `{f['target']}` | {f['base']:.3f} | {f['head']:.3f} |" + for f in flags[:30] + ] + if len(flags) > 30: + lines.append(f"\n{len(flags) - 30} more in the job log.") + else: + lines.append("None.") + lines += [ + "", + "Flags are for review; builds of the same code differ this much (#598).", + ] + return "\n".join(lines) + "\n" + + +def main(argv: Optional[list] = None) -> int: + parser = argparse.ArgumentParser( + prog="python -m policyengine_uk_data.utils.fit_report" + ) + sub = parser.add_subparsers(dest="command", required=True) + c = sub.add_parser( + "compare", help="compare a head build's final fit with its base's" + ) + c.add_argument("base") + c.add_argument("head") + c.add_argument("--markdown", help="write the comparison here as markdown") + c.add_argument("--policy", help="fit policy YAML (default targets/fit_policy.yaml)") + args = parser.parse_args(argv) + policy = load_policy(args.policy) + result = compare_fits(final_fit(args.base), final_fit(args.head), policy) + text = render(result, policy) + if args.markdown: + Path(args.markdown).write_text(text) + print(text) + for flag in result["flags"]: + print(f"flag: {flag['target']} {flag['base']:.4f} -> {flag['head']:.4f}") + return 1 if result["status"] == "fail" else 0 + + +if __name__ == "__main__": + sys.exit(main()) From 22e823f9f2c876fea171664dea0ed3127ed209c0 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Thu, 15 Oct 2026 09:20:28 -0400 Subject: [PATCH 2/8] Compare fit summaries, not raw calibration logs (Sol review of #599) - Raw calibration logs stay on the runner. Each build uploads a summary: whether every national and local target is within 10%, and the error of national targets supported by at least 10 households only (the constituency log gains a records column). An estimate from a few records, divided by a published count, could give one household's value back. The test job's raw-log uploads are replaced by the summary too. - The gate compares shares over the targets both builds have; added and removed targets are notes. - An empty national or local group, or a last epoch with fewer rows than the one before, fails. - The comment step writes a failure comment when the comparison never ran. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/pull_request.yaml | 57 ++-- .../local_areas/constituencies/calibrate.py | 7 + policyengine_uk_data/tests/test_fit_report.py | 243 ++++++++++------ policyengine_uk_data/utils/fit_report.py | 275 +++++++++++------- 4 files changed, 354 insertions(+), 228 deletions(-) diff --git a/.github/workflows/pull_request.yaml b/.github/workflows/pull_request.yaml index 4b035dc3c..f7cf0b8c6 100644 --- a/.github/workflows/pull_request.yaml +++ b/.github/workflows/pull_request.yaml @@ -86,17 +86,17 @@ jobs: run: uv run --frozen make data env: TESTING: "1" - - name: Save calibration log (constituencies) + # The raw calibration logs stay on the runner: a target supported by a + # few households, divided by a published count, could give one + # household's value back. Only the summary the fit comparison needs is + # uploaded (utils/fit_report.py). + - name: Summarise the calibration fit + run: uv run --frozen python -m policyengine_uk_data.utils.fit_report summarise constituency_calibration_log.csv --out fit_summary.json + - name: Save the fit summary uses: actions/upload-artifact@v7 with: - name: constituency_calibration_log.csv - path: constituency_calibration_log.csv - - - name: Save calibration log (local authorities) - uses: actions/upload-artifact@v7 - with: - name: la_calibration_log.csv - path: la_calibration_log.csv + name: fit_summary.json + path: fit_summary.json - name: Run tests run: uv run --frozen make test @@ -145,11 +145,15 @@ jobs: run: uv run --frozen make data env: TESTING: "1" - - name: Save the base calibration log + - name: Summarise the base's calibration fit + run: | + cp head/policyengine_uk_data/utils/fit_report.py base/policyengine_uk_data/utils/fit_report.py + uv run --frozen --project base python -m policyengine_uk_data.utils.fit_report summarise base/constituency_calibration_log.csv --out base_fit_summary.json + - name: Save the base fit summary uses: actions/upload-artifact@v7 with: - name: base_constituency_calibration_log.csv - path: base/constituency_calibration_log.csv + name: base_fit_summary.json + path: base_fit_summary.json fit-comparison: name: Calibration fit against the base needs: [test, base-build] @@ -171,37 +175,31 @@ jobs: python-version: 3.13 - name: Sync locked environment run: uv sync --frozen - - name: Download the head calibration log - uses: actions/download-artifact@v8 - continue-on-error: true - with: - name: constituency_calibration_log.csv - path: logs/head - - name: Download the base calibration log + - name: Download the fit summaries uses: actions/download-artifact@v8 continue-on-error: true with: - name: base_constituency_calibration_log.csv - path: logs/base + pattern: "*fit_summary.json" + path: logs + merge-multiple: true - name: Compare the final fits id: compare env: RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} run: | marker='' - if [ ! -f logs/head/constituency_calibration_log.csv ] || [ ! -f logs/base/constituency_calibration_log.csv ]; then + if [ ! -f logs/fit_summary.json ] || [ ! -f logs/base_fit_summary.json ]; then { echo "$marker" echo '## Calibration fit against the base build: FAIL' echo - echo "One of the builds produced no calibration log, so the fit could not be compared. See the [workflow run]($RUN_URL)." + echo "One of the builds produced no fit summary, so the fit could not be compared. See the [workflow run]($RUN_URL)." } > fit.md echo "status=1" >> "$GITHUB_OUTPUT" else set +e uv run --frozen python -m policyengine_uk_data.utils.fit_report compare \ - logs/base/constituency_calibration_log.csv \ - logs/head/constituency_calibration_log.csv --markdown body.md + logs/base_fit_summary.json logs/fit_summary.json --markdown body.md echo "status=$?" >> "$GITHUB_OUTPUT" set -e { echo "$marker"; cat body.md 2>/dev/null || echo '## Calibration fit against the base build: FAIL'; echo; echo "[Workflow run]($RUN_URL)"; } > fit.md @@ -214,6 +212,15 @@ jobs: PR: ${{ github.event.pull_request.number }} run: | marker='' + if [ ! -s fit.md ]; then + # The job failed before the comparison could write a comment. + { + echo "$marker" + echo '## Calibration fit against the base build: FAIL' + echo + echo "The comparison did not run. See the [workflow run]($GITHUB_SERVER_URL/$GITHUB_REPOSITORY/actions/runs/$GITHUB_RUN_ID)." + } > fit.md + fi existing=$(gh api --paginate "repos/$GITHUB_REPOSITORY/issues/$PR/comments" \ --jq ".[] | select(.user.login == \"github-actions[bot]\") | select(.body | startswith(\"$marker\")) | .id" \ | tail -n 1) diff --git a/policyengine_uk_data/datasets/local_areas/constituencies/calibrate.py b/policyengine_uk_data/datasets/local_areas/constituencies/calibrate.py index 2e35d3928..f361af6b5 100644 --- a/policyengine_uk_data/datasets/local_areas/constituencies/calibrate.py +++ b/policyengine_uk_data/datasets/local_areas/constituencies/calibrate.py @@ -1,3 +1,4 @@ +import numpy as np import pandas as pd from policyengine_uk_data.utils.calibrate import calibrate_local_areas from policyengine_uk_data.datasets.local_areas.constituencies.loss import ( @@ -91,6 +92,12 @@ def get_performance(weights, m_c, y_c, m_n, y_n, excluded_targets): } ) national_target_validation["target"] = national_actuals.values + # Households with a non-zero value for each national target: how many + # records support its estimate (utils/fit_report.py publishes a target's + # error only from 10 records up). + national_target_validation["records"] = ( + (np.asarray(national_target_matrix) != 0).sum(axis=0) + ) national_target_validation["error"] = ( national_target_validation["estimate"] - national_target_validation["target"] diff --git a/policyengine_uk_data/tests/test_fit_report.py b/policyengine_uk_data/tests/test_fit_report.py index 276540e26..8d427bf4d 100644 --- a/policyengine_uk_data/tests/test_fit_report.py +++ b/policyengine_uk_data/tests/test_fit_report.py @@ -1,4 +1,6 @@ -"""The final calibration fit and the no-worsening comparison (utils/fit_report.py).""" +"""The final calibration fit summary and the no-worsening comparison (utils/fit_report.py).""" + +import json import numpy as np import pandas as pd @@ -7,110 +9,143 @@ from hypothesis import strategies as st from policyengine_uk_data.utils.fit_report import ( + MIN_RECORDS, FitPolicy, - compare_fits, - final_fit, + compare, load_policy, main, + summarise, ) POLICY = FitPolicy(share_drop_allowance=0.02, target_flag_threshold=0.2) -def _log(national: dict, local=(), epochs=(0, 30)): +def _log(national: dict, local=(), epochs=(0, 30), records=100): """A calibration log: national {metric: (estimate, target)}, local [(est, target)].""" rows = [] for epoch in epochs: for metric, (estimate, target) in national.items(): - rows.append(("UK", metric, estimate, target, epoch)) + rows.append(("UK", 0, metric, estimate, target, epoch, records)) for i, (estimate, target) in enumerate(local): - rows.append((f"area {i}", f"local/{i}", estimate, target, epoch)) - return pd.DataFrame(rows, columns=["name", "metric", "estimate", "target", "epoch"]) + rows.append( + (f"area {i}", f"E{i}", f"local/{i}", estimate, target, epoch, np.nan) + ) + return pd.DataFrame( + rows, + columns=["name", "index", "metric", "estimate", "target", "epoch", "records"], + ) + + +LOCAL = [(100, 100)] * 10 -def test_final_fit_reads_the_last_epoch_and_drops_zero_targets(): +def test_summarise_reads_the_last_epoch_and_drops_zero_targets(): log = _log( {"a": (105, 100), "b": (150, 100), "zero": (5, 0)}, local=[(9, 10), (20, 10)] ) log.loc[log.epoch == 0, "estimate"] = 1 # earlier epochs are ignored - fit = final_fit(log) - assert fit.epoch == 30 - assert fit.national.to_dict() == pytest.approx({"a": 0.05, "b": 0.5}) - assert fit.national_within == 0.5 - assert fit.local_within == 0.5 + s = summarise(log) + assert s["epoch"] == 30 + assert s["national"]["a"] == {"within": True, "error": pytest.approx(0.05)} + assert s["national"]["b"] == {"within": False, "error": pytest.approx(0.5)} + assert "zero" not in s["national"] + assert s["local"] == {"E0|local/0": True, "E1|local/1": False} + + +def test_a_target_with_few_records_has_no_published_error(): + """Sol review of #599, P1: an estimate from a few records is not published.""" + s = summarise(_log({"rare": (123.45, 100)}, local=LOCAL, records=MIN_RECORDS - 1)) + assert s["national"]["rare"] == {"within": False, "error": None} + assert "123.45" not in json.dumps(s) + # A log without the records column publishes no errors at all. + s = summarise(_log({"a": (101, 100)}, local=LOCAL).drop(columns="records")) + assert s["national"]["a"]["error"] is None + # Local cells give only within 10% or not. + s = summarise(_log({"a": (101, 100)}, local=[(123.45, 100)])) + assert s["local"] == {"E0|local/0": False} def test_a_malformed_log_is_refused(): with pytest.raises(ValueError, match="no rows"): - final_fit(_log({}, epochs=())) - log = _log({"a": (np.inf, 100)}) + summarise(_log({}, epochs=())) with pytest.raises(ValueError, match="non-finite"): - final_fit(log) - log = pd.concat([_log({"a": (1, 1)}), _log({"a": (2, 1)})]) + summarise(_log({"a": (np.inf, 100)})) with pytest.raises(ValueError, match="repeats"): - final_fit(log) + summarise(pd.concat([_log({"a": (1, 1)}), _log({"a": (2, 1)})])) + + +def _summary(national_errors, local=LOCAL, epochs=(0, 30)): + return summarise( + _log( + {f"t{i}": (100 * (1 + e), 100) for i, e in enumerate(national_errors)}, + local, + epochs, + ) + ) def test_a_drop_beyond_the_allowance_fails_and_within_it_passes(): - targets = {f"t{i}": (100, 100) for i in range(100)} - base = final_fit(_log(targets, local=[(100, 100)] * 10)) - worse = dict(targets, **{f"t{i}": (150, 100) for i in range(3)}) - head = final_fit(_log(worse, local=[(100, 100)] * 10)) - result = compare_fits(base, head, POLICY) + base = _summary([0.0] * 100) + head = _summary([0.5, 0.5, 0.5] + [0.0] * 97) + result = compare(base, head, POLICY) assert result["status"] == "fail" assert ( "national targets within 10% falls from 100.0% to 97.0%" in result["failures"][0] ) assert [f["target"] for f in result["flags"]] == ["t0", "t1", "t2"] - slightly = dict(targets, t0=(150, 100)) - assert ( - compare_fits(base, final_fit(_log(slightly, local=[(100, 100)] * 10)), POLICY)[ - "status" - ] - == "pass" - ) + assert compare(base, _summary([0.5] + [0.0] * 99), POLICY)["status"] == "pass" -def test_builds_of_different_epochs_cannot_be_compared(): - base = final_fit(_log({"a": (1, 1)}, epochs=(0, 30))) - head = final_fit(_log({"a": (1, 1)}, epochs=(0, 510, 511))) - assert compare_fits(base, head, POLICY)["status"] == "fail" - # 32 epochs log last at 30, or at 31 once the final epoch is saved (#594). - final = final_fit(_log({"a": (1, 1)}, epochs=(0, 30, 31))) - result = compare_fits(base, final, POLICY) +def test_added_targets_do_not_count_against_the_existing_ones(): + """Sol review of #599, P2: 100 unchanged targets plus 3 poor new ones pass.""" + base = _summary([0.0] * 100) + head = summarise( + _log( + { + **{f"t{i}": (100, 100) for i in range(100)}, + **{f"new{i}": (200, 100) for i in range(3)}, + }, + LOCAL, + ) + ) + result = compare(base, head, POLICY) assert result["status"] == "pass" - assert "saves the final epoch" in result["notes"][0] + assert result["summary"]["national"]["compared"] == 100 + assert "3 national targets are new: new0, new1, new2" in result["notes"] -def test_the_pull_request_workflow_uploads_only_calibration_logs(): - """The datasets are licensed microdata: no job may upload or cache one.""" - from pathlib import Path +@pytest.mark.parametrize( + "head_log", + [ + _log({"a": (1, 1)}, local=()), # only national rows + _log({}, local=LOCAL), # only local rows + _log({"a": (1, 0)}, local=[(1, 0)]), # only zero targets + ], +) +def test_an_empty_target_group_fails(head_log): + """Sol review of #599, P2: empty groups used to give NaN shares and pass.""" + result = compare(_summary([0.0] * 10), summarise(head_log), POLICY) + assert result["status"] == "fail" + assert "has no" in result["failures"][0] - import yaml - workflow = Path(__file__).parents[2] / ".github" / "workflows" / "pull_request.yaml" - jobs = yaml.safe_load(workflow.read_text())["jobs"] - uploads = [] - for job in jobs.values(): - for step in job.get("steps", []): - uses = step.get("uses", "") - assert not uses.startswith("actions/cache"), step - if uses.startswith("actions/upload-artifact"): - uploads.append(step["with"]["path"]) - assert uploads and all(p.endswith("calibration_log.csv") for p in uploads), uploads - assert jobs["fit-comparison"]["needs"] == ["test", "base-build"] +def test_an_incomplete_last_epoch_fails(): + log = _log({f"t{i}": (100, 100) for i in range(10)}, LOCAL) + log = log[~((log.epoch == 30) & (log.metric == "t9"))] + result = compare(_summary([0.0] * 10), summarise(log), POLICY) + assert result["status"] == "fail" and "incomplete" in result["failures"][0] -def test_new_and_removed_targets_are_noted_not_failed(): - base = final_fit(_log({"a": (1, 1), "old": (1, 1)})) - head = final_fit(_log({"a": (1, 1), "new": (1, 1)})) - result = compare_fits(base, head, POLICY) +def test_builds_of_different_epochs_cannot_be_compared(): + base = _summary([0.0], epochs=(0, 30)) + assert ( + compare(base, _summary([0.0], epochs=(0, 510, 511)), POLICY)["status"] == "fail" + ) + # 32 epochs log last at 30, or at 31 once the final epoch is saved (#594). + result = compare(base, _summary([0.0], epochs=(0, 30, 31)), POLICY) assert result["status"] == "pass" - assert result["notes"] == [ - "1 national targets are new: new", - "1 national targets are gone: old", - ] + assert "saves the final epoch" in result["notes"][0] errors = st.lists(st.floats(0, 3), min_size=1, max_size=40) @@ -119,13 +154,8 @@ def test_new_and_removed_targets_are_noted_not_failed(): @settings(max_examples=200, deadline=None) @given(national=errors, local=errors) def test_a_build_compared_with_itself_passes_with_nothing_flagged(national, local): - fit = final_fit( - _log( - {f"t{i}": (100 * (1 + e), 100) for i, e in enumerate(national)}, - local=[(100 * (1 + e), 100) for e in local], - ) - ) - result = compare_fits(fit, fit, POLICY) + s = _summary(national, local=[(100 * (1 + e), 100) for e in local]) + result = compare(s, s, POLICY) assert result["status"] == "pass" and result["flags"] == [] assert all(row["change"] == 0 for row in result["summary"].values()) @@ -134,18 +164,13 @@ def test_a_build_compared_with_itself_passes_with_nothing_flagged(national, loca @given(base=errors, data=st.data()) def test_improving_every_target_never_fails_and_changes_are_antisymmetric(base, data): better = [data.draw(st.floats(0, e)) for e in base] - fit_base = final_fit( - _log({f"t{i}": (100 * (1 + e), 100) for i, e in enumerate(base)}) - ) - fit_head = final_fit( - _log({f"t{i}": (100 * (1 + e), 100) for i, e in enumerate(better)}) - ) - forward = compare_fits(fit_base, fit_head, POLICY) + forward = compare(_summary(base), _summary(better), POLICY) assert forward["status"] == "pass" and forward["flags"] == [] - backward = compare_fits(fit_head, fit_base, POLICY) + backward = compare(_summary(better), _summary(base), POLICY) for name in ("national", "local"): - a, b = forward["summary"][name]["change"], backward["summary"][name]["change"] - assert a == pytest.approx(-b) or (np.isnan(a) and np.isnan(b)) + assert forward["summary"][name]["change"] == pytest.approx( + -backward["summary"][name]["change"] + ) def test_the_shipped_policy_loads(): @@ -154,26 +179,56 @@ def test_the_shipped_policy_loads(): assert policy.target_flag_threshold == 0.2 -def test_the_command_exits_one_on_a_failure(tmp_path): +def test_the_commands_summarise_and_exit_one_on_a_failure(tmp_path): + base_log, head_log = tmp_path / "base.csv", tmp_path / "head.csv" targets = {f"t{i}": (100, 100) for i in range(50)} - _log(targets).to_csv(tmp_path / "base.csv", index=False) - _log(dict(targets, **{f"t{i}": (200, 100) for i in range(5)})).to_csv( - tmp_path / "head.csv", index=False + _log(targets, LOCAL).to_csv(base_log, index=False) + _log(dict(targets, **{f"t{i}": (200, 100) for i in range(5)}), LOCAL).to_csv( + head_log, index=False ) - out = tmp_path / "fit.md" - assert ( - main( - [ - "compare", - str(tmp_path / "base.csv"), - str(tmp_path / "head.csv"), - "--markdown", - str(out), - ] + for log in (base_log, head_log): + assert ( + main(["summarise", str(log), "--out", str(log.with_suffix(".json"))]) == 0 ) - == 1 + out = tmp_path / "fit.md" + code = main( + [ + "compare", + str(tmp_path / "base.json"), + str(tmp_path / "head.json"), + "--markdown", + str(out), + ] ) + assert code == 1 assert "## Calibration fit against the base build: fail" in out.read_text() assert ( - main(["compare", str(tmp_path / "base.csv"), str(tmp_path / "base.csv")]) == 0 + main(["compare", str(tmp_path / "base.json"), str(tmp_path / "base.json")]) == 0 + ) + + +def test_the_pull_request_workflow_uploads_only_summaries(): + """The datasets are licensed microdata, and raw calibration logs stay on the runner.""" + from pathlib import Path + + import yaml + + workflow = Path(__file__).parents[2] / ".github" / "workflows" / "pull_request.yaml" + jobs = yaml.safe_load(workflow.read_text())["jobs"] + uploads = sorted( + step["with"]["path"] + for job in jobs.values() + for step in job.get("steps", []) + if step.get("uses", "").startswith("actions/upload-artifact") + ) + assert uploads == ["base_fit_summary.json", "fit_summary.json"] + for job in jobs.values(): + for step in job.get("steps", []): + assert not step.get("uses", "").startswith("actions/cache"), step + assert jobs["fit-comparison"]["needs"] == ["test", "base-build"] + post = next( + s + for s in jobs["fit-comparison"]["steps"] + if s.get("name", "").startswith("Post") ) + assert "if [ ! -s fit.md ]" in post["run"] diff --git a/policyengine_uk_data/utils/fit_report.py b/policyengine_uk_data/utils/fit_report.py index ef2d2cd32..379a99dc1 100644 --- a/policyengine_uk_data/utils/fit_report.py +++ b/policyengine_uk_data/utils/fit_report.py @@ -1,29 +1,37 @@ -"""Final calibration fit from a build's log, and the comparison of two builds. +"""Final calibration fit of a build, summarised safely, and the comparison of two. A build writes ``constituency_calibration_log.csv``: each target's estimate -and value every tenth epoch, and at the last one. ``final_fit`` reads the -last epoch. A target's error is |estimate / target - 1|, and targets of zero -are left out. The national rows are those named ``UK``. +and value every tenth epoch and at the last one, nationally (rows named +``UK``) and for each constituency. The log stays on the runner that built +it. ``summarise`` reduces its last epoch to what the comparison needs, and +only the summary is uploaded: -``compare_fits`` checks that a pull request does not worsen the fit of the -build it would replace, both built the same way: same environment, epochs -and clone count. Two rules come from ``targets/fit_policy.yaml``: +- for every national and local target, whether its estimate is within 10% + of its value (|estimate / target - 1| <= 0.1), with zero targets left out; +- for a national target supported by at least ``MIN_RECORDS`` households + (the log's ``records`` column), its error. A target supported by fewer has + no error in the summary: an estimate built from a few records, divided + by a published count, could give one household's value back. -- **Fail** when the share of national (or local) targets within 10% falls - by more than the allowance. -- **Flag** each national target whose error rises by more than the - same-code spread, for review. +``compare`` checks that a pull request does not worsen the fit of the build +it would replace, built the same way (same environment, epochs and clones), +using the policy in ``targets/fit_policy.yaml``: -Per-target moves are not yet gates: builds of the same code currently -differ by up to 0.62 on one target (#598). +- **Fail** when the share of national, or of local, targets within 10% + falls by more than the allowance, over the targets both builds have. + Added and removed targets are reported, not counted. +- **Flag** each national target whose error rises by more than the + same-code spread, for review. Per-target moves do not fail while builds + of the same code differ by up to 0.62 on one target (#598). - python -m policyengine_uk_data.utils.fit_report compare BASE.csv HEAD.csv \\ - [--markdown out.md] + python -m policyengine_uk_data.utils.fit_report summarise LOG.csv --out summary.json + python -m policyengine_uk_data.utils.fit_report compare BASE.json HEAD.json [--markdown out.md] """ from __future__ import annotations import argparse +import json import sys from dataclasses import dataclass from pathlib import Path @@ -34,31 +42,10 @@ import yaml POLICY_PATH = Path(__file__).parents[1] / "targets" / "fit_policy.yaml" +SCHEMA = "policyengine-uk-data-fit-summary/1" WITHIN = 0.10 -_COLUMNS = ["name", "metric", "estimate", "target", "epoch"] - - -@dataclass(frozen=True) -class Fit: - epoch: int - national: pd.Series # error by target name - local_errors: np.ndarray - - @property - def national_within(self) -> float: - return ( - float(np.mean(self.national <= WITHIN)) - if len(self.national) - else float("nan") - ) - - @property - def local_within(self) -> float: - return ( - float(np.mean(self.local_errors <= WITHIN)) - if len(self.local_errors) - else float("nan") - ) +MIN_RECORDS = 10 +_COLUMNS = ["name", "index", "metric", "estimate", "target", "epoch"] @dataclass(frozen=True) @@ -78,104 +65,158 @@ def load_policy(path=None) -> FitPolicy: ) -def errors(frame: pd.DataFrame) -> pd.Series: - """|estimate / target - 1| for rows with a non-zero target.""" - frame = frame[frame.target != 0] - return (frame.estimate / frame.target - 1).abs() - - -def final_fit(log) -> Fit: - """The fit at a calibration log's last epoch.""" - df = ( - pd.read_csv(log, usecols=_COLUMNS) if not isinstance(log, pd.DataFrame) else log - ) +def summarise(log) -> dict: + """The last epoch of a calibration log, reduced to what may leave the runner.""" + if isinstance(log, pd.DataFrame): + df = log + else: + header = pd.read_csv(log, nrows=0).columns + usecols = _COLUMNS + (["records"] if "records" in header else []) + df = pd.read_csv(log, usecols=usecols) if df.empty: raise ValueError("the calibration log has no rows") - last = int(df.epoch.max()) + epochs = sorted(df.epoch.unique()) + last = int(epochs[-1]) rows = df[df.epoch == last] + previous = int((df.epoch == epochs[-2]).sum()) if len(epochs) > 1 else None national = rows[rows.name == "UK"] if national.metric.duplicated().any(): raise ValueError( "the calibration log repeats a national target at its last epoch" ) - national_errors = errors(national.set_index("metric")) - local_errors = errors(rows[rows.name != "UK"]).to_numpy() - if not np.isfinite(national_errors).all() or not np.isfinite(local_errors).all(): + national = national[national.target != 0].set_index("metric") + local = rows[(rows.name != "UK") & (rows.target != 0)] + n_errors = (national.estimate / national.target - 1).abs() + l_errors = (local.estimate / local.target - 1).abs() + if not np.isfinite(n_errors).all() or not np.isfinite(l_errors).all(): raise ValueError("the calibration log has non-finite estimates") - return Fit(last, national_errors, local_errors) + records = national["records"] if "records" in national else None + return { + "schema": SCHEMA, + "epoch": last, + "rows_last_epoch": int(len(rows)), + "rows_previous_epoch": previous, + "national": { + metric: { + "within": bool(error <= WITHIN), + "error": float(error) + if records is not None and int(records[metric]) >= MIN_RECORDS + else None, + } + for metric, error in n_errors.items() + }, + "local": { + f"{i}|{m}": bool(e <= WITHIN) + for i, m, e in zip(local["index"], local.metric, l_errors) + }, + } + + +def _within(value) -> bool: + return value["within"] if isinstance(value, dict) else bool(value) + +def _share(flags: list) -> float: + return float(np.mean(flags)) if flags else float("nan") -def compare_fits(base: Fit, head: Fit, policy: FitPolicy) -> dict: + +def compare(base: dict, head: dict, policy: FitPolicy) -> dict: """Failures, flags and the summary for a head build against its base.""" failures, notes = [], [] + for side, s in (("base", base), ("head", head)): + if s.get("schema") != SCHEMA: + failures.append(f"The {side} fit summary is missing or malformed.") + continue + if not s["national"] or not s["local"]: + missing = "national" if not s["national"] else "local" + failures.append(f"The {side} build's last epoch has no {missing} targets.") + previous = s["rows_previous_epoch"] + if previous and s["rows_last_epoch"] < previous: + failures.append( + f"The {side} build's last logged epoch ({s['epoch']}) has fewer rows " + f"({s['rows_last_epoch']}) than the one before ({previous}): the log " + "is incomplete." + ) + if failures: + return { + "status": "fail", + "failures": failures, + "notes": [], + "flags": [], + "summary": {}, + "targets_compared": 0, + } # A log's last epoch is a multiple of ten or, since #594, the final one; # the same epoch count gives last epochs less than ten apart. - if abs(base.epoch - head.epoch) >= 10: + if abs(base["epoch"] - head["epoch"]) >= 10: failures.append( - f"The builds ran different epochs (logs end at {base.epoch} and " - f"{head.epoch}), so their fits cannot be compared." + f"The builds' logs end at epochs {base['epoch']} and {head['epoch']}, so " + "they ran different epoch counts and their fits cannot be compared." ) - elif base.epoch != head.epoch: + elif base["epoch"] != head["epoch"]: notes.append( - f"The base log ends at epoch {base.epoch} and the head's at {head.epoch}: " - "one of them saves the final epoch (#594)." + f"The base log ends at epoch {base['epoch']} and the head's at " + f"{head['epoch']}: one of them saves the final epoch (#594)." ) summary = {} - for name, b, h in ( - ("national", base.national_within, head.national_within), - ("local", base.local_within, head.local_within), - ): - summary[name] = {"base": b, "head": h, "change": h - b} - if h < b - policy.share_drop_allowance: + for name in ("national", "local"): + b, h = base[name], head[name] + common = sorted(set(b) & set(h)) + added, removed = sorted(set(h) - set(b)), sorted(set(b) - set(h)) + before = _share([_within(b[k]) for k in common]) + after = _share([_within(h[k]) for k in common]) + summary[name] = { + "base": before, + "head": after, + "change": after - before, + "compared": len(common), + } + if added: + notes.append( + f"{len(added)} {name} targets are new: {', '.join(added[:10])}" + ) + if removed: + notes.append( + f"{len(removed)} {name} targets are gone: {', '.join(removed[:10])}" + ) + if not common: + failures.append(f"The builds have no {name} targets in common.") + elif after < before - policy.share_drop_allowance: failures.append( - f"The share of {name} targets within 10% falls from {b:.1%} to {h:.1%}, " - f"by more than the {policy.share_drop_allowance:.1%} allowed." + f"The share of {name} targets within 10% falls from {before:.1%} to " + f"{after:.1%}, over the {len(common)} targets both builds have, by " + f"more than the {policy.share_drop_allowance:.1%} allowed." ) - common = base.national.index.intersection(head.national.index) - added = sorted(set(head.national.index) - set(common)) - removed = sorted(set(base.national.index) - set(common)) - if added: - notes.append(f"{len(added)} national targets are new: {', '.join(added[:10])}") - if removed: - notes.append( - f"{len(removed)} national targets are gone: {', '.join(removed[:10])}" - ) - # Largest rise first; ties keep the log's order, so a comparison is - # reproducible. - rise = (head.national[common] - base.national[common]).sort_values( - ascending=False, kind="mergesort" - ) - flags = [ - {"target": t, "base": float(base.national[t]), "head": float(head.national[t])} - for t, r in rise.items() - if r > policy.target_flag_threshold - ] + flags = [] + for target in sorted(set(base["national"]) & set(head["national"])): + b = base["national"][target]["error"] + h = head["national"][target]["error"] + if b is not None and h is not None and h - b > policy.target_flag_threshold: + flags.append({"target": target, "base": b, "head": h}) + flags.sort(key=lambda f: f["base"] - f["head"]) return { "status": "fail" if failures else "pass", "failures": failures, "notes": notes, "flags": flags, "summary": summary, - "targets_compared": len(common), + "targets_compared": summary["national"]["compared"], } def render(result: dict, policy: FitPolicy) -> str: - lines = [ - f"## Calibration fit against the base build: {result['status']}", - "", - "| Targets within 10% | Base | Head | Change |", - "|---|---:|---:|---:|", - ] - for name, row in result["summary"].items(): - lines.append( - f"| {name.capitalize()} | {row['base']:.1%} | {row['head']:.1%} | {100 * row['change']:+.1f}pp |" - ) - lines.append("") - lines.append( - f"A drop of more than {policy.share_drop_allowance:.1%} fails. " - f"{result['targets_compared']} national targets were compared." - ) + lines = [f"## Calibration fit against the base build: {result['status']}", ""] + if result["summary"]: + lines += [ + "| Targets within 10% (those both builds have) | Base | Head | Change |", + "|---|---:|---:|---:|", + ] + for name, row in result["summary"].items(): + lines.append( + f"| {name.capitalize()} ({row['compared']:,}) | {row['base']:.1%} " + f"| {row['head']:.1%} | {100 * row['change']:+.1f}pp |" + ) + lines += ["", f"A drop of more than {policy.share_drop_allowance:.1%} fails."] if result["failures"]: lines += ["", "### Failures", ""] + [f"- {f}" for f in result["failures"]] if result["notes"]: @@ -183,7 +224,8 @@ def render(result: dict, policy: FitPolicy) -> str: flags = result["flags"] lines += [ "", - f"### National targets whose error rises by more than {policy.target_flag_threshold:.2f} ({len(flags)})", + "### National targets whose error rises by more than " + f"{policy.target_flag_threshold:.2f} ({len(flags)})", "", ] if flags: @@ -198,7 +240,9 @@ def render(result: dict, policy: FitPolicy) -> str: lines.append("None.") lines += [ "", - "Flags are for review; builds of the same code differ this much (#598).", + "Flags are for review; builds of the same code differ this much (#598). " + f"Targets supported by fewer than {MIN_RECORDS} households have no published " + "error.", ] return "\n".join(lines) + "\n" @@ -208,16 +252,29 @@ def main(argv: Optional[list] = None) -> int: prog="python -m policyengine_uk_data.utils.fit_report" ) sub = parser.add_subparsers(dest="command", required=True) + s = sub.add_parser("summarise", help="reduce a calibration log to a safe summary") + s.add_argument("log") + s.add_argument("--out", required=True) c = sub.add_parser( - "compare", help="compare a head build's final fit with its base's" + "compare", help="compare a head build's fit summary with its base's" ) c.add_argument("base") c.add_argument("head") c.add_argument("--markdown", help="write the comparison here as markdown") c.add_argument("--policy", help="fit policy YAML (default targets/fit_policy.yaml)") args = parser.parse_args(argv) + if args.command == "summarise": + summary = summarise(args.log) + Path(args.out).write_text(json.dumps(summary)) + print( + f"epoch {summary['epoch']}: {len(summary['national'])} national, " + f"{len(summary['local'])} local targets" + ) + return 0 policy = load_policy(args.policy) - result = compare_fits(final_fit(args.base), final_fit(args.head), policy) + base = json.loads(Path(args.base).read_text()) + head = json.loads(Path(args.head).read_text()) + result = compare(base, head, policy) text = render(result, policy) if args.markdown: Path(args.markdown).write_text(text) From 564f3702ef67cccf1741098eb5cea7d88b8c5534 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Thu, 15 Oct 2026 09:27:16 -0400 Subject: [PATCH 3/8] Format calibrate.py and say only the fit summary leaves the base build Co-Authored-By: Claude Opus 5.5 --- .github/workflows/pull_request.yaml | 7 ++++--- .../datasets/local_areas/constituencies/calibrate.py | 4 ++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/.github/workflows/pull_request.yaml b/.github/workflows/pull_request.yaml index f7cf0b8c6..8e7b30737 100644 --- a/.github/workflows/pull_request.yaml +++ b/.github/workflows/pull_request.yaml @@ -106,9 +106,10 @@ jobs: TESTING: "1" # The no-worsening check (W3). The base is built the same way as the head: # TESTING, one clone and the head's lockfile, so both builds run one model - # and environment and the comparison measures the data change. Only the - # base's calibration log leaves the job. Like the head's, it holds target - # estimates and values, aggregates; no dataset is uploaded or cached. + # and environment and the comparison measures the data change. As in the + # test job, only the fit summary leaves it. No dataset or calibration log + # is uploaded, and no dataset is cached (setup-uv caches Python packages + # only). base-build: name: Build the base for comparison runs-on: ubuntu-latest diff --git a/policyengine_uk_data/datasets/local_areas/constituencies/calibrate.py b/policyengine_uk_data/datasets/local_areas/constituencies/calibrate.py index f361af6b5..fd29cec73 100644 --- a/policyengine_uk_data/datasets/local_areas/constituencies/calibrate.py +++ b/policyengine_uk_data/datasets/local_areas/constituencies/calibrate.py @@ -96,8 +96,8 @@ def get_performance(weights, m_c, y_c, m_n, y_n, excluded_targets): # records support its estimate (utils/fit_report.py publishes a target's # error only from 10 records up). national_target_validation["records"] = ( - (np.asarray(national_target_matrix) != 0).sum(axis=0) - ) + np.asarray(national_target_matrix) != 0 + ).sum(axis=0) national_target_validation["error"] = ( national_target_validation["estimate"] - national_target_validation["target"] From b9571570b1e451276ebd7e9bdb8707b78c37a780 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Thu, 15 Oct 2026 09:35:18 -0400 Subject: [PATCH 4/8] Describe the fit summary in the changelog fragment Co-Authored-By: Claude Opus 5.5 --- changelog.d/fit-comparison.added.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/fit-comparison.added.md b/changelog.d/fit-comparison.added.md index 6c7bebf99..b0a949fea 100644 --- a/changelog.d/fit-comparison.added.md +++ b/changelog.d/fit-comparison.added.md @@ -1 +1 @@ -Build each pull request's base the same way as its head, with the head's lockfile, and fail when the share of national or local targets within 10% falls by more than 2 percentage points (`targets/fit_policy.yaml`). National targets whose error rises by more than 0.2 are flagged in a pull request comment. Adds `python -m policyengine_uk_data.utils.fit_report compare`. +Build each pull request's base the same way as its head, with the head's lockfile, and fail when the share of national or local targets within 10% falls by more than 2 percentage points over the targets both builds have (`targets/fit_policy.yaml`). National targets whose error rises by more than 0.2 are flagged in a pull request comment. The raw calibration logs stay on the runner; only a disclosure-controlled fit summary is uploaded (`python -m policyengine_uk_data.utils.fit_report summarise` and `compare`). From 342ea7968c677d3c7bbeb81f5ea8234b2f03d284 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Thu, 15 Oct 2026 09:35:58 -0400 Subject: [PATCH 5/8] Say the flag evidence predates the records filter Co-Authored-By: Claude Opus 5.5 --- policyengine_uk_data/targets/fit_policy.yaml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/policyengine_uk_data/targets/fit_policy.yaml b/policyengine_uk_data/targets/fit_policy.yaml index c0674db4c..fc717b79e 100644 --- a/policyengine_uk_data/targets/fit_policy.yaml +++ b/policyengine_uk_data/targets/fit_policy.yaml @@ -25,5 +25,7 @@ national_target: # pair (mean 1.8), so a list stays short. 0.1 gives up to 11 from noise # alone, and 0.3 would hide the largest same-code move on the OBR, ONS # population and DWP targets (0.21). Per-target moves don't fail until - # #598 is fixed. + # #598 is fixed. Those counts used every national target's error; the fit + # summary now publishes an error only for targets supported by at least + # 10 households (fit_report.MIN_RECORDS), so fewer can flag. flag_rise: 0.2 From 9f50a952b3954d540a9be49dfde24e927b99c9df Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Thu, 15 Oct 2026 15:35:30 -0400 Subject: [PATCH 6/8] Publish only which targets are within 10% (Sol re-review of #599) Per-target errors could not be published safely: the dataset repeats households (output-area clones, SPI and CGT donors), so a row count is not a respondent count, and one household can dominate a target. The fit summary now holds only within-10% booleans, and a national target that leaves the band on the head is flagged in place of an error rise of more than 0.2. On the five same-code builds, 1 to 10 of 644 national targets leave the band per pair (mean 4.5). The records column in the calibration log is reverted. Co-Authored-By: Claude Opus 5.5 --- changelog.d/fit-comparison.added.md | 2 +- .../local_areas/constituencies/calibrate.py | 7 -- policyengine_uk_data/targets/fit_policy.yaml | 23 ++++-- policyengine_uk_data/tests/test_fit_report.py | 59 +++++++------ policyengine_uk_data/utils/fit_report.py | 82 ++++++++----------- 5 files changed, 83 insertions(+), 90 deletions(-) diff --git a/changelog.d/fit-comparison.added.md b/changelog.d/fit-comparison.added.md index b0a949fea..b996a583a 100644 --- a/changelog.d/fit-comparison.added.md +++ b/changelog.d/fit-comparison.added.md @@ -1 +1 @@ -Build each pull request's base the same way as its head, with the head's lockfile, and fail when the share of national or local targets within 10% falls by more than 2 percentage points over the targets both builds have (`targets/fit_policy.yaml`). National targets whose error rises by more than 0.2 are flagged in a pull request comment. The raw calibration logs stay on the runner; only a disclosure-controlled fit summary is uploaded (`python -m policyengine_uk_data.utils.fit_report summarise` and `compare`). +Build each pull request's base the same way as its head, with the head's lockfile, and fail when the share of national or local targets within 10% falls by more than 2 percentage points over the targets both builds have (`targets/fit_policy.yaml`). National targets that are within 10% on the base and not on the head are flagged in a pull request comment. The raw calibration logs stay on the runner; only a summary of which targets are within 10% is uploaded, with no estimate or error (`python -m policyengine_uk_data.utils.fit_report summarise` and `compare`). diff --git a/policyengine_uk_data/datasets/local_areas/constituencies/calibrate.py b/policyengine_uk_data/datasets/local_areas/constituencies/calibrate.py index fd29cec73..2e35d3928 100644 --- a/policyengine_uk_data/datasets/local_areas/constituencies/calibrate.py +++ b/policyengine_uk_data/datasets/local_areas/constituencies/calibrate.py @@ -1,4 +1,3 @@ -import numpy as np import pandas as pd from policyengine_uk_data.utils.calibrate import calibrate_local_areas from policyengine_uk_data.datasets.local_areas.constituencies.loss import ( @@ -92,12 +91,6 @@ def get_performance(weights, m_c, y_c, m_n, y_n, excluded_targets): } ) national_target_validation["target"] = national_actuals.values - # Households with a non-zero value for each national target: how many - # records support its estimate (utils/fit_report.py publishes a target's - # error only from 10 records up). - national_target_validation["records"] = ( - np.asarray(national_target_matrix) != 0 - ).sum(axis=0) national_target_validation["error"] = ( national_target_validation["estimate"] - national_target_validation["target"] diff --git a/policyengine_uk_data/targets/fit_policy.yaml b/policyengine_uk_data/targets/fit_policy.yaml index fc717b79e..2fe344d20 100644 --- a/policyengine_uk_data/targets/fit_policy.yaml +++ b/policyengine_uk_data/targets/fit_policy.yaml @@ -20,12 +20,17 @@ share_within_10pct: allowed_drop: 0.02 national_target: - # Flag, for review, each national target whose error |estimate / target - - # 1| rises by more than this. The five builds give 0 to 6 such rises per - # pair (mean 1.8), so a list stays short. 0.1 gives up to 11 from noise - # alone, and 0.3 would hide the largest same-code move on the OBR, ONS - # population and DWP targets (0.21). Per-target moves don't fail until - # #598 is fixed. Those counts used every national target's error; the fit - # summary now publishes an error only for targets supported by at least - # 10 households (fit_report.MIN_RECORDS), so fewer can flag. - flag_rise: 0.2 + # Flag, for review, each national target within 10% of its value on the + # base build and not on the head. Across the 20 ordered pairs of the five + # builds above, 1 to 10 of the 644 national targets leave the band per + # pair (mean 4.5), so a list stays short. + # + # This replaces flagging an error rise of more than 0.2 (adopted under + # d1344 on 15 October), which needed each target's error in the uploaded + # summary. Sol's re-review of #599 found no safe way to publish them: the + # dataset repeats households (output-area clones, SPI and CGT donors), so + # a count of rows can't show how many respondents support an estimate, and + # one household can dominate a target. The within-10% booleans are bounds, + # not values. Same-code pairs gave 0 to 6 flags under the old rule. + # Per-target moves don't fail until #598 is fixed. + flag: left_band diff --git a/policyengine_uk_data/tests/test_fit_report.py b/policyengine_uk_data/tests/test_fit_report.py index 8d427bf4d..03cb056a8 100644 --- a/policyengine_uk_data/tests/test_fit_report.py +++ b/policyengine_uk_data/tests/test_fit_report.py @@ -9,7 +9,6 @@ from hypothesis import strategies as st from policyengine_uk_data.utils.fit_report import ( - MIN_RECORDS, FitPolicy, compare, load_policy, @@ -17,22 +16,19 @@ summarise, ) -POLICY = FitPolicy(share_drop_allowance=0.02, target_flag_threshold=0.2) +POLICY = FitPolicy(share_drop_allowance=0.02) -def _log(national: dict, local=(), epochs=(0, 30), records=100): +def _log(national: dict, local=(), epochs=(0, 30)): """A calibration log: national {metric: (estimate, target)}, local [(est, target)].""" rows = [] for epoch in epochs: for metric, (estimate, target) in national.items(): - rows.append(("UK", 0, metric, estimate, target, epoch, records)) + rows.append(("UK", 0, metric, estimate, target, epoch)) for i, (estimate, target) in enumerate(local): - rows.append( - (f"area {i}", f"E{i}", f"local/{i}", estimate, target, epoch, np.nan) - ) + rows.append((f"area {i}", f"E{i}", f"local/{i}", estimate, target, epoch)) return pd.DataFrame( - rows, - columns=["name", "index", "metric", "estimate", "target", "epoch", "records"], + rows, columns=["name", "index", "metric", "estimate", "target", "epoch"] ) @@ -46,23 +42,29 @@ def test_summarise_reads_the_last_epoch_and_drops_zero_targets(): log.loc[log.epoch == 0, "estimate"] = 1 # earlier epochs are ignored s = summarise(log) assert s["epoch"] == 30 - assert s["national"]["a"] == {"within": True, "error": pytest.approx(0.05)} - assert s["national"]["b"] == {"within": False, "error": pytest.approx(0.5)} + assert s["national"] == {"a": True, "b": False} assert "zero" not in s["national"] assert s["local"] == {"E0|local/0": True, "E1|local/1": False} -def test_a_target_with_few_records_has_no_published_error(): - """Sol review of #599, P1: an estimate from a few records is not published.""" - s = summarise(_log({"rare": (123.45, 100)}, local=LOCAL, records=MIN_RECORDS - 1)) - assert s["national"]["rare"] == {"within": False, "error": None} - assert "123.45" not in json.dumps(s) - # A log without the records column publishes no errors at all. - s = summarise(_log({"a": (101, 100)}, local=LOCAL).drop(columns="records")) - assert s["national"]["a"]["error"] is None - # Local cells give only within 10% or not. - s = summarise(_log({"a": (101, 100)}, local=[(123.45, 100)])) - assert s["local"] == {"E0|local/0": False} +@settings(max_examples=100, deadline=None) +@given( + estimates=st.lists(st.floats(0.01, 1e12, allow_nan=False), min_size=1, max_size=20), + target=st.floats(1.0, 1e12), +) +def test_the_summary_publishes_no_estimate_or_error(estimates, target): + """Sol re-reviews of #599: rows don't count respondents and one household + can dominate a target, so nothing per target but within 10% is published.""" + log = _log( + {f"t{i}": (e, target) for i, e in enumerate(estimates)}, + local=[(e, target) for e in estimates], + ) + s = summarise(log) + for group in ("national", "local"): + assert all(isinstance(v, bool) for v in s[group].values()) + text = json.dumps(s) + for e in estimates: + assert repr(e) not in text and repr(abs(e / target - 1)) not in text def test_a_malformed_log_is_refused(): @@ -97,6 +99,18 @@ def test_a_drop_beyond_the_allowance_fails_and_within_it_passes(): assert compare(base, _summary([0.5] + [0.0] * 99), POLICY)["status"] == "pass" +def test_a_target_leaving_the_band_flags_and_one_entering_is_a_note(): + base = _summary([0.0, 0.5] + [0.0] * 98) + head = _summary([0.5, 0.0] + [0.0] * 98) + result = compare(base, head, POLICY) + assert result["status"] == "pass" + assert result["flags"] == [{"target": "t0"}] + assert "1 national targets come within 10% on the head." in result["notes"] + # A move inside the band, or outside it, is not a flag. + assert compare(_summary([0.01]), _summary([0.09]), POLICY)["flags"] == [] + assert compare(_summary([0.2]), _summary([0.9]), POLICY)["flags"] == [] + + def test_added_targets_do_not_count_against_the_existing_ones(): """Sol review of #599, P2: 100 unchanged targets plus 3 poor new ones pass.""" base = _summary([0.0] * 100) @@ -176,7 +190,6 @@ def test_improving_every_target_never_fails_and_changes_are_antisymmetric(base, def test_the_shipped_policy_loads(): policy = load_policy() assert policy.share_drop_allowance == 0.02 - assert policy.target_flag_threshold == 0.2 def test_the_commands_summarise_and_exit_one_on_a_failure(tmp_path): diff --git a/policyengine_uk_data/utils/fit_report.py b/policyengine_uk_data/utils/fit_report.py index 379a99dc1..e0ea634af 100644 --- a/policyengine_uk_data/utils/fit_report.py +++ b/policyengine_uk_data/utils/fit_report.py @@ -4,14 +4,15 @@ and value every tenth epoch and at the last one, nationally (rows named ``UK``) and for each constituency. The log stays on the runner that built it. ``summarise`` reduces its last epoch to what the comparison needs, and -only the summary is uploaded: - -- for every national and local target, whether its estimate is within 10% - of its value (|estimate / target - 1| <= 0.1), with zero targets left out; -- for a national target supported by at least ``MIN_RECORDS`` households - (the log's ``records`` column), its error. A target supported by fewer has - no error in the summary: an estimate built from a few records, divided - by a published count, could give one household's value back. +only the summary is uploaded: for every national and local target, whether +its estimate is within 10% of its value (|estimate / target - 1| <= 0.1), +with zero targets left out. No estimate or error leaves the runner. An +estimate built from a few households, or dominated by one, divided by a +published count, could give that household's value back, and the dataset +repeats households (output-area clones, SPI and CGT donors), so a count of +rows can't show how many respondents support a figure (Sol re-review of +#599). Whether an estimate is within 10% of a public target is a bound, not +a value. ``compare`` checks that a pull request does not worsen the fit of the build it would replace, built the same way (same environment, epochs and clones), @@ -20,9 +21,9 @@ - **Fail** when the share of national, or of local, targets within 10% falls by more than the allowance, over the targets both builds have. Added and removed targets are reported, not counted. -- **Flag** each national target whose error rises by more than the - same-code spread, for review. Per-target moves do not fail while builds - of the same code differ by up to 0.62 on one target (#598). +- **Flag**, for review, each national target that is within 10% on the + base and not on the head. Per-target moves do not fail while builds of + the same code move 1 to 10 national targets out of the band (#598). python -m policyengine_uk_data.utils.fit_report summarise LOG.csv --out summary.json python -m policyengine_uk_data.utils.fit_report compare BASE.json HEAD.json [--markdown out.md] @@ -42,16 +43,14 @@ import yaml POLICY_PATH = Path(__file__).parents[1] / "targets" / "fit_policy.yaml" -SCHEMA = "policyengine-uk-data-fit-summary/1" +SCHEMA = "policyengine-uk-data-fit-summary/2" WITHIN = 0.10 -MIN_RECORDS = 10 _COLUMNS = ["name", "index", "metric", "estimate", "target", "epoch"] @dataclass(frozen=True) class FitPolicy: share_drop_allowance: float - target_flag_threshold: float def load_policy(path=None) -> FitPolicy: @@ -59,9 +58,10 @@ def load_policy(path=None) -> FitPolicy: raw = yaml.safe_load(f) if raw.get("schema_version") != 1: raise ValueError("fit policy: schema_version must be 1") + if raw.get("national_target", {}).get("flag") != "left_band": + raise ValueError("fit policy: national_target.flag must be left_band") return FitPolicy( share_drop_allowance=float(raw["share_within_10pct"]["allowed_drop"]), - target_flag_threshold=float(raw["national_target"]["flag_rise"]), ) @@ -70,9 +70,7 @@ def summarise(log) -> dict: if isinstance(log, pd.DataFrame): df = log else: - header = pd.read_csv(log, nrows=0).columns - usecols = _COLUMNS + (["records"] if "records" in header else []) - df = pd.read_csv(log, usecols=usecols) + df = pd.read_csv(log, usecols=_COLUMNS) if df.empty: raise ValueError("the calibration log has no rows") epochs = sorted(df.epoch.unique()) @@ -90,20 +88,13 @@ def summarise(log) -> dict: l_errors = (local.estimate / local.target - 1).abs() if not np.isfinite(n_errors).all() or not np.isfinite(l_errors).all(): raise ValueError("the calibration log has non-finite estimates") - records = national["records"] if "records" in national else None return { "schema": SCHEMA, "epoch": last, "rows_last_epoch": int(len(rows)), "rows_previous_epoch": previous, "national": { - metric: { - "within": bool(error <= WITHIN), - "error": float(error) - if records is not None and int(records[metric]) >= MIN_RECORDS - else None, - } - for metric, error in n_errors.items() + metric: bool(error <= WITHIN) for metric, error in n_errors.items() }, "local": { f"{i}|{m}": bool(e <= WITHIN) @@ -112,10 +103,6 @@ def summarise(log) -> dict: } -def _within(value) -> bool: - return value["within"] if isinstance(value, dict) else bool(value) - - def _share(flags: list) -> float: return float(np.mean(flags)) if flags else float("nan") @@ -163,8 +150,8 @@ def compare(base: dict, head: dict, policy: FitPolicy) -> dict: b, h = base[name], head[name] common = sorted(set(b) & set(h)) added, removed = sorted(set(h) - set(b)), sorted(set(b) - set(h)) - before = _share([_within(b[k]) for k in common]) - after = _share([_within(h[k]) for k in common]) + before = _share([bool(b[k]) for k in common]) + after = _share([bool(h[k]) for k in common]) summary[name] = { "base": before, "head": after, @@ -187,13 +174,13 @@ def compare(base: dict, head: dict, policy: FitPolicy) -> dict: f"{after:.1%}, over the {len(common)} targets both builds have, by " f"more than the {policy.share_drop_allowance:.1%} allowed." ) - flags = [] - for target in sorted(set(base["national"]) & set(head["national"])): - b = base["national"][target]["error"] - h = head["national"][target]["error"] - if b is not None and h is not None and h - b > policy.target_flag_threshold: - flags.append({"target": target, "base": b, "head": h}) - flags.sort(key=lambda f: f["base"] - f["head"]) + common = sorted(set(base["national"]) & set(head["national"])) + flags = [ + {"target": t} for t in common if base["national"][t] and not head["national"][t] + ] + entered = sum(1 for t in common if head["national"][t] and not base["national"][t]) + if entered: + notes.append(f"{entered} national targets come within 10% on the head.") return { "status": "fail" if failures else "pass", "failures": failures, @@ -224,25 +211,20 @@ def render(result: dict, policy: FitPolicy) -> str: flags = result["flags"] lines += [ "", - "### National targets whose error rises by more than " - f"{policy.target_flag_threshold:.2f} ({len(flags)})", + f"### National targets within 10% on the base and not on the head ({len(flags)})", "", ] if flags: - lines += ["| Target | Base error | Head error |", "|---|---:|---:|"] - lines += [ - f"| `{f['target']}` | {f['base']:.3f} | {f['head']:.3f} |" - for f in flags[:30] - ] + lines += [f"- `{f['target']}`" for f in flags[:30]] if len(flags) > 30: lines.append(f"\n{len(flags) - 30} more in the job log.") else: lines.append("None.") lines += [ "", - "Flags are for review; builds of the same code differ this much (#598). " - f"Targets supported by fewer than {MIN_RECORDS} households have no published " - "error.", + "Flags are for review: builds of the same code move 1 to 10 national " + "targets out of the band (#598). No target's estimate or error is " + "published.", ] return "\n".join(lines) + "\n" @@ -280,7 +262,7 @@ def main(argv: Optional[list] = None) -> int: Path(args.markdown).write_text(text) print(text) for flag in result["flags"]: - print(f"flag: {flag['target']} {flag['base']:.4f} -> {flag['head']:.4f}") + print(f"flag: {flag['target']} left the 10% band") return 1 if result["status"] == "fail" else 0 From 30fbb8353fb5250bf836167e969989508379ea39 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Thu, 15 Oct 2026 16:08:16 -0400 Subject: [PATCH 7/8] Publish error bands and flag a worse band (Sol re-review 2 of #599) The within-10% boolean could not see a target worsening outside the band. The summary now holds each target's error band (within 10%, 10-25%, 25-50%, 50-100%, over 100%), still a bound and not a value, and flags a national target whose band is worse on the head: 2 to 12 per same-code pair (mean 7). Also: repeated local targets are refused; the fit summary runs after the tests with !cancelled(); data jobs restore but never save uv's cache; fit_policy.yaml records the run ids, the log folders and the status of the replacement rule. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/pull_request.yaml | 39 ++++++---- changelog.d/fit-comparison.added.md | 2 +- policyengine_uk_data/targets/fit_policy.yaml | 29 +++++--- policyengine_uk_data/tests/test_fit_report.py | 37 ++++++++-- policyengine_uk_data/utils/fit_report.py | 74 ++++++++++++------- 5 files changed, 122 insertions(+), 59 deletions(-) diff --git a/.github/workflows/pull_request.yaml b/.github/workflows/pull_request.yaml index 8e7b30737..6d9e666f8 100644 --- a/.github/workflows/pull_request.yaml +++ b/.github/workflows/pull_request.yaml @@ -72,6 +72,10 @@ jobs: fetch-depth: 0 # Fetch all history for all tags and branches - name: Install uv uses: astral-sh/setup-uv@v8.1.0 + # Restore uv's package cache but never save one from a job that + # holds the data: its location can be moved (UV_CACHE_DIR). + with: + save-cache: false - name: Set up Python uses: actions/setup-python@v6 with: @@ -86,30 +90,31 @@ jobs: run: uv run --frozen make data env: TESTING: "1" - # The raw calibration logs stay on the runner: a target supported by a - # few households, divided by a published count, could give one - # household's value back. Only the summary the fit comparison needs is - # uploaded (utils/fit_report.py). + - name: Run tests + run: uv run --frozen make test + env: + # Relaxes built-dataset outcome tolerances to match the reduced + # fidelity of the TESTING build above (see test_cgt_band_donors). + TESTING: "1" + # After the tests, whatever they did. The raw calibration logs stay on + # the runner: a target supported by a few households, divided by a + # published count, could give one household's value back. Only the + # summary the fit comparison needs is uploaded (utils/fit_report.py). - name: Summarise the calibration fit + if: ${{ !cancelled() }} run: uv run --frozen python -m policyengine_uk_data.utils.fit_report summarise constituency_calibration_log.csv --out fit_summary.json - name: Save the fit summary + if: ${{ !cancelled() }} uses: actions/upload-artifact@v7 with: name: fit_summary.json path: fit_summary.json - - - name: Run tests - run: uv run --frozen make test - env: - # Relaxes built-dataset outcome tolerances to match the reduced - # fidelity of the TESTING build above (see test_cgt_band_donors). - TESTING: "1" # The no-worsening check (W3). The base is built the same way as the head: # TESTING, one clone and the head's lockfile, so both builds run one model # and environment and the comparison measures the data change. As in the # test job, only the fit summary leaves it. No dataset or calibration log - # is uploaded, and no dataset is cached (setup-uv caches Python packages - # only). + # is uploaded, and nothing is cached: setup-uv restores uv's package cache + # and saves none. base-build: name: Build the base for comparison runs-on: ubuntu-latest @@ -130,6 +135,10 @@ jobs: path: base - name: Install uv uses: astral-sh/setup-uv@v8.1.0 + # Restore uv's package cache but never save one from a job that + # holds the data: its location can be moved (UV_CACHE_DIR). + with: + save-cache: false - name: Set up Python uses: actions/setup-python@v6 with: @@ -170,6 +179,10 @@ jobs: - uses: actions/checkout@v6 - name: Install uv uses: astral-sh/setup-uv@v8.1.0 + # Restore uv's package cache but never save one from a job that + # holds the data: its location can be moved (UV_CACHE_DIR). + with: + save-cache: false - name: Set up Python uses: actions/setup-python@v6 with: diff --git a/changelog.d/fit-comparison.added.md b/changelog.d/fit-comparison.added.md index b996a583a..89d7d993e 100644 --- a/changelog.d/fit-comparison.added.md +++ b/changelog.d/fit-comparison.added.md @@ -1 +1 @@ -Build each pull request's base the same way as its head, with the head's lockfile, and fail when the share of national or local targets within 10% falls by more than 2 percentage points over the targets both builds have (`targets/fit_policy.yaml`). National targets that are within 10% on the base and not on the head are flagged in a pull request comment. The raw calibration logs stay on the runner; only a summary of which targets are within 10% is uploaded, with no estimate or error (`python -m policyengine_uk_data.utils.fit_report summarise` and `compare`). +Build each pull request's base the same way as its head, with the head's lockfile, and fail when the share of national or local targets within 10% falls by more than 2 percentage points over the targets both builds have (`targets/fit_policy.yaml`). National targets in a worse error band on the head are flagged in a pull request comment. The raw calibration logs stay on the runner; only each target's error band (within 10%, 10-25%, 25-50%, 50-100%, over 100%) is uploaded, with no estimate or error (`python -m policyengine_uk_data.utils.fit_report summarise` and `compare`). diff --git a/policyengine_uk_data/targets/fit_policy.yaml b/policyengine_uk_data/targets/fit_policy.yaml index 2fe344d20..5b71730bc 100644 --- a/policyengine_uk_data/targets/fit_policy.yaml +++ b/policyengine_uk_data/targets/fit_policy.yaml @@ -2,7 +2,7 @@ # (utils/fit_report.py). Both builds run the same way in pull request CI: # TESTING, 32 epochs, one output-area clone, the head's lockfile. # -# Adopted under cos d1344 (15 October 2026), from the final-epoch fits of +# The share allowance was adopted under cos d1344 (15 October 2026), from the final-epoch fits of # five builds of the same data code: docs, changelog and CI pull requests # while main's data code was unchanged from b34a4ea2 to 2b9c1c2c (runs # 38006434333, 37959581668, 38009046475, 37960852139, 37864653693). @@ -20,17 +20,28 @@ share_within_10pct: allowed_drop: 0.02 national_target: - # Flag, for review, each national target within 10% of its value on the - # base build and not on the head. Across the 20 ordered pairs of the five - # builds above, 1 to 10 of the 644 national targets leave the band per - # pair (mean 4.5), so a list stays short. + # Flag, for review, each national target whose error band is worse on the + # head than on the base. Bands of |estimate / target - 1|: within 10%, + # 10-25%, 25-50%, 50-100%, over 100% (fit_report.BANDS). Across the 20 + # ordered pairs of the five same-code builds above, 2 to 12 of the 644 + # national targets move to a worse band per pair (mean 7), so a list stays + # short. #551's build moves 31 and #579's 27 against the 1.58.1 release + # branch's build. # # This replaces flagging an error rise of more than 0.2 (adopted under # d1344 on 15 October), which needed each target's error in the uploaded # summary. Sol's re-review of #599 found no safe way to publish them: the # dataset repeats households (output-area clones, SPI and CGT donors), so # a count of rows can't show how many respondents support an estimate, and - # one household can dominate a target. The within-10% booleans are bounds, - # not values. Same-code pairs gave 0 to 6 flags under the old rule. - # Per-target moves don't fail until #598 is fixed. - flag: left_band + # one household can dominate a target. A band is a bound, not a value. + # The old rule gave 0 to 6 flags per same-code pair. Bands still can't see + # a move inside one band (say 30% to 45%). The replacement is a W3 + # threshold call under d1344's delegation (thresholds on evidence, with an + # independent review agreeing), pending that review. Per-target moves + # don't fail until #598 is fixed. + # + # Run ids of the five builds and their saved log folders: 38006434333 + # (docs-ebdbbce5), 37959581668 (docs-2a071400), 38009046475 + # (changelog-6af935fb), 37960852139 (changelog-bb41c3ca), 37864653693 + # (ci-push-gate-03b3ecd9); the 1.58.1 release branch is 38228500493. + flag: worse_band diff --git a/policyengine_uk_data/tests/test_fit_report.py b/policyengine_uk_data/tests/test_fit_report.py index 03cb056a8..9b567e786 100644 --- a/policyengine_uk_data/tests/test_fit_report.py +++ b/policyengine_uk_data/tests/test_fit_report.py @@ -9,7 +9,9 @@ from hypothesis import strategies as st from policyengine_uk_data.utils.fit_report import ( + BANDS, FitPolicy, + band, compare, load_policy, main, @@ -42,9 +44,10 @@ def test_summarise_reads_the_last_epoch_and_drops_zero_targets(): log.loc[log.epoch == 0, "estimate"] = 1 # earlier epochs are ignored s = summarise(log) assert s["epoch"] == 30 - assert s["national"] == {"a": True, "b": False} + # Bands of |estimate / target - 1|: 0.05 is within 10%, 0.5 is 25-50%. + assert s["national"] == {"a": 0, "b": 2} assert "zero" not in s["national"] - assert s["local"] == {"E0|local/0": True, "E1|local/1": False} + assert s["local"] == {"E0|local/0": 0, "E1|local/1": 3} @settings(max_examples=100, deadline=None) @@ -61,7 +64,7 @@ def test_the_summary_publishes_no_estimate_or_error(estimates, target): ) s = summarise(log) for group in ("national", "local"): - assert all(isinstance(v, bool) for v in s[group].values()) + assert all(type(v) is int and 0 <= v < len(BANDS) for v in s[group].values()) text = json.dumps(s) for e in estimates: assert repr(e) not in text and repr(abs(e / target - 1)) not in text @@ -99,16 +102,34 @@ def test_a_drop_beyond_the_allowance_fails_and_within_it_passes(): assert compare(base, _summary([0.5] + [0.0] * 99), POLICY)["status"] == "pass" -def test_a_target_leaving_the_band_flags_and_one_entering_is_a_note(): +def test_a_target_in_a_worse_band_flags_and_a_better_one_is_a_note(): base = _summary([0.0, 0.5] + [0.0] * 98) head = _summary([0.5, 0.0] + [0.0] * 98) result = compare(base, head, POLICY) assert result["status"] == "pass" - assert result["flags"] == [{"target": "t0"}] - assert "1 national targets come within 10% on the head." in result["notes"] - # A move inside the band, or outside it, is not a flag. + assert result["flags"] == [{"target": "t0", "base": "within 10%", "head": "25-50%"}] + assert "1 national targets are in a better band on the head." in result["notes"] + # Sol re-review of #599: a target worsening outside 10% flags too. + assert compare(_summary([0.15]), _summary([3.0]), POLICY)["flags"] == [ + {"target": "t0", "base": "10-25%", "head": "over 100%"} + ] + # A move inside one band is not a flag. assert compare(_summary([0.01]), _summary([0.09]), POLICY)["flags"] == [] - assert compare(_summary([0.2]), _summary([0.9]), POLICY)["flags"] == [] + assert compare(_summary([0.3]), _summary([0.45]), POLICY)["flags"] == [] + + +@pytest.mark.parametrize( + "error, expected", + [(0.0, 0), (0.1, 0), (0.1000001, 1), (0.25, 1), (0.5, 2), (1.0, 3), (1.01, 4)], +) +def test_band_edges_are_inclusive(error, expected): + assert band(error) == expected + + +def test_a_repeated_local_target_is_refused(): + log = _log({"a": (1, 1)}, local=[(1, 1)]) + with pytest.raises(ValueError, match="repeats a local target"): + summarise(pd.concat([log, log[log.name != "UK"]])) def test_added_targets_do_not_count_against_the_existing_ones(): diff --git a/policyengine_uk_data/utils/fit_report.py b/policyengine_uk_data/utils/fit_report.py index e0ea634af..9a42581b3 100644 --- a/policyengine_uk_data/utils/fit_report.py +++ b/policyengine_uk_data/utils/fit_report.py @@ -4,15 +4,16 @@ and value every tenth epoch and at the last one, nationally (rows named ``UK``) and for each constituency. The log stays on the runner that built it. ``summarise`` reduces its last epoch to what the comparison needs, and -only the summary is uploaded: for every national and local target, whether -its estimate is within 10% of its value (|estimate / target - 1| <= 0.1), -with zero targets left out. No estimate or error leaves the runner. An +only the summary is uploaded: for every national and local target, the band +its error |estimate / target - 1| falls in (``BANDS``: within 10%, 10-25%, +25-50%, 50-100%, over 100%), with zero targets left out. No estimate or +error leaves the runner. An estimate built from a few households, or dominated by one, divided by a published count, could give that household's value back, and the dataset repeats households (output-area clones, SPI and CGT donors), so a count of rows can't show how many respondents support a figure (Sol re-review of -#599). Whether an estimate is within 10% of a public target is a bound, not -a value. +#599). A band, at least 10% of a public target wide, is a bound, not a +value. ``compare`` checks that a pull request does not worsen the fit of the build it would replace, built the same way (same environment, epochs and clones), @@ -21,9 +22,9 @@ - **Fail** when the share of national, or of local, targets within 10% falls by more than the allowance, over the targets both builds have. Added and removed targets are reported, not counted. -- **Flag**, for review, each national target that is within 10% on the - base and not on the head. Per-target moves do not fail while builds of - the same code move 1 to 10 national targets out of the band (#598). +- **Flag**, for review, each national target whose band is worse on the + head than on the base. Per-target moves do not fail while builds of the + same code move 2 to 12 national targets to a worse band (#598). python -m policyengine_uk_data.utils.fit_report summarise LOG.csv --out summary.json python -m policyengine_uk_data.utils.fit_report compare BASE.json HEAD.json [--markdown out.md] @@ -43,8 +44,10 @@ import yaml POLICY_PATH = Path(__file__).parents[1] / "targets" / "fit_policy.yaml" -SCHEMA = "policyengine-uk-data-fit-summary/2" -WITHIN = 0.10 +SCHEMA = "policyengine-uk-data-fit-summary/3" +# Upper edges of the error bands; band 0 is within 10%. +BAND_EDGES = (0.10, 0.25, 0.50, 1.00) +BANDS = ("within 10%", "10-25%", "25-50%", "50-100%", "over 100%") _COLUMNS = ["name", "index", "metric", "estimate", "target", "epoch"] @@ -58,13 +61,18 @@ def load_policy(path=None) -> FitPolicy: raw = yaml.safe_load(f) if raw.get("schema_version") != 1: raise ValueError("fit policy: schema_version must be 1") - if raw.get("national_target", {}).get("flag") != "left_band": - raise ValueError("fit policy: national_target.flag must be left_band") + if raw.get("national_target", {}).get("flag") != "worse_band": + raise ValueError("fit policy: national_target.flag must be worse_band") return FitPolicy( share_drop_allowance=float(raw["share_within_10pct"]["allowed_drop"]), ) +def band(error: float) -> int: + """The index in ``BANDS`` of an error: 0 when within 10% (inclusive).""" + return int(np.searchsorted(BAND_EDGES, float(error), side="left")) + + def summarise(log) -> dict: """The last epoch of a calibration log, reduced to what may leave the runner.""" if isinstance(log, pd.DataFrame): @@ -84,6 +92,8 @@ def summarise(log) -> dict: ) national = national[national.target != 0].set_index("metric") local = rows[(rows.name != "UK") & (rows.target != 0)] + if local.duplicated(["index", "metric"]).any(): + raise ValueError("the calibration log repeats a local target at its last epoch") n_errors = (national.estimate / national.target - 1).abs() l_errors = (local.estimate / local.target - 1).abs() if not np.isfinite(n_errors).all() or not np.isfinite(l_errors).all(): @@ -93,11 +103,9 @@ def summarise(log) -> dict: "epoch": last, "rows_last_epoch": int(len(rows)), "rows_previous_epoch": previous, - "national": { - metric: bool(error <= WITHIN) for metric, error in n_errors.items() - }, + "national": {metric: band(error) for metric, error in n_errors.items()}, "local": { - f"{i}|{m}": bool(e <= WITHIN) + f"{i}|{m}": band(e) for i, m, e in zip(local["index"], local.metric, l_errors) }, } @@ -150,8 +158,8 @@ def compare(base: dict, head: dict, policy: FitPolicy) -> dict: b, h = base[name], head[name] common = sorted(set(b) & set(h)) added, removed = sorted(set(h) - set(b)), sorted(set(b) - set(h)) - before = _share([bool(b[k]) for k in common]) - after = _share([bool(h[k]) for k in common]) + before = _share([b[k] == 0 for k in common]) + after = _share([h[k] == 0 for k in common]) summary[name] = { "base": before, "head": after, @@ -175,12 +183,19 @@ def compare(base: dict, head: dict, policy: FitPolicy) -> dict: f"more than the {policy.share_drop_allowance:.1%} allowed." ) common = sorted(set(base["national"]) & set(head["national"])) + b_band, h_band = base["national"], head["national"] flags = [ - {"target": t} for t in common if base["national"][t] and not head["national"][t] + {"target": t, "base": BANDS[b_band[t]], "head": BANDS[h_band[t]]} + for t in common + if h_band[t] > b_band[t] ] - entered = sum(1 for t in common if head["national"][t] and not base["national"][t]) - if entered: - notes.append(f"{entered} national targets come within 10% on the head.") + # Largest moves first. + flags.sort( + key=lambda f: (BANDS.index(f["base"]) - BANDS.index(f["head"]), f["target"]) + ) + better = sum(1 for t in common if h_band[t] < b_band[t]) + if better: + notes.append(f"{better} national targets are in a better band on the head.") return { "status": "fail" if failures else "pass", "failures": failures, @@ -211,20 +226,23 @@ def render(result: dict, policy: FitPolicy) -> str: flags = result["flags"] lines += [ "", - f"### National targets within 10% on the base and not on the head ({len(flags)})", + f"### National targets in a worse error band on the head ({len(flags)})", "", ] if flags: - lines += [f"- `{f['target']}`" for f in flags[:30]] + lines += ["| Target | Base | Head |", "|---|---|---|"] + lines += [ + f"| `{f['target']}` | {f['base']} | {f['head']} |" for f in flags[:30] + ] if len(flags) > 30: lines.append(f"\n{len(flags) - 30} more in the job log.") else: lines.append("None.") lines += [ "", - "Flags are for review: builds of the same code move 1 to 10 national " - "targets out of the band (#598). No target's estimate or error is " - "published.", + "Flags are for review: builds of the same code move 2 to 12 national " + "targets to a worse band (#598). Bands are of |estimate / target - 1|; " + "no target's estimate or error is published.", ] return "\n".join(lines) + "\n" @@ -262,7 +280,7 @@ def main(argv: Optional[list] = None) -> int: Path(args.markdown).write_text(text) print(text) for flag in result["flags"]: - print(f"flag: {flag['target']} left the 10% band") + print(f"flag: {flag['target']} {flag['base']} -> {flag['head']}") return 1 if result["status"] == "fail" else 0 From dc81b55e727505d98f04c97b59125ced18395510 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Thu, 15 Oct 2026 23:16:12 -0400 Subject: [PATCH 8/8] Put an estimate exactly on a band edge inside it (delta review of #599) Co-Authored-By: Claude Opus 5.5 --- policyengine_uk_data/tests/test_fit_report.py | 6 ++++++ policyengine_uk_data/utils/fit_report.py | 9 +++++++-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/policyengine_uk_data/tests/test_fit_report.py b/policyengine_uk_data/tests/test_fit_report.py index 9b567e786..a116f7d56 100644 --- a/policyengine_uk_data/tests/test_fit_report.py +++ b/policyengine_uk_data/tests/test_fit_report.py @@ -126,6 +126,12 @@ def test_band_edges_are_inclusive(error, expected): assert band(error) == expected +def test_an_estimate_exactly_on_a_band_edge_is_inside_it(): + """Delta review of #599: 110 / 100 - 1 is 0.10000000000000009.""" + s = summarise(_log({"edge": (110, 100), "quarter": (125, 100)}, local=LOCAL)) + assert s["national"] == {"edge": 0, "quarter": 1} + + def test_a_repeated_local_target_is_refused(): log = _log({"a": (1, 1)}, local=[(1, 1)]) with pytest.raises(ValueError, match="repeats a local target"): diff --git a/policyengine_uk_data/utils/fit_report.py b/policyengine_uk_data/utils/fit_report.py index 9a42581b3..b4db12d7d 100644 --- a/policyengine_uk_data/utils/fit_report.py +++ b/policyengine_uk_data/utils/fit_report.py @@ -69,8 +69,13 @@ def load_policy(path=None) -> FitPolicy: def band(error: float) -> int: - """The index in ``BANDS`` of an error: 0 when within 10% (inclusive).""" - return int(np.searchsorted(BAND_EDGES, float(error), side="left")) + """The index in ``BANDS`` of an error: 0 when within 10% (inclusive). + + The error is rounded to 9 decimal places first, so an estimate exactly + 10% off (110 / 100 - 1 = 0.10000000000000009 in floating point) is + within 10%. + """ + return int(np.searchsorted(BAND_EDGES, round(float(error), 9), side="left")) def summarise(log) -> dict: