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.

Sign up for GitHub

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

Jump to bottom

feat(api): add meniscus wellOrigin enum #16139

Merged
merged 12 commits into from
Sep 17, 2024
Merged

feat(api): add meniscus wellOrigin enum #16139

merged 12 commits into from
Sep 17, 2024

Conversation

pmoegenburg
Copy link
Contributor

@pmoegenburg pmoegenburg commented Aug 27, 2024 •
edited
Loading

Overview

This adds MENISCUS to the WellOrigin enums. This also adds methods that support using WellOrigin.MENISCUS. With this PR, Protocol Engine State GeometryView get_well_position() now accepts calls specifying WellOrigin.MENISCUS and pulls a well's liquid height (relative to well bottom) from Protocol Engine State WellView. Currently, a well's liquid height is populated only once a successful liquid probe has been done for that well.

Test Plan and Hands on Testing

Changelog

Review requests

Risk assessment

@pmoegenburg pmoegenburg self-assigned this Aug 27, 2024
@pmoegenburg pmoegenburg requested a review from a team as a code owner August 27, 2024 16:09
@pmoegenburg pmoegenburg marked this pull request as draft August 27, 2024 16:09
@pmoegenburg pmoegenburg changed the title feat(api): add liquid_surface WellOrigin enum feat(api): add meniscus wellOrigin enum Sep 16, 2024
@pmoegenburg pmoegenburg marked this pull request as ready for review September 16, 2024 21:08
@pmoegenburg pmoegenburg requested a review from a team as a code owner September 16, 2024 21:08
sfoster1
sfoster1 requested changes Sep 17, 2024
View reviewed changes
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.

This is awesome. It's almost there, but let's get some tests added for the protocol API first.

api/src/opentrons/protocol_api/labware.py Outdated Show resolved Hide resolved
shared-data/command/schemas/9.json Show resolved Hide resolved
caila-marashaj
caila-marashaj reviewed Sep 17, 2024
View reviewed changes
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.

Agree with Seth and Max's comments, otherwise looking good!

sfoster1
sfoster1 requested changes Sep 17, 2024
View reviewed changes
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.

I think you haven't quite drawn the change to relative positioning all the way through yet. Also, please add a test for the actual protocol api well change, not just the core.

api/src/opentrons/protocol_engine/state/geometry.py Outdated Show resolved Hide resolved
sfoster1
sfoster1 approved these changes Sep 17, 2024
View reviewed changes
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.

Great! Please add a more descriptive PR message before merging and make sure it gets used as the commit message.

@pmoegenburg pmoegenburg merged commit 4064bdf into edge Sep 17, 2024
42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sfoster1 sfoster1 sfoster1 approved these changes

@ryanthecoder ryanthecoder Awaiting requested review from ryanthecoder

@SyntaxColoring SyntaxColoring Awaiting requested review from SyntaxColoring

@caila-marashaj caila-marashaj Awaiting requested review from caila-marashaj

Assignees

@pmoegenburg pmoegenburg

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pmoegenburg @sfoster1 @SyntaxColoring @caila-marashaj