refactor(engine): dodge thermocycler when moving pipettes #8931

sanni-t · 2021-11-29T22:30:29Z

Addresses #8911

Overview

Makes the pipettes move around the thermocycler instead of over it/ crashing into it. The logic is copied from the legacy movement planning code as is. It adds an intermediate waypoint over slot 5 when performing a move that would otherwise have to go over the slots containing a thermocycler.

Changelog

added thermocycler dodging to motion.py: pipettes will move through slot 5 to dodge.
added should_dodge_thermocycler function to ~~geometry.py~~ ModuleView
updated tests

Review requests

The above logic needs tracking of current well. What should happen if current_well is not known?
Is this missing anything?
Test on a real robot. Give it a move from and to locations that are in the BAD_PAIRS, once with thermocycler loaded and once without and make sure that the movement is routed over slot 5 when thermocycler is present.

Risk assessment

Medium. Should only affect limited set of movements when thermocycler is being used.

codecov · 2021-11-29T22:33:29Z

Codecov Report

Merging #8931 (cccfc93) into edge (3142446) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             edge    #8931      +/-   ##
==========================================
+ Coverage   74.79%   74.81%   +0.02%     
==========================================
  Files        1860     1860              
  Lines       49294    49355      +61     
  Branches     4852     4852              
==========================================
+ Hits        36869    36926      +57     
- Misses      11581    11585       +4     
  Partials      844      844

