Skip to content
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.

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

[rotations] add ResourceHolderMixin to fix placement in a number of resources #239

Merged
merged 38 commits into from
Sep 18, 2024
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
8380f87
Fix server tests (#209)
rickwierenga Aug 1, 2024
6d81719
Merge branch 'PyLabRobot:main' into main
jkhales Aug 15, 2024
2816879
Merge branch 'PyLabRobot:main' into main
fderop Aug 21, 2024
b990f17
Merge branch 'PyLabRobot:main' into main
fderop Aug 30, 2024
71cc945
Merge branch 'PyLabRobot:main' into main
jkhales Sep 6, 2024
41d51df
[rotations] add test for idempotent rotations
jkhales Sep 10, 2024
fcc5410
Merge branch 'main' into jkh/add-rot-test
jkhales Sep 10, 2024
f459e66
fix lint
jkhales Sep 10, 2024
eb90108
fix ResourceStack
jkhales Sep 11, 2024
cf88076
Merge branch 'main' into jkh/add-rot-test
jkhales Sep 11, 2024
726ea18
add resource_holder
jkhales Sep 11, 2024
c5d03f2
convert to a mixin
jkhales Sep 11, 2024
db96840
mixin everywhere
jkhales Sep 11, 2024
3017141
clean
jkhales Sep 11, 2024
bacfd5d
move to file
jkhales Sep 11, 2024
08b19ca
add to plate
jkhales Sep 11, 2024
49f8203
fix lint
jkhales Sep 11, 2024
9044ce1
fix typing
jkhales Sep 12, 2024
e677a02
Merge branch 'main' into jkh/add-rot-test
jkhales Sep 12, 2024
1c2b51a
update comment
jkhales Sep 16, 2024
eeef292
allow location to be overridden
jkhales Sep 17, 2024
87ba3b4
clean up machine
jkhales Sep 17, 2024
3878799
Merge branch 'main' into jkh/add-rot-test
jkhales Sep 17, 2024
1e39fa6
cr
jkhales Sep 18, 2024
522a56f
add todo
jkhales Sep 18, 2024
a9f8993
add deprecation warning
jkhales Sep 18, 2024
f9a083b
revert
jkhales Sep 18, 2024
f95e723
typo
jkhales Sep 18, 2024
9aea429
fix lint
jkhales Sep 18, 2024
828891e
remove _kwargs
rickwierenga Sep 18, 2024
5fdb842
use instances instead of class builder
rickwierenga Sep 18, 2024
c4167f8
use get_default_child_location here
rickwierenga Sep 18, 2024
4cc9561
fix type
rickwierenga Sep 18, 2024
aaa4429
freeze the ops classes and remove doc strings
rickwierenga Sep 18, 2024
7d00883
Merge branch 'main' into jkh/add-rot-test
rickwierenga Sep 18, 2024
ee84c68
freeze the ops classes and remove doc strings
rickwierenga Sep 18, 2024
0af7d77
Merge branch 'main' into jkh/add-rot-test
rickwierenga Sep 18, 2024
e7da4ac
Merge branch 'main' into jkh/add-rot-test
rickwierenga Sep 18, 2024
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
2 changes: 1 addition & 1 deletion pylabrobot/liquid_handling/backends/hamilton/STAR.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
from pylabrobot.resources.hamilton.hamilton_decks import STAR_SIZE_X, STARLET_SIZE_X
from pylabrobot.resources.liquid import Liquid
from pylabrobot.resources.ml_star import HamiltonTip, TipDropMethod, TipPickupMethod, TipSize
from pylabrobot.resources.utils import get_child_location
from pylabrobot.resources.resource_holder import get_child_location
from pylabrobot.utils.linalg import matrix_vector_multiply_3x3

T = TypeVar("T")
Expand Down
49 changes: 49 additions & 0 deletions pylabrobot/liquid_handling/liquid_handler_tests.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
""" Tests for LiquidHandler """
# pylint: disable=missing-class-docstring

import itertools
import pytest
import tempfile
from typing import Any, Dict, List, Optional, cast
Expand All @@ -9,8 +10,11 @@

from pylabrobot.liquid_handling.strictness import Strictness, set_strictness
from pylabrobot.resources import no_tip_tracking, set_tip_tracking, Liquid
from pylabrobot.resources.carrier import PlateCarrierSite
from pylabrobot.resources.errors import HasTipError, NoTipError, CrossContaminationError
from pylabrobot.resources.volume_tracker import set_volume_tracking, set_cross_contamination_tracking
from pylabrobot.resources.well import Well
from pylabrobot.resources.utils import create_ordered_items_2d

from . import backends
from .liquid_handler import LiquidHandler, OperationCallback
Expand All @@ -30,6 +34,7 @@
from pylabrobot.resources.hamilton import STARLetDeck
from pylabrobot.resources.ml_star import STF_L, HTF_L
from .standard import (
GripDirection,
Pickup,
Drop,
DropTipRack,
Expand Down Expand Up @@ -249,6 +254,50 @@ async def test_move_plate_onto_resource_stack_with_plate(self):
self.assertEqual(plate2.location.z, 15)
self.assertEqual(stack.get_absolute_size_z(), 30)

async def test_move_plate_rotation(self):
rotations = [0, 90, 270, 360]
grip_directions = [
(GripDirection.LEFT, GripDirection.RIGHT),
(GripDirection.FRONT, GripDirection.BACK),
]
site_classes = [ResourceStack, PlateCarrierSite]
rickwierenga marked this conversation as resolved.
Show resolved Hide resolved

test_cases = itertools.product(site_classes, rotations, grip_directions)

for site_class, rotation, (get_direction, put_direction) in test_cases:
with self.subTest(stack_type=site_class.__name__, rotation=rotation,
get_direction=get_direction, put_direction=put_direction):
if site_class == ResourceStack:
site = site_class(name=f"{site_class.__name__.lower()}", direction="z")
else:
site = site_class(
name=f"{site_class.__name__.lower()}",
size_x=100,
size_y=100,
size_z=15,
pedestal_size_z=1
)

self.deck.assign_child_resource(site, location=Coordinate(100, 100, 0))

plate = Plate("plate", size_x=100, size_y=100, size_z=15,
ordered_items=create_ordered_items_2d(
Well, num_items_x=1, num_items_y=1, dx=0, dy=0, dz=0,
item_dx=10, item_dy=10, size_x=10, size_y=10, size_z=10))
plate.rotate(z=rotation)
site.assign_child_resource(plate)
original_center = plate.get_absolute_location(x="c", y="c", z="c")
await self.lh.move_plate(plate, site, get_direction=get_direction,
put_direction=put_direction)
new_center = plate.get_absolute_location(x="c", y="c", z="c")

self.assertEqual(new_center, original_center,
f"Center mismatch for {site_class.__name__}, rotation {rotation}, "
f"get_direction {get_direction}, "
f"put_direction {put_direction}")
plate.unassign()
self.deck.unassign_child_resource(site)

def test_serialize(self):
serialized = self.lh.serialize()
deserialized = LiquidHandler.deserialize(serialized)
Expand Down
1 change: 1 addition & 0 deletions pylabrobot/machines/machine.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ def __init__(
backend: MachineBackend,
category: Optional[str] = None,
model: Optional[str] = None,
**_kwargs
rickwierenga marked this conversation as resolved.
Show resolved Hide resolved
):
super().__init__(name=name, size_x=size_x, size_y=size_y, size_z=size_z,
category=category, model=model)
Expand Down
10 changes: 5 additions & 5 deletions pylabrobot/plate_reading/plate_reader.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,14 @@
from pylabrobot.machines.machine import Machine, need_setup_finished
from pylabrobot.resources import Coordinate, Plate, Resource
from pylabrobot.plate_reading.backend import PlateReaderBackend
from pylabrobot.resources.utils import get_child_location
from pylabrobot.resources.resource_holder import ResourceHolderMixin


class NoPlateError(Exception):
pass


class PlateReader(Machine):
class PlateReader(ResourceHolderMixin, Machine):
""" The front end for plate readers. Plate readers are devices that can read luminescence,
absorbance, or fluorescence from a plate.

Expand Down Expand Up @@ -42,12 +42,12 @@ def __init__(

def assign_child_resource(self, resource: Resource, location: Optional[Coordinate]=None,
reassign: bool = True):
location = location or get_child_location(resource)
if len(self.children) >= 1:
raise ValueError("There already is a plate in the plate reader.")
if not isinstance(resource, Plate):
raise ValueError("The resource must be a Plate.")
super().assign_child_resource(resource, location=location)

super().assign_child_resource(resource, location=location, reassign=reassign)

def get_plate(self) -> Plate:
if len(self.children) == 0:
Expand Down Expand Up @@ -87,7 +87,7 @@ async def read_fluorescence(
emission_wavelength: int,
focal_height: float
) -> List[List[float]]:
"""
"""

Args:
excitation_wavelength: The excitation wavelength to read the fluorescence at, in nanometers.
Expand Down
14 changes: 7 additions & 7 deletions pylabrobot/resources/carrier.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import logging
from typing import Generic, List, Optional, Type, TypeVar, Union

from pylabrobot.resources.utils import get_child_location
from pylabrobot.resources.resource_holder import ResourceHolderMixin

from .coordinate import Coordinate
from .plate import Plate
Expand All @@ -14,7 +14,7 @@
logger = logging.getLogger("pylabrobot")


class CarrierSite(Resource):
class CarrierSite(ResourceHolderMixin, Resource):
""" A single site within a carrier. """

def __init__(self, name: str, size_x: float, size_y: float, size_z: float,
Expand All @@ -30,7 +30,6 @@ def assign_child_resource(
reassign: bool = True
):
self.resource = resource
location = location or get_child_location(resource)
return super().assign_child_resource(resource, location, reassign)

def unassign_child_resource(self, resource):
Expand Down Expand Up @@ -207,7 +206,6 @@ def __init__(self, name: str, size_x: float, size_y: float, size_z: float,

def assign_child_resource(self, resource: Resource, location: Optional[Coordinate] = None,
reassign: bool = True):
location = location or self._get_child_location(resource)
if isinstance(resource, ResourceStack):
if not resource.direction == "z":
raise ValueError("ResourceStack assigned to PlateCarrierSite must have direction 'z'")
Expand All @@ -219,7 +217,7 @@ def assign_child_resource(self, resource: Resource, location: Optional[Coordinat
f"resources, not {type(resource)}")
return super().assign_child_resource(resource, location, reassign)

def _get_child_location(self, resource: Resource) -> Coordinate:
def _get_sinking_depth(self, resource: Resource) -> Coordinate:
def get_plate_sinking_depth(plate: Plate):
# Sanity check for equal well clearances / dz
well_dz_set = {round(well.location.z, 2) for well in plate.get_all_children()
Expand All @@ -240,8 +238,10 @@ def get_plate_sinking_depth(plate: Plate):
z_sinking_depth = get_plate_sinking_depth(first_child)
resource.register_did_assign_resource_callback(self._update_resource_stack_location)
self.register_did_unassign_resource_callback(self._deregister_resource_stack_callback)
return -Coordinate(z=z_sinking_depth)
Copy link
Member

Choose a reason for hiding this comment

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

_get_sinking_depth should not handle callbacks

Copy link
Member

Choose a reason for hiding this comment

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

made an issue for this: #246

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'll add the issue to the code in a comment as well


return get_child_location(resource) - Coordinate(z=z_sinking_depth)
jkhales marked this conversation as resolved.
Show resolved Hide resolved
def get_default_child_location(self, resource: Resource) -> Coordinate:
return super().get_default_child_location(resource) + self._get_sinking_depth(resource)

def _update_resource_stack_location(self, resource: Resource):
""" Callback called when the lowest resource on a ResourceStack changes. Since the location of
Expand All @@ -254,7 +254,7 @@ def _update_resource_stack_location(self, resource: Resource):
resource_stack = resource.parent
assert isinstance(resource_stack, ResourceStack)
if resource_stack.children[0] == resource:
resource_stack.location = self._get_child_location(resource)
resource_stack.location = self._get_sinking_depth(resource)

def _deregister_resource_stack_callback(self, resource: Resource):
""" Callback called when a ResourceStack (or child) is unassigned from this PlateCarrierSite."""
Expand Down
3 changes: 2 additions & 1 deletion pylabrobot/resources/itemized_resource.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ def __init__(self, name: str, size_x: float, size_y: float, size_z: float,
ordered_items: Optional[Dict[str, T]] = None,
ordering: Optional[List[str]] = None,
category: Optional[str] = None,
model: Optional[str] = None):
model: Optional[str] = None,
**_kwargs):
""" Initialize an itemized resource

Args:
Expand Down
4 changes: 3 additions & 1 deletion pylabrobot/resources/plate.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

from typing import Dict, List, Optional, Sequence, Tuple, Union, cast, Literal

from pylabrobot.resources.resource_holder import ResourceHolderMixin


from .liquid import Liquid
from .itemized_resource import ItemizedResource
Expand Down Expand Up @@ -44,7 +46,7 @@ def serialize(self) -> dict:
return {**super().serialize(), "nesting_z_height": self.nesting_z_height}


class Plate(ItemizedResource[Well]):
class Plate(ResourceHolderMixin, ItemizedResource[Well]):
rickwierenga marked this conversation as resolved.
Show resolved Hide resolved
""" Base class for Plate resources. """

def __init__(
Expand Down
1 change: 1 addition & 0 deletions pylabrobot/resources/resource.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ def __init__(
rotation: Optional[Rotation] = None,
category: Optional[str] = None,
model: Optional[str] = None,
**_kwargs
):
self._name = name
self._size_x = size_x
Expand Down
41 changes: 41 additions & 0 deletions pylabrobot/resources/resource_holder.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
from typing import Optional
from pylabrobot.resources.coordinate import Coordinate
from pylabrobot.resources.resource import Resource

def get_child_location(resource: Resource) -> Coordinate:
"""
If a resource is rotated, its rotated around its local origin. This does not always
match up with the parent resource's origin. This function calculates the difference
between the two origins and calculates the location of the resource correctly.
"""
if not resource.rotation.y == resource.rotation.x == 0:
raise ValueError("Resource rotation must be 0 around the x and y axes")
if not resource.rotation.z % 90 == 0:
raise ValueError("Resource rotation must be a multiple of 90 degrees on the z axis")
location = {
0.0: Coordinate(x=0, y=0, z=0),
90.0: Coordinate(x=resource.get_size_y(), y=0, z=0),
180.0: Coordinate(x=resource.get_size_y(), y=resource.get_size_x(), z=0),
270.0: Coordinate(x=0, y=resource.get_size_x(), z=0),
}[resource.rotation.z % 360]
return location

class ResourceHolderMixin:
rickwierenga marked this conversation as resolved.
Show resolved Hide resolved
"""
A mixin class for resources that can hold other resources, like a plate or a lid.

This applies a linear transformation after the rotation to correctly place the child resource.
"""

def get_default_child_location(self, resource: Resource) -> Coordinate:
return get_child_location(resource)

def assign_child_resource(
self,
resource: Resource,
location: Optional[Coordinate] = None,
reassign: bool = True
):
location = location or self.get_default_child_location(resource)
# mypy doesn't play well with the Mixin pattern
return super().assign_child_resource(resource, location, reassign) # type: ignore
41 changes: 29 additions & 12 deletions pylabrobot/resources/resource_stack.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
from typing import List, Optional

from pylabrobot.resources.resource_holder import ResourceHolderMixin, get_child_location
from pylabrobot.resources.resource import Resource
from pylabrobot.resources.coordinate import Coordinate
from pylabrobot.resources.plate import Lid, Plate


class ResourceStack(Resource):
class ResourceStack(ResourceHolderMixin, Resource):
""" ResourceStack represent a group of resources that are stacked together and act as a single
unit. Stacks can grow be configured to be able to grow in x, y, or z direction. Stacks growing
in the x direction are from left to right. Stacks growing in the y direction are from front to
Expand Down Expand Up @@ -95,26 +96,42 @@ def get_actual_resource_height(resource: Resource) -> float:
return max(get_actual_resource_height(child) for child in self.children)
return sum(get_actual_resource_height(child) for child in self.children)

def assign_child_resource(self, resource: Resource, location: Optional[Coordinate] = None,
reassign: bool = False):

def get_resource_stack_size(self) -> Coordinate:
if self.direction == "x":
resource_location = Coordinate(self.get_size_x(), 0, 0)
Copy link
Member

Choose a reason for hiding this comment

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

potentially just put this in get_default_child_location. It seems incorrect to have a get_size method return a coordinate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's used in the assign lid to resource stack special case. I can rename to something if you don't like the size word.

Copy link
Member

Choose a reason for hiding this comment

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

for this particular function i don't like it :)

elif self.direction == "y":
resource_location = Coordinate(0, self.get_size_y(), 0)
elif self.direction == "z":
resource_location = Coordinate(0, 0, self.get_size_z())

# special handling for putting a lid on a plate
if len(self.children) > 0:
top_item = self.get_top_item()
if isinstance(resource, Lid) and isinstance(top_item, Plate):
resource_location.z -= resource.nesting_z_height
top_item.assign_child_resource(resource, location=resource_location)
return
else:
raise ValueError("self.direction must be one of 'x', 'y', or 'z'")

super().assign_child_resource(resource, location=resource_location)
return resource_location

def get_default_child_location(self, resource: Resource) -> Coordinate:
return super().get_default_child_location(resource) + self.get_resource_stack_size()

def assign_child_resource(self, resource: Resource, location: Optional[Coordinate] = None,
reassign: bool = False):

# special handling for putting a lid on a plate
if len(self.children) > 0:
top_item = self.get_top_item()
if isinstance(resource, Lid) and isinstance(top_item, Plate):
resource_location = self.get_resource_stack_size()
resource_location.z -= resource.nesting_z_height
top_item.assign_child_resource(
resource,
location=get_child_location(resource) + resource_location
)
return
rickwierenga marked this conversation as resolved.
Show resolved Hide resolved

super().assign_child_resource(
resource,
location=location,
reassign=reassign
)

def unassign_child_resource(self, resource: Resource):
if self.direction == "z" and resource != self.children[-1]: # no floating resources
Expand Down
18 changes: 0 additions & 18 deletions pylabrobot/resources/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -199,21 +199,3 @@ def query(
z=z,
))
return matched

