docs(plan): track H task 11 reads units, plan copies, scale and rooms once
The reconcile pass left one real defect for the reviewer. Groups U and N are built side by side, so the takeoffs carry their own unit bands, cluster grid, scale statistic and room faces, and nothing ever retires them. Two readers of one drawing can disagree. On Task 6's own inches-over-millimetres layout, infer_units without room faces ties three ways and falls back to the header, while the takeoff's bands say mm. Task 11 gains Steps 15-20, committed on their own before the README figures are retaken: - describe.drawing_units becomes the one unit call, used by describe, scale_check and the takeoffs; - the takeoffs read plan copies, scale and rooms through group U's modules; - the stand-ins are deleted; - tests/test_understand_one_reading.py pins one unit, one plan copy, one verdict and one set of rooms per drawing. Tasks 3, 4 and 6 now point at those steps. Review Focus 1 names parse_diameters, the function every task uses. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FKFStjEFzpGyLq5bMpWxTd
This commit is contained in:
Umut CAN
co-authored by
Claude Opus 5.5
Claude Opus 5.5
parent
7f82766d38
commit
9b45243bdd
@@ -65,7 +65,7 @@ Each line's test lives in the owning task.
|
||||
|
||||
1. **AutoCAD text codes in labels** — `%%c51` (AutoCAD's Ø), `\U+2205`,
|
||||
`⌀51`, `ø51` and MTEXT format runs (`{\fArial|b0;Ø51}`, `\P`) must all parse
|
||||
as the diameter `Ø51`. *Owner: Task 1 (`labels.parse_diameter`).*
|
||||
as the diameter `Ø51`. *Owner: Task 1 (`labels.parse_diameters`).*
|
||||
2. **Cyrillic look-alike letters in tags** — a Russian-labelled P&ID types
|
||||
`Т4100` or `М82` with Cyrillic Т/М; it must match the layout's Latin
|
||||
`T4100` / `M82`. *Owner: Task 1 (`labels.parse_tag`).*
|
||||
@@ -3683,7 +3683,7 @@ MSG
|
||||
- Produces:
|
||||
- `engineering/understand/units.py`: `MM_PER_UNIT: dict[str, float]`; `entity_box(rec: EntityRecord) -> tuple[float, float, float, float] | None`; `robust_extents(records: Sequence[EntityRecord]) -> dict` (`computed`, `robust`, `outliers: [{handle, type, layer, at, distance, confidence}]` farthest first, `stretch`, `method`); `wall_segments(records, *, is_wall=None) -> tuple[list[tuple[Pt, Pt]], int]`; `infer_units(snap: Snapshot, *, records=None, room_areas=()) -> dict` (`declared: {code, name, label}`, `inferred: str | None`, `mm_per_unit: float | None`, `agree: bool`, `confidence`, `scores`, `evidence`, `warning: str | None`).
|
||||
- `engineering/understand/clusters.py`: `find_clusters(records, *, exclude=(), cell=None) -> tuple[list[dict], dict[str, str]]` — rows `{id ("C1", "C2", … largest first), box, size, entities, layers, labels}` and handle → cluster id.
|
||||
- `engineering/understand/describe.py`: `describe(snap: Snapshot) -> dict` with the skeleton's keys `units`, `extents`, `clusters`, `layers`, `equipment_tags`, `rooms`, `languages`, `blocks`, `layouts`, `title_block`, `warnings` (plus `source`, `entities`, `room_faces`); `prepare(snap: Snapshot) -> dict` (`model`, `extents`, `kept`, `clusters`, `membership`, `diag`); `tag_occurrences(snap: Snapshot, *, prep: dict | None = None) -> list[dict]` (`tag`, `handle`, `at`, `space`, `layer`, `cluster`, `source`, `confidence`); `find_rooms(records, *, tol: float, membership: dict) -> dict` (`rooms`, `labelled: [(row, Face)]`, `faces`, `faces_unlabelled`, `wall_segments`, `arcs_skipped`, `skipped`); `text_language(text: str) -> str`; `FACE_TOL_FRACTION`. Task 4 imports `prepare`, `tag_occurrences`, `find_rooms` and `FACE_TOL_FRACTION`. The takeoffs (Task 6, group N) do not import them: group N is cut from `main` beside this branch, so `takeoff.py` carries its own narrow `unit_of` / `room_regions` (Task 6, "Why this module carries stand-ins").
|
||||
- `engineering/understand/describe.py`: `describe(snap: Snapshot) -> dict` with the skeleton's keys `units`, `extents`, `clusters`, `layers`, `equipment_tags`, `rooms`, `languages`, `blocks`, `layouts`, `title_block`, `warnings` (plus `source`, `entities`, `room_faces`); `prepare(snap: Snapshot) -> dict` (`model`, `extents`, `kept`, `clusters`, `membership`, `diag`); `tag_occurrences(snap: Snapshot, *, prep: dict | None = None) -> list[dict]` (`tag`, `handle`, `at`, `space`, `layer`, `cluster`, `source`, `confidence`); `find_rooms(records, *, tol: float, membership: dict) -> dict` (`rooms`, `labelled: [(row, Face)]`, `faces`, `faces_unlabelled`, `wall_segments`, `arcs_skipped`, `skipped`); `text_language(text: str) -> str`; `FACE_TOL_FRACTION`. Task 4 imports `prepare`, `tag_occurrences`, `find_rooms` and `FACE_TOL_FRACTION`. The takeoffs (Task 6, group N) do not import them: group N is cut from `main` beside this branch, so `takeoff.py` carries its own narrow `unit_of` / `room_regions` (Task 6, "Why this module carries stand-ins") until Task 11 (Steps 15-20) re-points them at these readers.
|
||||
- MCP tool `drawing_understand(path: str | None = None) -> dict` (core pack, read-only, tags `analysis` + `query`).
|
||||
|
||||
- [ ] **Step 1: Write the failing units test**
|
||||
@@ -5738,7 +5738,7 @@ MSG
|
||||
- Produces:
|
||||
- `engineering/understand/scale.py`: `match_tags(a: Snapshot, b: Snapshot, *, cluster_a=None, cluster_b=None) -> dict` — `{"matched": {tag: (pa, pb)}, "ambiguous": [tag, …], "occurrences": {tag: {"a": n, "b": m}}, "only_a": [...], "only_b": [...], "cluster_a", "cluster_b"}`; a cluster id is one of `find_clusters`' `"C1"`, `"C2"`, …, None is the whole model space, and an unknown id is refused with the ids the drawing has.
|
||||
- `scale_check(a: Snapshot, b: Snapshot, *, min_pair_mm=2000.0) -> dict` — `{"verdict": "to_scale"|"schematic"|"partly"|"insufficient", "factor", "median", "q1", "q3", "iqr", "within_10pct", "pair_count", "pairs", "pairs_truncated", "pairs_below_floor", "matched": [{tag, a, b}], "ambiguous", "occurrences", "only_a", "only_b", "groups": [{group, kind, tags, verdict, factor, median, q1, q3, iqr, within_10pct, pair_count}], "scope": {"cluster_a", "cluster_b"}, "units": {"a", "b"}, "min_pair_mm", "thresholds", "notes"}`; `b` is the reference.
|
||||
- Constants `WITHIN = 0.10`, `TO_SCALE_SHARE = 0.80`, `SCHEMATIC_SHARE = 0.50`, `MIN_TAGS = 3` (spec §6). Task 6 (group N, cut beside this branch) cannot import this module: `pipe_rows(..., scale=...)` accepts a `scale_check(pid, layout)` result in this shape (Task 12's benchmark, smoke and field-test script pass one) and otherwise computes its own `scale_stand_in`; its layout tag positions come from its own `layout_tags`, which applies the same rule — tags unique within the chosen cluster only, the rest `ambiguous` — so Review Focus 3 is reused as a rule, not as an import.
|
||||
- Constants `WITHIN = 0.10`, `TO_SCALE_SHARE = 0.80`, `SCHEMATIC_SHARE = 0.50`, `MIN_TAGS = 3` (spec §6). Task 6 (group N, cut beside this branch) cannot import this module: `pipe_rows(..., scale=...)` accepts a `scale_check(pid, layout)` result in this shape (Task 12's benchmark, smoke and field-test script pass one) and otherwise computes its own `scale_stand_in` until Task 11 (Steps 15-20) replaces it with this function; its layout tag positions come from its own `layout_tags`, which applies the same rule — tags unique within the chosen cluster only, the rest `ambiguous` — so Review Focus 3 is reused as a rule, not as an import.
|
||||
- MCP tool `drawing_scale_check(reference: str, path: str | None = None, min_pair_mm: float = 2000.0) -> dict` (core pack, read-only, tags `analysis` + `query`).
|
||||
|
||||
- [ ] **Step 1: Write the failing scale test**
|
||||
@@ -7915,7 +7915,7 @@ MSG
|
||||
- Consumes: Task 5 — `build_network(...) -> dict` (its `runs`, `attachments` and `stats.tol` / `stats.label_search` / `stats.overlap_length`), `Run`, `tag_occurrences(snap)`, `MODEL`, `TEXT_TYPES`; Task 1 — `Pt`, `Snapshot`, `read_snapshot`, `vocab.SERVICES`, `vocab.classify_layer`, `vocab.room_label(text) -> dict | None`; track F — `engineering.arch.faces.planar_faces`, `face_containing`, `Face`, `DEFAULT_TOL` (reused, not re-implemented); Task 2 — `build_plant_pair` truth keys `pid`, `layout`, `scale_verdict`, `layout_insunits`, `layout_true_unit`, `pipe_runs`, `rooms`. A `scale` argument in the shape of group U's `scale_check` result (`verdict`, `factor`, `median`, `iqr`, `within_10pct`, `pairs`, `matched`, `ambiguous`, `groups`) is consumed when a caller passes one.
|
||||
- Produces: in `engineering/understand/route.py` — `manhattan(a: Pt, b: Pt) -> float`, `rmst(points: Sequence[Pt]) -> tuple[float, tuple[tuple[int, int], ...]]`, `route_legs(a: Pt, b: Pt) -> tuple[tuple[Pt, Pt], ...]`. In `engineering/understand/takeoff.py` — `pipe_rows(network, pid: Snapshot, layout: Snapshot | None, *, scale=None, services=DEFAULT_SERVICES, allowance=0.20, vertical_allowance=0.0, length_source="auto", force=False) -> dict` (keys `kind`, `rows`, `runs`, `control`, `summary{by_service, by_diameter, by_room}`, `totals{routed_m, unroutable_m, net_m, allowance, allowance_m, total_m, flagged_runs}`, `length_source`, `scale`, `scale_verified`, `units{pid, layout}`, `warnings`, `method`); `DEFAULT_SERVICES = ("product", "cip_supply", "cip_return")`, `LENGTH_SOURCES`, `UNIT_METRES`; `check_pipe_options(*, services, allowance, vertical_allowance, length_source) -> dict`; `check_allowance(allowance) -> float`; `unit_of(snap) -> dict` (`declared`, `inferred`, `m_per_unit`, `warning`, `evidence`); `layout_tags(snap, wanted=()) -> dict` (`positions`, `ambiguous`, `cluster`, `clusters`); `scale_stand_in(pid, layout, *, pid_tags=None, layout_found=None, layout_unit=None) -> dict` (the subset of group U's `scale_check` shape a takeoff reads: `verdict`, `factor` (median, only when `to_scale`), `median`, `q1`, `q3`, `iqr`, `within_10pct`, `pair_count`, `pairs[{tags, a_mm, b_mm, ratio}]`, `matched[{tag, a, b}]`, `ambiguous`, `groups: []`, `notes`); `room_regions(snap, cluster=None) -> dict` (`faces: [(key, Face)]`, `labels: [(key, Pt)]`); `room_at(regions, p) -> str`; `apportion(legs, regions) -> dict[str, float]`.
|
||||
|
||||
**Why this module carries stand-ins.** Group N is cut from `main` beside groups U and D (skeleton, Execution), so `engineering/understand/{units,clusters,scale}.py` do not exist on this branch. `takeoff.py` therefore carries the narrow form of each that a takeoff needs — `unit_of`, `layout_tags` with its plan-copy clusters, and `scale_stand_in` (spec §6's statistics without per-room groups) — and `pipe_rows` uses a `scale` result it is given before its own. Tag positions on the layout always come from `layout_tags`, which applies Review Focus item 3's rule (only tags unique inside the chosen cluster; the rest `ambiguous`, never picked). The stand-in returns group U's result shape — the ratio is P&ID millimetres over layout millimetres, each drawing in its inferred unit, and `factor` is set only when the verdict is `to_scale` — so after the merge a caller can pass `scale_check(pid, layout)` into `pipe_rows(scale=...)` without touching a row — Task 12's benchmark task, correctness checks, smoke and field-test script do exactly that. No task of this plan swaps `unit_of` / `room_regions` for group U's `infer_units` / `find_rooms`; the stand-ins stay in `takeoff.py`, and the `pipe_takeoff` tool keeps computing its own `scale_stand_in`. The unit bands (text 0.5 mm – 1 m, span 0.1 m – 5 km) and the 2 % cluster gap are this module's heuristics, stated in its docstring — not standards values. Review Focus item 5 is owned here: `test_review_focus_5_inches_declared_over_millimetres` (hand-built) and `test_review_focus_5_the_synthetic_layout_is_read_in_millimetres` (the Task 2 pair).
|
||||
**Why this module carries stand-ins.** Group N is cut from `main` beside groups U and D (skeleton, Execution), so `engineering/understand/{units,clusters,scale}.py` do not exist on this branch. `takeoff.py` therefore carries the narrow form of each that a takeoff needs — `unit_of`, `layout_tags` with its plan-copy clusters, and `scale_stand_in` (spec §6's statistics without per-room groups) — and `pipe_rows` uses a `scale` result it is given before its own. Tag positions on the layout always come from `layout_tags`, which applies Review Focus item 3's rule (only tags unique inside the chosen cluster; the rest `ambiguous`, never picked). The stand-in returns group U's result shape — the ratio is P&ID millimetres over layout millimetres, each drawing in its inferred unit, and `factor` is set only when the verdict is `to_scale` — so after the merge a caller can pass `scale_check(pid, layout)` into `pipe_rows(scale=...)` without touching a row — Task 12's benchmark task, correctness checks, smoke and field-test script do exactly that. The stand-ins are debt with a due date: on main, Task 11 (Steps 15-20) re-points `unit_of`, `layout_tags` and `room_regions` at group U's readers and deletes `scale_stand_in`, so the four tools read one unit, one plan copy, one verdict and one set of rooms per drawing. Until then they must follow spec §5 / §6 closely enough that the swap changes no behavioural assertion of this task. The unit bands (text 0.5 mm – 1 m, span 0.1 m – 5 km) and the 2 % cluster gap are this module's heuristics, stated in its docstring — not standards values. Review Focus item 5 is owned here: `test_review_focus_5_inches_declared_over_millimetres` (hand-built) and `test_review_focus_5_the_synthetic_layout_is_read_in_millimetres` (the Task 2 pair).
|
||||
|
||||
- [ ] **Step 1: Write the failing route test**
|
||||
|
||||
@@ -15362,11 +15362,14 @@ MSG
|
||||
- Modify: `tests/test_tool_registry.py` (frozen group-size snapshot: 247 tools, 28 groups)
|
||||
- Modify: `docs/tool-inventory.json` (regenerated, never hand-edited)
|
||||
- Modify: `README.md` (release snapshot tools / advertised / collected tests, the alias-corpus sentence, the Discovery row, the `<sub>` pack line, the `TOOL_PROFILE` and `TOOL_PACKS` configuration rows)
|
||||
- Test: `tests/test_tool_packs.py`, `tests/test_discovery_aliases.py`, `tests/test_discovery_transform.py`, `tests/test_tool_registry.py`, `tests/test_token_suite.py`
|
||||
- Modify: `engineering/understand/describe.py` (`drawing_units`), `engineering/understand/scale.py` (units through `drawing_units`), `engineering/understand/takeoff.py` (the stand-ins give way to group U's readers)
|
||||
- Create: `tests/test_understand_one_reading.py`
|
||||
- Modify: `tests/test_understand_takeoff.py`, `tests/test_understand_report.py` (the assertions that pinned the stand-ins)
|
||||
- Test: `tests/test_tool_packs.py`, `tests/test_discovery_aliases.py`, `tests/test_discovery_transform.py`, `tests/test_tool_registry.py`, `tests/test_token_suite.py`, `tests/test_understand_one_reading.py`
|
||||
|
||||
**Interfaces:**
|
||||
- Consumes: the six tools the groups registered — SECTION 26 `drawing_understand`, `drawing_scale_check` (group U, Tasks 3-4, tagged `{"analysis", "query"}`), `drawing_diff`, `drawing_topology_check` (group D, Tasks 9-10, tagged `{"analysis", "query"}`); SECTION 27 `pipe_takeoff`, `cable_takeoff` (group N, Tasks 7-8, tagged `{"plant", "analysis"}`) — each with its alias record in `discovery/aliases.py` (every one `acad=()` except `drawing_diff`'s `("COMPARE",)`), and each branch's paragraph in the `test_tool_group_sizes_are_unchanged` docstring saying the six filed under `analysis` while `understand` / `plant` were unranked; `server._registered_tools()`, `server._tool_groups()`, `server._apply_tool_profile(profile) -> {"disabled_tools", "enabled_count", "tool_packs"}`; `discovery.aliases.TOOL_ALIASES`, `ToolAliases(acad=(), synonyms=())`, `SHARED_ACAD_COMMANDS`.
|
||||
- Produces: `server.TOOL_PACK_NAMES == ("core", "pid", "settings", "mech", "arch", "plant")`, `server.PACK_TOOL_NAMES["plant"] == frozenset({"pipe_takeoff", "cable_takeoff"})`, `len(server.LEAN_TOOL_NAMES) == 65` (55 with `TOOL_PACKS=core`), groups `understanding` (the four SECTION 26 tools, core pack) and `plant` (exactly `PACK_TOOL_NAMES["plant"]`) — 247 tools in 28 groups; `discovery/aliases.py::_TRACK_H_VOCABULARY` and `_fold_in(records, extra)`; eight golden tuning cases.
|
||||
- Consumes: the six tools the groups registered — SECTION 26 `drawing_understand`, `drawing_scale_check` (group U, Tasks 3-4, tagged `{"analysis", "query"}`), `drawing_diff`, `drawing_topology_check` (group D, Tasks 9-10, tagged `{"analysis", "query"}`); SECTION 27 `pipe_takeoff`, `cable_takeoff` (group N, Tasks 7-8, tagged `{"plant", "analysis"}`) — each with its alias record in `discovery/aliases.py` (every one `acad=()` except `drawing_diff`'s `("COMPARE",)`), and each branch's paragraph in the `test_tool_group_sizes_are_unchanged` docstring saying the six filed under `analysis` while `understand` / `plant` were unranked; `server._registered_tools()`, `server._tool_groups()`, `server._apply_tool_profile(profile) -> {"disabled_tools", "enabled_count", "tool_packs"}`; `discovery.aliases.TOOL_ALIASES`, `ToolAliases(acad=(), synonyms=())`, `SHARED_ACAD_COMMANDS`. For the one-reading steps (15-20): Task 3 — `describe.prepare(snap) -> dict` (`kept`, `clusters: [{id, box, ...}]`, `membership`, `diag`), `find_rooms(records, *, tol, membership) -> dict` (`rooms`, `labelled: [(row, Face)]`), `FACE_TOL_FRACTION`, `describe(snap)` (`units`, `rooms`), `units.infer_units(snap, *, records=None, room_areas=()) -> dict` (`declared: {code, name, label}`, `inferred`, `mm_per_unit`, `evidence`, `warning`); Task 4 — `scale.scale_check(a, b, *, min_pair_mm=2000.0) -> dict` (`units: {a, b: {unit, mm_per_unit, declared}}`, `groups`), `scale._side(snap)`; Task 6 — `takeoff.unit_of`, `layout_tags`, `scale_stand_in`, `room_regions`, `pipe_rows`, `UNIT_METRES`; Task 8 — `takeoff.cable_rows`.
|
||||
- Produces: `server.TOOL_PACK_NAMES == ("core", "pid", "settings", "mech", "arch", "plant")`, `server.PACK_TOOL_NAMES["plant"] == frozenset({"pipe_takeoff", "cable_takeoff"})`, `len(server.LEAN_TOOL_NAMES) == 65` (55 with `TOOL_PACKS=core`), groups `understanding` (the four SECTION 26 tools, core pack) and `plant` (exactly `PACK_TOOL_NAMES["plant"]`) — 247 tools in 28 groups; `discovery/aliases.py::_TRACK_H_VOCABULARY` and `_fold_in(records, extra)`; eight golden tuning cases. From the one-reading steps: `describe.drawing_units(snap, *, prep=None, rooms=None) -> dict` (an `infer_units` result, the one call `describe`, `scale_check` and the takeoffs make); `takeoff.unit_of(snap, *, prep=None)`, `layout_tags(snap, wanted=(), *, prep=None)` and `room_regions(snap, cluster=None, *, prep=None)` with their result shapes unchanged; `takeoff.scale_stand_in` and the takeoff's own unit bands and cluster grid removed.
|
||||
|
||||
- [ ] **Step 1: Write the failing test for the new surface — replace `tests/test_tool_packs.py`**
|
||||
|
||||
@@ -16253,7 +16256,418 @@ If the printed dict differs anywhere but by a tool that lacks its section tag (S
|
||||
Run: `uv run --frozen pytest tests/test_tool_registry.py tests/test_tool_inventory.py tests/test_release_consistency.py::test_section_header_counts_match_decorators -q`
|
||||
Expected: all pass.
|
||||
|
||||
- [ ] **Step 15: Retake the README figures the tree derives**
|
||||
- [ ] **Step 15: One reading — write the failing test**
|
||||
|
||||
Groups U and N were cut from one base side by side, so Task 6's `takeoff.py` carries narrow stand-ins for what group U reads properly: `unit_of` (its own text-height and span bands), a 2 % grid of plan copies inside `layout_tags`, `scale_stand_in` (spec §6 without per-room groups) and `room_regions`. On main both readers exist, and two readers of one drawing can disagree: on the hand-built layout of Task 6 (INSUNITS inches over millimetre geometry, no door, no wall pair), `infer_units` called without room faces scores mm, cm and inches 0.75 each and the header's tie-break answers inches, while the takeoff's bands answer mm. From here on every reader asks one function, `describe.drawing_units` (the call `describe` already makes: kept records, labelled room faces as evidence), and the takeoffs read plan copies, scale and rooms through group U's modules. The stand-ins go.
|
||||
|
||||
Create `tests/test_understand_one_reading.py`:
|
||||
|
||||
```python
|
||||
"""One reading of a drawing: the takeoffs see what drawing_understand and drawing_scale_check see.
|
||||
|
||||
Groups U and N were built side by side from one base, so Task 6 carried narrow
|
||||
stand-ins: its own unit bands, a 2 % grid of plan copies, a scale statistic
|
||||
without room groups, its own room faces. On main the takeoffs read units, plan
|
||||
copies, scale and rooms through group U's readers, so the four tools never
|
||||
give one drawing two units, two plan copies, two verdicts or two sets of rooms.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import pytest
|
||||
|
||||
from engineering.understand import takeoff
|
||||
from engineering.understand.describe import describe, drawing_units, prepare
|
||||
from engineering.understand.network import build_network
|
||||
from engineering.understand.scale import scale_check
|
||||
from engineering.understand.snapshot import Snapshot, read_snapshot
|
||||
from tests.fixtures.plant_pair import build_plant_pair
|
||||
|
||||
|
||||
@pytest.fixture(scope="module")
|
||||
def plant(tmp_path_factory):
|
||||
truth = build_plant_pair(tmp_path_factory.mktemp("one_reading"))
|
||||
return truth, read_snapshot(truth["pid"]), read_snapshot(truth["layout"])
|
||||
|
||||
|
||||
def empty(insunits):
|
||||
return Snapshot(
|
||||
source="empty.dxf",
|
||||
insunits=insunits,
|
||||
extmin=None,
|
||||
extmax=None,
|
||||
layers={},
|
||||
layouts=(),
|
||||
records=(),
|
||||
)
|
||||
|
||||
|
||||
def test_one_unit_per_drawing(plant):
|
||||
_truth, pid, layout = plant
|
||||
check = scale_check(pid, layout)
|
||||
for snap, side in ((pid, "a"), (layout, "b")):
|
||||
unit = takeoff.unit_of(snap)
|
||||
assert unit["inferred"] == describe(snap)["units"]["inferred"]
|
||||
assert unit["inferred"] == check["units"][side]["unit"]
|
||||
assert takeoff.unit_of(layout)["inferred"] == "mm" # INSUNITS says inches: Review Focus 5
|
||||
|
||||
|
||||
def test_the_takeoff_unit_carries_drawing_units_evidence_and_warning(plant):
|
||||
_truth, pid, layout = plant
|
||||
for snap in (pid, layout):
|
||||
mine, theirs = takeoff.unit_of(snap), drawing_units(snap)
|
||||
assert mine["declared"] == theirs["declared"]["name"]
|
||||
assert mine["m_per_unit"] == pytest.approx(theirs["mm_per_unit"] / 1000.0)
|
||||
assert mine["evidence"] == theirs["evidence"]
|
||||
expected = f"{snap.source}: {theirs['warning']}" if theirs["warning"] else None
|
||||
assert mine["warning"] == expected
|
||||
|
||||
|
||||
def test_a_unit_nothing_measures_keeps_the_declared_one():
|
||||
unit = takeoff.unit_of(empty(5))
|
||||
assert (unit["declared"], unit["inferred"], unit["m_per_unit"], unit["warning"]) == (
|
||||
"cm",
|
||||
"cm",
|
||||
0.01,
|
||||
None,
|
||||
)
|
||||
|
||||
|
||||
def test_a_unitless_drawing_nothing_measures_is_read_in_mm_and_says_so():
|
||||
unit = takeoff.unit_of(empty(0))
|
||||
assert (unit["declared"], unit["inferred"], unit["m_per_unit"]) == (None, "mm", 0.001)
|
||||
assert unit["warning"] == (
|
||||
"empty.dxf: INSUNITS declares Unitless (0) and no measurement settles the unit; "
|
||||
"the takeoff reads it as mm."
|
||||
)
|
||||
|
||||
|
||||
def test_plan_copies_are_the_clusters_drawing_understand_reports(plant):
|
||||
truth, _pid, layout = plant
|
||||
found = takeoff.layout_tags(layout, truth["tag_positions"]["layout"])
|
||||
boxes = [tuple(cluster["box"]) for cluster in prepare(layout)["clusters"]]
|
||||
assert found["clusters"] == len(boxes) == truth["cluster_count"]
|
||||
assert tuple(found["cluster"]) in boxes
|
||||
|
||||
|
||||
def test_the_takeoff_scale_is_drawing_scale_check(plant):
|
||||
_truth, pid, layout = plant
|
||||
result = takeoff.pipe_rows(build_network(pid), pid, layout)
|
||||
check = scale_check(pid, layout)
|
||||
for key in ("verdict", "factor", "median", "within_10pct", "pair_count", "groups"):
|
||||
assert result["scale"][key] == check[key], key
|
||||
|
||||
|
||||
def test_rooms_are_the_rooms_drawing_understand_reports(plant):
|
||||
truth, _pid, layout = plant
|
||||
cluster = takeoff.layout_tags(layout, truth["tag_positions"]["layout"])["cluster"]
|
||||
regions = takeoff.room_regions(layout, cluster)
|
||||
assert sorted(key for key, _face in regions["faces"]) == sorted(
|
||||
str(room["number"]) for room in truth["rooms"]
|
||||
)
|
||||
reported = {
|
||||
(str(row["number"] or row["name"]), round(row["face"]["area"], 3))
|
||||
for row in describe(layout)["rooms"]
|
||||
if row["face"] is not None
|
||||
}
|
||||
for key, face in regions["faces"]:
|
||||
assert (key, round(face.area, 3)) in reported
|
||||
|
||||
|
||||
def test_no_second_reader_is_left():
|
||||
for name in (
|
||||
"scale_stand_in",
|
||||
"INSUNITS_NAMES",
|
||||
"TEXT_BAND_M",
|
||||
"SPAN_BAND_M",
|
||||
"TYPICAL_SPAN_M",
|
||||
"TYPICAL_TEXT_M",
|
||||
"CLUSTER_GAP_SHARE",
|
||||
"MIN_PAIR_MM",
|
||||
"MIN_TAGS",
|
||||
"SCALE_BAND",
|
||||
"TO_SCALE_SHARE",
|
||||
"SCHEMATIC_SHARE",
|
||||
"_model_points",
|
||||
"_robust_box",
|
||||
"_text_heights",
|
||||
"_mark_segment",
|
||||
"_clusters",
|
||||
):
|
||||
assert not hasattr(takeoff, name), name
|
||||
```
|
||||
|
||||
- [ ] **Step 16: Run it to verify it fails**
|
||||
|
||||
Run: `uv run --frozen pytest tests/test_understand_one_reading.py -q`
|
||||
Expected: FAIL — the import of `drawing_units` fails first (`ImportError: cannot import name 'drawing_units'`).
|
||||
|
||||
- [ ] **Step 17: `describe.drawing_units` — the one call every reader makes**
|
||||
|
||||
In `engineering/understand/describe.py`, add `"drawing_units"` to `__all__` (after `"describe"`) and add this function directly after `find_rooms`:
|
||||
|
||||
```python
|
||||
def drawing_units(snap: Snapshot, *, prep: dict | None = None, rooms: dict | None = None) -> dict:
|
||||
"""`units.infer_units` the one way every reader calls it: on the records kept
|
||||
after the outliers, with the labelled room faces as evidence.
|
||||
|
||||
`describe`, `scale.scale_check` and the takeoffs all read a drawing's unit
|
||||
here, so no two tools report two units for one drawing. ``prep`` and
|
||||
``rooms`` are a `prepare` / `find_rooms` result the caller already holds.
|
||||
"""
|
||||
prep = prep or prepare(snap)
|
||||
if rooms is None:
|
||||
tol = prep["diag"] * FACE_TOL_FRACTION if prep["diag"] > 0 else 1.0
|
||||
rooms = find_rooms(prep["kept"], tol=tol, membership=prep["membership"])
|
||||
return infer_units(
|
||||
snap,
|
||||
records=prep["kept"],
|
||||
room_areas=[row["face"]["area"] for row, _face in rooms["labelled"]],
|
||||
)
|
||||
```
|
||||
|
||||
In `describe()`, replace
|
||||
|
||||
```python
|
||||
units = infer_units(
|
||||
snap,
|
||||
records=prep["kept"],
|
||||
room_areas=[row["face"]["area"] for row, _face in rooms["labelled"]],
|
||||
)
|
||||
```
|
||||
|
||||
with
|
||||
|
||||
```python
|
||||
units = drawing_units(snap, prep=prep, rooms=rooms)
|
||||
```
|
||||
|
||||
In `engineering/understand/scale.py`, import `drawing_units` from `engineering.understand.describe` beside `FACE_TOL_FRACTION, find_rooms, prepare, tag_occurrences`, drop the now unused `from engineering.understand.units import infer_units`, and in `scale_check`'s units loop replace
|
||||
|
||||
```python
|
||||
areas = [row["face"]["area"] for row, _f in rooms["labelled"]] if rooms else []
|
||||
u = infer_units(snap, records=side["kept"], room_areas=areas)
|
||||
```
|
||||
|
||||
with
|
||||
|
||||
```python
|
||||
u = drawing_units(snap, prep=side, rooms=rooms)
|
||||
```
|
||||
|
||||
(`side` is `_side(snap)`: `prepare`'s dict plus `tags`, so it carries `kept`, `diag` and `membership`. Drawing `a` now contributes its own labelled rooms as unit evidence, as `describe` always did; on a P&ID without room faces nothing changes.)
|
||||
|
||||
- [ ] **Step 18: The takeoffs read through group U**
|
||||
|
||||
In `engineering/understand/takeoff.py`:
|
||||
|
||||
1. Imports — add
|
||||
|
||||
```python
|
||||
from engineering.understand.describe import FACE_TOL_FRACTION, drawing_units, find_rooms, prepare
|
||||
from engineering.understand.scale import scale_check
|
||||
```
|
||||
|
||||
and let `uv run --frozen ruff check engineering/understand/takeoff.py` name what became unused (`statistics`, `DEFAULT_TOL`, `planar_faces` are the likely ones); remove exactly what it names.
|
||||
|
||||
2. Delete the stand-ins and everything only they read: `INSUNITS_NAMES`, `TEXT_BAND_M`, `SPAN_BAND_M`, `TYPICAL_SPAN_M`, `TYPICAL_TEXT_M`, `CLUSTER_GAP_SHARE`, `MIN_PAIR_MM`, `MIN_TAGS`, `SCALE_BAND`, `TO_SCALE_SHARE`, `SCHEMATIC_SHARE` (each with its `#:` comment), `_model_points`, `_robust_box`, `_text_heights`, `_mark_segment`, `_clusters` and `scale_stand_in`; remove `"scale_stand_in"` from `__all__`. `UNIT_METRES` stays: the takeoff still converts with it.
|
||||
|
||||
3. Replace `unit_of` with:
|
||||
|
||||
```python
|
||||
def unit_of(snap: Snapshot, *, prep: dict | None = None) -> dict:
|
||||
"""The unit a drawing's geometry implies - `describe.drawing_units`, the reading
|
||||
`drawing_understand` and `drawing_scale_check` give - in the shape a takeoff reads.
|
||||
|
||||
``{"declared", "inferred", "m_per_unit", "warning", "evidence"}``: ``declared``
|
||||
is the INSUNITS abbreviation (None when it names none of mm / cm / m / in /
|
||||
ft), ``evidence`` is `infer_units`' own list. When no measurement settles
|
||||
the unit the declared one stands, else mm, and the warning says which; a
|
||||
takeoff never leaves a length in unknown units.
|
||||
"""
|
||||
found = drawing_units(snap, prep=prep)
|
||||
name = found["declared"]["name"]
|
||||
declared = name if name in UNIT_METRES else None
|
||||
inferred, warning = found["inferred"], found["warning"]
|
||||
if inferred is None:
|
||||
inferred = declared or "mm"
|
||||
warning = (
|
||||
f"INSUNITS declares {found['declared']['label']} ({found['declared']['code']}) and "
|
||||
f"no measurement settles the unit; the takeoff reads it as {inferred}."
|
||||
)
|
||||
return {
|
||||
"declared": declared,
|
||||
"inferred": inferred,
|
||||
"m_per_unit": UNIT_METRES[inferred],
|
||||
"warning": f"{snap.source}: {warning}" if warning else None,
|
||||
"evidence": found["evidence"],
|
||||
}
|
||||
```
|
||||
|
||||
4. In `layout_tags`, give the signature a `prep` keyword — `def layout_tags(snap: Snapshot, wanted: Iterable[str] = (), *, prep: dict | None = None) -> dict:` — replace `_gap, boxes = _clusters(snap)` with
|
||||
|
||||
```python
|
||||
prep = prep or prepare(snap)
|
||||
boxes = [tuple(cluster["box"]) for cluster in prep["clusters"]]
|
||||
```
|
||||
|
||||
and in its docstring replace "Otherwise the drawing is split into clusters (plan copies, sheets, a far outlier) and the cluster holding the most `wanted` tags is chosen" with "Otherwise the drawing's clusters - `describe.prepare`'s, the plan copies, sheets and outliers `drawing_understand` reports - are read, and the cluster holding the most `wanted` tags is chosen". The rest of the function is unchanged; `cluster` stays a box.
|
||||
|
||||
5. Replace `room_regions` with:
|
||||
|
||||
```python
|
||||
def room_regions(snap: Snapshot, cluster=None, *, prep: dict | None = None) -> dict:
|
||||
"""Room faces and labels - `describe.find_rooms`, the rooms `drawing_understand`
|
||||
reports - inside ``cluster`` (a box) when one is given.
|
||||
|
||||
``{"faces": [(key, Face)], "labels": [(key, Pt)]}``; the key is the room
|
||||
number, else its name. A record belongs to the cluster when every point
|
||||
it has lies in the box.
|
||||
"""
|
||||
prep = prep or prepare(snap)
|
||||
|
||||
def keep(rec) -> bool:
|
||||
return cluster is None or all(
|
||||
cluster[0] <= p[0] <= cluster[2] and cluster[1] <= p[1] <= cluster[3]
|
||||
for p in rec.points
|
||||
)
|
||||
|
||||
tol = prep["diag"] * FACE_TOL_FRACTION if prep["diag"] > 0 else 1.0
|
||||
found = find_rooms(
|
||||
[rec for rec in prep["kept"] if keep(rec)], tol=tol, membership=prep["membership"]
|
||||
)
|
||||
|
||||
def key(row: dict) -> str:
|
||||
return str(row["number"] or row["name"] or row["text"])
|
||||
|
||||
return {
|
||||
"faces": [(key(row), face) for row, face in found["labelled"]],
|
||||
"labels": sorted((key(row), (row["at"][0], row["at"][1])) for row in found["rooms"]),
|
||||
}
|
||||
```
|
||||
|
||||
6. In `pipe_rows`, read each drawing's first pass once and take the scale from `scale_check`. Replace
|
||||
|
||||
```python
|
||||
pid_unit = unit_of(pid)
|
||||
layout_unit = unit_of(layout) if layout is not None else None
|
||||
wanted = {tag for run in network["runs"] for tag in run.tags}
|
||||
found = layout_tags(layout, wanted) if layout is not None else None
|
||||
if scale is None:
|
||||
scale = (
|
||||
scale_stand_in(pid, layout, layout_found=found, layout_unit=layout_unit)
|
||||
if layout is not None
|
||||
else {"verdict": "not_checked", "pair_count": 0, "matched": [], "ambiguous": []}
|
||||
)
|
||||
```
|
||||
|
||||
with
|
||||
|
||||
```python
|
||||
prep_pid = prepare(pid)
|
||||
prep_layout = prepare(layout) if layout is not None else None
|
||||
pid_unit = unit_of(pid, prep=prep_pid)
|
||||
layout_unit = unit_of(layout, prep=prep_layout) if layout is not None else None
|
||||
wanted = {tag for run in network["runs"] for tag in run.tags}
|
||||
found = layout_tags(layout, wanted, prep=prep_layout) if layout is not None else None
|
||||
if scale is None:
|
||||
scale = (
|
||||
scale_check(pid, layout)
|
||||
if layout is not None
|
||||
else {"verdict": "not_checked", "pair_count": 0, "matched": [], "ambiguous": []}
|
||||
)
|
||||
```
|
||||
|
||||
and
|
||||
|
||||
```python
|
||||
regions = (
|
||||
room_regions(layout, found["cluster"])
|
||||
if found is not None and source == "layout"
|
||||
else room_regions(pid)
|
||||
)
|
||||
```
|
||||
|
||||
with
|
||||
|
||||
```python
|
||||
regions = (
|
||||
room_regions(layout, found["cluster"], prep=prep_layout)
|
||||
if found is not None and source == "layout"
|
||||
else room_regions(pid, prep=prep_pid)
|
||||
)
|
||||
```
|
||||
|
||||
In its docstring, "`scale` is a scale-check result (`scale_check`'s shape); omitted, the stand-in runs when a layout is given." becomes "`scale` is a `scale_check` result; omitted, `scale_check(pid, layout)` runs when a layout is given."
|
||||
|
||||
7. In `cable_rows`, replace `layout_unit = unit_of(layout) if layout is not None else None` with
|
||||
|
||||
```python
|
||||
prep_layout = prepare(layout) if layout is not None else None
|
||||
layout_unit = unit_of(layout, prep=prep_layout) if layout is not None else None
|
||||
```
|
||||
|
||||
`placed = layout_tags(layout, set(loads) | set(pid_callouts))` with `placed = layout_tags(layout, set(loads) | set(pid_callouts), prep=prep_layout)`, and `regions = room_regions(layout, cluster) if layout is not None else room_regions(pid)` with `regions = room_regions(layout, cluster, prep=prep_layout) if layout is not None else room_regions(pid)`.
|
||||
|
||||
8. In the module docstring, replace the paragraph that begins "This module also carries the takeoff's own stand-ins" with:
|
||||
|
||||
```
|
||||
Units, plan copies, scale and rooms are read through group U's readers
|
||||
(`describe.drawing_units`, `describe.prepare`, `scale.scale_check`,
|
||||
`describe.find_rooms`), so a takeoff sees exactly what `drawing_understand`
|
||||
and `drawing_scale_check` report for the same drawing.
|
||||
```
|
||||
|
||||
- [ ] **Step 19: Re-point the tests that pinned the stand-ins**
|
||||
|
||||
In `tests/test_understand_takeoff.py`:
|
||||
|
||||
- add `from engineering.understand.scale import scale_check` between the `network` and `snapshot` imports, and delete `scale_stand_in,` from the `engineering.understand.takeoff` import list;
|
||||
- rename `test_the_stand_in_recognises_a_uniform_scale_and_a_rearranged_one` to `test_scale_check_recognises_a_uniform_scale_and_a_rearranged_one` and replace its two calls `scale_stand_in(half, layout)` and `scale_stand_in(other, layout)` with `scale_check(half, layout)` and `scale_check(other, layout)` — every assertion in it stays;
|
||||
- the unit warning now carries `infer_units`' wording (`INSUNITS declares Inches (1) but the geometry reads as mm: ...`, prefixed by the drawing's source), so `"INSUNITS declares in" in w and "as mm" in w` becomes `"INSUNITS declares Inches (1)" in w and "reads as mm" in w` in `test_review_focus_5_inches_declared_over_millimetres`, and `"INSUNITS declares in" in w` becomes `"INSUNITS declares Inches (1)" in w` in `test_review_focus_5_the_synthetic_layout_is_read_in_millimetres`.
|
||||
|
||||
In `tests/test_understand_report.py`, `test_the_method_sheet_is_generated_from_the_result`: `assert any("INSUNITS declares in" in w for w in warnings)` becomes `assert any("INSUNITS declares Inches (1)" in w for w in warnings)`. Its `method["Scale check: pairs"] == "12"` stays: that test passes its own `SCHEMATIC` result, and a given `scale` is still used as given.
|
||||
|
||||
No other assertion changes. If a behavioural assertion of groups U or N fails now — a unit, a length, a status, a room, a verdict — the reading is wrong, not the test: fix the reader against spec §5 / §6 and say which in the report; never weaken the assertion.
|
||||
|
||||
- [ ] **Step 20: Gates and commit**
|
||||
|
||||
```
|
||||
uv run --frozen ruff format engineering/understand tests/test_understand_one_reading.py tests/test_understand_takeoff.py tests/test_understand_report.py
|
||||
uv run --frozen ruff check engineering tests
|
||||
uv run --frozen pytest tests/test_understand_one_reading.py tests/test_understand_units.py tests/test_understand_describe.py tests/test_understand_scale.py tests/test_understand_scale_tools.py tests/test_understand_takeoff.py tests/test_understand_report.py tests/test_understand_cable.py tests/test_understand_tools.py -q
|
||||
uv run --frozen pytest -q --deselect tests/test_release_consistency.py::test_document_numbers_match_the_tree
|
||||
```
|
||||
|
||||
Expected: ruff clean; `tests/test_understand_one_reading.py` 8 passed; the whole suite green with the collected-tests figure deselected (this step adds eight tests; Step 21 retakes the figure).
|
||||
|
||||
```
|
||||
git add engineering/understand/describe.py engineering/understand/scale.py engineering/understand/takeoff.py tests/test_understand_one_reading.py tests/test_understand_takeoff.py tests/test_understand_report.py
|
||||
git -c user.name="Umut CAN" -c user.email="umutcan.edizaslan@gmail.com" commit -m "$(cat <<'MSG'
|
||||
refactor(understand): one reading of units, plan copies, scale and rooms
|
||||
|
||||
Groups U and N were built side by side, so the takeoffs carried narrow
|
||||
stand-ins for what group U reads properly. On main they read through U:
|
||||
|
||||
- describe.drawing_units is the one call every reader makes (kept records,
|
||||
labelled room faces as evidence); describe, scale_check and the takeoffs
|
||||
all use it. Without the room faces, a layout that declares inches over
|
||||
millimetres can tie three ways and fall to the header.
|
||||
- layout_tags takes its plan copies from describe.prepare, room_regions its
|
||||
rooms from describe.find_rooms, and pipe_rows its scale from scale_check,
|
||||
per-room groups included. scale_stand_in and the takeoff's own unit bands
|
||||
and cluster grid are gone.
|
||||
- test_understand_one_reading pins the agreement: one unit, one plan copy,
|
||||
one verdict and one set of rooms per drawing, across drawing_understand,
|
||||
drawing_scale_check and the two takeoffs.
|
||||
|
||||
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
||||
Claude-Session: https://claude.ai/code/session_01FKFStjEFzpGyLq5bMpWxTd
|
||||
MSG
|
||||
)"
|
||||
```
|
||||
|
||||
- [ ] **Step 21: Retake the README figures the tree derives**
|
||||
|
||||
```
|
||||
uv run --frozen python scripts/check_doc_numbers.py --list
|
||||
@@ -16286,7 +16700,7 @@ Then:
|
||||
Run: `uv run --frozen python scripts/check_doc_numbers.py`
|
||||
Expected: exit 0. This task changes no benchmark figure — `correctness_suite.py` still holds the 43 checks the published A/B report was run over — so the only numbers that moved are the ones just rewritten. A `DRIFT` line names the file and the figure; fix that figure, never the checker. (The README token table is remeasured by Task 12.)
|
||||
|
||||
- [ ] **Step 16: Gates**
|
||||
- [ ] **Step 22: Gates**
|
||||
|
||||
```
|
||||
uv run --frozen ruff format server.py discovery/aliases.py tests/test_tool_packs.py tests/test_tool_registry.py tests/test_token_suite.py tests/test_discovery_aliases.py tests/data/discovery_golden.py
|
||||
@@ -16295,9 +16709,9 @@ uv run --frozen python scripts/generate_tool_inventory.py --check
|
||||
uv run --frozen pytest -q
|
||||
```
|
||||
|
||||
Expected: ruff clean; no inventory drift; the whole suite green, `tests/test_release_consistency.py` in full included — the collected-tests figure was retaken in Step 15, so the test every earlier task deselected passes again on main.
|
||||
Expected: ruff clean; no inventory drift; the whole suite green, `tests/test_release_consistency.py` in full included — the collected-tests figure was retaken in Step 21, so the test every earlier task deselected passes again on main.
|
||||
|
||||
- [ ] **Step 17: Commit**
|
||||
- [ ] **Step 23: Commit**
|
||||
|
||||
```
|
||||
git add server.py discovery/aliases.py docs/tool-inventory.json README.md tests/test_tool_packs.py tests/test_tool_registry.py tests/test_token_suite.py tests/test_discovery_aliases.py tests/data/discovery_golden.py
|
||||
|
||||
Reference in New Issue
Block a user