Skip to content
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

feat(api): implement liquid probe protocol engine command #15351

Merged
merged 22 commits into from
Jun 18, 2024

Conversation

pmoegenburg
Copy link
Contributor

@pmoegenburg pmoegenburg commented Jun 5, 2024

Overview

This closes EXEC-436. Pulls well dimension data from Protocol Engine StateView, which is now used for process max_z_distance. Also bubbles up LiquidNotFoundError.

Test Plan

Added protocol engine command test.

Changelog

Review requests

Risk assessment

Low. Largely builds off existing Aspirate protocol engine command.

@pmoegenburg pmoegenburg requested a review from a team as a code owner June 5, 2024 21:51
@pmoegenburg pmoegenburg self-assigned this Jun 5, 2024
Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from fixing what's already failing tests, please either draft this or take a pass to remove note to self comments and write some tests. They can be pretty simple for now since this command doesn't do much, but they should be present.

@pmoegenburg pmoegenburg marked this pull request as draft June 6, 2024 15:19
class LiquidProbeParams(BaseModel):
"""Payload required to liquid probe."""

mount: MountType = Field(..., description="Instrument mount to liquid probe with.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are going to want the well we are using so we can get the well's max depth

ot3_mount is not OT3Mount.GRIPPER
), "Expected a Pipette mount but Gripper mount was provided."

z_pos = ot3_api.liquid_probe(mount=ot3_mount) # anything else?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are going to want to pass an Instance of LiquidProbeSettings.
we can get a copy of the defaults from ot3_api.config().liquid_sense
We are going to want to change the starting mount height and the max_z_distance based on the well we are probing

)

return SuccessData(
public=LiquidProbeResult(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might just be worth a comment somewhere that the position here is the position at the well where the probe starts, and z_pos is after the probe

Copy link
Contributor

@caila-marashaj caila-marashaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@pmoegenburg pmoegenburg marked this pull request as ready for review June 14, 2024 18:20
@pmoegenburg pmoegenburg requested a review from a team as a code owner June 14, 2024 18:20
@pmoegenburg pmoegenburg requested a review from ryanthecoder June 14, 2024 20:01
Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple last little changes but almost there!

Copy link
Contributor

A PR has been opened to address analyses snapshot changes. Please review the changes here: #15455

Copy link

codecov bot commented Jun 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.43%. Comparing base (5ce130b) to head (d961cf5).
Report is 33 commits behind head on edge.

Current head d961cf5 differs from pull request most recent head 72a0ac8

Please upload reports for the commit 72a0ac8 to get more accurate results.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #15351      +/-   ##
==========================================
- Coverage   60.43%   60.43%   -0.01%     
==========================================
  Files         154      154              
  Lines       12119    12116       -3     
==========================================
- Hits         7324     7322       -2     
+ Misses       4795     4794       -1     
Files Coverage Δ
...pentrons_hardware/hardware_control/sensor_utils.py 0.00% <ø> (ø)
...-data/python/opentrons_shared_data/errors/codes.py 93.97% <ø> (-0.08%) ⬇️
.../python/opentrons_shared_data/errors/exceptions.py 60.93% <ø> (-0.16%) ⬇️

Copy link
Contributor

A PR has been opened to address analyses snapshot changes. Please review the changes here: #15455

pmoegenburg and others added 2 commits June 18, 2024 15:16
This PR is an automated snapshot update request. Please review the
changes and merge if they are acceptable or find you bug and fix it.

Co-authored-by: pmoegenburg <[email protected]>
Co-authored-by: pmoegenburg <[email protected]>
Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice, love the changes, looks good to me!

@pmoegenburg pmoegenburg merged commit f8d7d64 into edge Jun 18, 2024
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants