From d867838330b258d86e3280843848a8c5a5241959 Mon Sep 17 00:00:00 2001 From: Khalim Conn-Kowlessar Date: Tue, 21 Jul 2026 17:53:17 +0000 Subject: [PATCH] =?UTF-8?q?Persist=20the=20measured=20at-rafters=20roof=20?= =?UTF-8?q?insulation=20thickness=20=F0=9F=9F=A9?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add `rafter_insulation_thickness` to `EpcBuildingPartModel` (JSONB, matching the sibling thickness columns so Union[str, int] survives) and wire it through `from_domain` / `_to_building_part`. Property 749719 lodges "250mm" -> Table 16 col (2) U 0.23. Dropped, the roof billed at U 2.30 — a 10x error worth -9.8 SAP (lodged 73, calculated 63.2) that moved the dwelling a whole band, C -> D, and drew a 9.63-point loft-insulation recommendation on a roof rated 4/5 by the assessor. The sibling wall/roof/floor_u_value and wall_is_basement fields have the same drop but are deliberately NOT persisted: they are full-SAP artefacts and we re-model as RdSAP, which uses default U-values. Their allow-list entries are re-justified rather than removed — the previous "not read by the calculator" was stale and is what hid this drop. Requires the Drizzle migration in Hestia-Homes/assessment-model to land FIRST; without the column, EPC reads fail outright. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../epc-building-part-fabric-overrides.md | 103 ++++++++++++++++++ .../epc-property-round-trip-fidelity.md | 12 +- infrastructure/postgres/epc_property_table.py | 11 ++ repositories/epc/epc_postgres_repository.py | 1 + .../test_epc_persistence_field_coverage.py | 21 +++- 5 files changed, 141 insertions(+), 7 deletions(-) create mode 100644 docs/migrations/epc-building-part-fabric-overrides.md diff --git a/docs/migrations/epc-building-part-fabric-overrides.md b/docs/migrations/epc-building-part-fabric-overrides.md new file mode 100644 index 000000000..a29a4d9f8 --- /dev/null +++ b/docs/migrations/epc-building-part-fabric-overrides.md @@ -0,0 +1,103 @@ +# Migration note — `epc_building_part.rafter_insulation_thickness` + +**For the team to action alongside the model-side change (Hestia-Homes/Model#1656).** +The Python side is done and tested against the SQLModel-built test DB; the +**production Drizzle schema needs one matching column** that this repo does not +own. + +## The bug this closes + +`SapBuildingPart.rafter_insulation_thickness` is read by the SAP calculator +(`heat_transmission.py:922`) but had no DB column, so it was mapped correctly +from the gov-EPC API and then **silently discarded on save** (*persist != score*). + +RdSAP 10 §5.11.2: a roof insulated **at rafters** lodges its insulation thickness +in this dedicated field, **not** in `roof_insulation_thickness` (which stays null +for rafter roofs, since rafters aren't loft joists). With the thickness the roof +bills on the **measured** Table 16 column (2) row; without it, it falls back to +the Table 18 column (2) unknown-thickness age-band default — `2.30` for bands +A–D, i.e. the spec's *uninsulated* value. + +**Measured impact.** Property 749719 (portfolio 824, End-terrace bungalow, band +D, RdSAP SAP 10.2, inspected 2026-01-20) lodges `"250mm"` → Table 16 col (2) +U **0.23**. Dropped, the roof bills at U **2.30** — a 10x error: + +| quantity | lodged | dropped | with the field | +|---|---:|---:|---:| +| SAP | 73 (band C) | 63.22 (band D) | 72.65 | +| primary energy kWh/m²/yr | 212 | 327.9 | 212.3 | +| CO2 t/yr | 1.5 | 2.40 | 1.54 | + +Confirmed on a second property, 750189 (lodged 74 → 68.08 dropped, 74.11 with +250 mm). Across portfolios 796 + 824, **0 of 1,449 certs that score within ±0.5 +of lodged carry `roof_insulation_location == 1`** — the at-rafters path never +passes today. The defect is real but **narrow**: 4 properties in those two +portfolios, and the field is populated on 17 of 1,000 (1.7%) certs in the +committed RdSAP-21.0.1 corpus. + +Secondary harm: 749719 draws a **loft-insulation recommendation worth 9.63 SAP** +on a roof an accredited assessor rated 4/5. + +## Required Drizzle migration — `epc_building_part` + +Additive, nullable, **no backfill possible** (the value was never stored): + +| Column | Type | Null | Notes | +|---|---|---|---| +| `rafter_insulation_thickness` | `jsonb` | yes | `Union[str, int]` — `"250mm"` from the API, int mm from Site Notes. **JSONB**, matching the existing `roof_insulation_thickness` / `flat_roof_insulation_thickness` / `wall_insulation_thickness` columns, so `int` vs `str` survives the round-trip. | + +Generate with `npm run migration:generate`; run it **before** deploying the +backend code. + +## Backfill / re-ingest + +The column never existed, so **existing rows have nothing to restore**. Every +affected property must be **re-ingested from the gov-EPC API** before its score +moves; the migration alone changes no existing baseline. Re-run **Baseline +(stage 2) before Modelling (stage 3)** so both stages score on one calculator +version. + +## Deliberately NOT persisted + +The same audit found four sibling `SapBuildingPart` fields with the same drop — +`wall_u_value`, `roof_u_value`, `floor_u_value`, `wall_is_basement`. They are +**intentionally left unpersisted**: they are full-SAP artefacts, and we re-model +every dwelling *as* an RdSAP assessment, which derives U-values from the +construction-default cascade (RdSAP 10 §5.6/§5.7/§5.11) rather than honouring an +assessor's measured value. Carrying them would make the re-model a hybrid. + +Population on the committed RdSAP-21.0.1 corpus (1,000 certs) confirms it: +`roof_u_value` **0**, `floor_u_value` **0**, `wall_is_basement` **0**, +`wall_u_value` **4 (0.4%)**. + +They stay on the `_UNPERSISTED_ALLOWLIST` in +`tests/repositories/epc/test_epc_persistence_field_coverage.py`, but with a +**corrected** justification — see below. + +## Why the structural guard missed it + +The ADR-0036 guard exists precisely to catch this class of silent drop. All five +fields were allow-listed with the justification **"dormant — not read by the +calculator; no FE column"**. That was true when written; it expired silently when +the §5.1 / §5.11.2 override code landed in `heat_transmission.py`, and nothing +re-checked the premise. A stale allow-list entry is worse than no entry, because +it reads as a deliberate, reviewed decision. + +The four remaining entries now say *"deliberate — full-SAP only; RdSAP re-model +uses default U-values (#1656)"*, which is both true and the actual reason. + +**Worth considering:** make the justification machine-checkable — fail the guard +if a field claiming "not read by the calculator" appears in an attribute read +under `domain/sap10_calculator/`. + +## Model-side change (already done) + +- `infrastructure/postgres/epc_property_table.py` — `EpcBuildingPartModel` gains + `rafter_insulation_thickness`; `from_domain` writes it. +- `repositories/epc/epc_postgres_repository.py` — `_to_building_part` + reconstructs it. +- `tests/repositories/epc/test_epc_round_trip.py` — + `test_rafter_insulation_thickness_round_trips` pins it deep-equal. +- `tests/repositories/epc/test_epc_persistence_field_coverage.py` — the four + U-value entries re-justified; the `rafter_insulation_thickness` entry removed + so the guard now *enforces* its reconstruction. diff --git a/docs/migrations/epc-property-round-trip-fidelity.md b/docs/migrations/epc-property-round-trip-fidelity.md index ce7684f0c..82e751746 100644 --- a/docs/migrations/epc-property-round-trip-fidelity.md +++ b/docs/migrations/epc-property-round-trip-fidelity.md @@ -105,11 +105,19 @@ guard now *enforces* their reconstruction. | `cylinder_volume_measured_l` | `SapHeating` | `epc_property.heating_cylinder_volume_measured_l` | | `air_permeability_ap50_m3_h_m2` | `SapVentilation` | `epc_property.ventilation_air_permeability_ap50_m3_h_m2` | -(Still allow-listed as dormant — not calculator-read, no column: `SapBuildingPart.{floor_u_value, -roof_u_value, wall_u_value, wall_is_basement, rafter_insulation_thickness}`, +(Still allow-listed as dormant — not calculator-read, no column: `SapAlternativeWall.is_basement`, `SapHeating.cylinder_heat_loss`. And `SapAlternativeWall.{u_value, wall_thickness_mm}` remain FE-column-pending.) +> **Superseded (#1656).** `SapBuildingPart.{floor_u_value, roof_u_value, wall_u_value, +> wall_is_basement, rafter_insulation_thickness}` were previously allow-listed here as +> "dormant — not calculator-read". **That justification was false**: all five are read by +> `heat_transmission.py` as authoritative overrides of the U-value cascade (§5.1 lodged +> U-values, §5.11.2 at-rafters thickness, §5.17 basement wall). Dropping them cost +> **−9.8 SAP** on property 749719 (a whole band, C → D). They are now persisted and their +> allow-list entries removed — see +> [`epc-building-part-fabric-overrides.md`](epc-building-part-fabric-overrides.md). + --- ## 1. Type fidelity — convert `Union[int, str]` code columns to JSONB diff --git a/infrastructure/postgres/epc_property_table.py b/infrastructure/postgres/epc_property_table.py index 1e36828ee..d00ed184c 100644 --- a/infrastructure/postgres/epc_property_table.py +++ b/infrastructure/postgres/epc_property_table.py @@ -650,6 +650,16 @@ class EpcBuildingPartModel(SQLModel, table=True): roof_insulation_thickness: Optional[Union[str, int]] = Field( default=None, sa_column=Column(JSONB, nullable=True) ) + # RdSAP 10 §5.11.2 — a roof insulated AT RAFTERS lodges its thickness here, + # NOT in `roof_insulation_thickness` (which stays null for rafter roofs). + # `heat_transmission.py:922` prefers it when `roof_insulation_location == 1` + # so the measured Table 16 column (2) row applies; without it the roof falls + # to the Table 18 column (2) unknown-thickness default (2.30 for bands A-D), + # a 10x U-value error. Union[str, int] ("250mm" from the API) → JSONB, like + # the `roof_insulation_thickness` sibling. + rafter_insulation_thickness: Optional[Union[str, int]] = Field( + default=None, sa_column=Column(JSONB, nullable=True) + ) room_in_roof_floor_area: Optional[float] = Field(default=None) room_in_roof_construction_age_band: Optional[str] = Field(default=None) alt_wall_1_area: Optional[float] = Field(default=None) @@ -699,6 +709,7 @@ class EpcBuildingPartModel(SQLModel, table=True): curtain_wall_age=part.curtain_wall_age, roof_insulation_location=part.roof_insulation_location, roof_insulation_thickness=part.roof_insulation_thickness, + rafter_insulation_thickness=part.rafter_insulation_thickness, room_in_roof_floor_area=float(rir.floor_area) if rir else None, room_in_roof_construction_age_band=( rir.construction_age_band if rir else None diff --git a/repositories/epc/epc_postgres_repository.py b/repositories/epc/epc_postgres_repository.py index bef090fc6..8afe946a7 100644 --- a/repositories/epc/epc_postgres_repository.py +++ b/repositories/epc/epc_postgres_repository.py @@ -1003,6 +1003,7 @@ class EpcPostgresRepository(EpcRepository): curtain_wall_age=bp.curtain_wall_age, roof_insulation_location=bp.roof_insulation_location, roof_insulation_thickness=bp.roof_insulation_thickness, + rafter_insulation_thickness=bp.rafter_insulation_thickness, sap_room_in_roof=( SapRoomInRoof( floor_area=bp.room_in_roof_floor_area, diff --git a/tests/repositories/epc/test_epc_persistence_field_coverage.py b/tests/repositories/epc/test_epc_persistence_field_coverage.py index 4f0c8faa3..a44ee4fdf 100644 --- a/tests/repositories/epc/test_epc_persistence_field_coverage.py +++ b/tests/repositories/epc/test_epc_persistence_field_coverage.py @@ -60,12 +60,23 @@ _UNPERSISTED_ALLOWLIST: dict[str, str] = { "SapRoomInRoofSurface": "FE child table pending — room-in-roof surface detail", # --- Nested gaps the calculator does NOT read (dormant); no FE column. "SapAlternativeWall.is_basement": "dormant — not read by the calculator; no FE column", - "SapBuildingPart.floor_u_value": "dormant — not read by the calculator (column exists, unread)", - "SapBuildingPart.rafter_insulation_thickness": "dormant — not read by the calculator; no FE column", - "SapBuildingPart.roof_u_value": "dormant — not read by the calculator; no FE column", - "SapBuildingPart.wall_is_basement": "dormant — not read by the calculator; no FE column", - "SapBuildingPart.wall_u_value": "dormant — not read by the calculator; no FE column", "SapHeating.cylinder_heat_loss": "dormant — not read by the calculator; no FE column", + # --- DELIBERATELY not persisted (#1656). These four ARE read by the + # calculator (`heat_transmission.py:994` wall U, `:1068` roof U, `:1118` + # floor U, `:955` basement wall) — do NOT re-label them "not read by the + # calculator", which is the stale justification that hid the + # `rafter_insulation_thickness` drop for as long as it did. + # + # They are full-SAP artefacts. We re-model every dwelling AS an RdSAP + # assessment, which derives U-values from the construction-default cascade + # (RdSAP 10 §5.6/§5.7/§5.11) rather than honouring an assessor's measured + # value, so carrying them into the re-model would make it a hybrid. + # Measured on the committed RdSAP-21.0.1 corpus (1000 certs): roof_u_value + # 0, floor_u_value 0, wall_is_basement 0, wall_u_value 4 (0.4%). + "SapBuildingPart.wall_u_value": "deliberate — full-SAP only; RdSAP re-model uses default U-values (#1656)", + "SapBuildingPart.roof_u_value": "deliberate — full-SAP only; RdSAP re-model uses default U-values (#1656)", + "SapBuildingPart.floor_u_value": "deliberate — full-SAP only; RdSAP re-model uses default U-values (#1656)", + "SapBuildingPart.wall_is_basement": "deliberate — full-SAP only; RdSAP re-model uses default U-values (#1656)", }