feat(api): implement engine core distribute liquid #17577
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #17577 +/- ##
===========================================
- Coverage 63.08% 25.65% -37.44%
===========================================
Files 2840 2840
Lines 218766 218568 -198
Branches 18142 17944 -198
===========================================
- Hits 138010 56067 -81943
- Misses 80564 162486 +81922
+ Partials 192 15 -177
Flags with carried forward coverage won't be shown. Click here to find out more. |
There was a problem hiding this comment.
I'm hitting the approve since this functionally looks good, most of my comments are code organization or style suggestions and a couple doc string fixes. If we clean those up I think these will all be looking good once we refactor a lot of the shared code between the three liquid class methods
| # This loop looks at the next volumes to dispense and calculates how many | ||
| # dispense volumes plus their conditioning & disposal volumes can fit into | ||
| # the tip. It then collects these volumes and their destinations in a list. | ||
| while not is_last_step and ( |
There was a problem hiding this comment.
This check seems similar to the one above (for seeing if multi-dispense is possible), and since both are long conditionals it might make the code neater to move this to a static helper function
| air_gap=0, | ||
| ) | ||
| ] | ||
| use_single_dispense = False |
There was a problem hiding this comment.
silly code organizational thing but can we move this to be in the same block as the below check?
There was a problem hiding this comment.
You mean have a blank line above this line instead of below?
| "The distribution uses a disposal volume but location for disposing off" | ||
| " the disposal volume is not found since blowout is disabled." | ||
| " Specify a blowout location and enable blowout when using a disposal volume." |
There was a problem hiding this comment.
| "The distribution uses a disposal volume but location for disposing off" | |
| " the disposal volume is not found since blowout is disabled." | |
| " Specify a blowout location and enable blowout when using a disposal volume." | |
| "Distribute liquid uses a disposal volume but location for disposing of" | |
| " the disposal volume cannot not found when blowout is disabled." | |
| " Specify a blowout location and enable blowout when using a disposal volume." |
| else True, | ||
| trash_location=trash_location, | ||
| ) | ||
| if use_single_dispense |
There was a problem hiding this comment.
another code opinion comment, but I'd prefer if this was a conditional rather than a ternary statement, again I think that's just easier to parse on first glance
| increment=None, | ||
| ) | ||
|
|
||
| tip_used = False |
There was a problem hiding this comment.
I think we can get rid of this boolean and just simply check for the three tip strategy cases like we do for consolidate:
- Pick up tips before the main loop and drop after for
once - Do the check for
alwaysin the loop and drop and pick up tips (you'll have to check to see we don't have a tip already but I think the variables I've added for return tip can be used for this) - Don't do anything for
never
There was a problem hiding this comment.
Will address this when I implement the return tip feature for distribute
| components_executor.retract_after_aspiration(volume=volume) | ||
| if ( | ||
| transfer_type == tx_comps_executor.TransferType.ONE_TO_MANY | ||
| and conditioning_volume not in [None, 0] |
There was a problem hiding this comment.
| and conditioning_volume not in [None, 0] | |
| and not conditioning_volume |
I'm not always a fan of relying on not 0 or not None to be used as booleans, but since both are valid and since I think it reads fine, I'd prefer this. Not a strong opinion though
There was a problem hiding this comment.
We want to check the opposite though.. that conditioning volume is a non-zero number. The alternative could be to just use .. and conditioning_volume or .. and conditioning_volume is not None and conditioning_volume > 0. The first one is concise but loses the detail that we are looking for a non-zero number, and the second one is too verbose. I think the ..not in [None, 0] is a good middle-ground.
| touch_tip_properties=retract_props.touch_tip, | ||
| location=touch_tip_and_air_gap_location, | ||
| well=touch_tip_and_air_gap_well, | ||
| skip_air_gap=not ( |
There was a problem hiding this comment.
I think the function signature for this (have to comment here because of github comment rules) should be add_air_gap, just to keep things consistent and remove the negation being done in this method (as long as it's clean elsewhere too)
There was a problem hiding this comment.
Yea.. the function name says do_touch_tip_and_air_gap so in that context the intention of skip_air_gap is clearer than that of add_air_gap (is it adding another air gap or is it talking about the air gap that the function was supposed to add originally?).
But I know what you mean, when rest of the booleans are talking about adding an air gap, it does get confusing when there's one that talks about skipping it instead. I'll change the arg name and add docstring explaining the meaning.
| ) | ||
| self._tip_state.ready_to_aspirate = False | ||
|
|
||
| is_final_air_gap_of_current_retract = ( |
There was a problem hiding this comment.
This block of code and the following could use a doc string to help explain what's happening with air gaps
| add_final_air_gap: bool, | ||
| expect_air_gap: bool, | ||
| ) -> None: | ||
| """It should execute steps to retract from well after a dispense.""" |
There was a problem hiding this comment.
It's technically not wrong lol. But I'll add more context to the docstrings 👍
| expect_air_gap: bool, | ||
| expect_blowout: bool, | ||
| ) -> None: | ||
| """It should execute steps to retract from well after a dispense.""" |
| air_gap=0, | ||
| ) | ||
| ] | ||
| use_single_dispense = False |
There was a problem hiding this comment.
You mean have a blank line above this line instead of below?
| touch_tip_properties=retract_props.touch_tip, | ||
| location=touch_tip_and_air_gap_location, | ||
| well=touch_tip_and_air_gap_well, | ||
| skip_air_gap=not ( |
There was a problem hiding this comment.
Yea.. the function name says do_touch_tip_and_air_gap so in that context the intention of skip_air_gap is clearer than that of add_air_gap (is it adding another air gap or is it talking about the air gap that the function was supposed to add originally?).
But I know what you mean, when rest of the booleans are talking about adding an air gap, it does get confusing when there's one that talks about skipping it instead. I'll change the arg name and add docstring explaining the meaning.
| add_final_air_gap: bool, | ||
| expect_air_gap: bool, | ||
| ) -> None: | ||
| """It should execute steps to retract from well after a dispense.""" |
There was a problem hiding this comment.
It's technically not wrong lol. But I'll add more context to the docstrings 👍
| ( | ||
| [22, 22, 22, 22], | ||
| ["dest1", "dest2", "dest3", "dest4"], | ||
| 50, | ||
| [(22, "dest1"), (22, "dest2"), (22, "dest3"), (22, "dest4")], |
There was a problem hiding this comment.
Oops! I was checking something here. Will remove it
Closes AUTH-1067
Overview
Implements the
InstrumentCore.distribute_liquid()methodTest Plan and Hands on Testing
Changelog
InstrumentCore.distribute_liquid()InstrumentCore.dispense_liquid_class_during_multi_dispense()to handle dispenses when doing a multi-dispenseTransferComponentsExecutor.retract_during_multi_dispensing()Review requests
Risk assessment
Medium-Low. Doesn't change any existing public features but does modify small parts of the liquid-class based transfer & consolidate steps.