From 736b5877e944104f6a3a12816d4aaf786e626805 Mon Sep 17 00:00:00 2001 From: Khalim Conn-Kowlessar Date: Wed, 1 Jul 2026 10:13:31 +0000 Subject: [PATCH 1/6] =?UTF-8?q?Carry=20a=20flat=20roof's=20known=20insulat?= =?UTF-8?q?ion=20depth=20into=20the=20overlay=20=F0=9F=9F=A5?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/domain/epc/test_roof_type_overlay.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/domain/epc/test_roof_type_overlay.py b/tests/domain/epc/test_roof_type_overlay.py index 050d76fa2..75aab9204 100644 --- a/tests/domain/epc/test_roof_type_overlay.py +++ b/tests/domain/epc/test_roof_type_overlay.py @@ -42,6 +42,20 @@ def test_each_loft_depth_is_parsed(roof_type_value: str, expected_mm: int) -> No ].roof_insulation_thickness == expected_mm +def test_flat_roof_depth_maps_to_roof_insulation_thickness() -> None: + # Act — a flat roof with a known depth. RdSAP 10 Table 16 col (1) scores flat + # roofs by insulation thickness ("joists at ceiling level and flat roof"), so + # the depth must reach the overlay, not be discarded for the age-band default + # (ADR-0041). + simulation = roof_overlay_for("Flat, 150 mm insulation", 0) + + # Assert — the flat shape and the real depth both ride on the overlay. + assert simulation is not None + overlay = simulation.building_parts[BuildingPartIdentifier.MAIN] + assert overlay.roof_construction_type == "Flat" + assert overlay.roof_insulation_thickness == 150 + + @pytest.mark.parametrize( "roof_type_value", [ From 463d4df048fbfa0fa5283233899ea173275b1097 Mon Sep 17 00:00:00 2001 From: Khalim Conn-Kowlessar Date: Wed, 1 Jul 2026 10:14:53 +0000 Subject: [PATCH 2/6] =?UTF-8?q?Carry=20a=20flat=20roof's=20known=20insulat?= =?UTF-8?q?ion=20depth=20into=20the=20overlay=20=F0=9F=9F=A9?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 4.8 (1M context) --- .../epc/property_overlays/roof_type_overlay.py | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/domain/epc/property_overlays/roof_type_overlay.py b/domain/epc/property_overlays/roof_type_overlay.py index aadcc3878..a2ba1811b 100644 --- a/domain/epc/property_overlays/roof_type_overlay.py +++ b/domain/epc/property_overlays/roof_type_overlay.py @@ -27,6 +27,8 @@ from datatypes.epc.domain.epc_property_data import BuildingPartIdentifier from domain.modelling.simulation import BuildingPartOverlay, EpcSimulation _LOFT_MM = re.compile(r"(\d+)\+?\s*mm loft insulation") +# A flat roof carries no loft, so its depth reads "N mm insulation" (no "loft"). +_FLAT_MM = re.compile(r"(\d+)\+?\s*mm insulation") def roof_overlay_for( @@ -49,8 +51,18 @@ def _overlay_for(roof_type_value: str) -> Optional[BuildingPartOverlay]: if match is not None: return BuildingPartOverlay(roof_insulation_thickness=int(match.group(1))) if roof_type_value.startswith("Flat,"): - # Flat roof: U-value is the age-band default; flag the shape and let the - # age-band overlay drive `_FLAT_ROOF_BY_AGE` (ADR-0033). + flat_depth = _FLAT_MM.search(roof_type_value) + if flat_depth is not None: + # A flat roof with a known depth: RdSAP 10 Table 16 col (1) ("joists at + # ceiling level and flat roof") scores it by thickness, so carry the + # depth (the calculator routes flat + thickness through Table 16, not + # the age-band default). ADR-0041. + return BuildingPartOverlay( + roof_construction_type="Flat", + roof_insulation_thickness=int(flat_depth.group(1)), + ) + # Flat roof with no stated depth: U-value is the age-band default; flag the + # shape and let the age-band overlay drive `_FLAT_ROOF_BY_AGE` (ADR-0033). return BuildingPartOverlay(roof_construction_type="Flat") if roof_type_value == "Pitched, Unknown loft insulation": # Unknown loft depth: U-value is the pitched age-band default. Assert the From acf05584d64602e740ad2aeb630a8ce2c6f8b7fa Mon Sep 17 00:00:00 2001 From: Khalim Conn-Kowlessar Date: Wed, 1 Jul 2026 10:16:35 +0000 Subject: [PATCH 3/6] =?UTF-8?q?Add=20flat-roof=20insulation-thickness=20ta?= =?UTF-8?q?xonomy=20members=20lock-step=20with=20the=20overlay=20?= =?UTF-8?q?=F0=9F=9F=A9?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 4.8 (1M context) --- domain/epc/property_overrides/roof_type.py | 22 +++++++++++++ tests/domain/epc/test_roof_type_overlay.py | 38 ++++++++++++++++++++++ 2 files changed, 60 insertions(+) diff --git a/domain/epc/property_overrides/roof_type.py b/domain/epc/property_overrides/roof_type.py index 56ef9e8ed..bc3073f86 100644 --- a/domain/epc/property_overrides/roof_type.py +++ b/domain/epc/property_overrides/roof_type.py @@ -22,6 +22,28 @@ class RoofType(Enum): FLAT_NO_INSULATION = "Flat, no insulation" FLAT_NO_INSULATION_ASSUMED = "Flat, no insulation (assumed)" + # A flat roof with a known insulation depth. RdSAP 10 Table 16 col (1) + # ("joists at ceiling level and flat roof") scores flat roofs by thickness, so + # these carry the depth into the overlay (mirroring the PITCHED_LOFT_* ladder; + # no "loft" — a flat roof has none). ADR-0041. The value column is an FE-owned + # pgEnum, so these members are added to the Drizzle enum by the FE owner. + FLAT_12MM = "Flat, 12 mm insulation" + FLAT_25MM = "Flat, 25 mm insulation" + FLAT_50MM = "Flat, 50 mm insulation" + FLAT_75MM = "Flat, 75 mm insulation" + FLAT_100MM = "Flat, 100 mm insulation" + FLAT_125MM = "Flat, 125 mm insulation" + FLAT_150MM = "Flat, 150 mm insulation" + FLAT_175MM = "Flat, 175 mm insulation" + FLAT_200MM = "Flat, 200 mm insulation" + FLAT_225MM = "Flat, 225 mm insulation" + FLAT_250MM = "Flat, 250 mm insulation" + FLAT_270MM = "Flat, 270 mm insulation" + FLAT_300MM = "Flat, 300 mm insulation" + FLAT_350MM = "Flat, 350 mm insulation" + FLAT_400MM = "Flat, 400 mm insulation" + FLAT_400_PLUS = "Flat, 400+ mm insulation" + PITCHED_INSULATED = "Pitched, insulated" PITCHED_INSULATED_ASSUMED = "Pitched, insulated (assumed)" PITCHED_INSULATED_AT_RAFTERS = "Pitched, insulated at rafters" diff --git a/tests/domain/epc/test_roof_type_overlay.py b/tests/domain/epc/test_roof_type_overlay.py index 75aab9204..1c39ec9f7 100644 --- a/tests/domain/epc/test_roof_type_overlay.py +++ b/tests/domain/epc/test_roof_type_overlay.py @@ -11,6 +11,26 @@ import pytest from datatypes.epc.domain.epc_property_data import BuildingPartIdentifier from domain.epc.property_overlays.roof_type_overlay import roof_overlay_for +from domain.epc.property_overrides.roof_type import RoofType + +_FLAT_THICKNESS_MEMBERS: list[tuple[RoofType, int]] = [ + (RoofType.FLAT_12MM, 12), + (RoofType.FLAT_25MM, 25), + (RoofType.FLAT_50MM, 50), + (RoofType.FLAT_75MM, 75), + (RoofType.FLAT_100MM, 100), + (RoofType.FLAT_125MM, 125), + (RoofType.FLAT_150MM, 150), + (RoofType.FLAT_175MM, 175), + (RoofType.FLAT_200MM, 200), + (RoofType.FLAT_225MM, 225), + (RoofType.FLAT_250MM, 250), + (RoofType.FLAT_270MM, 270), + (RoofType.FLAT_300MM, 300), + (RoofType.FLAT_350MM, 350), + (RoofType.FLAT_400MM, 400), + (RoofType.FLAT_400_PLUS, 400), +] def test_pitched_loft_depth_maps_to_roof_insulation_thickness() -> None: @@ -56,6 +76,24 @@ def test_flat_roof_depth_maps_to_roof_insulation_thickness() -> None: assert overlay.roof_insulation_thickness == 150 +@pytest.mark.parametrize(("member", "expected_mm"), _FLAT_THICKNESS_MEMBERS) +def test_every_flat_thickness_member_resolves_to_its_depth( + member: RoofType, expected_mm: int +) -> None: + # Each FLAT_*MM taxonomy member must stay lock-step with the overlay: its value + # resolves to the depth it names (so a landlord-stored member scores via Table + # 16 col (1), not the age-band default). ADR-0041. + + # Act + simulation = roof_overlay_for(member.value, 0) + + # Assert + assert simulation is not None + overlay = simulation.building_parts[BuildingPartIdentifier.MAIN] + assert overlay.roof_construction_type == "Flat" + assert overlay.roof_insulation_thickness == expected_mm + + @pytest.mark.parametrize( "roof_type_value", [ From 32e296ca3f8acdad5bc514d87940f73aa0527e8c Mon Sep 17 00:00:00 2001 From: Khalim Conn-Kowlessar Date: Wed, 1 Jul 2026 10:19:09 +0000 Subject: [PATCH 4/6] =?UTF-8?q?Re-classify=20flat-roof=20overrides=20with?= =?UTF-8?q?=20a=20known=20depth=20onto=20their=20thickness=20member=20?= =?UTF-8?q?=F0=9F=9F=A9?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One-time script (dry-run default, --apply in a transaction, idempotent) mapping a flat roof's stated depth (flat: Nmm) onto the nearest Table 16 rung member so the overlay scores it by thickness, not the age-band default. Updates property_overrides (TEXT) immediately; the roof_type pgEnum cache writes are deferred until the FE adds the FLAT_*MM members. Dry-run against the audited DB reports 102 rows, deferring Flat 50/100/150 mm. Co-Authored-By: Claude Opus 4.8 (1M context) --- scripts/reclassify_flat_roof_thickness.py | 169 ++++++++++++++++++ .../test_reclassify_flat_roof_thickness.py | 44 +++++ 2 files changed, 213 insertions(+) create mode 100644 scripts/reclassify_flat_roof_thickness.py create mode 100644 tests/scripts/test_reclassify_flat_roof_thickness.py diff --git a/scripts/reclassify_flat_roof_thickness.py b/scripts/reclassify_flat_roof_thickness.py new file mode 100644 index 000000000..487ee88bc --- /dev/null +++ b/scripts/reclassify_flat_roof_thickness.py @@ -0,0 +1,169 @@ +"""One-time re-classification of flat-roof overrides that discarded a known depth. + +#1376 / ADR-0041: a landlord flat-roof override carrying a real insulation depth +(``flat: 150mm``) was classified to ``"Flat, insulated (assumed)"`` — which the +roof Simulation Overlay resolves to the flat **age-band default**, discarding the +depth. RdSAP 10 Table 16 col (1) ("joists at ceiling level and flat roof") scores +flat roofs **by thickness**, so this re-maps those rows onto the new +``"Flat, N mm insulation"`` members (nearest tabulated rung ≤ the stated depth, +per Table 16), which the overlay now carries into the calculator. + +Only rows whose raw description is a flat roof with a numeric depth are touched; +``flat: unknown`` / ``flat: as built`` / ``flat: no insulation`` (no depth) keep +their age-band-default classification. + +Updates the TEXT ``property_overrides.override_value`` (what the modelling reads — +the immediate fix) always. The ``landlord_roof_type_overrides.value`` classifier +cache is a ``roof_type`` **pgEnum**: the new ``FLAT_*MM`` values are FE-owned and +must be added to the Drizzle enum first, so cache writes for a value the live enum +does not yet carry are **deferred** and reported (the Class-A/B pattern). + +DRY-RUN BY DEFAULT: prints the counts it would change and writes nothing. Pass +``--apply`` to execute inside a transaction. Idempotent — only rows whose stored +value differs from the target member are touched. +""" + +from __future__ import annotations + +import argparse +import re +from collections.abc import Iterable + +from sqlalchemy import Connection, text + +from scripts.e2e_common import build_engine, load_env + +# The flat-roof thickness rungs that have a taxonomy member (RdSAP 10 Table 16), +# ascending; a depth above the top maps to the "400+" member. +_FLAT_RUNGS: tuple[int, ...] = ( + 12, 25, 50, 75, 100, 125, 150, 175, 200, 225, 250, 270, 300, 350, 400, +) +_FLAT_DEPTH_RE = re.compile(r"^flat:\s*(\d+)\+?\s*mm") + + +def _flat_member_value(depth_mm: int) -> str: + """The ``"Flat, N mm insulation"`` member value for a stated depth — the + nearest tabulated rung ≤ the depth (Table 16), or the "400+" member above 400.""" + if depth_mm > 400: + return "Flat, 400+ mm insulation" + rung = _FLAT_RUNGS[0] + for candidate in _FLAT_RUNGS: + if depth_mm >= candidate: + rung = candidate + return f"Flat, {rung} mm insulation" + + +def flat_thickness_corrections( + stored: Iterable[tuple[str, str]], +) -> dict[str, str]: + """``(description, stored override_value)`` → the corrected flat-thickness member + value, for descriptions that are a flat roof with a numeric depth whose stored + value is not already that member. Depthless flat descriptions and non-flat + descriptions are omitted, so re-running against corrected data is a no-op.""" + corrections: dict[str, str] = {} + for description, value in stored: + match = _FLAT_DEPTH_RE.match(description.strip().lower()) + if match is None: + continue + target = _flat_member_value(int(match.group(1))) + if value != target: + corrections[description] = target + return corrections + + +_DISTINCT = text( + """ + SELECT DISTINCT lower(original_spreadsheet_description) AS description, + override_value AS value + FROM property_overrides + WHERE override_component = 'roof_type' + """ +) +_OVERRIDES_UPDATE = text( + """ + UPDATE property_overrides + SET override_value = :new_value + WHERE override_component = 'roof_type' + AND lower(original_spreadsheet_description) = :description + AND override_value <> :new_value + """ +) +_OVERRIDES_COUNT = text( + """ + SELECT count(*) FROM property_overrides + WHERE override_component = 'roof_type' + AND lower(original_spreadsheet_description) = :description + AND override_value <> :new_value + """ +) +_CACHE_UPDATE = text( + """ + UPDATE landlord_roof_type_overrides + SET value = :new_value, updated_at = now() + WHERE lower(description) = :description + AND value::text <> :new_value + """ +) +_ENUM_VALUES = text( + "SELECT e.enumlabel FROM pg_enum e JOIN pg_type t ON t.oid = e.enumtypid " + "WHERE t.typname = 'roof_type'" +) + + +def reclassify(conn: Connection, *, apply: bool) -> tuple[int, set[str]]: + """Re-map flat-depth roof overrides onto their thickness member. Returns the + number of ``property_overrides`` rows found and the set of target values the + live ``roof_type`` pgEnum does not yet carry (cache-deferred until the FE + migration).""" + stored = [(r.description, r.value) for r in conn.execute(_DISTINCT)] + enum_values = {r[0] for r in conn.execute(_ENUM_VALUES)} + total = 0 + deferred: set[str] = set() + for description, new_value in flat_thickness_corrections(stored).items(): + params = {"description": description, "new_value": new_value} + total += conn.execute(_OVERRIDES_COUNT, params).scalar() or 0 + in_enum = new_value in enum_values + if not in_enum: + deferred.add(new_value) + if apply: + conn.execute(_OVERRIDES_UPDATE, params) + if in_enum: + conn.execute(_CACHE_UPDATE, params) + return total, deferred + + +def main() -> None: + load_env() + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument( + "--apply", + action="store_true", + help="execute the updates (default: dry-run, writes nothing)", + ) + args = parser.parse_args() + + engine = build_engine() + with engine.begin() as conn: + conn.execute(text("SET statement_timeout = 120000")) + total, deferred = reclassify(conn, apply=args.apply) + + verb = "re-classified" if args.apply else "would re-classify" + print( + f"{verb} {total} flat-roof override row(s) from the age-band default onto " + "their Table 16 thickness member (property_overrides / TEXT)." + ) + if deferred: + print( + f"\n{len(deferred)} target value(s) NOT yet in the roof_type pgEnum — " + "their classifier-cache rows are deferred until the FE-repo enum " + "migration adds these members (property_overrides was still updated, " + "which is what the modelling reads):" + ) + for value in sorted(deferred): + print(f" {value!r}") + if not args.apply: + print("\nDRY-RUN — nothing written. Re-run with --apply to execute.") + + +if __name__ == "__main__": + main() diff --git a/tests/scripts/test_reclassify_flat_roof_thickness.py b/tests/scripts/test_reclassify_flat_roof_thickness.py new file mode 100644 index 000000000..5a0ae87c4 --- /dev/null +++ b/tests/scripts/test_reclassify_flat_roof_thickness.py @@ -0,0 +1,44 @@ +"""The flat-roof reclassify maps a stated depth onto its Table 16 thickness member, +leaves depthless / non-flat rows alone, and is idempotent (#1376 / ADR-0041).""" + +from __future__ import annotations + +from scripts.reclassify_flat_roof_thickness import flat_thickness_corrections + + +def test_flat_depth_rows_are_corrected_to_the_thickness_member() -> None: + # Arrange — flat roofs with a real depth (currently on the age-band-default + # value), alongside depthless flat rows and a pitched row that must be left. + stored = [ + ("flat: 150mm", "Flat, insulated (assumed)"), + ("flat: 50mm", "Flat, insulated (assumed)"), + ("flat: unknown", "Flat, insulated (assumed)"), + ("flat: as built", "Flat, insulated (assumed)"), + ("flat: no insulation", "Flat, no insulation"), + ("pitchednormalloftaccess: 100mm", "Pitched, 100 mm loft insulation"), + ] + + # Act + corrections = flat_thickness_corrections(stored) + + # Assert — only the flat-depth rows are re-mapped, to their thickness member. + assert corrections == { + "flat: 150mm": "Flat, 150 mm insulation", + "flat: 50mm": "Flat, 50 mm insulation", + } + + +def test_a_non_rung_depth_snaps_to_the_nearest_tabulated_rung() -> None: + # RdSAP Table 16 uses the nearest tabulated thickness <= the stated depth. + + # Act / Assert + assert flat_thickness_corrections( + [("flat: 60mm", "Flat, insulated (assumed)")] + ) == {"flat: 60mm": "Flat, 50 mm insulation"} + + +def test_already_corrected_flat_rows_need_no_change() -> None: + # Act / Assert — idempotent. + assert ( + flat_thickness_corrections([("flat: 150mm", "Flat, 150 mm insulation")]) == {} + ) From 3282cef04ede67f5cbbcadb75d65d608536a2239 Mon Sep 17 00:00:00 2001 From: Jun-te Kim Date: Wed, 1 Jul 2026 10:41:52 +0000 Subject: [PATCH 5/6] Fix RdSAP-17.0 mapper dropping lodged secondary heating MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `from_rdsap_schema_17_0` path hardcoded `sap_heating.secondary_heating_type` and `secondary_fuel_type` to None, unlike every other API path (19.0/20.0/…), which read them from `schema.sap_heating`. The gov EPC API *does* populate a secondary heating SAP code for a genuine fixed secondary (e.g. 691 "electric room heaters", 694), so the hardcode silently discarded it. Because `recommend_secondary_heating_removal` gates purely on `sap_heating.secondary_heating_type`, any 17.0 cert carrying a fixed secondary would never get its removal recommendation. Surfaced while auditing portfolio 814's recommendations; 814 has 17.0 certs but none with a fixed secondary, so the defect was latent there — this restores parity with the other paths so it cannot bite. Mirrors the 19.0 path exactly (secondary_fuel_type via `_api_secondary_fuel_type`). Adds a regression guard that injects a fixed secondary into a real 17.0 cert and asserts the code survives the mapping. Co-Authored-By: Claude Opus 4.8 (1M context) --- datatypes/epc/domain/mapper.py | 13 +++- .../test_mapper_secondary_heating_17_0.py | 61 +++++++++++++++++++ 2 files changed, 72 insertions(+), 2 deletions(-) create mode 100644 tests/datatypes/epc/domain/test_mapper_secondary_heating_17_0.py diff --git a/datatypes/epc/domain/mapper.py b/datatypes/epc/domain/mapper.py index e89062610..e17b43033 100644 --- a/datatypes/epc/domain/mapper.py +++ b/datatypes/epc/domain/mapper.py @@ -643,8 +643,17 @@ class EpcPropertyDataMapper: immersion_heating_type=schema.sap_heating.immersion_heating_type, cylinder_insulation_type=schema.sap_heating.cylinder_insulation_type, cylinder_thermostat=None, - secondary_fuel_type=None, - secondary_heating_type=None, + # Preserve the lodged secondary heating from the 17.0 sap_heating + # block, matching every other API path (19.0/20.0/…). Hardcoding + # these to None silently dropped a lodged FIXED secondary (SAP + # codes like 691/694 that the gov API does populate), which + # suppressed the secondary_heating_removal recommendation for any + # 17.0 cert carrying one. + secondary_fuel_type=_api_secondary_fuel_type( + schema.sap_heating.secondary_fuel_type, + schema.sap_heating.secondary_heating_type, + ), + secondary_heating_type=schema.sap_heating.secondary_heating_type, cylinder_insulation_thickness_mm=None, ), # ADR-0028: 990/1000 omit sap_windows -> synthesised from the diff --git a/tests/datatypes/epc/domain/test_mapper_secondary_heating_17_0.py b/tests/datatypes/epc/domain/test_mapper_secondary_heating_17_0.py new file mode 100644 index 000000000..0d94df908 --- /dev/null +++ b/tests/datatypes/epc/domain/test_mapper_secondary_heating_17_0.py @@ -0,0 +1,61 @@ +"""Regression: the RdSAP-Schema-17.0 mapper path must preserve a lodged secondary +heating system, like every other API path. + +`from_rdsap_schema_17_0` previously hardcoded `sap_heating.secondary_heating_type` +(and `secondary_fuel_type`) to None, unlike the 19.0/20.0/… paths which read +`schema.sap_heating.secondary_heating_type`. The gov EPC API *does* populate that +field for a genuine FIXED secondary (SAP codes such as 691 "electric room +heaters" / 694), so the hardcode silently dropped it — and because +`recommend_secondary_heating_removal` gates purely on +`sap_heating.secondary_heating_type`, a 17.0 cert with a fixed secondary would +never get its removal recommendation. Portfolio 814 happened not to contain such +a cert, so the bug was latent; this guard makes sure it stays fixed. +""" + +from __future__ import annotations + +import json +from pathlib import Path +from typing import Any + +from datatypes.epc.domain.mapper import EpcPropertyDataMapper + +# A real RdSAP-Schema-17.0 cert whose lodged secondary is absent (maps to None); +# we inject a fixed secondary to prove the 17.0 path now carries it through. +_FIXTURE = Path("tests/fixtures/epc_prediction/PE71NT/cert-26619d8e7f8e.json") + +# SAP 10.2 secondary-heating code for fixed electric room heaters, exactly as the +# gov API lodges it (seen live on portfolio-814 certs), + its off-peak fuel code. +_FIXED_SECONDARY_TYPE = 691 +_SECONDARY_FUEL = 29 + + +def _load_17_0_cert() -> dict[str, Any]: + data: dict[str, Any] = json.loads(_FIXTURE.read_text()) + assert data["schema_type"] == "RdSAP-Schema-17.0" + return data + + +def test_17_0_path_preserves_lodged_fixed_secondary() -> None: + # Arrange — a 17.0 cert carrying a lodged fixed secondary in sap_heating. + data = _load_17_0_cert() + data["sap_heating"]["secondary_heating_type"] = _FIXED_SECONDARY_TYPE + data["sap_heating"]["secondary_fuel_type"] = _SECONDARY_FUEL + + # Act + epc = EpcPropertyDataMapper.from_api_response(data) + + # Assert — the 17.0 mapper carries the code through (it used to be dropped to + # None), so secondary_heating_removal can fire on this dwelling. + assert epc.sap_heating.secondary_heating_type == _FIXED_SECONDARY_TYPE + + +def test_17_0_path_leaves_absent_secondary_as_none() -> None: + # Arrange — the unmodified cert has no lodged secondary. + data = _load_17_0_cert() + + # Act + epc = EpcPropertyDataMapper.from_api_response(data) + + # Assert — no secondary lodged still maps to None (no phantom removal). + assert epc.sap_heating.secondary_heating_type is None From 003defcf55a77989f92d2a97447a14ac30063fdd Mon Sep 17 00:00:00 2001 From: Jun-te Kim Date: Wed, 1 Jul 2026 11:06:03 +0000 Subject: [PATCH 6/6] Add find-weird-recommendations skill + its two detectors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A focused sibling to audit-ara-portfolio: that skill audits baselines/plans/SAP; this one audits the *recommendations themselves* — why a measure was or wasn't offered. Motivated by the portfolio-814 review (Khalim's HHRSH-on-community- heating, missing-HHRSH, missing-secondary-heating-removal, and a neighbour split). Adds: - .claude/skills/find-weird-recommendations/SKILL.md — scan -> neighbour scan -> live re-model deep-dive -> root-cause -> codify, with a seeded known-bug catalogue and the query-safety rules inherited from audit-ara-portfolio. - scripts/audit/anomalies.py: new `plan-stops-short-of-goal` HIGH check — the default plan ends below the goal band on an unlimited-budget scenario (the deterministic worklist for "why didn't this get recommended X"). Adds scenario_budget to the bundle/query so budget-capped scenarios are excluded. - scripts/audit/neighbour_divergence.py: groups a portfolio by (postcode, property_type, built_form) and flags effective-SAP outliers vs the cohort median. Never touches the 26m-row recommendation table, so it is safe portfolio-wide. - Tests for both (12 passing). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../find-weird-recommendations/SKILL.md | 242 ++++++++++++++++++ scripts/audit/anomalies.py | 30 ++- scripts/audit/neighbour_divergence.py | 226 ++++++++++++++++ tests/scripts/test_audit_anomalies.py | 69 ++++- tests/scripts/test_neighbour_divergence.py | 76 ++++++ 5 files changed, 636 insertions(+), 7 deletions(-) create mode 100644 .claude/skills/find-weird-recommendations/SKILL.md create mode 100644 scripts/audit/neighbour_divergence.py create mode 100644 tests/scripts/test_neighbour_divergence.py diff --git a/.claude/skills/find-weird-recommendations/SKILL.md b/.claude/skills/find-weird-recommendations/SKILL.md new file mode 100644 index 000000000..04e0ee36c --- /dev/null +++ b/.claude/skills/find-weird-recommendations/SKILL.md @@ -0,0 +1,242 @@ +--- +name: find-weird-recommendations +description: Hunt for dubious *recommendations* in a modelled portfolio — measures that were wrongly withheld (SAP could have moved to the goal band but didn't), measures that contradict the dwelling (e.g. HHRSH on community heating), and neighbours of the same type/postcode that were modelled differently. Deterministic scan → neighbour scan → deep-dive with the live re-model → root-cause → codify. Use when the tech team flags "weird" or "dubious" recommendations on a portfolio and wants to understand why the engine did it. Asks for portfolio id and scenario id. +--- + +# Find weird recommendations + +Sibling to `audit-ara-portfolio`. That skill audits **baselines, plans and SAP +scores**; this one audits the **recommendations themselves** — *why a measure was +or wasn't offered*. It exists because the tech team keeps finding individual +dodgy recommendations (portfolio 814: Khalim's HHRSH-on-community-heating, +missing-HHRSH, missing-secondary-heating-removal, and a neighbour split) and we +want a repeatable way to turn "this one looks wrong" into a root cause and a +durable check that makes the engine better. + +The engine's job on an unlimited-budget scenario is to move each dwelling to the +goal band (usually C) with a sensible, physically-valid set of measures. +"Weird" = the engine did something a competent surveyor wouldn't: **left SAP on +the table**, **recommended a measure the dwelling can't take**, or **treated two +identical neighbours differently**. Each has a deterministic tell; this skill +runs the tells, then uses the live re-model to explain each hit. + +## Input + +Ask for **portfolio_id** and **scenario_id** if the user didn't give them. The +scenario matters here: its `goal_value` is the band we expect the plan to reach, +and its `budget` decides whether a shortfall is a bug or just "the money ran +out". Without a scenario_id the deterministic scan audits each Property's +*default* plan (the FE-shown one). + +## Query safety (READ FIRST — inherited from `audit-ara-portfolio`) + +The `recommendation` table is **~26m rows with NO index on `plan_id`**. Any query +reaching it via `plan_id` (a `JOIN ... ON r.plan_id = pl.id`, or a correlated +`EXISTS`) forces a full seq-scan and can take the shared DB down — this is what +blocked the portfolio-796 audit. The full rules are in the `audit-ara-portfolio` +skill; the ones that bite *here*: + +- **Confirm any ad-hoc `recommendation` SQL with the user, and show the + `EXPLAIN` plan first** (no `ANALYZE`). If it contains `Seq Scan on + recommendation`, do NOT run it. +- **Reach `recommendation` only via the indexed `property_id`, scoped to a + handful of flagged property ids** — never portfolio-wide, never via `plan_id`. +- The two scripts below are safe by construction: `anomalies.py`'s recommendation + rollup is opt-in + EXPLAIN-gated, and `neighbour_divergence.py` never touches + `recommendation` at all. The measure-set comparison that Khalim's neighbour + case needs is done in the **deep-dive** (Phase 4) with the live re-model, not a + portfolio-wide `recommendation` query. + +## Phase 1 — Deterministic recommendation scan + +``` +python -m scripts.audit.anomalies --portfolio --scenario +``` + +Writes `modelling_audit.md` / `.csv`. The check that carries this skill: + +- **`plan-stops-short-of-goal`** (HIGH) — the default plan ends *below* the goal + band on an **unlimited-budget** scenario. This is the deterministic worklist + for the "why didn't this get recommended X" class (Khalim's pid 742121). With + no budget cap, the only reasons to fall short are (a) a measure was wrongly + withheld — a bug — or (b) the dwelling genuinely can't reach the band. Phase 4 + decides which. Budget-capped scenarios are excluded (falling short is expected). + +Also read `plan-below-baseline-band`, `already-meets-goal-with-works`, and — if +you pass `--with-recommendations` (EXPLAIN-gated; confirm with the user first) — +`excessive-solar-sap` / `low-solar-bill-savings`. These bound the "the plan did +something odd" surface. + +## Phase 2 — Neighbour-divergence scan + +``` +python -m scripts.audit.neighbour_divergence --portfolio +``` + +Writes `neighbour_divergence.md` / `.csv`. Groups the portfolio into cohorts of +(postcode, property_type, built_form) and flags any member whose **effective +baseline SAP** sits `--min-gap` (default 12) points from its cohort median. +Near-identical neighbours should model alike; a SAP split is what drives a +recommendation split (Khalim's neighbour case). This is the SAP half of the +heuristic — cheap and portfolio-wide because it never touches `recommendation`. +The **measure-set** half is Phase 4: re-model both neighbours and diff their +plans. + +Floor area is reported, not keyed on — discount a genuinely bigger/smaller +neighbour before trusting a hit. + +## Phase 3 — Triage the hits + +For each group, HIGH first: note the count, read a few rows, and give a one-line +**root-cause hypothesis** + a verdict (real bug / expected / threshold to tune). +Cross-check the two scans against each other — a `plan-stops-short-of-goal` +property that is *also* a divergent neighbour is a high-value case (the two +signals agree). + +## Phase 4 — Deep-dive with the live re-model (the core of this skill) + +Reproduce end-to-end, NO DB writes (never pass `--persist`): + +``` +python -m scripts.run_modelling_e2e [ ...] --scenario-id +``` + +This re-fetches the live EPC + solar and runs the full DDD Modelling stage. It +writes three files — read all three: + +- `modelling_e2e.md` — the chosen plan, measure by measure, with post-SAP. +- `modelling_e2e_candidates.csv` — **every candidate Option the generators + produced, including the ones the Optimiser didn't pick.** This is how you tell + a *withheld* measure from a *rejected* one: + - Measure **absent from candidates** → a **generator eligibility gate** + excluded it. This is the bug surface for "why no HHRSH / no + secondary_heating_removal". Open the generator and read its `_*_eligible` + predicate against the live EPC. + - Measure **present in candidates but not chosen** → the Optimiser judged it + not worth it (cost/SAP/goal already met). Usually correct; confirm the goal + was reached without it. +- `modelling_e2e.csv` — the row-level plan. + +For a neighbour pair, run **both** in one invocation and diff their candidate +sets and chosen plans — that is the measure-set comparison Khalim asked for, +done safely (no `recommendation` table). + +### Known recommendation-bug patterns (seed catalogue — grew from portfolio 814) + +Match each deep-dive against these before hypothesising a novel cause: + +- **HHRSH offered on a community-heated dwelling** (pids 742174, 742175). + **Confirmed bug.** The DDD generator's `_hhr_storage_eligible` + ([`domain/modelling/generators/heating_recommendation.py`](../../../domain/modelling/generators/heating_recommendation.py)) + returns eligible when the dwelling is `off_gas` — but a community-heated + dwelling is off-gas, and the legacy engine's + `is_high_heat_retention_valid` ([`recommendations/HeatingRecommender.py`](../../../recommendations/HeatingRecommender.py)) + gated on `main_fuel["is_community"]` and refused it. The community-heating + guard was **lost in the legacy→DDD translation**. Confirm by checking the EPC's + main fuel / heat-network flag, then fix by restoring the gate in + `_hhr_storage_eligible` (community heating ⇒ not HHR-eligible). A community + network is a shared asset a single dwelling can't rip out for storage heaters. +- **Measure withheld → plan stops short of goal** (pid 742121, "why no HHRSH"). + The dwelling can't reach the goal band, and the expected bundle is *absent from + candidates*. Read the generator's eligibility predicate against the live EPC — + an over-tight gate (a fuel/description/category condition that shouldn't + exclude this dwelling) is the usual cause. +- **Missing `secondary_heating_removal`** (pid 742264, "gov site shows secondary + heating"). Check whether the live EPC carries a secondary-heating system and + whether + [`domain/modelling/generators/secondary_heating_recommendation.py`](../../../domain/modelling/generators/secondary_heating_recommendation.py) + detects it. A gap between the gov register's secondary-heating field and what + our EpcPropertyData mapper carries is a common upstream cause — verify the + mapped value, not just the register. +- **Neighbour split** — two same-cohort dwellings, one gets a bundle the other + doesn't. Almost always traces to a **baseline-input divergence** (a different + mapped fuel / heating code / floor area / an override on one and not the + other), which then flips a generator's eligibility gate. Diff the two EPCs' + mapped inputs first; the recommendation split is downstream of that. + +## Phase 5 — Root-cause and cross-reference open work + +Isolate **where** it goes wrong: a generator eligibility gate, the EPC→ +EpcPropertyData mapping, an override, or the Optimiser. Then check nothing is +already in flight: + +``` +gh pr list --repo Hestia-Homes/Model --state open +gh pr list --repo Hestia-Homes/Model --state all --search "" +``` + +Map each confirmed cause to an existing PR/ADR or flag it as new work. State, +per case, the smallest correct fix (usually: tighten/loosen one generator +predicate, or fix one mapper field) and which real property ids reproduce it. + +## Phase 6 — Self-improve (the compounding loop) + +When the review confirms a **novel, systematic** recommendation problem, codify +it so every future run catches it — same gates as `audit-ara-portfolio` Phase 6: + +1. **Systematic** — reproduced on **≥ 5** properties and root-caused, not a + one-off (a single weird property is a ticket, not a check). +2. **Not already covered** — no existing check fires on it; no open/merged PR or + ADR already addresses the cause. +3. **Pressure-tested** — for any threshold/heuristic, run `/grill-me` on the + proposed check: false-positive rate on this portfolio? threshold defensible + against the real distribution? overlaps an existing check? + +**What to change, smallest first:** +- **A per-property tell** → a decorated `(PropertyAudit) -> Optional[str]` check + in `scripts/audit/anomalies.py` (docstring records the motivating pids + the + one-line cause, like `plan-stops-short-of-goal` does). Extend the + `PropertyAudit` bundle + `_QUERY` if it needs a field — keep every query bounded + by the portfolio and off the `recommendation` table's `plan_id`. +- **A cross-property tell** (a neighbour/cohort pattern) → a detector in + `scripts/audit/neighbour_divergence.py`. The obvious next one: promote the + measure-set diff into a scripted check that reaches `recommendation` via + property-id-scoped, EXPLAIN-gated queries per cohort (kept out of v1 to stay + portfolio-wide-safe — do it once the SAP tell's false-positive rate is known). +- **This catalogue** → add any newly-confirmed bug pattern to Phase 4's list with + its pids and root cause, so the next reviewer starts ahead. + +Commit each codified check on its own referencing the motivating run, then re-run +Phases 1–2 to confirm it fires on the motivating cases and nothing surprising +else. The check registry — with provenance — is the durable output of every hunt. + +## Notes + +- Read-only on the DB. `run_modelling_e2e` is a dry run — never `--persist`. +- **Two engines exist.** Portfolios are modelled by the **DDD** engine + (`domain/modelling/generators/…`, what `run_modelling_e2e` runs); the legacy + `recommendations/…` engine is the historical reference. When a gate looks + wrong, compare the DDD predicate against its legacy counterpart — several + portfolio-814 bugs are gates the DDD translation **dropped** (community heating + is the confirmed one). The legacy code is the spec of intent, not dead code. +- **Stored plan can be STALE vs live.** The persisted default plan is an earlier + run's output; `run_modelling_e2e` re-models against current logic + live EPC. + A stored-vs-live gap is a big driver of Phase 1 hits and is fixed by + re-modelling, not by debugging the calculator — confirm a sample with the + re-model before blaming a generator (see `audit-ara-portfolio` Notes). +- A `plan-stops-short-of-goal` hit is **not automatically a bug** — the user's + own framing: "surely there's a reason, but if a sensible reason we should + understand why". The deliverable for a physically-unreachable dwelling is the + *explanation* (which measures were tried, why the band is out of reach), not a + code change. +- **On flat/maisonette-heavy portfolios, `plan-stops-short-of-goal` fires in + bulk — mostly by design, not bugs.** Characterise the cohort by property_type + and post-band before deep-diving (cheap: `property` + `epc_property` + `plan`, + no `recommendation` table). Portfolio 814 (94% flats/maisonettes, goal band B) + landed 311/338 short of B, but **every** D-or-worse lander and every £0-works + shortfall was a flat/maisonette — all 19 houses reached C+. The structural + cause: the heating-decarbonisation levers are gated away from flats — + `_ashp_eligible` offers ASHP **only to houses/bungalows** (`_is_house_or_bungalow`), + the gas-boiler upgrade needs an existing **wet** boiler (point-of-use gas water + heaters like WHC 908 don't qualify — see pid 742121, a mains-gas maisonette at + E that correctly gets only lighting), and HHRSH needs electric/off-gas. So a + mains-gas or community-heated flat with insulated walls + DG has almost no lever + and lands at D/E legitimately. Two takeaways: (1) triage the *houses* short of + goal first — those are the real bugs; (2) the flat cohort is a **product** + question (should ASHP/communal-heat measures be offered to flats?), not a + per-property modelling bug — surface it as such. +- **A measure can be SELECTED while *lowering* SAP** if it's a forced companion + (pid 742175: `mechanical_ventilation` at **−4.1 SAP**, bundled after + `cavity_wall_insulation`). Net plan SAP still rose, but the companion drags it + and worsens the bill — worth a look when a plan's post-SAP barely moves or dips + (overlaps `plan-score-below-baseline`). Not yet root-caused; note the pids. diff --git a/scripts/audit/anomalies.py b/scripts/audit/anomalies.py index da334d564..7e9f627a8 100644 --- a/scripts/audit/anomalies.py +++ b/scripts/audit/anomalies.py @@ -74,6 +74,7 @@ class PropertyAudit: portfolio_id: int scenario_id: Optional[int] scenario_goal_band: Optional[str] + scenario_budget: Optional[float] # None == unlimited budget lodged_sap: Optional[float] lodged_band: Optional[str] effective_sap: Optional[float] @@ -148,6 +149,32 @@ def _already_meets_goal_with_works(a: PropertyAudit) -> Optional[str]: return f"already {a.effective_band} >= goal {a.scenario_goal_band} but cost_of_works £{a.cost_of_works:.0f}" +@check("plan-stops-short-of-goal", Severity.HIGH) +def _plan_stops_short_of_goal(a: PropertyAudit) -> Optional[str]: + """The default plan ends BELOW the scenario's goal band on a scenario that is + NOT budget-capped — with an unlimited budget a plan should reach the goal + unless it is physically impossible for that dwelling, so a shortfall is either + a measure wrongly withheld (a bug) or a "sensible reason" the engine should be + able to explain. + + This is the deterministic worklist for Khalim's "why didn't this get + recommended " class (portfolio 814, pid 742121 — a dwelling left + short of C with no HHRSH bundle). It only flags candidates; the deep-dive + (``run_modelling_e2e`` + the candidates CSV) decides withheld-measure vs + physically-unreachable. Budget-capped scenarios are excluded because there a + shortfall is expected — the money simply ran out.""" + goal, post = _band_rank(a.scenario_goal_band), _band_rank(a.post_band) + if goal is None or post is None or post <= goal: + return None + if a.scenario_budget is not None: + # Budget-capped: falling short is expected once the money runs out. + return None + return ( + f"post {a.post_band} ({a.post_sap}) short of goal {a.scenario_goal_band} " + f"on an unlimited-budget scenario (cost_of_works £{a.cost_of_works or 0:.0f})" + ) + + @check("post-band-score-mismatch", Severity.MEDIUM) def _post_band_score_mismatch(a: PropertyAudit) -> Optional[str]: """The persisted post band disagrees with the band the post SAP implies — a @@ -276,7 +303,7 @@ def _negative_bill_savings(a: PropertyAudit) -> Optional[str]: _QUERY = text( """ SELECT p.id, p.uprn, p.portfolio_id, - pl.scenario_id, s.goal_value AS goal_band, + pl.scenario_id, s.goal_value AS goal_band, s.budget AS scenario_budget, pbp.lodged_sap_score, pbp.lodged_epc_band, pbp.effective_sap_score, pbp.effective_epc_band, pbp.rebaseline_reason, pl.post_sap_points, pl.post_epc_rating, pl.cost_of_works, @@ -394,6 +421,7 @@ def _load( portfolio_id=m["portfolio_id"], scenario_id=m["scenario_id"], scenario_goal_band=m["goal_band"], + scenario_budget=m["scenario_budget"], lodged_sap=m["lodged_sap_score"], lodged_band=m["lodged_epc_band"], effective_sap=m["effective_sap_score"], diff --git a/scripts/audit/neighbour_divergence.py b/scripts/audit/neighbour_divergence.py new file mode 100644 index 000000000..d9fc008d5 --- /dev/null +++ b/scripts/audit/neighbour_divergence.py @@ -0,0 +1,226 @@ +"""Flag neighbouring dwellings that were modelled *differently despite looking +the same* — the SAP half of the "neighbours should agree" heuristic. + +Motivation (portfolio 814): Khalim found a dwelling that gets an HHRSH bundle +while its next-door neighbour of the same type does not. Neighbours in one +postcode, of the same property type and built form, are usually near-identical +stock; a big split in their *effective baseline SAP* is a strong tell that one of +the pair was mis-mapped, mis-overridden, or mis-rebaselined — and a SAP split is +what then drives a recommendation split. This script surfaces those pairs +cheaply so the deep-dive (``run_modelling_e2e`` on both neighbours) can compare +their actual measure sets and root-cause the divergence. + +It reaches only ``property`` + ``epc_property`` + ``property_baseline_performance``, +all via the indexed ``property_id`` — it NEVER touches the 26m-row +``recommendation`` table, so it is safe to run portfolio-wide (unlike the +measure-set comparison, which the skill does per flagged cohort in the deep-dive). + +Run: + python -m scripts.audit.neighbour_divergence --portfolio 814 + python -m scripts.audit.neighbour_divergence --portfolio 814 --min-gap 15 + +Writes ``neighbour_divergence.md`` + ``neighbour_divergence.csv`` and prints a +summary. Read-only: never writes to the DB. + +A cohort is the set of a portfolio's properties sharing (postcode, property_type, +built_form). Within a cohort of >= 2, any member whose effective SAP is >= +``--min-gap`` points from the cohort median is flagged. Floor area is reported, +not keyed on, so the reviewer can discount a genuinely bigger/smaller neighbour; +promoting an area guard into the cohort key is a clean future change once the +false-positive rate is measured on a real portfolio (skill Phase 6). +""" + +from __future__ import annotations + +import argparse +import csv +from dataclasses import dataclass +from statistics import median +from typing import Optional + +from sqlalchemy import text + +from scripts.e2e_common import build_engine, load_env + +# Hard ceiling so a bad plan aborts instead of saturating the shared DB, mirroring +# scripts/audit/anomalies.py. Every table here is reached via the indexed +# property_id and scoped to one portfolio, so this is a backstop, not a crutch. +_STATEMENT_TIMEOUT_MS = 120_000 + +# Default SAP gap (points from the cohort median) at which a neighbour is flagged. +# 12 is a starting threshold, not a tuned one — a full band is ~10-11 SAP points, +# so >= 12 means the neighbour sits a clear band apart from otherwise-identical +# stock. Re-justify against a real portfolio's distribution before trusting it +# (skill Phase 6); expose it as --min-gap so a run can sweep the threshold. +_DEFAULT_MIN_GAP = 12.0 + + +@dataclass(frozen=True) +class Neighbour: + """One modelled property placed in its postcode/type/form cohort.""" + + property_id: int + uprn: Optional[int] + postcode: Optional[str] + property_type: Optional[str] + built_form: Optional[str] + floor_area_m2: Optional[float] + effective_sap: Optional[float] + effective_band: Optional[str] + + +@dataclass(frozen=True) +class Divergence: + property_id: int + uprn: Optional[int] + cohort_key: str + cohort_size: int + detail: str + + +# DISTINCT ON picks the latest ingested epc_property row per property (its +# property_id is NOT unique — ingestion keeps history), ordered by id DESC. +_QUERY = text( + """ + SELECT p.id, p.uprn, p.postcode, + ep.property_type, ep.built_form, ep.total_floor_area_m2, + pbp.effective_sap_score, pbp.effective_epc_band + FROM property p + JOIN property_baseline_performance pbp ON pbp.property_id = p.id + LEFT JOIN LATERAL ( + SELECT property_type, built_form, total_floor_area_m2 + FROM epc_property e + WHERE e.property_id = p.id + ORDER BY e.id DESC + LIMIT 1 + ) ep ON TRUE + WHERE p.portfolio_id = :portfolio_id + ORDER BY p.id + """ +) + + +def _load(portfolio_id: int) -> list[Neighbour]: + engine = build_engine() + out: list[Neighbour] = [] + with engine.connect() as conn: + conn.execute(text(f"SET statement_timeout = {_STATEMENT_TIMEOUT_MS}")) + for row in conn.execute(_QUERY, {"portfolio_id": portfolio_id}): + m = row._mapping # noqa: SLF001 — SQLAlchemy row mapping, mirrors anomalies.py + out.append( + Neighbour( + property_id=m["id"], + uprn=m["uprn"], + postcode=m["postcode"], + property_type=m["property_type"], + built_form=m["built_form"], + floor_area_m2=m["total_floor_area_m2"], + effective_sap=m["effective_sap_score"], + effective_band=m["effective_epc_band"], + ) + ) + return out + + +def _cohort_key(n: Neighbour) -> Optional[str]: + """A stable cohort label, or None when the neighbour can't be placed (no + postcode or no property type — it has no comparable peers to diverge from).""" + if not n.postcode or not n.property_type: + return None + form = n.built_form or "?" + return f"{n.postcode.strip().upper()} · {n.property_type} · {form}" + + +def find_divergences( + neighbours: list[Neighbour], min_gap: float +) -> list[Divergence]: + cohorts: dict[str, list[Neighbour]] = {} + for n in neighbours: + key = _cohort_key(n) + if key is None or n.effective_sap is None: + continue + cohorts.setdefault(key, []).append(n) + + found: list[Divergence] = [] + for key, members in cohorts.items(): + if len(members) < 2: + continue + saps = [m.effective_sap for m in members if m.effective_sap is not None] + cohort_median = median(saps) + for m in members: + if m.effective_sap is None: + continue + gap = m.effective_sap - cohort_median + if abs(gap) < min_gap: + continue + area = f"{m.floor_area_m2:.0f}m²" if m.floor_area_m2 is not None else "?m²" + found.append( + Divergence( + property_id=m.property_id, + uprn=m.uprn, + cohort_key=key, + cohort_size=len(members), + detail=( + f"effective SAP {m.effective_sap:.0f} ({m.effective_band}) " + f"vs cohort median {cohort_median:.0f} (Δ{gap:+.0f}, " + f"{area}, {len(members)} neighbours)" + ), + ) + ) + found.sort(key=lambda d: (d.cohort_key, d.property_id)) + return found + + +def _write_reports(divergences: list[Divergence], scanned: int) -> None: + with open("neighbour_divergence.csv", "w", newline="") as f: + w = csv.writer(f) + w.writerow(["property_id", "uprn", "cohort", "cohort_size", "detail"]) + for d in divergences: + w.writerow([d.property_id, d.uprn, d.cohort_key, d.cohort_size, d.detail]) + + by_cohort: dict[str, list[Divergence]] = {} + for d in divergences: + by_cohort.setdefault(d.cohort_key, []).append(d) + lines = [ + "# Neighbour SAP-divergence audit", + "", + f"Scanned **{scanned}** properties · flagged **{len(divergences)}** " + f"divergent neighbours across **{len(by_cohort)}** cohorts.", + "", + ] + for key in sorted(by_cohort): + rows = by_cohort[key] + lines.append(f"## {key} — {len(rows)} divergent") + lines.append("") + for d in rows: + lines.append(f"- property **{d.property_id}** (uprn {d.uprn}): {d.detail}") + lines.append("") + with open("neighbour_divergence.md", "w") as f: + f.write("\n".join(lines)) + + +def main() -> None: + load_env() + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--portfolio", type=int, required=True, help="portfolio_id") + parser.add_argument( + "--min-gap", + type=float, + default=_DEFAULT_MIN_GAP, + help=f"SAP points from cohort median to flag (default {_DEFAULT_MIN_GAP})", + ) + args = parser.parse_args() + + neighbours = _load(args.portfolio) + divergences = find_divergences(neighbours, args.min_gap) + _write_reports(divergences, len(neighbours)) + + print( + f"scanned {len(neighbours)} properties · {len(divergences)} divergent " + f"neighbours (>= {args.min_gap:.0f} SAP from cohort median)" + ) + print("wrote neighbour_divergence.md / neighbour_divergence.csv") + + +if __name__ == "__main__": + main() diff --git a/tests/scripts/test_audit_anomalies.py b/tests/scripts/test_audit_anomalies.py index 280b7c3a9..2cc510bba 100644 --- a/tests/scripts/test_audit_anomalies.py +++ b/tests/scripts/test_audit_anomalies.py @@ -6,29 +6,36 @@ from scripts.audit.anomalies import ( PropertyAudit, _effective_lodged_divergence, _implausible_lodged_score, + _plan_stops_short_of_goal, ) def _make_audit( *, - lodged_sap: Optional[float], - effective_sap: Optional[float], + lodged_sap: Optional[float] = None, + effective_sap: Optional[float] = None, rebaseline_reason: str = "both", + scenario_goal_band: Optional[str] = None, + scenario_budget: Optional[float] = None, + post_band: Optional[str] = None, + post_sap: Optional[float] = None, + cost_of_works: Optional[float] = None, ) -> PropertyAudit: return PropertyAudit( property_id=1, uprn=None, portfolio_id=796, scenario_id=None, - scenario_goal_band=None, + scenario_goal_band=scenario_goal_band, + scenario_budget=scenario_budget, lodged_sap=lodged_sap, lodged_band=None, effective_sap=effective_sap, effective_band=None, rebaseline_reason=rebaseline_reason, - post_sap=None, - post_band=None, - cost_of_works=None, + post_sap=post_sap, + post_band=post_band, + cost_of_works=cost_of_works, energy_bill_savings=None, energy_consumption_savings=None, solar_sap_points=None, @@ -92,3 +99,53 @@ class TestImplausibleLodgedScore: # Assert assert result is None + + +class TestPlanStopsShortOfGoal: + def test_fires_when_short_of_goal_on_unlimited_budget(self) -> None: + # Arrange — goal C, plan lands at D, budget unlimited (None): a shortfall + # the engine should be able to explain (Khalim's 742121 class). + audit = _make_audit( + scenario_goal_band="C", + scenario_budget=None, + post_band="D", + post_sap=63.0, + cost_of_works=8000.0, + ) + + # Act + result = _plan_stops_short_of_goal(audit) + + # Assert + assert result is not None + assert "D" in result + assert "C" in result + + def test_silent_when_plan_meets_goal(self) -> None: + # Arrange — goal C, plan reaches C: nothing to explain. + audit = _make_audit( + scenario_goal_band="C", scenario_budget=None, post_band="C", post_sap=70.0 + ) + + # Act + result = _plan_stops_short_of_goal(audit) + + # Assert + assert result is None + + def test_silent_when_budget_capped(self) -> None: + # Arrange — goal C, plan short at D, but the scenario is budget-capped: + # falling short is expected once the money runs out, not an anomaly. + audit = _make_audit( + scenario_goal_band="C", + scenario_budget=5000.0, + post_band="D", + post_sap=63.0, + cost_of_works=5000.0, + ) + + # Act + result = _plan_stops_short_of_goal(audit) + + # Assert + assert result is None diff --git a/tests/scripts/test_neighbour_divergence.py b/tests/scripts/test_neighbour_divergence.py new file mode 100644 index 000000000..9bf02ef85 --- /dev/null +++ b/tests/scripts/test_neighbour_divergence.py @@ -0,0 +1,76 @@ +from typing import Optional + +from scripts.audit.neighbour_divergence import Neighbour, find_divergences + + +def _n( + property_id: int, + *, + effective_sap: Optional[float], + postcode: str = "AB1 2CD", + property_type: str = "Flat", + built_form: str = "Mid-Terrace", + floor_area_m2: float = 60.0, +) -> Neighbour: + return Neighbour( + property_id=property_id, + uprn=None, + postcode=postcode, + property_type=property_type, + built_form=built_form, + floor_area_m2=floor_area_m2, + effective_sap=effective_sap, + effective_band=None, + ) + + +class TestFindDivergences: + def test_flags_the_outlier_neighbour(self) -> None: + # Arrange — three identical-cohort flats, one sits a clear band below. + cohort = [ + _n(1, effective_sap=70.0), + _n(2, effective_sap=72.0), + _n(3, effective_sap=50.0), # Δ-20 vs median 70 + ] + + # Act + result = find_divergences(cohort, min_gap=12.0) + + # Assert — only the outlier is flagged. + assert [d.property_id for d in result] == [3] + + def test_silent_when_cohort_agrees(self) -> None: + # Arrange — neighbours within a few SAP points of each other. + cohort = [_n(1, effective_sap=70.0), _n(2, effective_sap=68.0)] + + # Act + result = find_divergences(cohort, min_gap=12.0) + + # Assert + assert result == [] + + def test_singletons_never_flagged(self) -> None: + # Arrange — one flat here, one house there: neither has a peer to diverge from. + lonely = [ + _n(1, effective_sap=30.0, property_type="Flat"), + _n(2, effective_sap=90.0, property_type="House"), + ] + + # Act + result = find_divergences(lonely, min_gap=12.0) + + # Assert + assert result == [] + + def test_different_postcode_is_a_different_cohort(self) -> None: + # Arrange — same type/form but different postcodes: not neighbours. + split = [ + _n(1, effective_sap=40.0, postcode="AB1 2CD"), + _n(2, effective_sap=80.0, postcode="ZZ9 9ZZ"), + ] + + # Act + result = find_divergences(split, min_gap=12.0) + + # Assert + assert result == []