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

feat(api): drop tips in predetermined locations #12960

Merged

Conversation

sanni-t
Copy link
Member

@sanni-t sanni-t commented Jun 22, 2023

Closes RQA-1007

Overview

Changes the drop tip randomization to drop tips in predetermined locations that it will alternate between. I've selected these two locations based on coverage area and the ability of pipettes to reach that location:

  1. location along x-axis from well top-center, at a distance closer to left edge of labware well
  2. location along x-axis from well top-center, at a distance closer to right edge of labware well

See comments and docstrings in the PR for detailed descriptions of these locations

Test Plan

Make sure the following tests succeed:

  • A single/8- channel pipette on LEFT mount is able to go through the predetermined locations without hitting the trash or walls when:

    • trash is in column 1
    • trash is in column 3
  • A single/8- channel pipette on RIGHT mount is able to go through the predetermined locations without hitting the trash or walls when:

    • trash is in column 1
    • trash is in column 3
  • A 96-channel pipette is able to go through the predetermined locations without hitting the trash or walls when:

    • trash is in column 1
    • trash is in column 3
  • Make sure that return_tip() behavior is not affected by this

Sample test protocol:

requirements = {
	"robotType": "Flex",
	"apiLevel": "2.15"
}

def run(protocol_context):
	# For Flex:
	tiprack1 = protocol_context.load_labware("opentrons_ot3_96_tiprack_1000ul", "D2")
	pipette = protocol_context.load_instrument("p1000_96", 'left', tip_racks=[tiprack1])
	# pipette = protocol_context.load_instrument("p50_single_gen3", "left", tip_racks=[tiprack1])

	# For OT-2
	# tiprack1 = protocol_context.load_labware("opentrons_96_tiprack_300ul", 2)
	# pipette = protocol_context.load_instrument("p300_single", "right", tip_racks=[tiprack1])

	for _ in range(10):
		pipette.pick_up_tip()
		pipette.drop_tip()
		
		# resets the tiprack so that we can use the same labware for testing w/ 96 channel
		# not needed if testing with other pipettes
		tiprack1.reset()	
		protocol_context.pause(msg="Reload the tips in the tiprack")

	pipette.pick_up_tip()
	pipette.drop_tip(pipette.trash_container['A1'].top())	# Should only go to trash center. `alternateDropLocation` should be false

For QA & ABR testing, we only need to test 1, 8 & multi-channel pipettes with the OT3 trash bin in slot A3. Placing the trash in the left column will currently not calculate labware position on deck correctly so we cannot use that for testing.

Should also be tested for OT2 but is not a blocker for internal QA/ science testing for flex.

Changelog

  • removed total randomized drop tip
  • added next tip drop location getter to geometry view
  • other changes to facilitate fetching the correct locations

Review requests

  • Code review
  • Make sure the above points in test plan are tested and they cover all scenarios
  • I have tested all pipette combinations on OT3 for the fixed trash using engine commands and verified that the gantry or pipettes don't crash into anything anymore. We should also test running a python protocol that has a few tip drops.

Risk assessment

  • Less risky than dropping tip at total randomized location, which this replaces. There is still a decent risk that this would cause the pipette to go to an inaccessible location that could result in pipette/ tips bumping against the labware, but hopefully the check for fixed trash suffices for now.

@sanni-t sanni-t changed the base branch from edge to internal-release_0.12.0 June 22, 2023 17:26
@codecov
Copy link

codecov bot commented Jun 22, 2023

Codecov Report

Merging #12960 (8908a96) into internal-release_0.13.0 (e04d096) will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@                     Coverage Diff                     @@
##           internal-release_0.13.0   #12960      +/-   ##
===========================================================
- Coverage                    72.82%   72.82%   -0.01%     
===========================================================
  Files                         2364     2364              
  Lines                        64466    64464       -2     
  Branches                      7191     7191              
===========================================================
- Hits                         46946    46944       -2     
  Misses                       15841    15841              
  Partials                      1679     1679              
Flag Coverage Δ
app 71.46% <ø> (ø)
g-code-testing 96.44% <ø> (ø)
notify-server 89.13% <ø> (ø)
react-api-client 64.25% <ø> (ø)

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

