feat(api): fully connected volume tracking #16532
Conversation
pmoegenburg
requested review from
sfoster1,
SyntaxColoring,
ryanthecoder and
caila-marashaj
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #16532 +/- ##
===========================================
+ Coverage 79.88% 92.43% +12.55%
===========================================
Files 120 77 -43
Lines 4410 1283 -3127
===========================================
- Hits 3523 1186 -2337
+ Misses 887 97 -790
Flags with carried forward coverage won't be shown. Click here to find out more. |
…ded tests for IncompleteLabwareDefinitionError
This is not the right thing to be doing, we want callers to be able to say what they want.
| well_name=action.error.private.well_name, | ||
| height=None, | ||
| time=action.failed_at, | ||
| state_update = get_state_update(action) |
There was a problem hiding this comment.
| state_update = get_state_update(action) | |
| state_update = get_state_update.get_state_update(action) |
| await self._command_executor.execute(command_id=command_id) | ||
| try: | ||
| await self._command_executor.execute(command_id=command_id) | ||
| except BaseException: |
There was a problem hiding this comment.
is this intentional?
There was a problem hiding this comment.
yes. it could also be a separate change probably but not having this is why tests would mysteriously hang sometimes
| ) | ||
| except KeyError: | ||
| return False | ||
| ) -> Tuple[ |
There was a problem hiding this comment.
a bit worried about returning a tuple with optional values?
| summaries = self._append_summaries_with_probed_heights(summaries=summaries) | ||
| summaries = self._append_summaries_with_probed_volumes(summaries=summaries) |
There was a problem hiding this comment.
this is a bit confusing to me. can we not return anything? the list is mutable
| class WellInfoSummary(BaseModel): | ||
| """Payload for a well's liquid info in StateSummary.""" | ||
|
|
||
| labware_id: str | ||
| well_name: str | ||
| height: float | ||
| last_measured: datetime | ||
| loaded_volume: Optional[float] = None | ||
| last_loaded: Optional[datetime] = None | ||
| operations_since_load: Optional[int] = None | ||
| probed_height: Optional[float] = None | ||
| probed_volume: Optional[float] = None | ||
| last_probed: Optional[datetime] = None | ||
| operations_since_probe: Optional[int] = None |
There was a problem hiding this comment.
Do we really want to expose all of these WellInfoSummary fields over HTTP and store them in the database? Is there a client need for all of these fields?
There was a problem hiding this comment.
I eagerly want to expose these fields over HTTP because I think this system is better when it exposes more information, though storing it in the database is a bit icky. There isn't a currently planned in the app use for them but there is a use for me wanting to see what the information is.
bit more readable, bit less datetime
api/src/opentrons/protocol_engine/commands/aspirate_in_place.py
Outdated
Show resolved
Hide resolved
| elif ( | ||
| well_liquid.probed_volume is not None | ||
| and well_liquid.probed_volume.volume is not None | ||
| ): | ||
| return self.get_well_height_at_volume( | ||
| labware_id=labware_id, | ||
| well_name=well_name, | ||
| volume=well_liquid.probed_volume.volume, |
There was a problem hiding this comment.
This ladder looks like:
- Return the last probed height, directly.
- Or return the height computed from the loaded volume, where the "loaded volume" is either from a
load_liquid(), or from adding upaspirate()s anddispense()s. - (these highlighted lines) Or return the height computed from the last probed volume.
But in part 3, wasn't the "last probed volume" computed from the last probed height? So isn't part 3 exactly equivalent to part 1? Is part 3 doing anything?
There was a problem hiding this comment.
no, because the key with (1) is that the last probed height is available if and only if you haven't since touched the volume, whereas (3) is always available
There was a problem hiding this comment.
the use cases for (1) look like, "do a probe immediately before every aspirate for maximum accuracy". the use case for (3) looks like "i have this one tube of master mix I fill up when it's getting empty and I don't care how much is in it beyond 'enough', so I'll probe it once at the beginning of the protocol and let the robot track volume from there"
There was a problem hiding this comment.
no, because the key with (1) is that the last probed height is available if and only if you haven't since touched the volume, whereas (3) is always available
Ohh I see, thanks. So when we touch the volume, the element in probed_heights is deleted, but the element in probed_volumes is not.
This certainly doesn't have to change in this PR, but what do you think about simplifying this by expressing it in WellState as attributes like:
current_baseline, which stores, per-well, either:- The most recent volume from
loadLiquid(even if there were a bunch of liquid handling ops since then) - Or the most recent height from
probeLiquid(even if there were a bunch of liquid handling ops since then)
- The most recent volume from
volume_change_since_baseline, which stores, per-well, the aspirate+dispense+etc. volume delta since then.
So then, going through the three modes, using your descriptions from Slack:
-
mode 1: meniscus relative aspiration from a recent LLD height estimation (ie. probe then aspirate). this is the "gold standard"...
This is when
current_baselineis aliquidProbeheight andvolume_change_since_baseline == 0. -
mode 2: meniscus relative aspiration from volume-based height estimation, from a volume entered in the protocol with load_liquid and tracked through pipette actions. less accurate than immediate LLD...
This is when
current_baselineis aloadLiquidvolume. -
mode 3: meniscus relative aspriation from volume-based height estimation from height-based volume estimation, i.e. you got your initial volume from an LLD pass. this is the worst because you suffer those errors twice...
This is when
current_baselineis aliquidProbeheight andvolume_change_since_baseline != 0.
I guess I'm shooting for WellState attributes to represent exactly what we know, in plain terms, without any editorializing. Then the GeometryView methods can implement the entirety of our business logic, with the cascading accuracy thing, based on those ground truths. Right now, I feel like the business logic is leaking into WellState and muddying what those attributes are. (probed_heights and probed_volumes differ in more ways than just "one is in height units and one is in volume units.")
There was a problem hiding this comment.
I see what you're getting at. My bias is towards doing that but also storing separate data separately. What I think that version would be is
loaded_volume_baseline: the lastload_volumeprobed_height_baseline: the lastprobe_liquid, whenever it wasvolume_change_since_loaded_volume_baselinevolume_change_since_probed_volume_baseline(we need both because you might have probed after some liquid operations on a loaded baseline)- something like
immediate_probed_height. This could also just beprobed_height_baselineif we're tracking how many operations it's been since then, but I think it's much nicer and inspectable to be able to look at the data and pick between the entries, rather than follow a chain of logic.
Overview
This PR aims to consolidate well volume data from our existing relevant commands into a single source of truth in WellStore. These commands are LiquidProbe, LoadLiquid, Aspirate, and Dispense.
volume estimation and height
Well volume and height are tracked in three parts:
LoadLiquidand was then modified by liquid volume tracking from aspirate and dispense volumesThe reason for having all three is that they have different accuracy characteristics, and we therefore use them for meniscus height estimation in an order of precedence:
The latter two values are only accessible if the labware the well is in has internal geometry data. Even when we do, that geometry data is noisy and can be inaccurate, making those values suitable for problems like "I don't want to have to accurately measure how much is in this tube" and not for problems like "I want to minimize external droplet formation on my 1uL aspiration".
This PR implements all that, as well as setting the movement data for actually going to a well meniscus.
volume offsets
A pretty important thing to consider for static meniscus-relative movements is that when you're aspirating you don't want to move to where the meniscus is but rather where it will be when the operation ends, so you don't leave your pipette high and dry. We signify this by allowing a "volume offset" in a location. This is like a height offset but is specified in uL or the special value "operationVolume". It will add an extra Z-offset to the positioning to account for a volume change in a well. This is calculated using the well's internal geometry and thus only available if the well has internal geometry. If the value is the string "operationVolume", which should be thought of as a sentinel value, we even calculate the volume offset from the volume you passed to
aspirate; but specifying the value as a float is available for things like amoveToWellpreceding anaspirateInPlace.There's some gross internals because of history. One particularly gross one is the Location data type; this is an old friend indeed, and its friendship continues to pay dividends since we want to add something to it that is somewhat internal, and we have to do so in a really gross way.
testing
We really need to test this in actually used protocols and this shouldn't be relied upon yet, but extensive test coverage should help.