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(protocol engine): calculate volume and height for irregular well shapes #16299

Merged
merged 7 commits into from
Sep 24, 2024

Conversation

caila-marashaj
Copy link
Contributor

@caila-marashaj caila-marashaj commented Sep 19, 2024

Overview

Previously in frustum_helpers, we were only handling the cases where wells had either all circular or all rectangular cross sections. If neither of these is the case, let's iterate through all the cross-sections and find the volume using the volume-of-a-frustum formula.

Changelog

  • add calculations using the volume-of-a-frustum formula
  • make sure there are no repeat roots found in reject_unacceptable_heights- this happened with 0.0
  • add tests for all the frustum math we have so far

Review Requests

Pls help me make sure I input the formula correctly

@caila-marashaj caila-marashaj requested a review from a team as a code owner September 19, 2024 15:30
@caila-marashaj caila-marashaj changed the title feat(protocol engine): add frustum helpers compatibility with irregular well shapes feat(protocol engine): calculate volume and height for irregular well shapes Sep 19, 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.

Excellent. Let's add some tests though. Remember, the code's obvious now, but somebody might accidentally change it later and if tests fail they'll know they messed up.

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.

Would love to see an explicit isclose() compare instead of set identity for floats.

Also, since this is a math thing, it might be worth investigating using hypothesis, but that doesn't have to be in this pr.

@caila-marashaj
Copy link
Contributor Author

Would love to see an explicit isclose() compare instead of set identity for floats.

Also, since this is a math thing, it might be worth investigating using hypothesis, but that doesn't have to be in this pr.

https://opentrons.atlassian.net/jira/software/c/projects/EXEC/list?selectedIssue=EXEC-726

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.

lgtm!

@caila-marashaj caila-marashaj merged commit 98639a9 into edge Sep 24, 2024
21 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.

2 participants