-
Notifications
You must be signed in to change notification settings - Fork 184
feat(api): allow disposal locations as destinations for transfer and consolidate with liquid class #18196
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
Conversation
…d_disposal_locations_as_lc_transfer_dest
…d_disposal_locations_as_lc_transfer_dest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few docstring comments. Thanks for making the initial changes! If you take any of my suggestions, make sure to apply them to the parallel places across the other two _with_liquid
methods.
raise ValueError( | ||
"Sources and destinations should be of the same length in order to perform a transfer." | ||
" To transfer liquid from one source to many destinations, use 'distribute_liquid'," | ||
" to transfer liquid onto one destinations from many sources, use 'consolidate_liquid'." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
editor brain, engage
" to transfer liquid onto one destinations from many sources, use 'consolidate_liquid'." | |
" to transfer liquid to one destination from many sources, use 'consolidate_liquid'." |
:param source: A single well or group of wells for a multi-channel pipette to | ||
aspirate liquid from. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get what we're going for here, but maybe it's clearer if we say something like this:
A single well for the pipette to target. Single-channel pipettes and multi-channel pipettes in
SINGLE
configuration will only aspirate from this well. Multi-channel pipettes in other configurations will place the primary nozzle over this well and aspirate from several wells.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ecormany you are correct that we should rewrite the description to be clearer. But your suggested description implies that one can only specify a single well here, which is not true. If you are using a multi-channel pipette, then you can specify a list of wells here as long as all the wells are simultaneously accessed by the active channels.
@@ -1641,7 +1645,7 @@ def transfer_with_liquid_class( | |||
:param volume: The amount, in µL, to aspirate from each source and dispense to | |||
each destination. | |||
:param source: A single well or a list of wells to aspirate liquid from. | |||
:param dest: A single well or a list of wells to dispense liquid into. | |||
:param dest: A single well, list of wells, or trash bin or waste chute to dispense liquid into. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two options here.
Treat all as equals:
A single well, list of wells, trash bin, or waste chute…
Combine trash containers, using that term:
A single well, list of wells, or trash container…
We use "trash container" in the docstring for several other location-specifying parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The term "trash container" includes a labware set as the trash as well, which can be confusing here. I think the 'treat all as equal' option is better here.
Should we go ahead and give this capability to standard |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Just some small suggestions
@@ -1641,7 +1645,7 @@ def transfer_with_liquid_class( | |||
:param volume: The amount, in µL, to aspirate from each source and dispense to | |||
each destination. | |||
:param source: A single well or a list of wells to aspirate liquid from. | |||
:param dest: A single well or a list of wells to dispense liquid into. | |||
:param dest: A single well, list of wells, or trash bin or waste chute to dispense liquid into. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The term "trash container" includes a labware set as the trash as well, which can be confusing here. I think the 'treat all as equal' option is better here.
:param source: A single well or group of wells for a multi-channel pipette to | ||
aspirate liquid from. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ecormany you are correct that we should rewrite the description to be clearer. But your suggested description implies that one can only specify a single well here, which is not true. If you are using a multi-channel pipette, then you can specify a list of wells here as long as all the wells are simultaneously accessed by the active channels.
api/tests/opentrons/protocol_api/core/engine/test_transfer_components_executor.py
Outdated
Show resolved
Hide resolved
api/tests/opentrons/protocol_api/core/engine/test_transfer_components_executor.py
Outdated
Show resolved
Hide resolved
api/tests/opentrons/protocol_api/core/engine/test_transfer_components_executor.py
Outdated
Show resolved
Hide resolved
…consolidate with liquid class (#18196) adds support for having a trash bin or waste chute as a destination for `transfer_with_liquid_class` and `consolidate_with_liquid_class`
…consolidate with liquid class (#18196) adds support for having a trash bin or waste chute as a destination for `transfer_with_liquid_class` and `consolidate_with_liquid_class`
…consolidate with liquid class (#18196) adds support for having a trash bin or waste chute as a destination for `transfer_with_liquid_class` and `consolidate_with_liquid_class`
…consolidate with liquid class (#18196) adds support for having a trash bin or waste chute as a destination for `transfer_with_liquid_class` and `consolidate_with_liquid_class`
…consolidate with liquid class (#18196) adds support for having a trash bin or waste chute as a destination for `transfer_with_liquid_class` and `consolidate_with_liquid_class`
…consolidate with liquid class (#18196) adds support for having a trash bin or waste chute as a destination for `transfer_with_liquid_class` and `consolidate_with_liquid_class`
Overview
Addresses AUTH-1477.
This PR adds support for having a trash bin or waste chute as a destination for
transfer_with_liquid_class
andconsolidate_with_liquid_class
. Only one trash bin or waste chute can be assigned as the destination, and this is separate from thetrash_location
argument (though there is nothing stopping one from making it the same).When a trash bin is a destination, a few steps of the liquid class transfer process are modified. We will not submerge or retract into the trash bin or waste chute, but simply just move to the default location (unless a trash bin/waste chute with an offset is given via
.top()
) and dispense from there. Additionally, we will not touch tip inside the trash bin or waste chute, but we will still blow-out.Test Plan and Hands on Testing
Tested the following protocol on a robot
Changelog
transfer_with_liquid_class
andconsolidate_with_liquid_class
dest
argument can now acceptTrashBin
andWasteChute
objectsReview requests
Should we keep the delays used after the submerge and retract movement steps? We aren't actually submerging or retracting out of liquid, but it's unclear whether we should leave this in for less differences between well/trash destination, or if they are completely unnecessary.
Also, this PR only allows a singular trash bin or waste chute as a destination for transfer, you can not mix and match disposal locations in a list. This matches what PD allows, but should we be more permissive in the Python API?
Risk assessment
Low-medium. Really only touches liquid class submerge and retract after single dispense steps as far as implementation changes. Any regressions should be caught in snapshot tests.