-
Notifications
You must be signed in to change notification settings - Fork 179
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(engine): consolidate_liquid
engine core implementation
#17458
base: edge
Are you sure you want to change the base?
Conversation
…nsolidate_papi_engine_implementation
… an aspirate would exceed the max
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.
Mostly looks good!
We should add tests for the changes in transfer_components_executor
too
if ( | ||
blow_out_properties.enabled | ||
and blow_out_properties.location == BlowoutLocation.SOURCE | ||
): | ||
raise RuntimeError( | ||
'Blowout location "source" incompatible with consolidate liquid.' | ||
' Please choose "destination" or "trash".' | ||
) |
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.
We should highlight this and mix, pre-wet behavior in docs. I guess it's okay to raise this error instead of simply skipping the blowout, as long as our built-in classes don't have blowout with source location enabled by default, because otherwise a lot of protocols will run into errors due to no fault of the users. Liquid classes are just supposed to work without having to modify for each transfer.
if ( | ||
not mix_properties.enabled | ||
or self._transfer_type == TransferType.MANY_TO_ONE | ||
): |
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.
Sorry, this is my fault; the docstring I wrote is misleading. I think we do want to run the post-dispense mix, while skipping the pre-aspirate mix. One way to do that is do the gating inside InstrumentCore.aspirate_liquid()
and InstrumentCore.dispense_liquid()
and the other is to somehow pass this function (or the class) the info that this mix is a part of aspirate/dispense.
Overview
Closes AUTH-1066.
This PR adds the implementation for
consolidate_liquid
for use with liquid classes, using a similar pattern totransfer_liquid
, but instead built around a many-to-one transfer. Like the legacyconsolidate
, we will attempt to aspirate as much liquid from as many wells as we can fit in a single tip, taking into account the air gap that may be added between movements, followed by a dispense of all liquid in the tip. This will continue until all wells have been consolidated into the destination.Certain transfer properties will not be taken into account during
consolidate
, even if they are enabled, such aspre_wet
and anymix
. Another incompatible property is setting a blowout location to besource
, as since it is impossible to consistently choose a source from the many different possible combinations of source wells given. This will raise an error. Another similar error will be raised if the transfer tip policy is set toPER_SOURCE
Test Plan and Hands on Testing
Tested on a robot with the following protocol. Integration tests have also been added, and unit tests will be added in a future PR after distribute is done and refactor work can be done to clean up the liquid class functions.
Changelog
consolidate_liquid
Review requests
Should have
pre_wet
ormix
enabled be an error, or should we just skip those steps as it is currently written?Also general look through of the logic for correctness.
Risk assessment
Low, new function and changes to common transfer code are gated behind the
MANY_TO_ONE
transfer type consolidate uses.