Skip to content

ResourceStack, Plate, Lid inheritance logic fix #546

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
+39 −9
Open

ResourceStack, Plate, Lid inheritance logic fix #546

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8307b34
fix: resource stack handling for Lid in LiquidHandler and PlateHolder.
ericguan04 Jun 3, 2025
5f517a1
Merge branch 'PyLabRobot:main' into main
ericguan04 Jun 3, 2025
5acdc82
fix: improve formatting in LiquidHandler class for better readability…
ericguan04 Jun 3, 2025
0a4eeef
Fixed linting, formatting, and type inspection issues
ericguan04 Jun 3, 2025
8bb93da
Remove ROS line in .vscode/settings.json
ericguan04 Jun 5, 2025
3ead5b1
Add comment
ericguan04 Jun 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 20 additions & 7 deletions pylabrobot/liquid_handling/liquid_handler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1813,7 +1813,7 @@ async def drop_resource(
)

# get the location of the destination
if isinstance(destination, ResourceStack):
if isinstance(destination, ResourceStack) and not isinstance(resource, Lid):
assert (
destination.direction == "z"
), "Only ResourceStacks with direction 'z' are currently supported"
Expand All @@ -1835,13 +1835,26 @@ async def drop_resource(
resource.rotated(z=resource_rotation_wrt_destination_wrt_local)
).rotated(destination.get_absolute_rotation())
to_location = destination.get_absolute_location() + adjusted_plate_anchor
elif isinstance(destination, Plate) and isinstance(resource, Lid):
# Automatically assign lid to plate as child when dropping a lid to a plate (on or off resource stack)
elif isinstance(destination, (Plate, ResourceStack)) and isinstance(resource, Lid):
lid = resource
plate_location = destination.get_absolute_location()
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
if isinstance(destination, ResourceStack):
if destination.direction != "z":
raise ValueError("Only ResourceStacks with direction 'z' are currently supported")
if len(destination.children) == 0:
to_location = destination.get_absolute_location(z="top")
else:
top_item = destination.get_top_item()
if isinstance(top_item, Plate):
destination = top_item
else:
to_location = destination.get_absolute_location(z="top")
if isinstance(destination, Plate):
plate_location = destination.get_absolute_location()
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
Comment on lines +1841 to +1857
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps it makes more sense to make these different cases ((destination = ResourceStack, resource = Lid), (destination = Plate, resource = Lid)) into two 'top level' if statements?

or merge the (destination = ResourceStack, resource = Lid) case into the (destination=ResourceStack) cause above?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps it makes more sense to make these different cases ((destination = ResourceStack, resource = Lid), (destination = Plate, resource = Lid)) into two 'top level' if statements?

or merge the (destination = ResourceStack, resource = Lid) case into the (destination=ResourceStack) cause above?

Hello, thank you for the suggestions! I noticed before that you left a comment for carrier.py but I can't see it here anymore. I thought the method you outlined there was very clean and a more catch all solution for finding the parent ResourceStack. For the liquid_handler.py, I can look into making the logic more clean via your suggestions above!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i deleted the other comment because I realized i was wrong :)

(i made a comment suggesting to get the resource stack in the "parent-path" until the root. That is wrong because this method specifically exists to update the location of the first resource in the resource stack: if there are resources in between the given resource and the stack, it should probably be the location of the direct child of the resource that determines its location, not some [grand]child.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah got it, that makes sense!

else:
to_location = destination.get_absolute_location()

Expand Down
21 changes: 19 additions & 2 deletions pylabrobot/resources/carrier.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,8 +234,25 @@ def _update_resource_stack_location(self, resource: Resource):
Args:
resource: The Resource on the ResourceStack tht was assigned.
"""
resource_stack = resource.parent
assert isinstance(resource_stack, ResourceStack)

if isinstance(resource, Lid):
lid_parent = resource.parent
if lid_parent is None:
raise ValueError("Lid has no parent. ResourceStack not found for Lid")
if isinstance(lid_parent, ResourceStack):
resource_stack = lid_parent
elif isinstance(lid_parent.parent, ResourceStack):
resource_stack = lid_parent.parent
Comment on lines +244 to +245
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in this case, the parent is likely a plate. in that case, the location of the plate wrt the stack should be updated (and the plate should have a callback already registered) - not the lid.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you elaborate a little bit on what should be done with the plate / how the plate should be updated? For some more context, in the original codebase, I ran into an issue with move_lid where moving a lid to a plate on a stack would raise an exception because of the assert isinstance(resource_stack, ResourceStack) - 238. Thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be removing this case from the if statement

i think the problem with moving a lid to a plate on a stack is with liquid handler (since this callback should not be called in that case. if it is, there's a problem elsewhere)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it! I was also a little confused as to why this callback was called. I tried tracing out the code earlier today to see why but I had some trouble following it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lid gets properly attached (lh.summary() shows this) with move_lid, but the code won't fully execute because of the assertion line

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then it seems that this callback is incorrectly called.

it should only be called when the first child of the ResourceStack changes

else:
raise ValueError("ResourceStack not found for Lid")
else:
if resource.parent is None:
raise ValueError("ResourceStack not found for resource")
if not isinstance(resource.parent, ResourceStack):
raise TypeError(
f"Resource {resource} is not a child of a ResourceStack, but of {type(resource.parent)}"
)
resource_stack = resource.parent
if resource_stack.children[0] == resource:
resource_stack.location = self.get_default_child_location(resource)

Expand Down