From 1e9f2f28cbf5519cc148e6c81bd6dd94e443c300 Mon Sep 17 00:00:00 2001 From: Camillo Moschner Date: Wed, 8 Jul 2026 19:47:12 +0100 Subject: [PATCH 1/9] `Container`: accept lids via a `Liddable` mixin, centred on the top face Lid support lived only on `Plate`; a `Container` (trough, tube, petri dish, well) could not take a lid, and placement was origin-aligned so an off-footprint lid landed in a corner rather than centred. This proposes a `Liddable` mixin as the shared home for lid behaviour on `Plate` and `Container`, and centres lid placement on the parent's top face. The mixin is one option for where this lives; it is small and easy to relocate. - `resources/lid.py`: `Lid` (moved from `plate.py`, re-exported for back-compat) + the `Liddable` mixin (`_lid`, `lid`, `has_lid`, centred `get_lid_location`, assign/unassign, double-lid and size guards). - `Plate(Liddable, ItemizedResource["Well"])` and `Container(Liddable, Resource)` so troughs, tubes, petri dishes and wells inherit lids. - `drop_resource` and `move_lid` gate on `Liddable` instead of `Plate`. Behaviour: footprint-matched lids (every existing lidded plate) are byte-identical at all rotations; off-footprint lids now centre (sub-mm, the rotated STAR lid test is re-pinned accordingly). Rotation alignment stays a move/arm concern, so the resource layer is geometry-only. Tests: `lid_tests.py` covers centred placement, container lids, the double-lid and size guards, and a serialize round-trip; full suite, ruff and mypy clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../backends/hamilton/STAR_tests.py | 12 +- pylabrobot/liquid_handling/liquid_handler.py | 11 +- pylabrobot/resources/__init__.py | 3 +- pylabrobot/resources/container.py | 8 +- pylabrobot/resources/lid.py | 129 ++++++++++++++++++ pylabrobot/resources/lid_tests.py | 60 ++++++++ pylabrobot/resources/plate.py | 88 +----------- 7 files changed, 212 insertions(+), 99 deletions(-) create mode 100644 pylabrobot/resources/lid.py create mode 100644 pylabrobot/resources/lid_tests.py diff --git a/pylabrobot/liquid_handling/backends/hamilton/STAR_tests.py b/pylabrobot/liquid_handling/backends/hamilton/STAR_tests.py index 78574f81406..214f5beefd8 100644 --- a/pylabrobot/liquid_handling/backends/hamilton/STAR_tests.py +++ b/pylabrobot/liquid_handling/backends/hamilton/STAR_tests.py @@ -1880,22 +1880,22 @@ async def test_move_lid_across_rotated_resources(self): self.STAR._write_and_read_command.assert_has_calls( [ _any_write_and_read_command_call( - "C0PPid0001xs04829xd0yj1142yd0zj2242zd0gr1th2800te2800gw4go1308gb1245gt20ga0gc0", + "C0PPid0001xs04829xd0yj1141yd0zj2242zd0gr1th2800te2800gw4go1308gb1245gt20ga0gc0", ), _any_write_and_read_command_call( - "C0PRid0002xs02318xd0yj1644yd0zj1983zd0th2800te2800gr4go1308ga0gc0", + "C0PRid0002xs02317xd0yj1644yd0zj1983zd0th2800te2800gr4go1308ga0gc0", ), _any_write_and_read_command_call( - "C0PPid0003xs02318xd0yj1644yd0zj1983zd0gr1th2800te2800gw4go0885gb0822gt20ga0gc0", + "C0PPid0003xs02317xd0yj1644yd0zj1983zd0gr1th2800te2800gw4go0885gb0822gt20ga0gc0", ), _any_write_and_read_command_call( - "C0PRid0004xs02315xd0yj3104yd0zj1983zd0th2800te2800gr3go0885ga0gc0", + "C0PRid0004xs02317xd0yj3104yd0zj1983zd0th2800te2800gr3go0885ga0gc0", ), _any_write_and_read_command_call( - "C0PPid0005xs02315xd0yj3104yd0zj1983zd0gr1th2800te2800gw4go0885gb0822gt20ga0gc0", + "C0PPid0005xs02317xd0yj3104yd0zj1983zd0gr1th2800te2800gw4go0885gb0822gt20ga0gc0", ), _any_write_and_read_command_call( - "C0PRid0006xs04829xd0yj1142yd0zj2242zd0th2800te2800gr4go0885ga0gc0", + "C0PRid0006xs04829xd0yj1141yd0zj2242zd0th2800te2800gr4go0885ga0gc0", ), ] ) diff --git a/pylabrobot/liquid_handling/liquid_handler.py b/pylabrobot/liquid_handling/liquid_handler.py index c81909a8890..9e3bc17eec9 100644 --- a/pylabrobot/liquid_handling/liquid_handler.py +++ b/pylabrobot/liquid_handling/liquid_handler.py @@ -39,6 +39,7 @@ Coordinate, Deck, Lid, + Liddable, Plate, PlateAdapter, PlateHolder, @@ -2206,13 +2207,13 @@ async def drop_resource( resource.rotated(z=resource_rotation_wrt_destination_wrt_local) ).rotated(destination.get_absolute_rotation()) to_location = destination.get_location_wrt(self.deck) + adjusted_plate_anchor - elif isinstance(destination, Plate) and isinstance(resource, Lid): + elif isinstance(destination, Liddable) and isinstance(resource, Lid): lid = resource - plate_location = destination.get_location_wrt(self.deck) + parent_location = destination.get_location_wrt(self.deck) child_wrt_parent = destination.get_lid_location( lid.rotated(z=resource_rotation_wrt_destination_wrt_local) ).rotated(destination.get_absolute_rotation()) - to_location = plate_location + child_wrt_parent + to_location = parent_location + child_wrt_parent else: to_location = destination.get_location_wrt(self.deck) @@ -2258,7 +2259,7 @@ async def drop_resource( destination.assign_child_resource( resource, location=destination.compute_plate_location(resource) ) - elif isinstance(destination, Plate) and isinstance(resource, Lid): + elif isinstance(destination, Liddable) and isinstance(resource, Lid): destination.assign_child_resource(resource) elif isinstance(destination, Trash): pass # don't assign to trash, resource will simply be unassigned @@ -2348,7 +2349,7 @@ async def move_resource( async def move_lid( self, lid: Lid, - to: Union[Plate, ResourceStack, Coordinate], + to: Union[Liddable, ResourceStack, Coordinate], intermediate_locations: Optional[List[Coordinate]] = None, pickup_offset: Coordinate = Coordinate.zero(), destination_offset: Coordinate = Coordinate.zero(), diff --git a/pylabrobot/resources/__init__.py b/pylabrobot/resources/__init__.py index 4652e9a859b..ca92f0edc5b 100644 --- a/pylabrobot/resources/__init__.py +++ b/pylabrobot/resources/__init__.py @@ -28,12 +28,13 @@ from .greiner import * from .hamilton import * from .itemized_resource import ItemizedResource +from .lid import Lid, Liddable from .liquid import Liquid from .nest import * from .opentrons import * from .perkin_elmer import * from .petri_dish import PetriDish, PetriDishHolder -from .plate import Lid, Plate +from .plate import Plate from .plate_adapter import PlateAdapter from .porvair import * from .powder import Powder diff --git a/pylabrobot/resources/container.py b/pylabrobot/resources/container.py index 46b7aa0176a..03a0b080eef 100644 --- a/pylabrobot/resources/container.py +++ b/pylabrobot/resources/container.py @@ -4,12 +4,16 @@ from pylabrobot.utils.interpolation import interpolate_1d from .coordinate import Coordinate +from .lid import Liddable from .resource import Resource from .volume_tracker import VolumeTracker -class Container(Resource): - """A container is an abstract base class for a resource that can hold liquid.""" +class Container(Liddable, Resource): + """A container is an abstract base class for a resource that can hold liquid. + + Via the :class:`Liddable` mixin, a container (trough, tube, petri dish, well) can host a lid. + """ def __init__( self, diff --git a/pylabrobot/resources/lid.py b/pylabrobot/resources/lid.py new file mode 100644 index 00000000000..39d5fda3e9a --- /dev/null +++ b/pylabrobot/resources/lid.py @@ -0,0 +1,129 @@ +from __future__ import annotations + +from typing import Optional + +from pylabrobot.resources.resource_holder import get_child_location + +from .resource import Coordinate, Resource + +# A lid may be modelled up to this much smaller than the resource it covers - its rim sits just +# inside the outer edge, so real plate lids run a few tenths of a mm under. A larger shortfall means +# the wrong lid. Used by Liddable.assign_child_resource. +LID_UNDERSIZE_TOLERANCE_MM = 1.0 + + +class Lid(Resource): + """A removable cover seated on top of a :class:`Liddable` resource. + + Any liddable resource - a ``Plate`` or a ``Container`` (trough, tube, petri dish, well) - can carry + one. A lid is a standalone resource, moved with the gripper (``LiquidHandler.move_lid``) or + assigned as a child; when seated it is centred on the parent's top face and lowered by + ``nesting_z_height``, the vertical overlap between the lid and the parent it rests on. + """ + + def __init__( + self, + name: str, + size_x: float, + size_y: float, + size_z: float, + nesting_z_height: float, + category: str = "lid", + model: Optional[str] = None, + ): + """Create a lid. + + Args: + name: Name of the lid. + size_x: Size of the lid in x-direction. + size_y: Size of the lid in y-direction. + size_z: Size of the lid in z-direction. + nesting_z_height: the overlap in mm between the lid and its parent (in the z-direction). + """ + super().__init__( + name=name, + size_x=size_x, + size_y=size_y, + size_z=size_z, + category=category, + model=model, + ) + self.nesting_z_height = nesting_z_height + if nesting_z_height == 0: + print(f"{self.name}: Are you certain that the lid nests 0 mm with its parent?") + + def serialize(self) -> dict: + return { + **super().serialize(), + "nesting_z_height": self.nesting_z_height, + } + + +class Liddable(Resource): + """Mixin: a resource that can host a single :class:`Lid` on its top face. + + Mixed into :class:`~pylabrobot.resources.plate.Plate` and + :class:`~pylabrobot.resources.container.Container` (so troughs, tubes, petri dishes, and wells + can carry a lid). The lid is seated centred on the parent's top face and sunk by its own + ``nesting_z_height``; a resource may hold at most one lid at a time. + """ + + _lid: Optional[Lid] = None + + def has_lid(self) -> bool: + return self._lid is not None + + @property + def lid(self) -> Optional[Lid]: + return self._lid + + @lid.setter + def lid(self, lid: Optional[Lid]) -> None: + if lid is None: + if self._lid is not None: + self.unassign_child_resource(self._lid) + else: + self.assign_child_resource(lid) + self._lid = lid + + def get_lid_location(self, lid: Lid) -> Coordinate: + """Location of ``lid`` seated centred on this resource's top face, sunk by nesting_z_height. + + Centres the lid on the footprint - a no-op when the lid shares the footprint (e.g. plate lids), + so this is backwards-compatible - and drops its origin to ``size_z - nesting_z_height``. + ``get_child_location`` keeps a rotated lid's footprint aligned. + """ + return ( + get_child_location(lid) + + self.get_anchor(x="c", y="c", z="t") + - lid.get_anchor(x="c", y="c", z="b") + - Coordinate(0, 0, lid.nesting_z_height) + ) + + def assign_child_resource( + self, + resource: Resource, + location: Optional[Coordinate] = None, + reassign: bool = True, + ): + if isinstance(resource, Lid): + if self.has_lid(): + raise ValueError(f"'{self.name}' already has a lid.") + if ( + resource.get_size_x() < self.get_size_x() - LID_UNDERSIZE_TOLERANCE_MM + or resource.get_size_y() < self.get_size_y() - LID_UNDERSIZE_TOLERANCE_MM + ): + raise ValueError( + f"Lid '{resource.name}' ({resource.get_size_x()} x {resource.get_size_y()} mm) is smaller " + f"than '{self.name}' ({self.get_size_x()} x {self.get_size_y()} mm) and cannot cover it." + ) + self._lid = resource + location = location or self.get_lid_location(resource) + else: + assert location is not None, "Location must be specified if resource is not a lid." + return super().assign_child_resource(resource, location=location, reassign=reassign) + + def unassign_child_resource(self, resource: Resource): + if isinstance(resource, Lid) and resource is self._lid: + self._lid = None + return super().unassign_child_resource(resource) diff --git a/pylabrobot/resources/lid_tests.py b/pylabrobot/resources/lid_tests.py new file mode 100644 index 00000000000..a65e900ba6d --- /dev/null +++ b/pylabrobot/resources/lid_tests.py @@ -0,0 +1,60 @@ +import unittest + +from pylabrobot.resources import Container, Lid, Liddable, Plate, Resource + + +class LidTests(unittest.TestCase): + """The Liddable mixin: any Plate or Container can host a lid, seated centred on its top.""" + + def test_plate_lid_placement_unchanged(self): + """Non-breaking: a footprint-matched lid seats origin-aligned at ``top - nesting``.""" + plate = Plate("p", size_x=127, size_y=86, size_z=14, ordered_items={}) + lid = Lid("l", size_x=127, size_y=86, size_z=10, nesting_z_height=3) + loc = plate.get_lid_location(lid) + self.assertEqual( + (loc.x, loc.y, loc.z), (0, 0, 11) + ) # == get_child_location + (0,0,size_z-nesting) + + def test_container_lid_is_centred_and_sunk(self): + """A larger-footprint lid on a container centres on the top face and sinks by nesting.""" + c = Container("c", size_x=37, size_y=118, size_z=95, material_z_thickness=1.5) + lid = Lid("cl", size_x=44, size_y=125, size_z=10, nesting_z_height=4) + loc = c.get_lid_location(lid) + self.assertEqual((loc.x, loc.y, loc.z), (-3.5, -3.5, 91)) # (37-44)/2, (118-125)/2, 95-4 + + def test_containers_are_liddable_lids_are_not(self): + self.assertIsInstance(Container("c", 10, 10, 10, material_z_thickness=1), Liddable) + self.assertIsInstance(Plate("p", 10, 10, 10, ordered_items={}), Liddable) + self.assertNotIsInstance(Lid("l", 10, 10, 5, nesting_z_height=1), Liddable) # can't lid a lid + + def test_double_lid_is_guarded(self): + c = Container("c", 40, 40, 20, material_z_thickness=1) + c.assign_child_resource(Lid("l1", 40, 40, 5, nesting_z_height=1)) + self.assertTrue(c.has_lid()) + with self.assertRaises(ValueError): + c.assign_child_resource(Lid("l2", 40, 40, 5, nesting_z_height=1)) + + def test_lid_round_trip_restores_state(self): + c = Container("c", 40, 40, 20, material_z_thickness=1) + c.assign_child_resource(Lid("l", 40, 40, 5, nesting_z_height=1)) + c2 = Resource.deserialize(c.serialize()) + assert isinstance(c2, Container) + self.assertTrue(c2.has_lid()) + assert c2.lid is not None + self.assertEqual(c2.lid.name, "l") + + def test_lid_size_check(self): + """Reject a lid clearly smaller than the parent; allow within-tolerance-under, equal, or larger.""" + c = Container("c", 40, 40, 20, material_z_thickness=1) + with self.assertRaises(ValueError): + c.assign_child_resource(Lid("sx", 35, 40, 5, nesting_z_height=1)) # x well under + with self.assertRaises(ValueError): + c.assign_child_resource(Lid("sy", 40, 35, 5, nesting_z_height=1)) # y well under + c.assign_child_resource( + Lid("ok", 39.5, 45, 5, nesting_z_height=1) + ) # 0.5mm under (x) + larger (y) + self.assertTrue(c.has_lid()) + + +if __name__ == "__main__": + unittest.main() diff --git a/pylabrobot/resources/plate.py b/pylabrobot/resources/plate.py index bb62a598e13..a33f1c9cdd0 100644 --- a/pylabrobot/resources/plate.py +++ b/pylabrobot/resources/plate.py @@ -15,57 +15,16 @@ ) from pylabrobot.resources.liquid import Liquid -from pylabrobot.resources.resource_holder import get_child_location from .itemized_resource import ItemizedResource -from .resource import Coordinate, Resource +from .lid import Lid, Liddable +from .resource import Resource if TYPE_CHECKING: from .well import Well -class Lid(Resource): - """Lid for plates.""" - - def __init__( - self, - name: str, - size_x: float, - size_y: float, - size_z: float, - nesting_z_height: float, - category: str = "lid", - model: Optional[str] = None, - ): - """Create a lid for a plate. - - Args: - name: Name of the lid. - size_x: Size of the lid in x-direction. - size_y: Size of the lid in y-direction. - size_z: Size of the lid in z-direction. - nesting_z_height: the overlap in mm between the lid and its parent plate (in the z-direction). - """ - super().__init__( - name=name, - size_x=size_x, - size_y=size_y, - size_z=size_z, - category=category, - model=model, - ) - self.nesting_z_height = nesting_z_height - if nesting_z_height == 0: - print(f"{self.name}: Are you certain that the lid nests 0 mm with its parent plate?") - - def serialize(self) -> dict: - return { - **super().serialize(), - "nesting_z_height": self.nesting_z_height, - } - - -class Plate(ItemizedResource["Well"]): +class Plate(Liddable, ItemizedResource["Well"]): """Base class for Plate resources.""" def __init__( @@ -106,50 +65,12 @@ def __init__( category=category, model=model, ) - self._lid: Optional[Lid] = None self.plate_type = plate_type self.stacking_z_height = stacking_z_height if lid is not None: self.assign_child_resource(lid) - @property - def lid(self) -> Optional[Lid]: - return self._lid - - @lid.setter - def lid(self, lid: Optional[Lid]) -> None: - if lid is None: - self.unassign_child_resource(self._lid) - else: - self.assign_child_resource(lid) - self._lid = lid - - def get_lid_location(self, lid: Lid) -> Coordinate: - """Get location of the lid when assigned to the plate. Takes into account sinking and rotation.""" - return get_child_location(lid) + Coordinate(0, 0, self.get_size_z() - lid.nesting_z_height) - - def assign_child_resource( - self, - resource: Resource, - location: Optional[Coordinate] = None, - reassign: bool = True, - ): - if isinstance(resource, Lid): - if self.has_lid(): - raise ValueError(f"Plate '{self.name}' already has a lid.") - self._lid = resource - default_location = self.get_lid_location(resource) - location = location or default_location - else: - assert location is not None, "Location must be specified for if resource is not a lid." - return super().assign_child_resource(resource, location=location, reassign=reassign) - - def unassign_child_resource(self, resource): - if isinstance(resource, Lid) and resource == self.lid: - self._lid = None - return super().unassign_child_resource(resource) - def serialize(self) -> dict: return { **super().serialize(), @@ -193,9 +114,6 @@ def get_wells(self, identifier: Union[str, Sequence[int]]) -> List["Well"]: return super().get_items(identifier) - def has_lid(self) -> bool: - return self.lid is not None - def set_well_volumes( self, volumes: List[float], From 9e554473b8ecc43da5ec6d0b6b00efab10ba5f28 Mon Sep 17 00:00:00 2001 From: Camillo Moschner Date: Thu, 9 Jul 2026 16:06:46 +0100 Subject: [PATCH 2/9] LiquidHandler: guard aspirate/dispense against lidded containers, not just plates The lid guard only fired when the well's parent was a lidded Plate, so aspirating or dispensing directly from a lidded Container (e.g. a Trough) slipped through. Generalize it: check the resource's own has_lid() and, for wells, an isinstance(parent, Liddable) parent. The 96-head paths gain the same check on their Container branch. Error messages now name the offending resource and say to remove the lid. Adds aspirate-with-lid tests for a Hamilton trough and a petri dish, and a trough lid-centring test. Co-Authored-By: Claude Fable 5 --- pylabrobot/liquid_handling/liquid_handler.py | 38 ++++++++++++++++--- .../liquid_handling/liquid_handler_tests.py | 30 +++++++++++++++ pylabrobot/resources/lid_tests.py | 25 +++++++++++- 3 files changed, 86 insertions(+), 7 deletions(-) diff --git a/pylabrobot/liquid_handling/liquid_handler.py b/pylabrobot/liquid_handling/liquid_handler.py index 9e3bc17eec9..76798909d15 100644 --- a/pylabrobot/liquid_handling/liquid_handler.py +++ b/pylabrobot/liquid_handling/liquid_handler.py @@ -947,8 +947,15 @@ async def aspirate( # Checks for resource in resources: - if isinstance(resource.parent, Plate) and resource.parent.has_lid(): - raise ValueError("Aspirating from a well with a lid is not supported.") + if resource.has_lid(): + raise ValueError( + f"Cannot aspirate from {resource.name!r}: it has a lid. Remove the lid first." + ) + if isinstance(resource.parent, Liddable) and resource.parent.has_lid(): + raise ValueError( + f"Cannot aspirate from {resource.name!r}: its parent {resource.parent.name!r} has a lid. " + "Remove the lid first." + ) self._make_sure_channels_exist(use_channels) for n, p in [ @@ -1161,8 +1168,15 @@ async def dispense( raise BlowOutVolumeError("Blowout volume is larger than aspirated volume") for resource in resources: - if isinstance(resource.parent, Plate) and resource.parent.has_lid(): - raise ValueError("Dispensing to plate with lid") + if resource.has_lid(): + raise ValueError( + f"Cannot dispense to {resource.name!r}: it has a lid. Remove the lid first." + ) + if isinstance(resource.parent, Liddable) and resource.parent.has_lid(): + raise ValueError( + f"Cannot dispense to {resource.name!r}: its parent {resource.parent.name!r} has a lid. " + "Remove the lid first." + ) for n, p in [ ("resources", resources), @@ -1734,9 +1748,15 @@ async def aspirate96( containers: Sequence[Container] if isinstance(resource, Plate): if resource.has_lid(): - raise ValueError("Aspirating from plate with lid") + raise ValueError( + f"Cannot aspirate from {resource.name!r}: it has a lid. Remove the lid first." + ) containers = resource.get_all_items() if resource.num_items > 1 else [resource.get_item(0)] elif isinstance(resource, Container): + if resource.has_lid(): + raise ValueError( + f"Cannot aspirate from {resource.name!r}: it has a lid. Remove the lid first." + ) containers = [resource] elif isinstance(resource, list) and all(isinstance(w, Well) for w in resource): containers = resource @@ -1884,9 +1904,15 @@ async def dispense96( containers: Sequence[Container] if isinstance(resource, Plate): if resource.has_lid(): - raise ValueError("Dispensing to plate with lid is not possible. Remove the lid first.") + raise ValueError( + f"Cannot dispense to {resource.name!r}: it has a lid. Remove the lid first." + ) containers = resource.get_all_items() if resource.num_items > 1 else [resource.get_item(0)] elif isinstance(resource, Container): + if resource.has_lid(): + raise ValueError( + f"Cannot dispense to {resource.name!r}: it has a lid. Remove the lid first." + ) containers = [resource] elif isinstance(resource, list) and all(isinstance(w, Well) for w in resource): containers = resource diff --git a/pylabrobot/liquid_handling/liquid_handler_tests.py b/pylabrobot/liquid_handling/liquid_handler_tests.py index 547d32209a0..b6f56c447d7 100644 --- a/pylabrobot/liquid_handling/liquid_handler_tests.py +++ b/pylabrobot/liquid_handling/liquid_handler_tests.py @@ -24,11 +24,13 @@ Coordinate, Deck, Lid, + PetriDish, Plate, ResourceNotFoundError, ResourceStack, TipRack, cor_96_wellplate_360uL_Fb, + hamilton_1_trough_200mL_Vb, nest_1_troughplate_195000uL_Vb, no_tip_tracking, set_tip_tracking, @@ -915,6 +917,34 @@ async def test_aspirate_with_lid(self): with self.assertRaises(ValueError): await self.lh.aspirate([well], vols=[10]) + async def test_aspirate_from_trough_with_lid(self): + trough = hamilton_1_trough_200mL_Vb(name="trough") + trough.lid = Lid( + "trough_lid", + size_x=trough.get_size_x() + 2, + size_y=trough.get_size_y() + 2, + size_z=10, + nesting_z_height=4, + ) + t = self.tip_rack.get_item("A1").get_tip() + self.lh.update_head_state({0: t}) + with self.assertRaises(ValueError): + await self.lh.aspirate([trough], vols=[10]) + + async def test_aspirate_from_petri_dish_with_lid(self): + dish = PetriDish("petri_dish", diameter=90, height=15) + dish.lid = Lid( + "petri_dish_lid", + size_x=dish.get_size_x() + 4, + size_y=dish.get_size_y() + 4, + size_z=10, + nesting_z_height=4, + ) + t = self.tip_rack.get_item("A1").get_tip() + self.lh.update_head_state({0: t}) + with self.assertRaises(ValueError): + await self.lh.aspirate([dish], vols=[10]) + @pytest.mark.filterwarnings("ignore:Extra arguments to backend") async def test_strictness(self): # Create a mock backend with a custom pick_up_tips that checks arguments diff --git a/pylabrobot/resources/lid_tests.py b/pylabrobot/resources/lid_tests.py index a65e900ba6d..8cbf99a90e8 100644 --- a/pylabrobot/resources/lid_tests.py +++ b/pylabrobot/resources/lid_tests.py @@ -1,6 +1,13 @@ import unittest -from pylabrobot.resources import Container, Lid, Liddable, Plate, Resource +from pylabrobot.resources import ( + Container, + Lid, + Liddable, + Plate, + Resource, + hamilton_1_trough_200mL_Vb, +) class LidTests(unittest.TestCase): @@ -22,6 +29,22 @@ def test_container_lid_is_centred_and_sunk(self): loc = c.get_lid_location(lid) self.assertEqual((loc.x, loc.y, loc.z), (-3.5, -3.5, 91)) # (37-44)/2, (118-125)/2, 95-4 + def test_hamilton_trough_larger_lid_is_centred(self): + """A real Hamilton trough with a directly-built lid 2 mm larger in x and y: the lid centres on + the top face, overhanging 1 mm each side.""" + trough = hamilton_1_trough_200mL_Vb(name="trough") + lid = Lid( + "trough_lid", + size_x=trough.get_size_x() + 2, + size_y=trough.get_size_y() + 2, + size_z=10, + nesting_z_height=4, + ) + loc = trough.get_lid_location(lid) + self.assertEqual((loc.x, loc.y, loc.z), (-1, -1, 91)) # (37-39)/2, (118-120)/2, 95-4 + trough.assign_child_resource(lid) + self.assertTrue(trough.has_lid()) + def test_containers_are_liddable_lids_are_not(self): self.assertIsInstance(Container("c", 10, 10, 10, material_z_thickness=1), Liddable) self.assertIsInstance(Plate("p", 10, 10, 10, ordered_items={}), Liddable) From 3bc250f1b01dd3b378f99d086d8845919c51ec4a Mon Sep 17 00:00:00 2001 From: Rick Wierenga Date: Thu, 9 Jul 2026 12:53:35 -0700 Subject: [PATCH 3/9] fix(liquid_handling): block pipetting on a lid anywhere in the ancestry The lid guard only checked the resource and its direct parent, so a lid on a grandparent or nested holder slipped through. Walk the full parent chain in a shared `_lidded_ancestor` helper and route every aspirate/dispense (incl. 96-head) check through `_check_no_lid`. Co-Authored-By: Claude Opus 4.8 (1M context) --- pylabrobot/liquid_handling/liquid_handler.py | 68 +++++++++---------- .../liquid_handling/liquid_handler_tests.py | 24 +++++++ 2 files changed, 58 insertions(+), 34 deletions(-) diff --git a/pylabrobot/liquid_handling/liquid_handler.py b/pylabrobot/liquid_handling/liquid_handler.py index 76798909d15..395694894e7 100644 --- a/pylabrobot/liquid_handling/liquid_handler.py +++ b/pylabrobot/liquid_handling/liquid_handler.py @@ -92,6 +92,34 @@ class BlowOutVolumeError(Exception): pass +def _lidded_ancestor(resource: Resource) -> Optional[Liddable]: + """Return the nearest lidded resource at or above ``resource``, walking up the parent chain. + + A lid anywhere in the ancestry blocks pipetting, not just one on the direct parent: a well is + enclosed by its plate, but plates may in turn sit in lidded holders or nested containers. Returns + ``resource`` itself if it carries a lid, else the closest lidded ancestor, else ``None``. + """ + current: Optional[Resource] = resource + while current is not None: + if isinstance(current, Liddable) and current.has_lid(): + return current + current = current.parent + return None + + +def _check_no_lid(resource: Resource, action: str) -> None: + """Raise if ``resource`` or any ancestor carries a lid. ``action`` is a verb phrase for the error.""" + lidded = _lidded_ancestor(resource) + if lidded is None: + return + if lidded is resource: + raise ValueError(f"Cannot {action} {resource.name!r}: it has a lid. Remove the lid first.") + raise ValueError( + f"Cannot {action} {resource.name!r}: its enclosing resource {lidded.name!r} has a lid. " + "Remove the lid first." + ) + + class LiquidHandler(Resource, Machine): """ Front end for liquid handlers. @@ -947,15 +975,7 @@ async def aspirate( # Checks for resource in resources: - if resource.has_lid(): - raise ValueError( - f"Cannot aspirate from {resource.name!r}: it has a lid. Remove the lid first." - ) - if isinstance(resource.parent, Liddable) and resource.parent.has_lid(): - raise ValueError( - f"Cannot aspirate from {resource.name!r}: its parent {resource.parent.name!r} has a lid. " - "Remove the lid first." - ) + _check_no_lid(resource, "aspirate from") self._make_sure_channels_exist(use_channels) for n, p in [ @@ -1168,15 +1188,7 @@ async def dispense( raise BlowOutVolumeError("Blowout volume is larger than aspirated volume") for resource in resources: - if resource.has_lid(): - raise ValueError( - f"Cannot dispense to {resource.name!r}: it has a lid. Remove the lid first." - ) - if isinstance(resource.parent, Liddable) and resource.parent.has_lid(): - raise ValueError( - f"Cannot dispense to {resource.name!r}: its parent {resource.parent.name!r} has a lid. " - "Remove the lid first." - ) + _check_no_lid(resource, "dispense to") for n, p in [ ("resources", resources), @@ -1747,16 +1759,10 @@ async def aspirate96( # Convert Plate to either one Container (single well) or a list of Wells containers: Sequence[Container] if isinstance(resource, Plate): - if resource.has_lid(): - raise ValueError( - f"Cannot aspirate from {resource.name!r}: it has a lid. Remove the lid first." - ) + _check_no_lid(resource, "aspirate from") containers = resource.get_all_items() if resource.num_items > 1 else [resource.get_item(0)] elif isinstance(resource, Container): - if resource.has_lid(): - raise ValueError( - f"Cannot aspirate from {resource.name!r}: it has a lid. Remove the lid first." - ) + _check_no_lid(resource, "aspirate from") containers = [resource] elif isinstance(resource, list) and all(isinstance(w, Well) for w in resource): containers = resource @@ -1903,16 +1909,10 @@ async def dispense96( # Convert Plate to either one Container (single well) or a list of Wells containers: Sequence[Container] if isinstance(resource, Plate): - if resource.has_lid(): - raise ValueError( - f"Cannot dispense to {resource.name!r}: it has a lid. Remove the lid first." - ) + _check_no_lid(resource, "dispense to") containers = resource.get_all_items() if resource.num_items > 1 else [resource.get_item(0)] elif isinstance(resource, Container): - if resource.has_lid(): - raise ValueError( - f"Cannot dispense to {resource.name!r}: it has a lid. Remove the lid first." - ) + _check_no_lid(resource, "dispense to") containers = [resource] elif isinstance(resource, list) and all(isinstance(w, Well) for w in resource): containers = resource diff --git a/pylabrobot/liquid_handling/liquid_handler_tests.py b/pylabrobot/liquid_handling/liquid_handler_tests.py index b6f56c447d7..a6b34d2bb30 100644 --- a/pylabrobot/liquid_handling/liquid_handler_tests.py +++ b/pylabrobot/liquid_handling/liquid_handler_tests.py @@ -26,6 +26,7 @@ Lid, PetriDish, Plate, + Resource, ResourceNotFoundError, ResourceStack, TipRack, @@ -945,6 +946,29 @@ async def test_aspirate_from_petri_dish_with_lid(self): with self.assertRaises(ValueError): await self.lh.aspirate([dish], vols=[10]) + async def test_lidded_ancestor_walks_full_chain(self): + # A lid on any ancestor blocks pipetting, not just the direct parent: nest a target two levels + # under a lidded container and check the walk finds it. + from pylabrobot.liquid_handling.liquid_handler import _lidded_ancestor + + trough = hamilton_1_trough_200mL_Vb(name="trough") + trough.lid = Lid( + "trough_lid", + size_x=trough.get_size_x() + 2, + size_y=trough.get_size_y() + 2, + size_z=10, + nesting_z_height=4, + ) + middle = Resource("middle", size_x=1, size_y=1, size_z=1) + target = Resource("target", size_x=1, size_y=1, size_z=1) + trough.assign_child_resource(middle, location=Coordinate.zero()) + middle.assign_child_resource(target, location=Coordinate.zero()) + + self.assertIs(_lidded_ancestor(target), trough) + self.assertIs(_lidded_ancestor(middle), trough) + trough.lid = None + self.assertIsNone(_lidded_ancestor(target)) + @pytest.mark.filterwarnings("ignore:Extra arguments to backend") async def test_strictness(self): # Create a mock backend with a custom pick_up_tips that checks arguments From 592317edd3ec790d25fc87fbde4fab6d6031f77b Mon Sep 17 00:00:00 2001 From: Camillo Moschner Date: Fri, 10 Jul 2026 12:35:39 +0100 Subject: [PATCH 4/9] `Liddable`: raise `ValueError` instead of asserting on a missing child location `assert` statements are removed by the interpreter under `python -O`, so the guard against a non-lid child arriving without a location silently vanished on optimised runs and `None` was passed straight through to `Resource.assign_child_resource`. Raise a `ValueError` so the check always holds. Message and behaviour are otherwise unchanged. Addresses a review comment on PR #1160. Co-Authored-By: Claude Fable 5 --- pylabrobot/resources/lid.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pylabrobot/resources/lid.py b/pylabrobot/resources/lid.py index 39d5fda3e9a..997d4440b47 100644 --- a/pylabrobot/resources/lid.py +++ b/pylabrobot/resources/lid.py @@ -119,8 +119,8 @@ def assign_child_resource( ) self._lid = resource location = location or self.get_lid_location(resource) - else: - assert location is not None, "Location must be specified if resource is not a lid." + elif location is None: + raise ValueError("Location must be specified if resource is not a lid.") return super().assign_child_resource(resource, location=location, reassign=reassign) def unassign_child_resource(self, resource: Resource): From f7f3035a9da2c6293fa769acfb8b822a7bdd77ae Mon Sep 17 00:00:00 2001 From: Camillo Moschner Date: Fri, 10 Jul 2026 14:34:03 +0100 Subject: [PATCH 5/9] `Liddable`: derive `lid` from the children instead of a `_lid` field The lid is already a child resource, so tracking it in a separate `_lid` field duplicated state that had to be kept in sync on every assign and unassign. Compute `lid` from `children` on access, matching `ResourceHolder.resource` and `PetriDishHolder.dish`, which hold a single child the same way. Removes the `_lid` field, its writes in `assign_child_resource` and the `lid` setter, and the `unassign_child_resource` override, which existed only to clear the field. Unassigning the lid child directly now updates `has_lid()` on its own. Also closes a state desync: `assign_child_resource` wrote `_lid` before calling `super()`, so a rejected assignment (a naming conflict, a callback that raises) left the parent reporting `has_lid()` as True with no lid among its children, and every later lid assignment failed with "already has a lid". `has_lid()` now scans the children rather than reading a field, costing a few microseconds on a 384-well plate. Serialization is unchanged: `_lid` was never serialized, and a round-tripped plate rebuilds its lid from `children`. Addresses a review comment on PR #1160. Co-Authored-By: Claude Fable 5 --- pylabrobot/resources/lid.py | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/pylabrobot/resources/lid.py b/pylabrobot/resources/lid.py index 997d4440b47..3c9d2ca2a33 100644 --- a/pylabrobot/resources/lid.py +++ b/pylabrobot/resources/lid.py @@ -68,23 +68,22 @@ class Liddable(Resource): ``nesting_z_height``; a resource may hold at most one lid at a time. """ - _lid: Optional[Lid] = None - def has_lid(self) -> bool: - return self._lid is not None + return self.lid is not None @property def lid(self) -> Optional[Lid]: - return self._lid + """The lid seated on this resource, or ``None``. Derived from the children.""" + return next((child for child in self.children if isinstance(child, Lid)), None) @lid.setter def lid(self, lid: Optional[Lid]) -> None: if lid is None: - if self._lid is not None: - self.unassign_child_resource(self._lid) + current_lid = self.lid + if current_lid is not None: + self.unassign_child_resource(current_lid) else: self.assign_child_resource(lid) - self._lid = lid def get_lid_location(self, lid: Lid) -> Coordinate: """Location of ``lid`` seated centred on this resource's top face, sunk by nesting_z_height. @@ -117,13 +116,7 @@ def assign_child_resource( f"Lid '{resource.name}' ({resource.get_size_x()} x {resource.get_size_y()} mm) is smaller " f"than '{self.name}' ({self.get_size_x()} x {self.get_size_y()} mm) and cannot cover it." ) - self._lid = resource location = location or self.get_lid_location(resource) elif location is None: raise ValueError("Location must be specified if resource is not a lid.") return super().assign_child_resource(resource, location=location, reassign=reassign) - - def unassign_child_resource(self, resource: Resource): - if isinstance(resource, Lid) and resource is self._lid: - self._lid = None - return super().unassign_child_resource(resource) From f32b2f2c32b47751258b236909d1976f2916bf4f Mon Sep 17 00:00:00 2001 From: Camillo Moschner Date: Fri, 10 Jul 2026 17:24:30 +0100 Subject: [PATCH 6/9] `Liddable`: drop the `_MM` suffix from `LID_UNDERSIZE_TOLERANCE` PyLabRobot measures distances in millimetres by default, so distance names carry no unit suffix. Rename the constant to match; value unchanged. Co-Authored-By: Claude Fable 5 --- pylabrobot/resources/lid.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pylabrobot/resources/lid.py b/pylabrobot/resources/lid.py index 3c9d2ca2a33..ca2ba3f9759 100644 --- a/pylabrobot/resources/lid.py +++ b/pylabrobot/resources/lid.py @@ -9,7 +9,7 @@ # A lid may be modelled up to this much smaller than the resource it covers - its rim sits just # inside the outer edge, so real plate lids run a few tenths of a mm under. A larger shortfall means # the wrong lid. Used by Liddable.assign_child_resource. -LID_UNDERSIZE_TOLERANCE_MM = 1.0 +LID_UNDERSIZE_TOLERANCE = 1.0 class Lid(Resource): @@ -109,8 +109,8 @@ def assign_child_resource( if self.has_lid(): raise ValueError(f"'{self.name}' already has a lid.") if ( - resource.get_size_x() < self.get_size_x() - LID_UNDERSIZE_TOLERANCE_MM - or resource.get_size_y() < self.get_size_y() - LID_UNDERSIZE_TOLERANCE_MM + resource.get_size_x() < self.get_size_x() - LID_UNDERSIZE_TOLERANCE + or resource.get_size_y() < self.get_size_y() - LID_UNDERSIZE_TOLERANCE ): raise ValueError( f"Lid '{resource.name}' ({resource.get_size_x()} x {resource.get_size_y()} mm) is smaller " From 7eb062764613f30cf412901339e5e81d977df5f0 Mon Sep 17 00:00:00 2001 From: Camillo Moschner Date: Fri, 10 Jul 2026 17:45:53 +0100 Subject: [PATCH 7/9] `LiquidHandler`: route a lid discarded to the `Trash` through the discard path `Trash` is a `Container`, which the lid work made `Liddable`, so `move_lid(lid, trash)` matched the "seat a lid on a Liddable" branch: it aimed at the centre of the trash's top face and then tried to assign the lid as a child, which the undersize guard rejected - after the backend drop had already run and the lid had been unassigned. Match `Trash` ahead of every branch that places a resource on its destination, in both the location and assignment chains, so a lid (or any resource) sent to the trash is simply discarded, as it was before. Co-Authored-By: Claude Fable 5 --- pylabrobot/liquid_handling/liquid_handler.py | 10 +++++++-- .../liquid_handling/liquid_handler_tests.py | 22 +++++++++++++++++++ 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/pylabrobot/liquid_handling/liquid_handler.py b/pylabrobot/liquid_handling/liquid_handler.py index 395694894e7..dd155fc55c1 100644 --- a/pylabrobot/liquid_handling/liquid_handler.py +++ b/pylabrobot/liquid_handling/liquid_handler.py @@ -2218,6 +2218,10 @@ async def drop_resource( ).rotated(destination.get_absolute_rotation()) elif isinstance(destination, Coordinate): to_location = destination + elif isinstance(destination, Trash): + # A Trash destination discards whatever is dropped on it, so it is never a seat. Checked + # before every branch that places a resource on its destination. + to_location = destination.get_location_wrt(self.deck) elif isinstance(destination, ResourceHolder): if destination.resource is not None and destination.resource is not resource: raise RuntimeError("Destination already has a plate") @@ -2269,6 +2273,10 @@ async def drop_resource( if isinstance(destination, Coordinate): to_location -= self.deck.location # passed as an absolute location, but stored as relative self.deck.assign_child_resource(resource, location=to_location) + elif isinstance(destination, Trash): + # A Trash destination discards: `resource.unassign()` above already detached it, and leaving + # it unparented is the discard. Checked before every branch that assigns to the destination. + pass elif isinstance(destination, PlateHolder): # .zero() resources destination.assign_child_resource(resource) elif isinstance(destination, ResourceHolder): # .zero() resources @@ -2287,8 +2295,6 @@ async def drop_resource( ) elif isinstance(destination, Liddable) and isinstance(resource, Lid): destination.assign_child_resource(resource) - elif isinstance(destination, Trash): - pass # don't assign to trash, resource will simply be unassigned else: destination.assign_child_resource(resource, location=to_location) diff --git a/pylabrobot/liquid_handling/liquid_handler_tests.py b/pylabrobot/liquid_handling/liquid_handler_tests.py index a6b34d2bb30..edbf1de3e63 100644 --- a/pylabrobot/liquid_handling/liquid_handler_tests.py +++ b/pylabrobot/liquid_handling/liquid_handler_tests.py @@ -313,6 +313,28 @@ async def test_move_lid(self): == lid.get_absolute_location().z ) + async def test_move_lid_to_trash(self): + # Trash is a Container, and so Liddable: a discarded lid must not be seated on it. The trash is + # always larger than the lid, so seating it would also trip the undersize guard. + plate = Plate("plate", size_x=100, size_y=100, size_z=15, ordered_items={}) + self.deck.assign_child_resource(plate, location=Coordinate(0, 0, 100)) + lid = Lid(name="lid", size_x=100, size_y=100, size_z=10, nesting_z_height=10) + plate.assign_child_resource(lid) + trash = self.deck.get_trash_area() + + with unittest.mock.patch.object( + self.lh.backend, "drop_resource", wraps=self.lh.backend.drop_resource + ) as mock_drop: + await self.lh.move_lid(lid, trash) + + self.assertEqual( + mock_drop.call_args.kwargs["drop"].destination, + trash.get_location_wrt(self.deck), + ) + self.assertFalse(plate.has_lid()) + self.assertIsNone(lid.parent) + self.assertNotIn(lid, trash.children) + async def test_move_plate_onto_resource_stack_with_lid(self): plate = Plate("plate", size_x=100, size_y=100, size_z=15, ordered_items={}) lid = Lid( From 5d459a1b23dbadb5697d9883bca6395ee395307d Mon Sep 17 00:00:00 2001 From: Camillo Moschner Date: Fri, 10 Jul 2026 17:46:02 +0100 Subject: [PATCH 8/9] `Liddable`: keep the non-lid location requirement on `Plate`, not every container Moving lid handling into `Liddable` also moved the "a non-lid child must be given a location" check onto `Container`, so troughs, tubes, wells and the trash began rejecting `assign_child_resource(child)` calls that were valid before. Only lid placement belongs in `Liddable`; drop the check there and keep it on `Plate`, where it lived, raising `ValueError` rather than the previous `assert` (which is stripped under `python -O`). Containers regain their prior behaviour. Co-Authored-By: Claude Fable 5 --- pylabrobot/resources/lid.py | 2 -- pylabrobot/resources/plate.py | 12 +++++++++++- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/pylabrobot/resources/lid.py b/pylabrobot/resources/lid.py index ca2ba3f9759..5dd3eaf8388 100644 --- a/pylabrobot/resources/lid.py +++ b/pylabrobot/resources/lid.py @@ -117,6 +117,4 @@ def assign_child_resource( f"than '{self.name}' ({self.get_size_x()} x {self.get_size_y()} mm) and cannot cover it." ) location = location or self.get_lid_location(resource) - elif location is None: - raise ValueError("Location must be specified if resource is not a lid.") return super().assign_child_resource(resource, location=location, reassign=reassign) diff --git a/pylabrobot/resources/plate.py b/pylabrobot/resources/plate.py index a33f1c9cdd0..70dfdf6edd2 100644 --- a/pylabrobot/resources/plate.py +++ b/pylabrobot/resources/plate.py @@ -18,7 +18,7 @@ from .itemized_resource import ItemizedResource from .lid import Lid, Liddable -from .resource import Resource +from .resource import Coordinate, Resource if TYPE_CHECKING: from .well import Well @@ -71,6 +71,16 @@ def __init__( if lid is not None: self.assign_child_resource(lid) + def assign_child_resource( + self, + resource: Resource, + location: Optional[Coordinate] = None, + reassign: bool = True, + ): + if not isinstance(resource, Lid) and location is None: + raise ValueError("Location must be specified if resource is not a lid.") + return super().assign_child_resource(resource, location=location, reassign=reassign) + def serialize(self) -> dict: return { **super().serialize(), From ad011eeb9654495d0b89d7b8335792b1e8af4316 Mon Sep 17 00:00:00 2001 From: Camillo Moschner Date: Fri, 10 Jul 2026 17:55:44 +0100 Subject: [PATCH 9/9] `LiquidHandler`: correct `unparented` typo and tighten the `Trash` comments `unparented` fails the spell check; reword to `parentless`. Also trim the two `Trash`-branch comments and the discard test's comment to the surrounding style. No behaviour change. Co-Authored-By: Claude Fable 5 --- pylabrobot/liquid_handling/liquid_handler.py | 6 ++---- pylabrobot/liquid_handling/liquid_handler_tests.py | 3 +-- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/pylabrobot/liquid_handling/liquid_handler.py b/pylabrobot/liquid_handling/liquid_handler.py index dd155fc55c1..869695be714 100644 --- a/pylabrobot/liquid_handling/liquid_handler.py +++ b/pylabrobot/liquid_handling/liquid_handler.py @@ -2219,8 +2219,7 @@ async def drop_resource( elif isinstance(destination, Coordinate): to_location = destination elif isinstance(destination, Trash): - # A Trash destination discards whatever is dropped on it, so it is never a seat. Checked - # before every branch that places a resource on its destination. + # discarded, never seated - kept above the branches that place a resource on its destination to_location = destination.get_location_wrt(self.deck) elif isinstance(destination, ResourceHolder): if destination.resource is not None and destination.resource is not resource: @@ -2274,8 +2273,7 @@ async def drop_resource( to_location -= self.deck.location # passed as an absolute location, but stored as relative self.deck.assign_child_resource(resource, location=to_location) elif isinstance(destination, Trash): - # A Trash destination discards: `resource.unassign()` above already detached it, and leaving - # it unparented is the discard. Checked before every branch that assigns to the destination. + # discarded: `resource.unassign()` above already detached it, so leave it detached pass elif isinstance(destination, PlateHolder): # .zero() resources destination.assign_child_resource(resource) diff --git a/pylabrobot/liquid_handling/liquid_handler_tests.py b/pylabrobot/liquid_handling/liquid_handler_tests.py index edbf1de3e63..ed88934d186 100644 --- a/pylabrobot/liquid_handling/liquid_handler_tests.py +++ b/pylabrobot/liquid_handling/liquid_handler_tests.py @@ -314,8 +314,7 @@ async def test_move_lid(self): ) async def test_move_lid_to_trash(self): - # Trash is a Container, and so Liddable: a discarded lid must not be seated on it. The trash is - # always larger than the lid, so seating it would also trip the undersize guard. + # a lid moved to the trash is discarded, not seated on it plate = Plate("plate", size_x=100, size_y=100, size_z=15, ordered_items={}) self.deck.assign_child_resource(plate, location=Coordinate(0, 0, 100)) lid = Lid(name="lid", size_x=100, size_y=100, size_z=10, nesting_z_height=10)