feat(api): add WellVolumeOffset to WellLocation #16302
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #16302 +/- ##
===========================================
+ Coverage 63.28% 79.43% +16.14%
===========================================
Files 300 118 -182
Lines 15786 4283 -11503
===========================================
- Hits 9990 3402 -6588
+ Misses 5796 881 -4915
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| @@ -239,8 +239,31 @@ class WellOffset(BaseModel): | |||
| z: float = 0 | |||
|
|
|||
|
|
|||
| # get rid of operationVolume below and operation_volume parameter in methods? | |||
| # TODO(pm, 2024-09-23): PE should raise error if volumeOffset specified with a tip rack location | |||
| class WellLocation(BaseModel): | |||
There was a problem hiding this comment.
I think we should call this LiquidHandlingWellLocation, and then have WellLocation = LiquidHandlingWellLocation if we don't want to change all the papi code
| ending_volume = starting_volume + volume | ||
| ending_height = self.get_height_at_volume(labware_id=labware_id, well_id=well_id, target_volume=ending_volume) | ||
| # return ending_height | ||
| return starting_height # delete, use line above once sub-methods implemented |
There was a problem hiding this comment.
if we merge this, we should merge the return ending_height, and not this comment
There was a problem hiding this comment.
Looking great and almost there! Inline comments for change requests.
Generally, I do not think we should be validating scientific reasonableness in the engine - that should happen in the input layers like PD and PAPI. Specifically that's things like forcing dispense volumeOffset to 0, and requiring aspirate z offsets to be <0. Let the engine execute what it's given as long as it won't cause an outright error, like a collision.
| if invalid: | ||
| if isinstance(well_location, PickUpTipWellLocation): | ||
| raise OperationLocationNotInWellError( | ||
| f"Specifying {well_location.origin} with an offset of {well_location.offset} results in an operation location below the bottom of the well" |
There was a problem hiding this comment.
not always true that this is below the well, right? it could also be below the minimum height. Might be worth raising these exceptions from inside the logic above so we can take that into account.
There was a problem hiding this comment.
Updated docstring
| @@ -244,6 +244,34 @@ class WellLocation(BaseModel): | |||
|
|
|||
| origin: WellOrigin = WellOrigin.TOP | |||
| offset: WellOffset = Field(default_factory=WellOffset) | |||
| volumeOffset: float = Field( | |||
There was a problem hiding this comment.
are we keeping WellLocation around for backwards compatibility? If so, maybe we can avoid adding volumeOffset to it.
There was a problem hiding this comment.
volumeOffset was added to WellLocation at your suggestion to enable more flexibility. Specifically for MoveToWell and TouchTip commands
There was a problem hiding this comment.
okay, cool! sorry for forgetting :)
shared-data/python/opentrons_shared_data/protocol/models/shared_models.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Types mostly look good, just a couple places that I think need to be fixed.
Feel free to merge without fixing these, if you need to unblock something. But I do think we should fix these before release.
I found these by running a dev server (make -C robot-server dev), opening the HTTP API documentation (http://localhost:31950/redoc), and clicking through the params fields of the many commands in the GET /runs/{runId}/commands/{commandId} documentation. Another way of doing this would have been to feed the command schema into a viewer like https://json-schema.app/start.
I haven't looked closely at the opentrons.protocol_engine logic part of this since it seems like @sfoster1 already has, but I'm happy to if anyone thinks it's worthwhile.
… command to use WellLocation
<!-- Thanks for taking the time to open a Pull Request (PR)! Please make sure you've read the "Opening Pull Requests" section of our Contributing Guide: https://github.com/Opentrons/opentrons/blob/edge/CONTRIBUTING.md#opening-pull-requests GitHub provides robust markdown to format your PR. Links, diagrams, pictures, and videos along with text formatting make it possible to create a rich and informative PR. For more information on GitHub markdown, see: https://docs.github.com/en/get-started/writing-on-github/getting-started-with-writing-and-formatting-on-github/basic-writing-and-formatting-syntax To ensure your code is reviewed quickly and thoroughly, please fill out the sections below to the best of your ability! --> # Overview <!-- Describe your PR at a high level. State acceptance criteria and how this PR fits into other work. Link issues, PRs, and other relevant resources. --> This PR enables static meniscus-relative aspirate and dispense via the Protocol API. To enable this, WellVolumeOffset has been added to WellLocation. This is a volume of liquid to account for when executing commands with an origin of WellOrigin.MENISCUS. Specifying `operationVolume` results in this class acting as a sentinel and should be used when volume can be determined from the command parameters, for example commanding Aspirate. A volume should be specified when it cannot be determined from the command parameters, for example commanding MoveToWell prior to AspirateInPlace. ## Test Plan and Hands on Testing <!-- Describe your testing of the PR. Emphasize testing not reflected in the code. Attach protocols, logs, screenshots and any other assets that support your testing. --> Successfully tested the following protocol on a robot multiple times in a row. Aspiration from meniscus was not tested due to not-yet-available InnerWellGeometry: ``` from opentrons.protocol_api import ProtocolContext metadata = {"protocolName": "Test LLD"} requirements = {"robotType": "Flex", "apiLevel":"2.21"} def run(ctx: ProtocolContext) -> None: """Run.""" tiprack = ctx.load_labware(f"opentrons_flex_96_tiprack_1000uL", "A3") source = ctx.load_labware("nest_12_reservoir_15ml", "C2") sink = ctx.load_labware("nest_96_wellplate_100ul_pcr_full_skirt", "D2") pipette = ctx.load_instrument("flex_1channel_1000", "left", liquid_presence_detection = True) pipette.pick_up_tip(tiprack) pipette.measure_liquid_height(sink["A1"]) pipette.aspirate(10, source["A1"]) pipette.dispense(10, sink["A1"].meniscus(-2)) pipette.return_tip() ``` ## Changelog <!-- List changes introduced by this PR considering future developers and the end user. Give careful thought and clear documentation to breaking changes. --> ## Review requests <!-- - What do you need from reviewers to feel confident this PR is ready to merge? - Ask questions. --> ## Risk assessment <!-- - Indicate the level of attention this PR needs. - Provide context to guide reviewers. - Discuss trade-offs, coupling, and side effects. - Look for the possibility, even if you think it's small, that your change may affect some other part of the system. - For instance, changing return tip behavior may also change the behavior of labware calibration. - How do your unit tests and on hands on testing mitigate this PR's risks and the risk of future regressions? - Especially in high risk PRs, explain how you know your testing is enough. -->
Overview
This PR enables static meniscus-relative aspirate and dispense via the Protocol API. To enable this, WellVolumeOffset has been added to WellLocation. This is a volume of liquid to account for when executing commands with an origin of WellOrigin.MENISCUS. Specifying
operationVolumeresults in this class acting as a sentinel and should be used when volume can be determined from the command parameters, for example commanding Aspirate. A volume should be specified when it cannot be determined from the command parameters, for example commanding MoveToWell prior to AspirateInPlace.Test Plan and Hands on Testing
Successfully tested the following protocol on a robot multiple times in a row. Aspiration from meniscus was not tested due to not-yet-available InnerWellGeometry:
Changelog
Review requests
Risk assessment