Flag	Coverage Δ
api	`84.92% <100.00%> (+0.03%)`	⬆️
robot-server	`91.99% <ø> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
api/src/opentrons/protocol_engine/state/state.py	`97.64% <ø> (ø)`
...pi/src/opentrons/protocol_engine/state/geometry.py	`100.00% <100.00%> (ø)`
api/src/opentrons/protocol_engine/state/labware.py	`100.00% <100.00%> (ø)`
api/src/opentrons/protocol_engine/state/modules.py	`72.52% <100.00%> (+4.17%)`	⬆️
api/src/opentrons/protocol_engine/state/motion.py	`100.00% <100.00%> (ø)`
...c/opentrons/protocol_engine/execution/equipment.py	`89.65% <0.00%> (-3.12%)`	⬇️
robot-server/robot_server/hardware.py	`75.24% <0.00%> (-1.00%)`	⬇️
.../opentrons/protocol_engine/commands/load_module.py	`100.00% <0.00%> (ø)`
.../protocol_runner/legacy_labware_offset_provider.py	`100.00% <0.00%> (ø)`

mcous

Organizational comments below

api/src/opentrons/protocol_engine/state/geometry.py

mcous · 2021-11-30T19:34:13Z

api/src/opentrons/protocol_engine/state/geometry.py

@@ -216,3 +232,34 @@ def get_tip_drop_location(
                z=well_location.offset.z - tip_z_offset,
            )
        )
+
+    def should_dodge_thermocycler(


Would this make more sense in the MotionView class? All other logic in this class is related to static geometry

If this is put in the MotionView class then the test for get_movement_waypoints becomes much more complicated, and would require stubbing out an internal function, which we are trying to avoid.

Oops, I think I meant ModuleView. If the signature changes to

def should_dodge_thermocycler( self, start: DeckSlotName, end: DeckSlotName, ) -> bool:

...then ModuleView has everything it needs, and MotionView can call it by calling self._geometry.get_ancestor_slot_name

Another idea that does not involve moving this function would be to have it return the a list of extra_waypoints directly (or an empty list), simplifying the logic in MotionView

mcous · 2021-11-30T19:38:19Z

api/src/opentrons/protocol_engine/state/geometry.py

+        if self._modules.get_all() and ModuleModel.THERMOCYCLER_MODULE_V1 in [
+            mod.model for mod in self._modules.get_all()
+        ]:
+            transit = (
+                get_slot_name(from_labware).value,
+                get_slot_name(to_labware).value,
+            )
+            if transit in BAD_PAIRS:
+                return True


This could all be moved to a selector in ModulesView, and it might be easier to test/reason with if you do. Something along the lines of:

def should_dodge_thermocycler(self, start: DeckSlotName, end: DeckSlotName) -> bool: has_tc = any( m.model == ModuleModel.THERMOCYCLER_MODULE_V1 for m in self._state.modules_by_id.values() ) ...

This logic requires access to both Modules and Labware. GeometryView already has both so it seems appropriate to keep it here. Even though should_dodge_thermocycler primarily depends on the presence of a module, it is a concern of how non-module labware are placed on deck so GeometryView does feel like the appropriate place, no?

…e internal method

SyntaxColoring

Code LGTM if this works on a robot.

api/tests/opentrons/protocol_engine/state/test_labware_view.py

SyntaxColoring · 2021-11-30T22:31:04Z

api/tests/opentrons/protocol_engine/state/test_labware_view.py

+    slot_def = standard_deck_def["locations"]["orderedSlots"][1]
+    slot_pos = slot_def["position"]
+    expected_center = Point(
+        x=slot_pos[0] + slot_def["boundingBox"]["xDimension"] / 2,
+        y=slot_pos[1] + slot_def["boundingBox"]["yDimension"] / 2,
+        z=slot_pos[2] + slot_def["boundingBox"]["zDimension"] / 2,
+    )
+    result = subject.get_slot_center_position(DeckSlotName.SLOT_2)
+    assert result == expected_center


This test is mirroring the production code in how it calculates the center point. Should we hard-code absolute deck coordinates in this test, instead?

mcous · 2021-11-30T22:57:26Z

api/src/opentrons/protocol_engine/state/geometry.py

@@ -216,3 +232,34 @@ def get_tip_drop_location(
                z=well_location.offset.z - tip_z_offset,
            )
        )
+
+    def should_dodge_thermocycler(


Oops, I think I meant ModuleView. If the signature changes to

def should_dodge_thermocycler( self, start: DeckSlotName, end: DeckSlotName, ) -> bool:

...then ModuleView has everything it needs, and MotionView can call it by calling self._geometry.get_ancestor_slot_name

Another idea that does not involve moving this function would be to have it return the a list of extra_waypoints directly (or an empty list), simplifying the logic in MotionView

mcous · 2021-11-30T22:58:40Z

api/src/opentrons/protocol_engine/state/geometry.py

+        if self._modules.get_all() and ModuleModel.THERMOCYCLER_MODULE_V1 in [
+            mod.model for mod in self._modules.get_all()
+        ]:


Why the multiple calls to get_all? Could we create ModuleView.has_thermocycler or something instead? This suggestion is moot if should_dodge_thermocycler moves to ModuleView

mcous · 2021-11-30T22:59:46Z

api/tests/opentrons/protocol_engine/state/test_motion_view.py

@@ -188,6 +188,8 @@ class WaypointSpec:
    labware_z: Optional[float] = None
    all_labware_z: Optional[float] = None
    max_travel_z: float = 50
+    should_dodge_thermocycler: bool = False
+    extra_waypoints: Sequence[Tuple[float, float]] = ()


 # TODO(mc, 2021-01-14): these tests probably need to be rethought; this fixture


Sorry for leaving these tests in such an awful state...

mcous · 2021-11-30T23:01:26Z

api/src/opentrons/protocol_engine/state/motion.py

@@ -104,6 +105,16 @@ def get_movement_waypoints(
        else:
            move_type = MoveType.GENERAL_ARC
            min_travel_z = self._geometry.get_all_labware_highest_z()
+            if location is not None:


Can we somehow encode this branch logic into should_dodge_thermocycler? Perhaps by making the starting slot/labware argument optional?

~~How would that relate to the bad pairs then?~~ nvm, I misunderstood. Sure, that won't be too bad

Decided to leave this as is. If location is None then location.labware_id doesn't exist. So we have to check it here anyway.

mcous · 2021-11-30T23:03:56Z

Code looks pretty good! Will approve once tested on a real robot (lmk if that's already happened)

sanni-t · 2021-11-30T23:24:52Z

Code looks pretty good! Will approve once tested on a real robot (lmk if that's already happened)

Yep, tested on a robot and verified that pipette moves through slot 5 when thermocycler is loaded while moves directly when there's no thermocycler.

mcous

🌡️

Co-authored-by: Max Marrone <max@opentrons.com>

mcous

I know this is already merged but just chiming in to say I like where this ended up 👍

added thermocycler dodging, need to check for current_well tracking

fe0d767

sanni-t added 2 commits November 29, 2021 18:30

corrected thermocycler lookup

075c993

fixed errors, added tests

cf818ba

mcous reviewed Nov 30, 2021

View reviewed changes

sanni-t added 2 commits November 30, 2021 15:24

get slot center, not origin

581f8c1

address comments- switch to SlotTransit namedtuple, make get_slot_nam…

32de850

…e internal method

sanni-t marked this pull request as ready for review November 30, 2021 21:20

sanni-t requested a review from a team as a code owner November 30, 2021 21:20

sanni-t added 2 commits November 30, 2021 16:39

Merge branch 'edge' into moves_dodge_thermocycler

04f3799

fix test

a2e2879

SyntaxColoring approved these changes Nov 30, 2021

View reviewed changes

SyntaxColoring mentioned this pull request Nov 30, 2021

Flesh out Protocol Engine's handling of Thermocycler movements #8911

Closed

2 tasks

mcous reviewed Nov 30, 2021

View reviewed changes

mcous approved these changes Dec 1, 2021

View reviewed changes

sanni-t and others added 3 commits December 1, 2021 14:48

Update api/tests/opentrons/protocol_engine/state/test_labware_view.py

5f70cfe

Co-authored-by: Max Marrone <max@opentrons.com>

hard-code slot coordinates in test

285e615

moved should_dodge_thermocycler to ModuleView with updated signature

cccfc93

sanni-t merged commit a0e5924 into edge Dec 2, 2021

nusrat813 self-requested a review December 2, 2021 16:55

mcous reviewed Dec 2, 2021

View reviewed changes

sanni-t deleted the moves_dodge_thermocycler branch December 2, 2021 19:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(engine): dodge thermocycler when moving pipettes #8931

refactor(engine): dodge thermocycler when moving pipettes #8931

sanni-t commented Nov 29, 2021 •

edited

Loading

codecov bot commented Nov 29, 2021 •

edited

Loading

mcous left a comment

mcous Nov 30, 2021

sanni-t Nov 30, 2021

mcous Nov 30, 2021

mcous Nov 30, 2021

sanni-t Nov 30, 2021

SyntaxColoring left a comment

SyntaxColoring Nov 30, 2021

mcous Nov 30, 2021

mcous Nov 30, 2021

mcous Nov 30, 2021

mcous Nov 30, 2021

sanni-t Nov 30, 2021 •

edited

Loading

sanni-t Dec 1, 2021 •

edited

Loading

mcous commented Nov 30, 2021

sanni-t commented Nov 30, 2021

mcous left a comment

mcous left a comment •

edited

Loading

refactor(engine): dodge thermocycler when moving pipettes #8931

refactor(engine): dodge thermocycler when moving pipettes #8931

Conversation

sanni-t commented Nov 29, 2021 • edited Loading

Overview

Changelog

Review requests

Risk assessment

codecov bot commented Nov 29, 2021 • edited Loading

Codecov Report

mcous left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SyntaxColoring left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanni-t Nov 30, 2021 • edited Loading

Choose a reason for hiding this comment

sanni-t Dec 1, 2021 • edited Loading

Choose a reason for hiding this comment

mcous commented Nov 30, 2021

sanni-t commented Nov 30, 2021

mcous left a comment

Choose a reason for hiding this comment

mcous left a comment • edited Loading

Choose a reason for hiding this comment

sanni-t commented Nov 29, 2021 •

edited

Loading

codecov bot commented Nov 29, 2021 •

edited

Loading

sanni-t Nov 30, 2021 •

edited

Loading

sanni-t Dec 1, 2021 •

edited

Loading

mcous left a comment •

edited

Loading