feat(api): introduce correction volume into the liquid classes based transfer flow #17322
Conversation
…irate and dispense methods
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #17322 +/- ##
==========================================
+ Coverage 73.84% 79.04% +5.19%
==========================================
Files 43 120 +77
Lines 3304 4587 +1283
==========================================
+ Hits 2440 3626 +1186
- Misses 864 961 +97
Flags with carried forward coverage won't be shown. Click here to find out more. |
sanni-t
added
the
gen-analyses-snapshot-pr
|
A PR has been opened to address analyses snapshot changes. Please review the changes here: #17337 |
sanni-t
removed
the
gen-analyses-snapshot-pr
| if ul == 0: | ||
| position = instr.plunger_positions.bottom | ||
| else: | ||
| multiplier = 1.0 + (correction_volume / ul) |
| mm = ul / instr.ul_per_mm(ul, action) | ||
| position = instr.plunger_positions.bottom - mm | ||
| if ul == 0: | ||
| position = instr.plunger_positions.bottom |
There was a problem hiding this comment.
Question 1: Do you need this if block here, or does the code below still work correctly even when ul == 0?
There was a problem hiding this comment.
Nope. It will result in a divide by zero error
There was a problem hiding this comment.
Oh, I meant with the alternative expression suggested below :)
In general, I prefer to write equations to avoid singularities when describing physical systems. Conceptually, the position varies smoothly with the ul volume even at 0, so I want the code to reflect a smooth function as well, without the discontinuity at ul==0.
| multiplier = 1.0 + (correction_volume / ul) | ||
| mm_dist_from_bottom = ul / instr.ul_per_mm(ul, action) | ||
| mm_dist_from_bottom_corrected = mm_dist_from_bottom * multiplier | ||
| position = instr.plunger_positions.bottom - mm_dist_from_bottom_corrected |
There was a problem hiding this comment.
Question 2: Sorry, I'm bad at math. But is this expression just equivalent to:
position = (
instr.plunger_positions.bottom -
(ul + correction_volume) / instr.ul_per_mm(ul, action)
)
If so, I think all the work of introducing another concept like multiplier just makes the math more convoluted and harder to follow.
There was a problem hiding this comment.
The concept of multiplier actually makes the math, or rather, the reason behind the math, more understandable to me. It shows how we are modifying the distance_from_bottom value according to the correction volume. But if others feel it's confusing as well I can modify it.
| """Aspirate a given volume of air from the current location of the pipette. | ||
| Args: | ||
| volume: The volume of air to aspirate, in microliters. | ||
| flow_rate: The flow rate of air into the pipette, in microliters. | ||
| correction_volume: The correction volume in uL. |
There was a problem hiding this comment.
I think a slightly more detailed explanation would be helpful. Like, is the correction added or subtracted from the volume (so that users know what the sign should be).
There was a problem hiding this comment.
👍 Will address this separately. We need to update descriptions of most of the arguments related to liquid classes.
| multiplier = 1.0 + (correction_volume / ul) | ||
| mm_dist_from_bottom = ul / instr.ul_per_mm(ul, action) | ||
| mm_dist_from_bottom_corrected = mm_dist_from_bottom * multiplier | ||
| position = instr.plunger_positions.bottom - mm_dist_from_bottom_corrected | ||
| return round(position, 6) |
There was a problem hiding this comment.
Oh, this is not part of your diff, but I'm opposed to rounding on inner functions like this. You can round the values for display if you want, but rounding as part of the calculation here runs the risk of destroying precision.
(Also, it's a bit silly to rely on rounding when you're using floats. If rounding was really that important, these functions should be implemented with Decimal instead.)
There was a problem hiding this comment.
I believe the rounding is needed because of lower level driver restrictions.
There was a problem hiding this comment.
As for whether we should use Decimal, I think you are right that it's more correct. But since this line has been here since beginning of Flex, I am hesitant to change it now. Maybe @andySigler knows better about the consequences of changing it now.
|
Going to merge this in to unblock the next work but will keep it open to reviews. Will also keep the branch available for testing. |
Closes AUTH-964
Overview
Adds
correctionVolumeto protocol engine's aspirate and dispense commands and also adds it as argument to rest of the steps in the pipeline to get the correction volume value from the transfer flow to hardware controller.The hardware controller then wires it up to move the plunger in proportion to the correction volume.
Test Plan and Hands on Testing
For the PAPI part, unit and integration tests will be sufficient. For hardware control changes, an on-robot test should be performed.
Review requests
Risk assessment
Low