From cb23f3170002c2ea7d658234cbcc5e1a7419dfb3 Mon Sep 17 00:00:00 2001 From: Umut CAN Date: Tue, 15 Sep 2026 15:27:23 +0300 Subject: [PATCH] fix(task-7): instrument bubble outline per ISA-5.1 Table 5.4.1, location lines clipped to the outline build_instrument drew a circle for every type, so `computer` shipped as circle + hexagon and `plc` as circle + square + diamond (10 of the 20 bubble blocks wrong, and the circle poked 0.67 mm outside the hexagon's flat top). Spec 4.4: discrete = circle; dcs = circle inside a square; computer = hexagon; plc = diamond inside a square. The outline is now chosen per type. Location lines were always drawn from -r to +r, overshooting the hexagon's slanted sides by 0.46 mm at y=+/-0.8 and the circle's chord by 0.06 mm. They now end on the outline (`_bubble_half_width`): chord for the circle, full width for the square, `r - |y| / tan 60` for the hexagon. Tests now assert the exact primitive list per type and that every location line stays on/inside its outline (both fail against the previous code). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FKFStjEFzpGyLq5bMpWxTd --- engineering/pid/symbols.py | 30 ++++++++++++++++------ tests/test_pid_symbols.py | 51 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 7 deletions(-) diff --git a/engineering/pid/symbols.py b/engineering/pid/symbols.py index 7086690..e5e27a5 100644 --- a/engineering/pid/symbols.py +++ b/engineering/pid/symbols.py @@ -242,20 +242,35 @@ def make_spec( # ── instrument bubble (ISA-5.1-2009 Table 5.4.1) ───────────────────────────── +def _bubble_half_width(type: str, y: float) -> float: + """Half-width of the instrument outline at height ``y`` so a location line + ends on the outline instead of overshooting it (circle: chord; square: + full width; flat-topped hexagon: ``r - |y| / tan 60``).""" + r = BUBBLE_RADIUS + if type == "discrete": + return math.sqrt(max(r * r - y * y, 0.0)) + if type == "computer": + return r - abs(y) / math.tan(math.radians(60.0)) + return r # dcs / plc: the square is the outline + + def build_instrument(type: str = "discrete", location: str = "field") -> SymbolSpec: if type not in INSTRUMENT_TYPES: raise ValueError(f"instrument: type must be one of {', '.join(INSTRUMENT_TYPES)}") if location not in INSTRUMENT_LOCATIONS: raise ValueError(f"instrument: location must be one of {', '.join(INSTRUMENT_LOCATIONS)}") r = BUBBLE_RADIUS - prims: list[dict] = [circle(0, 0, r)] square = polyline([(-r, -r), (r, -r), (r, r), (-r, r)]) - if type == "dcs": - prims.append(square) + # Outline per type (spec 4.4): discrete = circle; dcs = circle inside a square; + # computer = hexagon; plc = diamond inside a square -- no circle in the last two. + if type == "discrete": + prims: list[dict] = [circle(0, 0, r)] + elif type == "dcs": + prims = [circle(0, 0, r), square] elif type == "computer": - prims.append(polyline(regular_polygon(0, 0, r, 6, 0.0))) - elif type == "plc": - prims += [square, polyline([(0, r), (r, 0), (0, -r), (-r, 0)])] + prims = [polyline(regular_polygon(0, 0, r, 6, 0.0))] + else: # plc + prims = [square, polyline([(0, r), (r, 0), (0, -r), (-r, 0)])] ys = { "field": [], "primary": [0.0], @@ -264,7 +279,8 @@ def build_instrument(type: str = "discrete", location: str = "field") -> SymbolS "auxiliary_rear": [0.8, -0.8], }[location] for y in ys: - prims += dashed_line(-r, y, r, y) if location.endswith("_rear") else [line(-r, y, r, y)] + hw = _bubble_half_width(type, y) + prims += dashed_line(-hw, y, hw, y) if location.endswith("_rear") else [line(-hw, y, hw, y)] attdefs = ( attdef("FUNC", 0.0, 2.0, DESC_HEIGHT, align="middle_center"), attdef("LOOP", 0.0, -2.0, DESC_HEIGHT, align="middle_center"), diff --git a/tests/test_pid_symbols.py b/tests/test_pid_symbols.py index 8db28cb..e3f33bd 100644 --- a/tests/test_pid_symbols.py +++ b/tests/test_pid_symbols.py @@ -2,6 +2,7 @@ from __future__ import annotations +import math import re import pytest @@ -111,6 +112,56 @@ def test_instrument_generator_covers_four_types_by_five_locations(): assert len(hexagon["points"]) == 6 +def test_instrument_outline_per_type_follows_isa_table_5_4_1(): + """Spec 4.4: discrete = circle; dcs = circle inside a square; computer = hexagon; + plc = diamond inside a square -- no circle in the last two.""" + outline = { + t: [p["type"] for p in resolve("instrument", type=t, location="field").primitives] + for t in INSTRUMENT_TYPES + } + assert outline == { + "discrete": ["circle"], + "dcs": ["circle", "polyline"], + "computer": ["polyline"], + "plc": ["polyline", "polyline"], + } + plc = resolve("instrument", type="plc", location="field").primitives + assert sorted(len(p["points"]) for p in plc) == [4, 4], "square + diamond" + + +def test_instrument_location_lines_end_on_the_outline(): + """A location line must not poke outside the bubble outline: chord of the circle, + full width of the square, clipped to the flat-topped hexagon's slanted sides.""" + r = BUBBLE_RADIUS + half_width = { + "discrete": lambda y: math.sqrt(r * r - y * y), + "dcs": lambda y: r, + "plc": lambda y: r, + "computer": lambda y: r - abs(y) / math.tan(math.radians(60.0)), + } + for t in INSTRUMENT_TYPES: + for loc in INSTRUMENT_LOCATIONS: + spec = resolve("instrument", type=t, location=loc) + lines = [p for p in spec.primitives if p["type"] == "line"] + ys = {p["y1"] for p in lines} + expected_ys = {"field": set(), "primary": {0.0}, "auxiliary": {0.8, -0.8}}[ + loc.removesuffix("_rear") + ] + assert ys == expected_ys, (t, loc) + for p in lines: + assert p["y1"] == p["y2"], (t, loc) + hw = half_width[t](p["y1"]) + assert -hw - 1e-9 <= min(p["x1"], p["x2"]), (t, loc, p) + assert max(p["x1"], p["x2"]) <= hw + 1e-9, (t, loc, p) + # a solid line spans the whole outline (not merely stays inside it); a dashed + # one starts on it and its last dash may stop short of the far side + if lines: + hw = half_width[t](next(iter(ys))) + assert min(p["x1"] for p in lines) == pytest.approx(-hw) + if not loc.endswith("_rear"): + assert max(p["x2"] for p in lines) == pytest.approx(hw) + + def test_unknown_symbol_or_option_names_the_valid_values(): with pytest.raises(ValueError, match="instrument"): resolve("thermometer")