Impacted Files Coverage Δ
...i/src/opentrons/protocol_api/instrument_context.py 88.95% <ø> (ø)
...c/opentrons/protocol_engine/clients/sync_client.py 100.00% <ø> (ø)
...src/opentrons/protocol_engine/commands/drop_tip.py 100.00% <ø> (ø)
...pi/src/opentrons/protocol_engine/state/geometry.py 100.00% <ø> (ø)
api/src/opentrons/protocol_engine/state/labware.py 100.00% <ø> (ø)
...pi/src/opentrons/protocol_engine/state/pipettes.py 100.00% <ø> (ø)
...a/python/opentrons_shared_data/pipette/__init__.py 94.28% <ø> (-0.16%) ⬇️

@sanni-t sanni-t marked this pull request as ready for review June 23, 2023 07:44
@sanni-t sanni-t requested review from a team as code owners June 23, 2023 07:44
@@ -225,6 +221,7 @@ def __init__(self, state: LabwareState) -> None:
state: Labware state dataclass used for all calculations.
"""
self._state = state
self._last_drop_tip_location_index = 0
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
self._last_drop_tip_location_index = 0

96: 161,
}

# PIPETTE_WELL_ACCESS_MARGIN:
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
# PIPETTE_WELL_ACCESS_MARGIN:

Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

Does this prevent tip stackup? It kind of seems like it would make the tip stackup just happen in two places.

)


# TODO (spp, 2023-06-22: should probably move this to definition)
Copy link
Member

Choose a reason for hiding this comment

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

the v2 pipette definitions either do currently or will soon (ish) have a labware-def-style list out of all the channels that should help

Copy link
Contributor

@SyntaxColoring SyntaxColoring left a comment

Choose a reason for hiding this comment

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

Mostly looks good to me, but one question about the calculations.

Comment on lines -721 to +729
"randomizeDropLocation": {
"title": "Randomizedroplocation",
"description": "Whether to randomize the location where tip is dropped within the labware. If True, this command will ignore the wellLocation provided and drop tip at a random location within a set area of the specified labware well. If False, the tip will be dropped at the top center of the well.",
"alternateDropLocation": {
"title": "Alternatedroplocation",
"description": "Whether to alternate location where tip is dropped within the labware. If True, this command will ignore the wellLocation provided and alternate between dropping tips at two predetermined locations inside the specified labware well. If False, the tip will be dropped at the top center of the well.",
Copy link
Contributor

Choose a reason for hiding this comment

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

The fact that we alternate between two locations feels like an implementation detail that we should not document as part of the public interface. I think we want to leave room for changing this back to be random, or changing it to have more points, if we determine that it's necessary to stop tips from piling up.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point! Any suggestions for the param names? 'random' is no longer relevant since we actually don't want to use random locations. I had 'cycleDropLocations' at one point but wasn't sure if that made sense.

the fixed trash in slot 12. From API version 2.15 on, the exact position
where the pipette drops the tip(s) within the trash will be randomized
the fixed trash in slot 12. From API version 2.15 on, the API will default to
alternating between two different drop tip locations within the trash container
Copy link
Contributor

Choose a reason for hiding this comment

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

See other comment about not guaranteeing exactly two alternating drop locations.

from opentrons_shared_data.pipette.dev_types import ChannelCount


SLOT_WIDTH = 128
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we take this from the deck definition instead of hard-coding it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, why do we need the slot width at all? Isn't this totally dependent on whether the trash well partially lies outside of the safely addressable X/Y rectangle?

I remember a problem a while back on the OT-2 where a single-channel pipette on the left mount couldn't touch-tip with wells in the rightmost column. So this isn't just specific to labware that are bigger than deck slots, either.

Copy link
Member Author

@sanni-t sanni-t Jun 26, 2023

Choose a reason for hiding this comment

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

Oh!! I was assuming that all pipettes are able to access the full area of all deck slots and hence the different calculations based on whether a labware is bigger than the slot. I did not know about the issue on OT2. But I would still stick with this calculation because it does add a margin to the slot width so that the tip gets dropped slightly inside the labware's or the slot's outer edge. So this should still be fine unless the unreachable area for the OT2 left pipette is more than this margin (37.5mm) from the outer edge of the slot.

) -> float:
"""Get the well x offset for DropTipWellLocation."""
drop_location_margin_from_labware_edge = (
PIPETTE_X_SPAN[cast(ChannelCount, pipette_channels)] / 2
Copy link
Contributor

@SyntaxColoring SyntaxColoring Jun 23, 2023

Choose a reason for hiding this comment

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

Can you explain why PIPETTE_X_SPAN factors into these calculations?

If I understand correctly, PIPETTE_X_SPAN is the x-dimension of the bounding box around the entire pipette.

But, for example, the leftmost addressable x-coordinate of the pipette doesn't depend on that, does it? It depends on:

  • How far -x the gantry can move
  • The x-offset of the pipette's mount within the gantry
  • The x-offsets of the pipette's nozzles from the pipette's mount

Copy link
Member Author

Choose a reason for hiding this comment

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

It is actually not the x-dimension of the bounding box of the pipette. That's why I avoided calling it the pipette mount width or anything similar to misinform what it is (although pipette x span is also not great, evidently lol). This value actually includes the bounding box + a margin if a pipette needs more than width/2 space from the edge of the slot to drop tips into the labware reliably. This is actually needed for even the ot3 to make sure that the mount assembly doesn't hit the robot walls even a tiny bit, and even if the pipette can actually drop tips to the edge of slot.

@sanni-t
Copy link
Member Author

sanni-t commented Jun 26, 2023

@sfoster1 , true that it will probably create two piles of tips instead of one and will only mitigate the tip piling issue to a certain extent, but it will be an improvement. The main reason for deciding on two locations was that the 96 channel's tip piling would actually be best resolved when there's only two drop locations since the trash bin is slightly less than twice the width of the 96 channel. If we go for 3 locations, the fallen tips will overlap more number of times than if there were only 2 locations. And I was told the tip piling is worst for 96-ch.
We could still have 2 locations only for 96 channel and more for the others but even with the other two pipettes, there's a dead zone in the trash that they can't reach (like the right half of the ot3 bin for left pipette). So the area for dropping tips reliably is much smaller than the big bin and we might be able to add maybe one or two more locations max.

So all the above things considered, I concluded we could start with a blanket 2 locations for all pipettes and then add more for pipettes and situations that make more sense. And @SyntaxColoring has a good point above considering that plan, that calling them 'alternate' or 'alternating' locations is not a good idea. So I'm open to suggestions for naming this feature.

Copy link
Contributor

@jbleon95 jbleon95 left a comment

Choose a reason for hiding this comment

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

This was tested on Friday, June 23rd on Flex DVT hardware, with both single/multi channel pipettes on both mounts, as well as the 96 channel pipette. This was only tested with trash in slot 12/A3/right column.

Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

This looks great in structure. I would love to continue to bikeshed names ("cycling drop tip"? "spread drop tip"?) but I think alternate is fine as long as we're pronouncing it like the verb.

That said, there's still a bunch of comments and stuff that say randomize, or two, or whatever. I think we can clean those up later if you want though.

sanni-t and others added 2 commits June 29, 2023 16:14
…ned locations, updated v7 schema

Co-authored-by: Max Marrone <max@opentrons.com>
@sanni-t sanni-t force-pushed the api-drop_tips_in_predetermined_locations branch from 5e6b0c0 to 8908a96 Compare June 29, 2023 20:24
@sanni-t sanni-t requested a review from a team as a code owner June 29, 2023 20:24
@sanni-t sanni-t requested review from TamarZanzouri and removed request for a team June 29, 2023 20:24
@sanni-t sanni-t changed the base branch from internal-release_0.12.0 to internal-release_0.13.0 June 29, 2023 20:24
@sanni-t sanni-t merged commit 37609fd into internal-release_0.13.0 Jun 29, 2023
45 checks passed
@sanni-t sanni-t deleted the api-drop_tips_in_predetermined_locations branch June 29, 2023 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants