-
Notifications
You must be signed in to change notification settings - Fork 186
feat(api): add x,y WellOffset for wells with an even number of subsections #19155
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
base: edge
Are you sure you want to change the base?
Conversation
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.
TY!
I question the premise that the robot should do this automatically. I worry that we're missing a bigger problem by thinking of this as a collision avoidance fixup, as opposed to, like, a more holistic thing about our liquid and labware modeling. I elaborate on this in #19155 (comment). This doesn't block merging if you all are really sure about this.
Other comments are code-level things, assuming we are going with this basic premise.
# move over into the middle of the nearest subection | ||
y_offset = subsection_y_dimension / 2 | ||
return WellOffset(x=x_offset, y=y_offset, z=0) | ||
|
||
def get_well_position( | ||
self, |
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.
Nit: Can we add unit tests covering the new get_well_position()
behavior?
xy_offset = self.get_xy_offset_if_needed( | ||
labware_id=labware_id, well_name=well_name | ||
) | ||
offset = WellOffset(x=xy_offset.x, y=xy_offset.y, z=well_depth) |
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.
Do we not need some kind of cleverness to handle the multi-channel cases, where the channels are already aligned to sub-wells? I haven't tested this, but at a glance, it looks like channels that previously would have naturally gone to the center of sub-wells will now to to the top of the "mountain" between sub-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.
If I understand correctly, those sub-wells will be the same width as the channels, meaning there's only one, and therefore no x-offset for example should get applied because the xCount is an odd number
xy_offset = self.get_xy_offset_if_needed( | ||
labware_id=labware_id, well_name=well_name | ||
) | ||
offset = WellOffset(x=xy_offset.x, y=xy_offset.y, z=well_depth) |
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.
OK here's the premise-questioning comment.
If we want this use case to work, don't we also need to change the way that height<->volume calculations work, if not more?
Suppose we have a 1000 µL single-well reservoir, where once we've aspirated all but 200 µL, we've reached the top of a bottom zone of 96 sub-wells.
As I understand it, the premise of the existing calculations is that if you aspirate 1000 µL, you take 800 µL from the top monolithic sections, and then you take 200 µL from the collection of sub-wells, evenly distributed amongst them. So, ~2.08 µL per sub-well. (Please double-check my assumptions on this.)
So now, with this new code, imagine attempting to aspirate 1000 µL with a single-channel pipette. The robot automatically nudges the pipette over in x/y, so it looks, at first, like it knows how to make this use case "just work." The first 800 µL works as normal. But once we get to the sub-well part, I think things go subtly wrong. The existing height<->volume calculations assume we're aspirating 200 µL from the 96 sub-wells collectively, but we're not—we're physically attempting to aspirate 200 µL from a single sub-well.
One implication of this is that the height doesn't go down as quickly as it should. So if you're tracking the meniscus, you won't keep up with it. Another implication is that even if we fixed the height tracking, we'd quickly run out of liquid in the sub-well and bottom out, since it could never hold 200 µL to begin with, only ~2.08 µL.
If we really wanted aspirations to "just automatically work" all the way down into these sub-wells, we'd need to iterate over them or something. Which I think we can agree is way too much for aspirate
to do.
7f72fd9
to
933ad22
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## edge #19155 +/- ##
=======================================
Coverage 24.19% 24.19%
=======================================
Files 3377 3377
Lines 297001 297001
Branches 31544 31545 +1
=======================================
Hits 71870 71870
Misses 225108 225108
Partials 23 23
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Overview
Fixes this issue in ABR where during meniscus-relative pipetting, a tip will hit the top of a reservoir's sub-wells at the bottom and cause an
OverPressure
error.When resolving the geometric position of a well, we can check if the well's bottom has an even number of rows or columns of sub-wells. If it does, add an offset to move into the middle of the nearest sub-well to allow the tip to go down into the bottom.
Enabling pipetting like this into the bottom of reservoirs might result in running out of liquid in some sub-wells and not the whole reservoir, but I think that's fine.
Changelog
geometry.py
to check if there are an even number of subwells, and return an offset into the middle of the nearest subwell if soget_well_position
TODO