diff --git a/applications/landlord_description_overrides/handler.py b/applications/landlord_description_overrides/handler.py index 4020bc512..a43413b4f 100644 --- a/applications/landlord_description_overrides/handler.py +++ b/applications/landlord_description_overrides/handler.py @@ -14,6 +14,12 @@ from domain.epc.property_overrides.main_fuel_type import MainFuelType from domain.epc.property_overrides.main_heating_system_type import MainHeatingSystemType from domain.epc.property_overrides.property_type import PropertyType from domain.epc.property_overrides.roof_type import RoofType +from domain.epc.property_overrides.roof_party_ceiling_guard import ( + roof_party_ceiling_guard, +) +from domain.data_transformation.guarded_column_classifier import ( + GuardedColumnClassifier, +) from domain.epc.property_overrides.water_heating_type import WaterHeatingType from domain.epc.property_overrides.wall_type import WallType from domain.epc.property_overrides.wall_type_construction_dates import ( @@ -115,8 +121,13 @@ def _build_columns( "roof_type": lambda src: ClassifiableColumn( name="roof_type", source_column=src, - classifier=ChatGptColumnClassifier( - chat_gpt, RoofType, RoofType.UNKNOWN + # A party ceiling ("another/same dwelling or premises above") has ~0 + # heat loss and must never be classified as an external roof; the + # deterministic guard resolves those markers and the LLM handles the + # rest (#1376). + classifier=GuardedColumnClassifier( + guard=roof_party_ceiling_guard, + fallback=ChatGptColumnClassifier(chat_gpt, RoofType, RoofType.UNKNOWN), ), repo=LandlordOverridesRepository[RoofType]( session, LandlordRoofTypeOverrideRow diff --git a/docs/adr/0041-flat-roofs-scored-by-insulation-thickness.md b/docs/adr/0041-flat-roofs-scored-by-insulation-thickness.md new file mode 100644 index 000000000..f5f7e4621 --- /dev/null +++ b/docs/adr/0041-flat-roofs-scored-by-insulation-thickness.md @@ -0,0 +1,30 @@ +# Flat roofs are scored by insulation thickness (RdSAP Table 16 col 1), not the age-band default + +## Context + +A **Landlord Override** naming a flat roof with a known insulation depth (`flat: 50mm` / `100mm` / `150mm`) is classified to the canonical `RoofType` value `"Flat, insulated (assumed)"`, which the roof **Simulation Overlay** resolves to `roof_construction_type='Flat'` with **no thickness**. The calculator then takes the U-value from the flat age-band default (`_FLAT_ROOF_BY_AGE`, RdSAP Table 18 column (3)). The stated depth is **discarded** — ~102 `property_overrides` rows. + +The overlay's own comment asserted *"No flat RoofType value carries an explicit mm depth"*. That is a **taxonomy artefact, not a truth about the data**: the `RoofType` enum has flat members only for insulation *state* (`FLAT_INSULATED / _ASSUMED / LIMITED / NO_INSULATION`) and **no flat-thickness members** — whereas pitched has the full `PITCHED_LOFT_12MM … 400MM` ladder. So a flat depth has nowhere to land and is lost at **classification**, before the overlay ever runs. + +Reviewing the **RdSAP 10 Specification (10-06-2025)** during grilling (issue #1361 override audit / #1376) settled the domain fact: **Table 16** ("Roof U-values when loft insulation thickness is known"), **column (1)**, is headed *"Insulation at joists at ceiling level **and flat roof**"*. A flat roof **is** scored by thickness — 50 mm → 0.68, 100 mm → 0.40, 150 mm → 0.30. Table 18 (the age-band default) is explicitly only the fallback *"used when thickness of insulation cannot be determined"*. The earlier assumption that flat roofs are age-band-only was wrong. + +The calculator already implements this correctly: `u_roof` routes a flat roof **with** a thickness through `_ROOF_BY_THICKNESS` (which *is* Table 16 col (1)). So a flat roof given its depth scores right today — the only defect is that the depth never reaches the calculator, because the taxonomy can't carry it. + +This is the opposite of the wall / flat "as built (assumed)" cases (ADR-0033), where **no** real datum exists and the age band is the correct lever. Here a **real measurement** exists and RdSAP scores it. + +## Decision + +Make a flat roof's known insulation thickness first-class, so it reaches Table 16 col (1): + +- Add **flat-thickness members** to the `RoofType` taxonomy — `FLAT_12MM … FLAT_400_PLUS_MM` (values like `"Flat, 150 mm insulation"` — **not** "loft"; a flat roof has no loft). Minimum set for today's data: 50, 100, 150; the full ladder mirrors the pitched members for symmetry. +- The roof **Simulation Overlay** emits `roof_insulation_thickness` for a flat roof with a known depth (reusing the existing mm regex), alongside `roof_construction_type='Flat'`, so `u_roof` reaches Table 16 col (1) instead of the age-band default. +- **Reclassify** the existing `flat: Nmm` rows off `"Flat, insulated (assumed)"` onto the new members. +- Correct the misleading overlay comment. + +The flat/pitched distinction is retained even though Table 16 col (1) gives the same U at a given thickness for both — it stays load-bearing for non-U concerns (measure eligibility, shape), so flat depths are **not** collapsed onto the pitched members. + +## Consequences + +- ~102 flat-roof properties re-score from the age-band default to their true thickness-based U-value — **both directions** (a newer-band flat roof with 150 mm improves; an old-band flat with 50 mm may worsen relative to its default). +- **Cross-repo (FE-owned pgEnum, Dan).** The `RoofType` `value` column is a Drizzle-owned Postgres enum (see `main-heating-system-pgenum-is-fe-owned`; PR #1361 Class A/B). The new members must be added to the pgEnum by the FE owner before the classifier-cache `value` writes; the `property_overrides` (TEXT) reclassify is immediate. So this is an **enum-dependent slice**, grouped with the other new-archetype slices of #1376 — not the no-enum roof/glazing resolver slice. +- The reclassify follows the established one-time-script shape (dry-run default, `--apply` in a transaction, idempotent), and surfaces any members the live enum does not yet carry as deferred, exactly as Class A did. diff --git a/docs/adr/0042-glazing-override-reconciles-against-cert-composition.md b/docs/adr/0042-glazing-override-reconciles-against-cert-composition.md new file mode 100644 index 000000000..2b82c472a --- /dev/null +++ b/docs/adr/0042-glazing-override-reconciles-against-cert-composition.md @@ -0,0 +1,31 @@ +# Landlord glazing override reconciles against the cert's per-window composition, not a whole-dwelling type + +## Context + +A **Landlord Override** for glazing is a whole-dwelling categorical (`GlazingType` — Single / Double pre- and post-2002 / Triple / Unknown). The glazing **Simulation Overlay** resolves it to one SAP10 `glazing_type` code, and `_fold_glazing` overwrites **every** window's `glazing_type` (and clears its lodged U-/g-value) — flattening the cert to a single type. + +The landlord descriptions are frequently **aggregate splits** ("40% double, 60% single"), and the LLM classifier collapses *any-double-present* to `"Double glazing"`. So ~319 `property_overrides` rows describing a **single-dominant** mix are scored as **fully double-glazed** — a material over-credit to the window U-values, hence to SAP. + +The Effective EPC already carries **per-window glazing**: `sap_windows`, each with its own `glazing_type` **and** `window_width`×`window_height` (so area is derivable), plus the dwelling-level `multiple_glazed_proportion`. This is **more granular** than any whole-dwelling summary, and a whole-dwelling percentage **cannot be faithfully assigned to specific windows** — "which windows are the single ones" is unknowable from an aggregate. `_fold_glazing` already holds the EPC at apply time, so the per-window data is available where the override is applied. + +Fixing this by "dominant type wins" would still **clobber real per-window variation** (a 60/40 dwelling flattened to one type). The faithful move is to treat the cert's per-window data as **authoritative** and the landlord aggregate as a **check** on it — the descriptions are, after all, aggregations of the same per-window split the cert records. + +## Decision + +The landlord glazing override **reconciles against the cert's per-window composition** instead of imposing a whole-dwelling type. + +- **Classifier emits a proportion, not just a dominant type.** The asserted glazing **proportion** (e.g. multiple-glazed %) is extracted by the **LLM** — which is there precisely because landlord inputs vary; we do **not** hard-parse one string format. +- **Compute the cert's actual composition** from `sap_windows`, **area-weighted** by window (bigger windows dominate the dwelling U-window). +- **Reconcile in `_fold_glazing`** (it holds the EPC), three outcomes: + - **Uniform assertion (~100% one type)** → apply the blanket type. Unambiguous, and a real correction (e.g. a full reglaze). Unchanged from today for clean cases. + - **Mixed, proportion within tolerance of the cert** → **no-op**. The cert already reflects it, per-window and more precisely — leave the better data alone. + - **Mixed, proportion materially different from the cert** → **no-op + flag**. The landlord genuinely disagrees, but per-window assignment is unknowable, so **do not fabricate a split** — surface it for review. +- **Tolerance** is a tunable band on the area-weighted multiple-glazed %, pinned against the real distribution, not guessed. + +## Consequences + +- The ~319 over-credited rows stop being flattened to double; a mix that matches the cert is left on its per-window data, and a clean uniform reglaze still applies. +- **No new enum.** A no-op reconciliation leaves the cert glazing untouched (as `Unknown` already does). But it needs a **classifier-output change** (emit a proportion) **plus** the reconcile in the **apply seam** (`_fold_glazing`) — so it is its **own slice**, distinct from the no-enum roof party-ceiling fix that ships first. +- Genuine landlord-vs-cert glazing disagreements are **surfaced** rather than silently trusted (old behaviour: flatten to double) or silently overwritten. +- **Alternatives rejected.** *Dominant type wins* — still clobbers per-window variation. *Carry a full landlord composition to re-derive per-window types* — per-window assignment from an aggregate is unknowable; we decline to fabricate it. *Deterministic regex of "X% double, Y% single"* — brittle against varied input, which is the LLM's job. +- Pairs with the per-window fidelity the calculator already relies on: the reconciliation is only correct on a faithful Effective EPC whose `sap_windows` round-trip (cf. ADR-0040). diff --git a/domain/data_transformation/guarded_column_classifier.py b/domain/data_transformation/guarded_column_classifier.py new file mode 100644 index 000000000..f84ab6ac2 --- /dev/null +++ b/domain/data_transformation/guarded_column_classifier.py @@ -0,0 +1,42 @@ +from __future__ import annotations + +from enum import Enum +from typing import Callable, Optional, TypeVar + +from domain.data_transformation.column_classifier import ColumnClassifier + +E = TypeVar("E", bound=Enum) + + +class GuardedColumnClassifier(ColumnClassifier[E]): + """A ``ColumnClassifier`` that resolves the descriptions a deterministic guard + is certain about, and delegates the rest to a fallback classifier. + + The ``guard`` maps a raw description to a category member when it recognises it + deterministically (e.g. a party-ceiling roof marker — #1376), else ``None``. + Guard hits never reach the fallback, so an unreliable classifier (the LLM) + cannot override a description the guard is sure of — and the LLM is not billed + for it. Every description still appears in the result (guarded or fallen-back). + """ + + def __init__( + self, + guard: Callable[[str], Optional[E]], + fallback: ColumnClassifier[E], + ) -> None: + self._guard = guard + self._fallback = fallback + + def classify(self, descriptions: set[str]) -> dict[str, E]: + guarded: dict[str, E] = {} + misses: set[str] = set() + for description in descriptions: + member = self._guard(description) + if member is not None: + guarded[description] = member + else: + misses.add(description) + # Only the misses reach the fallback — a fully-guarded batch never calls it. + if misses: + guarded.update(self._fallback.classify(misses)) + return guarded diff --git a/domain/epc/property_overrides/roof_party_ceiling_guard.py b/domain/epc/property_overrides/roof_party_ceiling_guard.py new file mode 100644 index 000000000..359cc219e --- /dev/null +++ b/domain/epc/property_overrides/roof_party_ceiling_guard.py @@ -0,0 +1,43 @@ +from __future__ import annotations + +import re +from typing import Optional + +from domain.epc.property_overrides.roof_type import RoofType + +# The roof-type token (before any ``: `` suffix), normalised to lower +# alphanumerics, → its party-ceiling RoofType member. Normalising strips spacing +# and case so both ``"another dwelling above"`` and ``"anotherdwellingabove"`` +# match the same marker. +_PARTY_CEILING_MARKERS: dict[str, RoofType] = { + "anotherdwellingabove": RoofType.ADJACENT_ANOTHER_DWELLING_ABOVE, + "samedwellingabove": RoofType.ADJACENT_SAME_DWELLING_ABOVE, + "otherpremisesabove": RoofType.ADJACENT_OTHER_PREMISES_ABOVE, + # Both the "(another premises above)" adjacency and the redundant "Another + # Premises Above" taxonomy member normalise here; map to the parenthesised + # family (both resolve to no overlay, so there is no scoring difference). + "anotherpremisesabove": RoofType.ADJACENT_ANOTHER_PREMISES_ABOVE, +} + + +def _normalise_roof_token(description: str) -> str: + token = description.split(":", 1)[0] + return re.sub(r"[^a-z0-9]", "", token.lower()) + + +def roof_party_ceiling_guard(description: str) -> Optional[RoofType]: + """Deterministically resolve a party-ceiling roof description to its RoofType. + + A building part whose top boundary is another (or the same) dwelling / premises + above has ~0 heat loss (RdSAP 10 Table 18: "There is no heat loss through the + roof of a building part that has the same dwelling or another dwelling above"), + so it must resolve to a party-ceiling `RoofType` member — which the roof + Simulation Overlay leaves as no overlay, keeping the lodged EPC — and never to + an external `Pitched` / `Flat` roof. + + Recognises the party-ceiling markers regardless of a trailing insulation token + (``: 100mm`` / ``: unknown``) or spacing/case, and returns ``None`` for anything + that is not a party-ceiling marker so the LLM classifier still handles it + (#1376). + """ + return _PARTY_CEILING_MARKERS.get(_normalise_roof_token(description)) diff --git a/scripts/reclassify_party_ceiling_roofs.py b/scripts/reclassify_party_ceiling_roofs.py new file mode 100644 index 000000000..3727fa7ef --- /dev/null +++ b/scripts/reclassify_party_ceiling_roofs.py @@ -0,0 +1,132 @@ +"""One-time re-classification of party-ceiling roof overrides mis-read as roofs. + +#1376: a landlord roof description that is a **party-ceiling** marker ("another / +same dwelling or premises above") was occasionally classified by the LLM to an +external ``Pitched, N mm loft insulation`` value when it carried a trailing depth — +inventing roof heat loss where a party ceiling has ~0 (RdSAP 10 Table 18: "There +is no heat loss through the roof of a building part that has the same dwelling or +another dwelling above"). ~106 ``property_overrides`` rows (party-ceiling markers +on any non-party-ceiling value), inconsistent with the ~13k of the same family +already resolving to the party-ceiling member. + +The live classifier now applies ``roof_party_ceiling_guard`` deterministically +(so new intakes are correct); this fixes the rows written before it. The **same +guard** decides the correction here, so the backfill and the live path cannot +drift. + +Updates the TEXT ``property_overrides.override_value`` (what the modelling reads — +the actual fix) and the ``landlord_roof_type_overrides.value`` classifier cache. +The party-ceiling members already exist in the roof pgEnum (13k rows store them), +so no FE migration is needed. + +DRY-RUN BY DEFAULT: prints the row count it would change and writes nothing. Pass +``--apply`` to execute inside a transaction. Idempotent — only rows whose stored +value differs from the guard's member are touched, so a second run is a no-op. +""" + +from __future__ import annotations + +import argparse +from collections.abc import Iterable + +from sqlalchemy import Connection, text + +from domain.epc.property_overrides.roof_party_ceiling_guard import ( + roof_party_ceiling_guard, +) +from scripts.e2e_common import build_engine, load_env + + +def party_ceiling_corrections( + stored: Iterable[tuple[str, str]], +) -> dict[str, str]: + """``(description, stored override_value)`` → the corrected value, for the + descriptions the party-ceiling guard resolves whose stored value is not already + the guard's member. Descriptions the guard leaves to the LLM (``None``) and + rows already on the right member are omitted — so the result is exactly the set + to fix, and re-running against corrected data yields an empty map (idempotent). + """ + corrections: dict[str, str] = {} + for description, value in stored: + member = roof_party_ceiling_guard(description) + if member is not None and value != member.value: + corrections[description] = member.value + 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 + """ +) +_CACHE_UPDATE = text( + """ + UPDATE landlord_roof_type_overrides + SET value = :new_value, updated_at = now() + WHERE lower(description) = :description + AND value::text <> :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 + """ +) + + +def reclassify(conn: Connection, *, apply: bool) -> int: + """NULL-free re-map: set every mis-classified party-ceiling roof override to the + guard's member. Returns the number of ``property_overrides`` rows found (that + ``--apply`` would / did correct).""" + stored = [(r.description, r.value) for r in conn.execute(_DISTINCT)] + total = 0 + for description, new_value in party_ceiling_corrections(stored).items(): + params = {"description": description, "new_value": new_value} + total += conn.execute(_OVERRIDES_COUNT, params).scalar() or 0 + if apply: + conn.execute(_OVERRIDES_UPDATE, params) + conn.execute(_CACHE_UPDATE, params) + return total + + +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 = reclassify(conn, apply=args.apply) + + verb = "re-classified" if args.apply else "would re-classify" + print( + f"{verb} {total} party-ceiling roof override row(s) from an external " + "roof value to the party-ceiling member." + ) + if not args.apply: + print("\nDRY-RUN — nothing written. Re-run with --apply to execute.") + + +if __name__ == "__main__": + main() diff --git a/tests/domain/data_transformation/__init__.py b/tests/domain/data_transformation/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/domain/data_transformation/test_guarded_column_classifier.py b/tests/domain/data_transformation/test_guarded_column_classifier.py new file mode 100644 index 000000000..a6ec38b3a --- /dev/null +++ b/tests/domain/data_transformation/test_guarded_column_classifier.py @@ -0,0 +1,45 @@ +from __future__ import annotations + +from enum import Enum +from typing import Optional + +from domain.data_transformation.column_classifier import ColumnClassifier +from domain.data_transformation.guarded_column_classifier import ( + GuardedColumnClassifier, +) + + +class _Category(Enum): + GUARDED = "guarded" + FALLBACK = "fallback" + UNKNOWN = "unknown" + + +class _RecordingFallback(ColumnClassifier[_Category]): + """Stands in for the LLM: records what it was asked and maps everything it + sees to FALLBACK, so the test can see which descriptions reached it.""" + + def __init__(self) -> None: + self.asked: set[str] = set() + + def classify(self, descriptions: set[str]) -> dict[str, _Category]: + self.asked = set(descriptions) + return {d: _Category.FALLBACK for d in descriptions} + + +def _guard(description: str) -> Optional[_Category]: + return _Category.GUARDED if description == "marker" else None + + +def test_guard_hits_win_and_misses_fall_through_to_the_fallback() -> None: + # Arrange + fallback = _RecordingFallback() + classifier = GuardedColumnClassifier(guard=_guard, fallback=fallback) + + # Act + result = classifier.classify({"marker", "other"}) + + # Assert — the guarded description takes the guard's member and never reaches + # the fallback; the unrecognised one is resolved by the fallback. + assert result == {"marker": _Category.GUARDED, "other": _Category.FALLBACK} + assert fallback.asked == {"other"} diff --git a/tests/domain/epc/test_roof_party_ceiling_guard.py b/tests/domain/epc/test_roof_party_ceiling_guard.py new file mode 100644 index 000000000..8ac91add4 --- /dev/null +++ b/tests/domain/epc/test_roof_party_ceiling_guard.py @@ -0,0 +1,62 @@ +from __future__ import annotations + +import pytest + +from domain.epc.property_overrides.roof_type import RoofType +from domain.epc.property_overrides.roof_party_ceiling_guard import ( + roof_party_ceiling_guard, +) + + +def test_party_ceiling_marker_with_a_trailing_depth_resolves_to_the_party_ceiling_member() -> None: + # Arrange — the bug: a party ceiling ("another dwelling above") carrying a + # trailing loft depth the LLM misreads as an external pitched roof. + description = "another dwelling above: 100mm" + + # Act + result = roof_party_ceiling_guard(description) + + # Assert — it is a party ceiling (~0 heat loss), not a Pitched roof. + assert result is RoofType.ADJACENT_ANOTHER_DWELLING_ABOVE + + +@pytest.mark.parametrize( + ("description", "expected"), + [ + ("same dwelling above: unknown", RoofType.ADJACENT_SAME_DWELLING_ABOVE), + ("other premises above", RoofType.ADJACENT_OTHER_PREMISES_ABOVE), + ("another premises above: 150mm", RoofType.ADJACENT_ANOTHER_PREMISES_ABOVE), + # the unspaced source form and the "Another Premises Above" member value + # both normalise to the same marker. + ("Another Premises Above", RoofType.ADJACENT_ANOTHER_PREMISES_ABOVE), + ("samedwellingabove: 300mm", RoofType.ADJACENT_SAME_DWELLING_ABOVE), + ], +) +def test_every_party_ceiling_marker_variant_resolves_to_its_member( + description: str, expected: RoofType +) -> None: + # Act + result = roof_party_ceiling_guard(description) + + # Assert + assert result is expected + + +@pytest.mark.parametrize( + "description", + [ + "pitched, normal loft access: 100mm", + "flat: 150mm", + "pitched with sloping ceiling: unknown", + "roof room(s), insulated", + ], +) +def test_a_genuine_roof_description_is_left_to_the_llm(description: str) -> None: + # A non-party-ceiling roof must return None so the LLM classifier still + # resolves it — the guard only claims the markers it is certain about. + + # Act + result = roof_party_ceiling_guard(description) + + # Assert + assert result is None diff --git a/tests/scripts/test_reclassify_party_ceiling_roofs.py b/tests/scripts/test_reclassify_party_ceiling_roofs.py new file mode 100644 index 000000000..513e510d5 --- /dev/null +++ b/tests/scripts/test_reclassify_party_ceiling_roofs.py @@ -0,0 +1,33 @@ +"""The party-ceiling roof reclassify corrects only the mis-read rows, to the +guard's member, and is idempotent (#1376).""" + +from __future__ import annotations + +from scripts.reclassify_party_ceiling_roofs import party_ceiling_corrections + + +def test_only_misclassified_party_ceiling_rows_are_corrected() -> None: + # Arrange — a party ceiling mis-read as an external pitched roof (the bug), + # one already on the correct member, and a genuine pitched roof. + stored = [ + ("another dwelling above: 100mm", "Pitched, 100 mm loft insulation"), + ("another dwelling above: unknown", "(another dwelling above)"), + ("pitched, normal loft access: 150mm", "Pitched, 150 mm loft insulation"), + ] + + # Act + corrections = party_ceiling_corrections(stored) + + # Assert — only the mis-read party ceiling is corrected, to its member value; + # the already-correct row and the genuine pitched roof are left alone. + assert corrections == { + "another dwelling above: 100mm": "(another dwelling above)" + } + + +def test_already_corrected_data_yields_no_further_corrections() -> None: + # Arrange — a re-run over data the first pass already fixed. + stored = [("another dwelling above: 100mm", "(another dwelling above)")] + + # Act / Assert — idempotent: nothing left to change. + assert party_ceiling_corrections(stored) == {}