rickwierenga marked this conversation as resolved.
Show resolved Hide resolved
def get_child_location(resource: Resource) -> Coordinate:
"""
If a resource is rotated, its rotated around its local origin. This does not always
match up with the parent resource's origin. This function calculates the difference
between the two origins and calculates the location of the resource correctly.
"""
if not resource.rotation.y == resource.rotation.x == 0:
raise ValueError("Resource rotation must be 0 around the x and y axes")
if not resource.rotation.z % 90 == 0:
raise ValueError("Resource rotation must be a multiple of 90 degrees on the z axis")
location = {
0.0: Coordinate(x=0, y=0, z=0),
90.0: Coordinate(x=resource.get_size_y(), y=0, z=0),
180.0: Coordinate(x=resource.get_size_y(), y=resource.get_size_x(), z=0),
270.0: Coordinate(x=0, y=resource.get_size_x(), z=0),
}[resource.rotation.z % 360]
return location
3 changes: 2 additions & 1 deletion pylabrobot/shaking/shaker.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,12 @@
from typing import Optional

from pylabrobot.machines.machine import Machine
from pylabrobot.resources.resource_holder import ResourceHolderMixin

from .backend import ShakerBackend


class Shaker(Machine):
class Shaker(ResourceHolderMixin, Machine):
""" A shaker machine """

def __init__(
Expand Down
Loading
Loading