Container: accept lids via a Liddable mixin, centred on the top face#1160
Merged
Conversation
BioCam
force-pushed
the
generalize-lid-support
branch
2 times, most recently
from
396a611 to
659b93d
Compare
rickwierenga
reviewed
Jul 9, 2026
Member
There was a problem hiding this comment.
making lid a lazily computed property would really simplify this class
rickwierenga
reviewed
Jul 9, 2026
Comment on lines
+122
to
+123
| else: | ||
| assert location is not None, "Location must be specified if resource is not a lid." |
BioCam
added a commit
to BioCam/pylabrobot
that referenced
this pull request
Jul 10, 2026
…d 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 PyLabRobot#1160. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
BioCam
added a commit
to BioCam/pylabrobot
that referenced
this pull request
Jul 10, 2026
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 PyLabRobot#1160. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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) <noreply@anthropic.com>
… 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 <noreply@anthropic.com>
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) <noreply@anthropic.com>
…d 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 PyLabRobot#1160. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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 PyLabRobot#1160. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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 <noreply@anthropic.com>
…card 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 <noreply@anthropic.com>
…ry 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 <noreply@anthropic.com>
…mments `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 <noreply@anthropic.com>
BioCam
force-pushed
the
generalize-lid-support
branch
from
d516a4a to
ad011ee
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Platewas the only resource that could hold a lid, and placement was origin-aligned - so a lid that isn't the plate's exact footprint (a trough lid, or a slightly-undersized plate lid) sat in a corner.Proposes a
Liddablemixin shared byPlateandContainer, with the lid centred on the parent's top face. (Where the mixin lives is open - it's small and easy to relocate.)lid.py:Lid(moved fromplate.py, re-exported for back-compat) +Liddable(.lid,has_lid(), centredget_lid_location, double-lid and minimum-size guards).Plate(Liddable, ItemizedResource["Well"])andContainer(Liddable, Resource)- troughs, tubes, petri dishes and wells inherit lids;drop_resource/move_lidgate onLiddable.Footprint-matched lids are byte-identical (verified across every
with_lidplate); off-footprint lids now centre - sub-mm (the one rotated STAR lid test is re-pinned). Rotation alignment is left to the move/arm layer, so the resource stays geometry-only.Tests:
lid_tests.py(centred placement, container lids, guards, serialize round-trip); full suite, ruff and mypy clean.🤖 Generated with Claude Code