From 9bb57cde57b2976bb62b3dc0a1027e439c7f498c Mon Sep 17 00:00:00 2001 From: Khalim Conn-Kowlessar Date: Tue, 21 Jul 2026 22:31:06 +0000 Subject: [PATCH] =?UTF-8?q?Spec=20the=20room-in-roof=20persistence=20gap?= =?UTF-8?q?=20for=20the=20FE=20migration=20=F0=9F=93=8B?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `SapRoomInRoof.detailed_surfaces` is read by the SAP calculator (heat_transmission.py:590, :690-696, :1311-1470) but has no Postgres column, so the per-surface geometry is silently discarded on save. Without it the cascade abandons the RdSAP 10 §3.10 "Detailed" route — each surface at its own Table 17 / Table 4 U-value — and bills the whole notional roof shell at the Table 18 column (4) age-band default, typically 2.30 W/m2K for pre-1976 stock. Measured on the committed 1000-cert RdSAP-21.0.1 corpus: 108 certs (10.8%) lodge RIR geometry across 118 building parts; 107 of 108 score differently when it is dropped; 15 change EPC band; worst case -15.21 SAP (uprn 10012119141, top-floor maisonette, band E -> F). Larger in both population and magnitude than the rafter_insulation_thickness gap that opened #1656. Doc only — the Drizzle tables are owned by Hestia-Homes/assessment-model and the model-side wiring follows once they exist. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/migrations/epc-room-in-roof-geometry.md | 269 +++++++++++++++++++ 1 file changed, 269 insertions(+) create mode 100644 docs/migrations/epc-room-in-roof-geometry.md diff --git a/docs/migrations/epc-room-in-roof-geometry.md b/docs/migrations/epc-room-in-roof-geometry.md new file mode 100644 index 000000000..c1fe12532 --- /dev/null +++ b/docs/migrations/epc-room-in-roof-geometry.md @@ -0,0 +1,269 @@ +# Migration note — `epc_room_in_roof` + `epc_room_in_roof_surface` + +**For the team to action alongside the model-side change.** +The Python domain + calculator are already correct; the **production Drizzle +schema needs two new child tables** that this repo does not own. Until they +exist, every room-in-roof dwelling is re-modelled on a corrupted fabric picture. + +This is the same defect class as +[`epc-building-part-fabric-overrides.md`](../../../../workspaces/model/docs/migrations/epc-building-part-fabric-overrides.md) +(#1656, `rafter_insulation_thickness`) but **6x the population and 1.5x the +worst-case magnitude**. It is the §2.2 sketch in +[`epc-property-round-trip-fidelity.md`](../../../../workspaces/model/docs/migrations/epc-property-round-trip-fidelity.md), +promoted from **P1** to **P0** on the evidence below. + +--- + +## The bug this closes + +`SapBuildingPart.sap_room_in_roof` (`SapRoomInRoof`) is flattened on save to +exactly **two** columns on `epc_building_part` +(`infrastructure/postgres/epc_property_table.py:663-664`, written at +`:713-716`, reconstructed at `repositories/epc/epc_postgres_repository.py:979-988`): + +| kept | dropped | +|---|---| +| `floor_area` → `room_in_roof_floor_area` (`double precision`) | `detailed_surfaces: List[SapRoomInRoofSurface]` — **the entire §3.10 / §3.9.2 per-surface geometry** | +| `construction_age_band` → `room_in_roof_construction_age_band` (`text`) | `common_wall_length_m`, `common_wall_height_m`, `gable_1_length_m`, `gable_1_height_m`, `gable_2_length_m`, `gable_2_height_m` | + +`detailed_surfaces` is read by the SAP calculator in three places — +`heat_transmission.py:590` (suppresses the Simplified `A_RR` shell), +`heat_transmission.py:690-696` (rooflight-absorption discriminator) and +`heat_transmission.py:1311-1470` (the whole Detailed-RR per-surface billing +loop) — and mapped from the gov-EPC API by +`datatypes/epc/domain/mapper.py:5507-5556` (`_api_build_room_in_roof`). It is +then **silently discarded on save** (*persist != score*). + +### Why dropping it moves SAP so far + +With `detailed_surfaces`, the calculator bills each surface at its own U-value +and routes it to the right worksheet line — slopes/flat ceilings/stud walls to +roof (line 30) at **Table 17** U-values (RdSAP 10 Specification 10-06-2025, +**p.44**), gables to party at **U = 0.25** or to walls at the main-wall U per +**Table 4** (**p.22**), connected walls at **U = 0** — and derives the §3.10.1 +residual roof area (**p.24**). + +Without it, `_part_geometry` falls to the §3.9.1 Simplified Type 1 branch +(`heat_transmission.py:590-591`) and bills the **whole shell** +`A_RR = 12.5 × √(A_RR_floor / 1.5)` (spec §3.9.1(d), **p.22**) at the single +**Table 18 column (4) "Room-in-roof, all elements"** age-band default +(`domain/sap10_ml/rdsap_uvalues.py:1250`, spec **p.45**) — typically +**2.30 W/m²K** for pre-1976 bands. Every party gable that should have billed at +0.25, every connected gable that should have billed at 0, and every measured +insulation thickness is lost, and none of them deduct from the residual. + +So the loss is not a small perturbation of one element — it **switches the +dwelling from the Detailed cascade to the Simplified all-elements default**. + +--- + +## Measured impact + +**Corpus:** `backend/epc_api/json_samples/RdSAP-Schema-21.0.1/corpus.jsonl`, +1,000 certs, 1,000 mapped, 0 skips. No production DB touched. + +**Method** (script: `rir_impact.py` / `rir_v2` in this scratchpad, reproducible): + +1. `EpcPropertyDataMapper.from_api_response(cert)` → `EpcPropertyData`. +2. `Sap10Calculator().calculate(epc).sap_score_continuous` → `sap_with`. +3. Emulate the round-trip loss: for every building part carrying a + `SapRoomInRoof`, replace it with + `SapRoomInRoof(floor_area=..., construction_age_band=...)` — i.e. **exactly** + what `_to_building_part` can reconstruct today. +4. Re-score → `sap_without`. `delta = sap_without − sap_with`. + +### Population + +| measure | count | of 1,000 | +|---|---:|---:| +| certs lodging a room-in-roof | **108** | 10.8% | +| **building parts** lodging a RIR | **118** | — | +| certs whose RIR carries `detailed_surfaces` | **108** | 10.8% (100% of RIR certs) | +| total surfaces that would be dropped | **350** | avg 3.2 per cert | +| certs where the drop moves SAP at all | **107 / 108** | 99.1% | +| certs where the drop moves the **integer** SAP | **72 / 108** | 66.7% | +| certs where the drop moves the **EPC band** | **15 / 108** | 13.9% | +| mean \|Δ SAP\| over the 107 movers | **1.96** | | +| median \|Δ SAP\| | **1.50** | | +| direction | 78 worse, 29 better | | + +Surface-kind distribution across the 350 dropped surfaces: +`gable_wall` 140, `gable_wall_external` 45, `common_wall` 44, `slope` 31, +`gable_wall_sheltered` 27, `stud_wall` 23, `connected_wall` 22, +`flat_ceiling` 18. + +Per-surface field population (of 350): `kind` 350, `area_m2` 350, +`insulation_thickness_mm` 33, `insulation_type` 0, `u_value` 0. + +### Worst 15 certs + +| uprn | dwelling type | lodged SAP | with RIR geom | without | Δ | band | +|---|---|---:|---:|---:|---:|---| +| 10012119141 | Top-floor maisonette | 45 | 45.89 | 30.68 | **−15.21** | E → F | +| 100090399615 | Semi-detached house | 67 | 67.75 | 58.83 | −8.92 | D → E | +| 100040268694 | Mid-terrace house | 58 | 57.97 | 49.16 | −8.81 | D → E | +| 100032223878 | End-terrace bungalow | 11 | 17.67 | 9.04 | −8.62 | G → G | +| 22077224 | Semi-detached bungalow | 37 | 33.90 | 27.14 | −6.76 | F → F | +| 100040259104 | Detached house | 68 | 68.82 | 62.88 | −5.94 | C → D | +| 90077194 | Mid-terrace house | 65 | 63.69 | 58.69 | −5.00 | D → D | +| 100061646773 | Top-floor maisonette | 68 | 63.46 | 58.52 | −4.94 | D → D | +| 100031270375 | Mid-terrace house | 69 | 70.93 | 66.29 | −4.64 | C → D | +| 10009812472 | Detached house | 40 | 39.40 | 35.14 | −4.26 | E → F | +| 100040550095 | Mid-terrace house | 40 | 29.87 | 25.67 | −4.20 | F → F | +| 100031601615 | Mid-terrace house | 73 | 73.01 | 69.12 | −3.89 | C → C | +| 100050549118 | Semi-detached bungalow | 69 | 68.70 | 64.85 | −3.85 | C → D | +| 100050964834 | Semi-detached house | 60 | 62.47 | 58.66 | −3.81 | D → D | +| 100011565830 | Semi-detached house | 72 | 72.64 | 68.84 | −3.80 | C → C | + +Note the **`with RIR geom` column tracks the lodged SAP closely** — across these +15, median |lodged − with| = **0.89**, median |lodged − without| = **5.12**. The +geometry is not a modelling nicety, it is what makes the engine agree with the +accredited assessor. + +The 29 "better" cases are real too, and just as wrong: a dwelling whose gables +are exposed (`gable_wall_external`, billed at the main-wall U, often well above +the Table 18 col (4) default) can score **higher** once its Detailed surfaces are +dropped. Worst positive drift is **+3.31 SAP** (uprn 72351021). Both directions corrupt the baseline against which every recommendation +is priced. + +--- + +## Premise correction — the scalar geometry fields are dormant + +The §2.2 sketch, and the framing of this ticket, treats the six scalar +Type-2 fields (`common_wall_*`, `gable_{1,2}_{length,height}_m`) as co-equal +with `detailed_surfaces`. **They are not.** Corpus measurement: + +| field | populated (of 118 RIR building parts) | +|---|---:| +| `floor_area` | 118 | +| `construction_age_band` | 118 | +| `detailed_surfaces` | **118** | +| `gable_1_length_m` | 80 | +| `gable_2_length_m` | 80 | +| `common_wall_length_m` | **0** | +| `common_wall_height_m` | **0** | +| `gable_1_height_m` | **0** | +| `gable_2_height_m` | **0** | + +`common_wall_length_m`, `common_wall_height_m`, `gable_1_height_m` and +`gable_2_height_m` are **never written by any mapper** — a repo-wide grep for +assignments finds only `mapper.py:5534-5535` (the two `gable_*_length_m`). And +the two lengths that *are* set are themselves inert: `_part_geometry`'s gable +formula (`heat_transmission.py:605-619`) `continue`s unless a matching **height** +is present, which on the API path it never is. The mapper comment at +`mapper.py:5536-5546` says so explicitly ("The scalar `gable_*_length_m` fields +alone can't trigger this ... billing the WHOLE shell as roof"). + +**Verified by control experiment:** stripping only the six scalars while keeping +`detailed_surfaces` produces **Δ = 0.000 on all 108 certs**. The entire measured +impact is attributable to `detailed_surfaces`. + +They should still be persisted — the Elmhurst/Site-Notes ingestion path and the +§3.9.2 Type-2 branch can populate them, and leaving a calculator-read field +without a column is exactly how #1656 happened — but they are **P2 completeness**, +not P0. Ship them in the same migration (they are six nullable floats on a table +we are creating anyway) and note the priority split honestly rather than +inflating the case. + +**Second correction:** "118 of 1,000 corpus **certs**" is a building-part count. +The cert-level figure is **108 of 1,000 (10.8%)** across **118 building parts**. +And "101 of 108 change SAP" is **107 of 108** at continuous resolution, **72 of +108** at integer resolution. The headline −15.21 worst case is confirmed exactly. + +--- + +## Required Drizzle migration + +Additive, two new tables, **no backfill possible** (the values were never +stored). Column-type conventions mirror the existing `epc_building_part` / +`epc_floor_dimension` / `epc_photovoltaic_array` tables. + +### `epc_room_in_roof` — 0..1 per `epc_building_part` + +| Column | Type | Null | Notes | +|---|---|---|---| +| `id` | `serial` PK | no | | +| `epc_building_part_id` | `integer` FK → `epc_building_part.id`, **unique** | no | `SapBuildingPart.sap_room_in_roof` is `Optional[SapRoomInRoof]`, not a list — one row max. Unique constraint enforces it. `ON DELETE CASCADE`. | +| `floor_area` | `double precision` | no | `Union[int, float]`, always populated. **Not** `jsonb` — unlike the code fields, int-vs-float is not semantically load-bearing here (the calculator does `float(rir.floor_area)` at `heat_transmission.py:581`). This matches the existing `room_in_roof_floor_area` column, which the migration **supersedes**. | +| `construction_age_band` | `text` | no | Always populated. Keys Table 17 / Table 18 lookups. | +| `common_wall_length_m` | `double precision` | **yes** | §3.9.2. Null ⇒ no Type-2 common wall; `heat_transmission.py:596` requires **both** length and height non-null before billing `L × (0.25 + H)`. Nullability is the discriminator — a `0.0` default would be wrong but harmless; a `NOT NULL DEFAULT 0` is a silent behaviour change. | +| `common_wall_height_m` | `double precision` | **yes** | As above. Also feeds the gable quadratic correction at `heat_transmission.py:604,612-618`. | +| `gable_1_length_m` | `double precision` | **yes** | §3.9.1/§3.9.2. `heat_transmission.py:609` skips the gable unless length **and** height are non-null. | +| `gable_1_height_m` | `double precision` | **yes** | As above. | +| `gable_2_length_m` | `double precision` | **yes** | As above. | +| `gable_2_height_m` | `double precision` | **yes** | As above. | + +### `epc_room_in_roof_surface` — 0..n per `epc_room_in_roof` + +| Column | Type | Null | Notes | +|---|---|---|---| +| `id` | `serial` PK | no | | +| `epc_room_in_roof_id` | `integer` FK → `epc_room_in_roof.id` | no | `ON DELETE CASCADE`. | +| `surface_index` | `integer` | no | **Ordering column — required.** `detailed_surfaces` is a `List`; without a stable order the round-trip deep-equality test cannot pass and the reload order is whatever Postgres returns. Same convention as `epc_photovoltaic_array.array_index` (`epc_property_table.py:844`). Unique together with `epc_room_in_roof_id`. | +| `kind` | `text` | no | One of `slope`, `flat_ceiling`, `stud_wall`, `gable_wall`, `gable_wall_external`, `gable_wall_sheltered`, `common_wall`, `connected_wall`. **Do not** make this a Postgres enum — the domain types it as `str` and the set has grown twice already (`gable_wall_sheltered`, `connected_wall`). A CHECK constraint would turn a future mapper addition into a save-time 500. | +| `area_m2` | `double precision` | no | **Can be negative** — a §3.9.2 step (b) absent-gable adjustment produces a signed area that deducts from the A_RR residual (`heat_transmission.py:1362-1369`, `:1377-1389`). **No CHECK ≥ 0.** | +| `insulation_thickness_mm` | `integer` | **yes** | Null ⇒ fall back to Table 18 col (4) age-band default (`rdsap_uvalues.py:1176-1177`); non-null ⇒ Table 17 measured row. **The null-vs-zero distinction is the U-value branch** (spec p.44: the `none` row is 2.3, but 12 mm is 1.5). A `DEFAULT 0` would silently bill every measured roof at the uninsulated value. | +| `insulation_type` | `text` | **yes** | `mineral_wool` / `eps` / `pur` / `pir`. Selects Table 17 sub-column (a) mineral-wool/EPS vs (b) PUR/PIR (`rdsap_uvalues.py:1155-1161`). Null ⇒ column (a). 0/350 populated in the current API corpus but set by the Elmhurst/Site-Notes path. | +| `u_value` | `double precision` | **yes** | Assessor-lodged U override (W/m²K). Read at `heat_transmission.py:1385` (`gable_wall_external`), `:1400` (`gable_wall_sheltered`), `:1416` (`common_wall`). Null ⇒ fall through to the Table 4 cascade (main-wall U / sheltered formula). **Null is meaningful — it is not "unknown, assume 0".** | + +### Supersession of the flattened columns + +`epc_building_part.room_in_roof_floor_area` and +`room_in_roof_construction_age_band` become redundant. **Do not drop them in +this migration.** Land the new tables additively, deploy the backend, verify the +round-trip, then drop the two old columns in a follow-up. Dropping them in the +same migration makes the deploy non-reversible. + +Generate with **`npm run migration:generate`** — do **not** hand-write the +snapshot metadata. Run the migration **before** deploying the backend code. + +--- + +## Backfill / re-ingest + +The columns never existed, so **existing rows have nothing to restore**. The +migration alone changes no baseline. Every affected property must be +**re-ingested from the gov-EPC API** (the raw JSON is still in `epc_cache`, so +the source is recoverable) before its score moves. + +Expected blast radius: **~11% of the estate** — any dwelling with a room in +roof. Re-run **Baseline (stage 2) before Modelling (stage 3)** so both stages +score on one calculator version, otherwise a portfolio will show a fabricated +~2-point improvement that is really just the baseline being corrected. + +--- + +## Model-side changes required (not yet done) + +- `infrastructure/postgres/epc_property_table.py` — new + `EpcRoomInRoofModel` and `EpcRoomInRoofSurfaceModel` SQLModels following the + `EpcPhotovoltaicArrayModel` shape (`:833-863`), each with a `from_domain` + classmethod. Remove the `rir` flattening from + `EpcBuildingPartModel.from_domain` (`:684`, `:713-716`) once the old columns + are dropped. +- `repositories/epc/epc_postgres_repository.py` — save the RIR + surface rows in + the building-part save loop; reconstruct in `_to_building_part`, replacing the + two-field rebuild at `:979-988`. Surfaces must be selected + `ORDER BY surface_index`. +- `tests/repositories/epc/test_epc_round_trip.py` — a + `test_room_in_roof_detailed_surfaces_round_trip` asserting deep-equality, + including a **negative `area_m2`** surface and a **null vs 0** + `insulation_thickness_mm` pair. +- `tests/repositories/epc/test_epc_persistence_field_coverage.py` — remove the + room-in-roof `_UNPERSISTED_ALLOWLIST` entry so the ADR-0036 structural guard + *enforces* reconstruction. +- `docs/migrations/epc-property-round-trip-fidelity.md` — §2.2 marked done; + the "Still open" list at `:61-66` updated. + +## Why the structural guard missed it + +The ADR-0036 guard recurses into nested dataclasses (that is how the PV-array +and floor-flag drops were caught). `SapRoomInRoof` is on the +`_UNPERSISTED_ALLOWLIST` with a *correct* justification — "no FE column" — which +is precisely the problem: an accurate allow-list entry with no attached +**severity** reads as "known and accepted" rather than "known and costing 15 SAP +points". The #1656 note already proposes making the justification +machine-checkable. This case argues for a second field: an allow-list entry for a +**calculator-read** attribute should be required to carry a measured corpus +impact, so "we know" and "we've decided it's fine" stop looking identical.