diff --git a/gently/app/tools/acquisition_tools.py b/gently/app/tools/acquisition_tools.py index bfc892ea..204d210e 100644 --- a/gently/app/tools/acquisition_tools.py +++ b/gently/app/tools/acquisition_tools.py @@ -11,6 +11,7 @@ import numpy as np +from gently.harness import calibration_gate from gently.harness.tools.helpers import ctx_get, get_embryo_or_error from gently.harness.tools.registry import ToolCategory, ToolExample, tool @@ -103,9 +104,17 @@ async def acquire_volume( num_slices: int = 50, exposure_ms: float = 10.0, z_buffer_um: float | None = None, + allow_uncalibrated: bool = False, context: dict | None = None, ) -> str: - """Acquire single volume - moves to embryo first, uses calibration""" + """Acquire single volume - moves to embryo first, uses calibration. + + Refuses an uncalibrated embryo unless ``allow_uncalibrated``. The four + galvo/piezo parameters below used to fall back to literals with the comment + "use defaults if not calibrated" — so a volume acquired from an embryo that + had never been calibrated used invented geometry and reported success. The + resulting data is indistinguishable from real data, which is the problem. + """ agent = ctx_get(context, "agent") client = ctx_get(context, "client") @@ -116,13 +125,18 @@ async def acquire_volume( if err: return err + if not allow_uncalibrated and not calibration_gate.is_calibrated(embryo): + return f"Error: {calibration_gate.refusal_detail([embryo_id])}" + try: # Move to embryo position first pos = embryo.stage_position if pos and pos.get("x") is not None and pos.get("y") is not None: await client.move_to_position(pos["x"], pos["y"]) - # Get calibration parameters (use defaults if not calibrated) + # Calibrated by the time we get here, unless the caller explicitly + # accepted a guess. The literals below are that guess, and they are + # only ever reached on that path. cal = embryo.calibration or {} galvo_amplitude = cal.get("galvo_amplitude", 0.5) galvo_center = cal.get("galvo_center", 0.0) diff --git a/gently/harness/calibration_gate.py b/gently/harness/calibration_gate.py new file mode 100644 index 00000000..b4a98d38 --- /dev/null +++ b/gently/harness/calibration_gate.py @@ -0,0 +1,86 @@ +"""Has this embryo actually been calibrated? + +One predicate, used by the run routes and by `acquire_volume`, so the answer +cannot differ between the surface an operator drives and the tool an agent +calls. + +WHY THIS EXISTS + +Nothing checked. `POST /api/devices/timelapse/start` validated +`interval_seconds > 0` and that the embryo ids existed, and started. And the +acquisition layer did not fail on an uncalibrated embryo either — it invented a +complete set of scan geometry, with the comment saying so: + + # Get calibration parameters (use defaults if not calibrated) + galvo_amplitude = cal.get("galvo_amplitude", 0.5) + galvo_center = cal.get("galvo_center", 0.0) + piezo_amplitude = cal.get("piezo_amplitude", 25.0) + piezo_center = cal.get("piezo_center", 50.0) + slope = cal.get("slope_um_per_deg", 100.0) + +So a timelapse could run to completion on embryos that had never been +calibrated, using five made-up numbers, and report success. The data looks +exactly like real data. That is worse than a refusal, because a refusal is +noticed. + +It also inverted the order the team stated out loud on 2026-08-07 — Ryan: "the +main thing is just making sure that we can get the calibration to work"; +Kesavan: "Calibration has to work. Embryo navigation has to work. Then +timelapse setup has to work." The workflow has a hard dependency; now the code +has one too. + +WHAT COUNTS AS CALIBRATED + +A finite `slope_um_per_deg` in `embryo.calibration`. Every successful fit +writes it (`calibration_tools.py:891`), and it is the number the scan geometry +is derived from — without it there is nothing to derive from but a guess. + +Fit *quality* is deliberately not judged here. `r_squared` is hardcoded to 0.85 +on the vision-guided path, so it is not currently a measurement, and inventing +a threshold against a literal would be theatre. Presence is checked; quality is +reported for the operator to judge. +""" + +from __future__ import annotations + +from typing import Any + +# The one field that means a fit happened. +FIT_KEY = "slope_um_per_deg" + + +def is_calibrated(embryo: Any) -> bool: + """True when this embryo carries a usable galvo→piezo fit.""" + cal = getattr(embryo, "calibration", None) or {} + if not isinstance(cal, dict): + return False + slope = cal.get(FIT_KEY) + try: + value = float(slope) # type: ignore[arg-type] + except (TypeError, ValueError): + return False + # A zero slope is not a calibration either: the scan would have no extent. + return value == value and value not in (float("inf"), float("-inf")) and value != 0.0 + + +def uncalibrated(embryos: dict[str, Any], embryo_ids: list[str] | None) -> list[str]: + """Which of `embryo_ids` have no fit. `None` means every known embryo. + + Unknown ids are not reported here — the routes already reject those, and + conflating "does not exist" with "not calibrated" would send an operator + to the wrong fix. + """ + ids = list(embryo_ids) if embryo_ids else list(embryos.keys()) + return [eid for eid in ids if eid in embryos and not is_calibrated(embryos[eid])] + + +def refusal_detail(missing: list[str]) -> str: + """The message an operator gets, naming the embryos and the way past it.""" + names = ", ".join(missing) + return ( + f"not calibrated: {names}. " + "An uncalibrated embryo has no galvo/piezo fit, so the scan geometry " + "would be invented rather than measured. Calibrate them, or pass " + "allow_uncalibrated=true to acquire anyway and accept that the volume " + "geometry is a guess." + ) diff --git a/gently/ui/web/routes/data.py b/gently/ui/web/routes/data.py index f0ed9a0b..759968a2 100644 --- a/gently/ui/web/routes/data.py +++ b/gently/ui/web/routes/data.py @@ -7,6 +7,7 @@ import yaml from fastapi import APIRouter, Body, Depends, HTTPException +from gently.harness import calibration_gate from gently.ui.web.auth import require_control logger = logging.getLogger(__name__) @@ -100,6 +101,30 @@ def _require_agent_with_experiment(): raise HTTPException(status_code=503, detail="Agent not ready") return agent + def _require_calibrated(embryo_ids, payload): + """Refuse to start unless every named embryo carries a real fit. + + Server-side on purpose: the agent reaches these routes too, and a check + that lives in operate.js is a check the agent walks past. See + gently/harness/calibration_gate.py for why an uncalibrated run is worse + than a refused one — the acquisition layer invents five numbers and + reports success. + + `allow_uncalibrated: true` still goes through. Someone who means it + should be able to say so; the point is that they have to say it. + """ + if payload.get("allow_uncalibrated"): + return + bridge = getattr(server, "agent_bridge", None) + agent = bridge.agent if bridge is not None else None + experiment = getattr(agent, "experiment", None) if agent else None + embryos = getattr(experiment, "embryos", None) + if not embryos: + return # nothing known to check against + missing = calibration_gate.uncalibrated(embryos, embryo_ids) + if missing: + raise HTTPException(status_code=409, detail=calibration_gate.refusal_detail(missing)) + @router.put("/api/embryos/{embryo_id}/position", dependencies=[Depends(require_control)]) async def update_embryo_position( embryo_id: str, @@ -1389,6 +1414,7 @@ async def operate_run_tactic(payload: dict = Body(...)): # noqa: B008 ) agent = _require_agent_with_experiment() + _require_calibrated(payload.get("embryo_ids") or None, payload) tactic = payload.get("tactic") lib_id = payload.get("library_id") if tactic is None and lib_id: @@ -1540,6 +1566,12 @@ async def acquire_volume(payload: dict = Body(...)): # noqa: B008 can send "ALL OFF" for brightfield-safe Manual-view captures. piezo_center and galvo_center capture at the dialled focal plane. """ + # Optional on purpose: Manual-mode snapping images the current stage + # position with no embryo in mind, which is legitimate. When a caller + # DOES name an embryo, it gets checked. + eid = payload.get("embryo_id") + if eid: + _require_calibrated([str(eid)], payload) client = _resolve_client() if client is None: raise HTTPException(status_code=503, detail="Microscope not connected") @@ -1617,6 +1649,9 @@ async def timelapse_start(payload: dict = Body(...)): # noqa: B008 stop_condition = str(payload.get("stop_condition") or "manual") embryo_ids = payload.get("embryo_ids") or None + # A timelapse is the longest-running thing this UI starts; an invented + # scan geometry would be baked into every timepoint of it. + _require_calibrated(embryo_ids, payload) condition_value = payload.get("condition_value") monitoring_mode = payload.get("monitoring_mode") or None diff --git a/gently/ui/web/static/js/operate.js b/gently/ui/web/static/js/operate.js index a468f93c..9c8775ea 100644 --- a/gently/ui/web/static/js/operate.js +++ b/gently/ui/web/static/js/operate.js @@ -1151,6 +1151,13 @@ const OperateManager = (function () { _acquiring = true; renderSubnavMeta(); try { await postJSON('/api/devices/acquire/volume', { + // Say WHICH embryo. The route used to receive no id at + // all, so it imaged wherever the stage happened to be + // and could not check whether that embryo had ever been + // calibrated. Manual-mode snapping still omits it on + // purpose — a test shot at the current position is a + // real thing to want. + embryo_id: _selected, num_slices: Math.max(1, Number(($('op-vol-slices') || {}).value) || 50), exposure_ms: Math.max(1, Number(($('op-vol-exp') || {}).value) || 10), }); diff --git a/tests/test_calibration_gate.py b/tests/test_calibration_gate.py new file mode 100644 index 00000000..9fbc1a90 --- /dev/null +++ b/tests/test_calibration_gate.py @@ -0,0 +1,127 @@ +"""A run must not start on an uncalibrated embryo. + +Nothing checked. `POST /api/devices/timelapse/start` validated +`interval_seconds > 0` and that the ids existed, and started — and the +acquisition layer did not fail either, it invented the scan geometry: + + # Get calibration parameters (use defaults if not calibrated) + galvo_amplitude = cal.get("galvo_amplitude", 0.5) + ... + slope = cal.get("slope_um_per_deg", 100.0) + +So a timelapse could run to completion on embryos that had never been +calibrated, using five literals, and report success. The output is +indistinguishable from real data, which is why a refusal is the kinder answer. +""" + +from __future__ import annotations + +from dataclasses import dataclass, field +from typing import Any + +from gently.harness import calibration_gate + + +@dataclass +class _Embryo: + calibration: dict[str, Any] = field(default_factory=dict) + + +def _fit(slope: float = 42.0) -> dict[str, Any]: + """The shape a successful fit writes (calibration_tools.py:891).""" + return {"slope_um_per_deg": slope, "offset_um": 1.0, "r_squared": 0.85} + + +# ── is_calibrated ──────────────────────────────────────────────────────────── + + +def test_a_real_fit_counts() -> None: + assert calibration_gate.is_calibrated(_Embryo(_fit())) is True + + +def test_an_empty_calibration_does_not() -> None: + assert calibration_gate.is_calibrated(_Embryo({})) is False + + +def test_scan_parameters_without_a_fit_do_not_count() -> None: + """The exact trap: the literals the old code substituted are not a fit. + + An embryo carrying galvo/piezo values but no slope has never been + calibrated — those are the invented defaults, not a measurement. + """ + partial = { + "galvo_amplitude": 0.5, + "galvo_center": 0.0, + "piezo_amplitude": 25.0, + "piezo_center": 50.0, + } + assert calibration_gate.is_calibrated(_Embryo(partial)) is False + + +def test_a_zero_slope_is_not_a_calibration() -> None: + """Zero slope means the scan has no extent — a fit that failed, not one.""" + assert calibration_gate.is_calibrated(_Embryo(_fit(0.0))) is False + + +def test_junk_in_the_slope_is_not_a_calibration() -> None: + junk: list[Any] = [None, "", "n/a", float("nan"), float("inf"), [], {}] + for bad in junk: + assert calibration_gate.is_calibrated(_Embryo({"slope_um_per_deg": bad})) is False, bad + + +def test_a_missing_calibration_attribute_does_not_raise() -> None: + class Bare: + pass + + assert calibration_gate.is_calibrated(Bare()) is False + + +# ── uncalibrated() ─────────────────────────────────────────────────────────── + + +def _roster() -> dict[str, _Embryo]: + return { + "e1": _Embryo(_fit()), + "e2": _Embryo({}), + "e3": _Embryo(_fit()), + "e4": _Embryo({"galvo_center": 0.0}), + } + + +def test_reports_only_the_uncalibrated_ones() -> None: + assert calibration_gate.uncalibrated(_roster(), ["e1", "e2", "e3", "e4"]) == ["e2", "e4"] + + +def test_none_means_every_known_embryo() -> None: + """`embryo_ids: null` on the route means "all active embryos".""" + assert calibration_gate.uncalibrated(_roster(), None) == ["e2", "e4"] + + +def test_a_fully_calibrated_subset_is_allowed_through() -> None: + assert calibration_gate.uncalibrated(_roster(), ["e1", "e3"]) == [] + + +def test_unknown_ids_are_not_reported_as_uncalibrated() -> None: + """ "Does not exist" and "not calibrated" send an operator to different fixes. + + The routes already reject unknown ids; conflating the two here would tell + someone to calibrate an embryo that is not there. + """ + assert calibration_gate.uncalibrated(_roster(), ["e1", "ghost"]) == [] + + +def test_an_empty_roster_reports_nothing() -> None: + assert calibration_gate.uncalibrated({}, None) == [] + assert calibration_gate.uncalibrated({}, ["e1"]) == [] + + +# ── the message ────────────────────────────────────────────────────────────── + + +def test_the_refusal_names_the_embryos_and_the_way_past_it() -> None: + msg = calibration_gate.refusal_detail(["e2", "e4"]) + assert "e2" in msg and "e4" in msg + # An operator at a microscope needs the remedy in the message, not in a + # dependency file or a route signature. + assert "allow_uncalibrated" in msg + assert "invented" in msg or "guess" in msg diff --git a/tests/test_calibration_gate_routes.py b/tests/test_calibration_gate_routes.py new file mode 100644 index 00000000..49720aaf --- /dev/null +++ b/tests/test_calibration_gate_routes.py @@ -0,0 +1,130 @@ +"""The calibration gate must live on the routes, not in the browser. + +The agent reaches these endpoints too, so a check in `operate.js` is a check +the agent walks past. These tests pin the refusal to the HTTP boundary. + +409 rather than 400: the request is well formed, the instrument is simply not +in a state where it can be honoured. +""" + +from __future__ import annotations + +from dataclasses import dataclass, field +from typing import Any +from unittest.mock import AsyncMock, MagicMock + +from fastapi import FastAPI +from fastapi.testclient import TestClient + +import gently.ui.web.auth as auth +from gently.ui.web.routes.data import create_router + + +@dataclass +class _Embryo: + calibration: dict[str, Any] = field(default_factory=dict) + + +FIT = {"slope_um_per_deg": 42.0, "offset_um": 1.0, "r_squared": 0.85} + + +def _app(embryos: dict[str, _Embryo]): + """Routes wired against a roster, with a working orchestrator and client.""" + server = MagicMock() + agent = server.agent_bridge.agent + agent.experiment.embryos = embryos + agent.timelapse_orchestrator.start_timelapse = AsyncMock(return_value={"success": True}) + agent.client.acquire_volume = AsyncMock(return_value={"success": True}) + app = FastAPI() + app.include_router(create_router(server)) + app.dependency_overrides[auth.require_control] = lambda: True + return TestClient(app) + + +ROSTER = {"e1": _Embryo(dict(FIT)), "e2": _Embryo({})} + + +# ── timelapse ──────────────────────────────────────────────────────────────── + + +def test_timelapse_refuses_an_uncalibrated_embryo() -> None: + r = _app(ROSTER).post( + "/api/devices/timelapse/start", + json={"interval_seconds": 120, "embryo_ids": ["e1", "e2"]}, + ) + assert r.status_code == 409, r.text + detail = r.json()["detail"] + assert "e2" in detail + assert "e1" not in detail, "a calibrated embryo must not be named in the refusal" + assert "allow_uncalibrated" in detail + + +def test_timelapse_allows_a_fully_calibrated_set() -> None: + r = _app(ROSTER).post( + "/api/devices/timelapse/start", + json={"interval_seconds": 120, "embryo_ids": ["e1"]}, + ) + assert r.status_code != 409, r.text + + +def test_null_embryo_ids_still_checks_the_whole_roster() -> None: + """`embryo_ids: null` means "all active embryos" — including the bad one.""" + r = _app(ROSTER).post("/api/devices/timelapse/start", json={"interval_seconds": 120}) + assert r.status_code == 409, r.text + assert "e2" in r.json()["detail"] + + +def test_the_override_is_honoured() -> None: + """Someone who means it can say so. The point is that they must say it.""" + r = _app(ROSTER).post( + "/api/devices/timelapse/start", + json={"interval_seconds": 120, "embryo_ids": ["e1", "e2"], "allow_uncalibrated": True}, + ) + assert r.status_code != 409, r.text + + +def test_the_gate_runs_before_the_interval_check_is_irrelevant() -> None: + """A bad interval must still be a 400 — the gate does not mask validation.""" + r = _app(ROSTER).post( + "/api/devices/timelapse/start", + json={"interval_seconds": 0, "embryo_ids": ["e1"]}, + ) + assert r.status_code == 400, r.text + + +# ── single volume ──────────────────────────────────────────────────────────── + + +def test_volume_refuses_a_named_uncalibrated_embryo() -> None: + r = _app(ROSTER).post("/api/devices/acquire/volume", json={"embryo_id": "e2"}) + assert r.status_code == 409, r.text + + +def test_volume_without_an_embryo_id_is_not_gated() -> None: + """Manual-mode snapping images the current stage position deliberately. + + Optional by design, not by oversight: a test shot with no embryo in mind is + a real thing to want, and there is nothing to check it against. + """ + r = _app(ROSTER).post("/api/devices/acquire/volume", json={"num_slices": 5}) + assert r.status_code != 409, r.text + + +# ── tactics ────────────────────────────────────────────────────────────────── + + +def test_run_tactic_refuses_an_uncalibrated_embryo() -> None: + r = _app(ROSTER).post( + "/api/operate/run-tactic", + json={"library_id": "whatever", "embryo_ids": ["e2"]}, + ) + assert r.status_code == 409, r.text + + +# ── no roster ──────────────────────────────────────────────────────────────── + + +def test_an_empty_roster_does_not_block() -> None: + """With nothing known, there is nothing to assert — do not invent a refusal.""" + r = _app({}).post("/api/devices/timelapse/start", json={"interval_seconds": 120}) + assert r.status_code != 409, r.text