Skip to content

refactor(api): height from volume binary search #18081

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

Merged
merged 9 commits into from
Apr 30, 2025

Conversation

caila-marashaj
Copy link
Contributor

@caila-marashaj caila-marashaj commented Apr 15, 2025

Overview

For circular frusta, you can use calculus to determine a volume from a height, and not a height from a volume (the equation isolating height is a higher degree polynomial and gets pretty gross to try and solve analytically). The way we were getting around this before was by creating a lookup table during a protocol run, when a labware was loaded with a CircularFrustum, at a fixed interval of every 0.5 mm to maintain accuracy. This became a problem for very tall circular frusta (like test tubes), and protocols that used them with this code would silently pause for 2 minutes at the start to fill out these very large tables.

This implements a binary search instead to find a height from a volume within a circular frustum, upon request.

Copy link

codecov bot commented Apr 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 25.67%. Comparing base (2a32194) to head (674f39c).
Report is 6 commits behind head on edge.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             edge   #18081       +/-   ##
===========================================
- Coverage   57.52%   25.67%   -31.85%     
===========================================
  Files        3043     2938      -105     
  Lines      255340   227623    -27717     
  Branches    30544    27497     -3047     
===========================================
- Hits       146879    58446    -88433     
- Misses     108275   169160    +60885     
+ Partials      186       17      -169     
Flag Coverage Δ
protocol-designer 19.04% <ø> (+0.05%) ⬆️
step-generation 4.37% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 2344 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@caila-marashaj caila-marashaj force-pushed the height-from-volume-binary-search-draft branch 2 times, most recently from 93d653b to 13bb526 Compare April 22, 2025 18:50
@caila-marashaj caila-marashaj marked this pull request as ready for review April 22, 2025 19:00
@caila-marashaj caila-marashaj requested a review from a team as a code owner April 22, 2025 19:00
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.

there's definitely a limit to the number of checks that we'll require, but it's still generally iffy to do a bunch of recursion in python (sadly!) because it can have a pretty heavy stack frame, so let's make this not recursive.

Let's also get this tested with hypothesis if we can, or ideally with some pretty rigorous numerics testing beyond just the boundary tests.

other than that, looks great!

@caila-marashaj caila-marashaj changed the title refactor(api): height from volume binary search draft refactor(api): height from volume binary search Apr 29, 2025
@caila-marashaj caila-marashaj force-pushed the height-from-volume-binary-search-draft branch from 084d34a to 3e79491 Compare April 29, 2025 22:38
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 looks great and I'm always hype for hypothesis (#hypeforhypothesis)

@caila-marashaj caila-marashaj merged commit 5af0934 into edge Apr 30, 2025
68 of 69 checks passed
ddcc4 pushed a commit that referenced this pull request May 17, 2025
ddcc4 pushed a commit that referenced this pull request May 17, 2025
ddcc4 pushed a commit that referenced this pull request May 17, 2025
ddcc4 pushed a commit that referenced this pull request May 17, 2025
ddcc4 pushed a commit that referenced this pull request May 17, 2025
ddcc4 pushed a commit that referenced this pull request May 17, 2025
ddcc4 pushed a commit that referenced this pull request May 19, 2025
ddcc4 pushed a commit that referenced this pull request May 19, 2025
ddcc4 pushed a commit that referenced this pull request May 19, 2025
ddcc4 pushed a commit that referenced this pull request May 20, 2025
ddcc4 pushed a commit that referenced this pull request May 20, 2025
ddcc4 pushed a commit that referenced this pull request May 22, 2025
ddcc4 pushed a commit that referenced this pull request May 23, 2025
ddcc4 pushed a commit that referenced this pull request May 24, 2025
ddcc4 pushed a commit that referenced this pull request May 24, 2025
ddcc4 pushed a commit that referenced this pull request May 29, 2025
ddcc4 pushed a commit that referenced this pull request May 29, 2025
ddcc4 pushed a commit that referenced this pull request May 29, 2025
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