feat(engine): add mmFromEdge parameter to touchTip #17107
Conversation
| @@ -45,6 +49,12 @@ class TouchTipParams(PipetteIdMixin, WellLocationMixin): | |||
| ), | |||
| ) | |||
|
|
|||
| mmToEdge: Optional[float] = Field( | |||
| None, | |||
| description="Offset away from the the well edge, in millimeters." | |||
There was a problem hiding this comment.
If you describe this value as the offset "from the the well edge," do you want to call it mmFromEdge?
There was a problem hiding this comment.
Hmmm, you have a point here, it does make more sense to me reading the variable name to have it by "from" edge.
| @@ -89,6 +99,11 @@ async def execute( | |||
| labware_id = params.labwareId | |||
| well_name = params.wellName | |||
|
|
|||
| if params.radius != 1.0 and params.mmToEdge is not None: | |||
There was a problem hiding this comment.
Is the radius of 1.0 a ratio, or is it a value in millimeters?
There was a problem hiding this comment.
It's a ratio, where 1.0 is the exact radius of the well as defined in the labware definition
| right=center + Point(x=x_radius, y=0, z=0), | ||
| left=center + Point(x=-x_radius, y=0, z=0), | ||
| right=center + Point(x=x_radius - mm_to_edge, y=0, z=0), | ||
| left=center + Point(x=-x_radius + mm_to_edge, y=0, z=0), |
There was a problem hiding this comment.
Points are supposed to be an arithmetic type, right? Is it intended to be used like:
right = center + Point(x=x_radius) - Point(x=mm_to_edge)
left = center + Point(x=x_radius) + Point(x=mm_to_edge)
?
| @@ -327,6 +327,7 @@ def get_touch_tip_waypoints( | |||
| labware_id: str, | |||
| well_name: str, | |||
| center_point: Point, | |||
| mm_to_edge: float, | |||
There was a problem hiding this comment.
Why isn't this defaulted to 0.0 here?
| critical_point=CriticalPoint.XY_CENTER, | ||
| ), | ||
| Waypoint( | ||
| position=Point(x=44, y=55, z=66), |
There was a problem hiding this comment.
Are these dummy return values supposed to be consistent with the size and center of the well at all?
There was a problem hiding this comment.
Nope, just dummy data, value doesn't matter only type for this test only structure
| labware_id="123", | ||
| well_name="A3", | ||
| radius=1.0, | ||
| mm_to_edge=0.789, |
There was a problem hiding this comment.
I'm confused what this is testing. Where in these tests do they demonstrate the right=center+Point(x=x_radius-mm_to_edge) etc. calculations that you implemented?
I was going to call you out for using non-fraction-of-2 values in the test :) expecting that your test would fail because 0.789 is not representable exactly as a float and you would hit some inexact rounding behavior when your test ran, but I'm not sure where in this test you trigger the arithmetic.
There was a problem hiding this comment.
This test is testing the strucutre of the touchTip implementation, this one specifically checking that the parameter mmToEdge is correctly passed to the get_touch_tip_waypoints function in the motion state view.
test_move_types.py contains a test that is specifically checking that mm to edge is added/subtracted from the points, and that will fail if the logic in get_edge_point_list is changed incorrectly.
|
Thanks for fixing the test. I would still consider |
jbleon95
changed the title
Overview
Closes AUTH-1185.
This PR adds a new optional parameter
mmFromEdgeto the protocol enginetouchTipcommand, primarily for usage for transfer liquids defined by a liquid class. If provided, it adds an offset in millimeters to the left, right, forward and back touch tip waypoints, where a positive value is closer to the well center and a negative value targets outside the well. If this parameter is not provided, it will default to 0. There are no limits or checks on the value given, and if it is over the radius of the well it will result in touch tip going in the opposite directions (left then right, and then back and forward). It is up to the caller of this to provide a sensible value.This parameter is not compatible when providing a
radiusthat is not equal to 1.0 (which is whatradiusdefaults to if not included), and will raise a newTouchTipIncompatibleArgumentsErrorif that occurs. The rationale behind this is that with a non-default radius, you are no longer representing the edges of the well.Test Plan and Hands on Testing
Tested command on robot with Postman along with unit test coverage.
Changelog
mmFromEdgeparameter totouchTipRisk assessment
Low, not providing this value does not change the output of existing touch tip commands at all.